bake: support += operator to append with overrides

[crazy-max]

• fixes How can I append tags with --set ? #631
• fixes bake --set does not allow multiple tags, cache-to or cache-from #901
• closes bake: test secrets override #2912

Adds support for += operator to append through --set flag for supported array value typed fields.

target "default" {
  platforms = ["linux/amd64"]
}

$ docker buildx bake --set *.platform+=linux/arm64 --print
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 51B / 51B done
#1 DONE 0.0s
{
  "group": {
    "default": {
      "targets": [
        "default"
      ]
    }
  },
  "target": {
    "default": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "platforms": [
        "linux/amd64",
        "linux/arm64"
      ]
    }
  }
}

Note that it already appends for entitlements, annotations and attest fields. This has been documented to avoid confusion but wonder if we should make them consistent as follow-up. Would be a breaking change though so maybe first we should warn about this case?

[crazy-max]

This case is edgy and don't think current implementation is right. If last override does not append then it should only take this value and not append with previous one:

target "default" {
  platforms = ["linux/amd64"]
}

$ docker buildx bake --set *.platform+=linux/arm64 --set *.platform=linux/riscv64 --print
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 51B / 51B done
#1 DONE 0.0s
{
  "group": {
    "default": {
      "targets": [
        "default"
      ]
    }
  },
  "target": {
    "default": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "platforms": [
        "linux/arm64",
        "linux/riscv64"
      ]
    }
  }
}

Should be instead:

$ docker buildx bake --set *.platform+=linux/arm64 --set *.platform=linux/riscv64 --print
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 51B / 51B done
#1 DONE 0.0s
{
  "group": {
    "default": {
      "targets": [
        "default"
      ]
    }
  },
  "target": {
    "default": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "platforms": [
        "linux/riscv64"
      ]
    }
  }
}

Maybe that's fine for this PR to start with but I think we should refactor the override logic for such case.

[tonistiigi]

is there ever a case where = and += for same override key make sense? It would not be append anyway. Even if you follow the order of arguments, then one way it is a replace, and the other way some overrides get ignored.

[crazy-max]

Not for this PR but if we append a secret with the same ID it doesn't seem to override the existing one from the bake definition but append instead:

target "default" {
  secret = [
    "id=FOO,env=FOO"
  ]
}

$ FOO=foo BAR=bar docker buildx bake -f secret.hcl --set *.secrets+=id=FOO,env=BAR --print
#1 [internal] load local bake definitions
#1 reading secret.hcl 59B / 59B done
#1 DONE 0.0s
{
  "group": {
    "default": {
      "targets": [
        "default"
      ]
    }
  },
  "target": {
    "default": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "secret": [
        {
          "id": "FOO",
          "env": "FOO"
        },
        {
          "id": "FOO",
          "env": "BAR"
        }
      ]
    }
  }
}

Building with this Dockerfile:

FROM busybox
RUN --mount=type=secret,id=FOO cat /run/secrets/FOO

We have:

#0 building with "default" instance using docker driver

#1 [internal] load local bake definitions
#1 reading secret.hcl 59B / 59B done
#1 DONE 0.0s

#2 [internal] load build definition from Dockerfile
#2 transferring dockerfile: 102B done
#2 DONE 0.1s

#3 [internal] load metadata for docker.io/library/busybox:latest
#3 DONE 0.0s

#4 [internal] load .dockerignore
#4 transferring context:
#4 transferring context: 2B done
#4 DONE 0.1s

#5 [stage-0 1/2] FROM docker.io/library/busybox:latest
#5 DONE 0.0s

#6 [stage-0 2/2] RUN --mount=type=secret,id=FOO cat /run/secrets/FOO
#6 0.335 bar
#6 DONE 0.4s

#7 exporting to image
#7 exporting layers 0.0s done
#7 writing image sha256:0d8fffa9b8b5bee5151adc9fd83c9477653621c751f143a5fe6e44600bbbed52 done
#7 DONE 0.1s

Which is ok as it takes the last one defined but we should dedup early.

@jsternberg Maybe having a custom dedup logic for secrets in

buildx/util/buildflags/secrets.go

Line 30 in 4ed1e07

return removeDupes(s)

[crazy-max]

Opened #3032

[tonistiigi]

out of scope: why isn't the labels behaving the same

[crazy-max]

I guess annotations and labels don't behave like args because key can contain invalid chars when used with override like . Reminds me of this conversation with @jsternberg #2997 (review)

[tonistiigi]

Why would invalid characters matter, the "parser" uses splitn(,3). Annotations would be different as that is an array, not a map.

[tonistiigi]

this doesn't look very logical to do multiple TrimSuffix. Instead there should be one TrimSuffix for parts[0]