From b53ff7b3b72325ac1b887540c9f99811ad962c93 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sun, 21 Apr 2024 21:35:55 -0500 Subject: [PATCH 01/38] feat: transparent buildx support for remote buildkit (#6732) * detect-buildx global config option for backward compatibility * cache-tag global config option to customize cache destination * new CacheTo in DockerArtifact in configuration yaml (for docker build --cache-to) * export LoadDockerConfig to read ~/.docker/config.json for buildx detection * fix avoid loading image via buildx if no docker daemon is accessible * fix remote lookup / import missing in buildx workaround * fix image import if no docker daemon is available (under buildx) * adjust cache reference preserving tag and default cacheTo if not given * parse buildx metadata to extract ImageID digest Initially based on ebekebe's #8172 patch https://github.com/ebekebe/skaffold/commit/1c1fdeb18f4d2847e65e283fba498a14745039af Signed-off-by: reingart@gmail.com --- pkg/skaffold/build/cache/cache.go | 6 ++ pkg/skaffold/build/cache/lookup.go | 41 ++++++--- pkg/skaffold/build/docker/docker.go | 107 +++++++++++++++++++--- pkg/skaffold/build/docker/types.go | 4 +- pkg/skaffold/build/local/local.go | 9 +- pkg/skaffold/build/local/types.go | 8 +- pkg/skaffold/config/global_config.go | 2 + pkg/skaffold/config/util.go | 23 +++++ pkg/skaffold/docker/auth.go | 20 +++- pkg/skaffold/docker/buildx.go | 27 ++++++ pkg/skaffold/docker/image.go | 4 + pkg/skaffold/runner/runcontext/context.go | 12 +++ pkg/skaffold/schema/latest/config.go | 5 + 13 files changed, 234 insertions(+), 34 deletions(-) create mode 100644 pkg/skaffold/docker/buildx.go diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 371ffb104f2..d29a4a6a995 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -58,6 +58,7 @@ type cache struct { isLocalImage func(imageName string) (bool, error) importMissingImage func(imageName string) (bool, error) lister DependencyLister + buildx bool } // DependencyLister fetches a list of dependencies for an artifact @@ -69,6 +70,7 @@ type Config interface { GetPipelines() []latest.Pipeline DefaultPipeline() latest.Pipeline GetCluster() config.Cluster + DetectBuildX() bool CacheArtifacts() bool CacheFile() string Mode() config.RunMode @@ -118,6 +120,9 @@ func NewCache(ctx context.Context, cfg Config, isLocalImage func(imageName strin return pipeline.Build.LocalBuild.TryImportMissing, nil } + // for backward compatibility, extended build capabilities with BuildKit are disabled by default + buildx := cfg.DetectBuildX() + return &cache{ artifactCache: artifactCache, hashByName: hashByName, @@ -129,6 +134,7 @@ func NewCache(ctx context.Context, cfg Config, isLocalImage func(imageName strin isLocalImage: isLocalImage, importMissingImage: importMissingImage, lister: dependencies, + buildx: buildx, }, nil } diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index f9634dcfda7..346cd99f66d 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -87,6 +87,7 @@ func (c *cache) lookup(ctx context.Context, out io.Writer, a *latest.Artifact, t } if isLocal, err := c.isLocalImage(a.ImageName); err != nil { + log.Entry(ctx).Debugf("isLocalImage failed %w", err) return failed{err} } else if isLocal { return c.lookupLocal(ctx, hash, tag, entry) @@ -122,9 +123,12 @@ func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDe func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform, entry ImageDetails) cacheDetails { if remoteDigest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil { // Image exists remotely with the same tag and digest + log.Entry(ctx).Debugf("RemoteDigest: %s entry.Digest %s", remoteDigest, entry.Digest) if remoteDigest == entry.Digest { return found{hash: hash} } + } else { + log.Entry(ctx).Debugf("RemoteDigest error %w", err) } // Image exists remotely with a different tag @@ -152,23 +156,32 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h return ImageDetails{}, fmt.Errorf("import of missing images disabled") } - if !c.client.ImageExists(ctx, tag) { - log.Entry(ctx).Debugf("Importing artifact %s from docker registry", tag) - err := c.client.Pull(ctx, io.Discard, tag, pl) + // under buildx, docker daemon is not really needed and could be disabled + load := true + if c.buildx { + _, err := c.client.ServerVersion(ctx) + load = err == nil + if !load { + log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %w", err) + } + } + if load { + if !c.client.ImageExists(ctx, tag) { + log.Entry(ctx).Debugf("Importing artifact %s from docker registry", tag) + err := c.client.Pull(ctx, io.Discard, tag, pl) + if err != nil { + return entry, err + } + } else { + log.Entry(ctx).Debugf("Importing artifact %s from local docker", tag) + } + imageID, err := c.client.ImageID(ctx, tag) if err != nil { return entry, err } - } else { - log.Entry(ctx).Debugf("Importing artifact %s from local docker", tag) - } - - imageID, err := c.client.ImageID(ctx, tag) - if err != nil { - return entry, err - } - - if imageID != "" { - entry.ID = imageID + if imageID != "" { + entry.ID = imageID + } } if digest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil { diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 53166e303a3..caca5e9b371 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -19,6 +19,7 @@ package docker import ( "bytes" "context" + "encoding/json" "fmt" "io" "os" @@ -26,6 +27,7 @@ import ( v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/instrumentation" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" @@ -46,7 +48,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, if len(matcher.Platforms) == 1 { pl = util.ConvertToV1Platform(matcher.Platforms[0]) } - a = adjustCacheFrom(a, tag) + a = b.adjustCache(ctx, a, tag) instrumentation.AddAttributesToCurrentSpanFromContext(ctx, map[string]string{ "BuildType": "docker", "Context": instrumentation.PII(a.Workspace), @@ -82,7 +84,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, return "", newBuildError(err, b.cfg) } - if b.pushImages { + if !b.useCLI && b.pushImages { // TODO (tejaldesai) Remove https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/errors/err_map.go#L56 // and instead define a pushErr() method here. return b.localDocker.Push(ctx, out, tag) @@ -120,10 +122,31 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string args = append(args, "--platform", pl.String()) } - if b.useBuildKit != nil && *b.useBuildKit && !b.pushImages { - args = append(args, "--load") + if b.useBuildKit != nil && *b.useBuildKit { + if !b.pushImages { + load := true + if b.buildx { + // if docker daemon is not used, do not try to load the image (a buildx warning will be logged) + _, err := b.localDocker.ServerVersion(ctx) + load = err == nil + } + if load { + args = append(args, "--load") + } + } else if b.buildx { + // with buildx, push the image directly to the registry (not using the docker daemon) + args = append(args, "--push") + } } + // temporary file for buildx metadata containing the image digest: + var metadata *os.File + if b.buildx { + metadata, err = os.CreateTemp("", "metadata.json") + metadata.Close() + defer os.Remove(metadata.Name()) + args = append(args, "--metadata-file", metadata.Name()) + } cmd := exec.CommandContext(ctx, "docker", args...) cmd.Env = append(util.OSEnviron(), b.localDocker.ExtraEnv()...) if b.useBuildKit != nil { @@ -146,11 +169,16 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string return "", tryExecFormatErr(fmt.Errorf("running build: %w", err), errBuffer) } - return b.localDocker.ImageID(ctx, opts.Tag) + if !b.buildx { + return b.localDocker.ImageID(ctx, opts.Tag) + } else { + return getBuildxDigest(ctx, metadata.Name()) + } } func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *latest.DockerArtifact, pl v1.Platform) error { - if len(a.CacheFrom) == 0 { + // when using buildx, avoid pulling as the builder not necessarily uses the local docker daemon + if len(a.CacheFrom) == 0 || b.buildx { return nil } @@ -172,26 +200,81 @@ func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *lat return nil } -// adjustCacheFrom returns an artifact where any cache references from the artifactImage is changed to the tagged built image name instead. -func adjustCacheFrom(a *latest.Artifact, artifactTag string) *latest.Artifact { +// adjustCache returns an artifact where any cache references from the artifactImage is changed to the tagged built image name instead. +// Under buildx, if cache-tag is configured, it will be used instead of the generated artifact tag (registry preserved) +// if no cacheTo was specified in the skaffold yaml, it will add a tagged destination using the same cache source reference +func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactTag string) *latest.Artifact { + cacheRef := a.ImageName // lookup value to be replaced + cacheTag := artifactTag // full reference to be used if os.Getenv("SKAFFOLD_DISABLE_DOCKER_CACHE_ADJUSTMENT") != "" { // allow this behaviour to be disabled return a } - - if !stringslice.Contains(a.DockerArtifact.CacheFrom, a.ImageName) { + if b.buildx { + // compute the full cache reference (including registry, preserving tag) + tag, _ := config.GetCacheTag(b.cfg.GlobalConfig()) + imgRef, err := docker.ParseReference(artifactTag) + if err != nil { + log.Entry(ctx).Errorf("couldn't parse image tag: %w", err) + } else if tag != "" { + cacheTag = fmt.Sprintf("%s/%s:%s", imgRef.Repo, cacheRef, tag) + } + } + if !stringslice.Contains(a.DockerArtifact.CacheFrom, cacheRef) { return a } cf := make([]string, 0, len(a.DockerArtifact.CacheFrom)) + ct := make([]string, 0, len(a.DockerArtifact.CacheTo)) for _, image := range a.DockerArtifact.CacheFrom { - if image == a.ImageName { - cf = append(cf, artifactTag) + if image == cacheRef { + // change cache reference to to the tagged image name (built or given, including registry) + log.Entry(ctx).Debugf("Adjusting cache source image ref: %s", cacheTag) + cf = append(cf, cacheTag) + if b.buildx { + // add cache destination reference, only if we're pushing to a registry and not given in config + if len(a.DockerArtifact.CacheTo) == 0 && b.pushImages { + log.Entry(ctx).Debugf("Adjusting cache destination image ref: %s", cacheTag) + ct = append(ct, fmt.Sprintf("type=registry,ref=%s,mode=max", cacheTag)) + } + } } else { cf = append(cf, image) } } + // just copy any other cache destination given in the config file: + for _, image := range a.DockerArtifact.CacheTo { + ct = append(cf, image) + } copy := *a copy.DockerArtifact.CacheFrom = cf + copy.DockerArtifact.CacheTo = ct return © } + +func getBuildxDigest(ctx context.Context, filename string) (string, error) { + var metadata map[string]interface{} + data, err := os.ReadFile(filename) + if err == nil { + err = json.Unmarshal(data, &metadata) + } + if err == nil { + // avoid panic: interface conversion: interface {} is nil, not string (if keys don't exists) + var digest string + if value := metadata["containerimage.digest"]; value != nil { + digest = value.(string) + } + var name string + if value := metadata["image.name"]; value != nil { + name = value.(string) + } + if digest != "" { + log.Entry(ctx).Debugf("Image digest found in buildx metadata: %s for %s", digest, name) + return digest, nil + } + } + log.Entry(ctx).Warnf("No digest found in buildx metadata: %w", err) + // if image is not pushed, it could not contain the digest log for debugging: + log.Entry(ctx).Debugf("Full buildx metadata: %s", data) + return "", err +} diff --git a/pkg/skaffold/build/docker/types.go b/pkg/skaffold/build/docker/types.go index 00ad02ff680..68ad789f6d2 100644 --- a/pkg/skaffold/build/docker/types.go +++ b/pkg/skaffold/build/docker/types.go @@ -30,6 +30,7 @@ type Builder struct { pushImages bool useCLI bool useBuildKit *bool + buildx bool artifacts ArtifactResolver sourceDependencies TransitiveSourceDependenciesResolver } @@ -45,13 +46,14 @@ type TransitiveSourceDependenciesResolver interface { } // NewBuilder returns an new instance of a docker builder -func NewArtifactBuilder(localDocker docker.LocalDaemon, cfg docker.Config, useCLI bool, useBuildKit *bool, pushImages bool, ar ArtifactResolver, dr TransitiveSourceDependenciesResolver) *Builder { +func NewArtifactBuilder(localDocker docker.LocalDaemon, cfg docker.Config, useCLI bool, useBuildKit *bool, buildx bool, pushImages bool, ar ArtifactResolver, dr TransitiveSourceDependenciesResolver) *Builder { return &Builder{ localDocker: localDocker, pushImages: pushImages, cfg: cfg, useCLI: useCLI, useBuildKit: useBuildKit, + buildx: buildx, artifacts: ar, sourceDependencies: dr, } diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 368894b367b..8a9f061e5bc 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -89,9 +89,10 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Ar if b.pushImages { // only track images for pruning when building with docker // if we're pushing a bazel image, it was built directly to the registry + // buildx also has its own build cache, and image load will not be attempted if no docker daemon is accessible if a.DockerArtifact != nil { imageID, err := b.getImageIDForTag(ctx, tag) - if err != nil { + if err != nil && !b.buildx { log.Entry(ctx).Warn("unable to inspect image: built images may not be cleaned up correctly by skaffold") } if imageID != "" { @@ -103,6 +104,8 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Ar return build.TagWithDigest(tag, digest), nil } + // TODO: prune buildx cache using digest? + imageID := digestOrImageID if b.mode == config.RunModes.Dev { artifacts, err := b.artifactStore.GetArtifacts([]*latest.Artifact{a}) @@ -123,8 +126,8 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Ar } func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, tag string, platforms platform.Matcher) (string, error) { - if !b.pushImages { - // All of the builders will rely on a local Docker: + if !b.buildx && !b.pushImages { + // Most builders will rely on a local Docker (except when using a remote buildkit via buildx): // + Either to build the image, // + Or to docker load it. // Let's fail fast if Docker is not available diff --git a/pkg/skaffold/build/local/types.go b/pkg/skaffold/build/local/types.go index fef419ae43a..1a4f07407ab 100644 --- a/pkg/skaffold/build/local/types.go +++ b/pkg/skaffold/build/local/types.go @@ -52,6 +52,7 @@ type Builder struct { skipTests bool mode config.RunMode kubeContext string + buildx bool builtImages []string insecureRegistries map[string]bool muted build.Muted @@ -66,6 +67,7 @@ type Config interface { GlobalConfig() string GetKubeContext() string GetCluster() config.Cluster + DetectBuildX() bool SkipTests() bool Mode() config.RunMode NoPruneChildren() bool @@ -103,6 +105,9 @@ func NewBuilder(ctx context.Context, bCtx BuilderContext, buildCfg *latest.Local tryImportMissing := buildCfg.TryImportMissing + // for backward compatibility, extended build capabilities with BuildKit are disabled by default + buildx := bCtx.DetectBuildX() + return &Builder{ local: *buildCfg, cfg: bCtx, @@ -111,6 +116,7 @@ func NewBuilder(ctx context.Context, bCtx BuilderContext, buildCfg *latest.Local localCluster: cluster.Local, pushImages: pushImages, tryImportMissing: tryImportMissing, + buildx: buildx, skipTests: bCtx.SkipTests(), mode: bCtx.Mode(), prune: bCtx.Prune(), @@ -133,7 +139,7 @@ type artifactBuilder interface { func newPerArtifactBuilder(b *Builder, a *latest.Artifact) (artifactBuilder, error) { switch { case a.DockerArtifact != nil: - return dockerbuilder.NewArtifactBuilder(b.localDocker, b.cfg, b.local.UseDockerCLI, b.local.UseBuildkit, b.pushImages, b.artifactStore, b.sourceDependencies), nil + return dockerbuilder.NewArtifactBuilder(b.localDocker, b.cfg, b.local.UseDockerCLI, b.local.UseBuildkit, b.buildx, b.pushImages, b.artifactStore, b.sourceDependencies), nil case a.BazelArtifact != nil: return bazel.NewArtifactBuilder(b.localDocker, b.cfg, b.pushImages), nil diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go index a0dffaaae6f..4d7a754910e 100644 --- a/pkg/skaffold/config/global_config.go +++ b/pkg/skaffold/config/global_config.go @@ -33,6 +33,8 @@ type ContextConfig struct { InsecureRegistries []string `yaml:"insecure-registries,omitempty"` // DebugHelpersRegistry is the registry from which the debug helper images are used. DebugHelpersRegistry string `yaml:"debug-helpers-registry,omitempty"` + CacheTag string `yaml:"cache-tag,omitempty"` + DetectBuildX *bool `yaml:"detect-buildx,omitempty"` UpdateCheck *bool `yaml:"update-check,omitempty"` Survey *SurveyConfig `yaml:"survey,omitempty"` KindDisableLoad *bool `yaml:"kind-disable-load,omitempty"` diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index bd8ecb647cc..e020ee376a3 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -217,6 +217,29 @@ func GetDebugHelpersRegistry(configFile string) (string, error) { return constants.DefaultDebugHelpersRegistry, nil } +func GetCacheTag(configFile string) (string, error) { + cfg, err := GetConfigForCurrentKubectx(configFile) + if err != nil { + log.Entry(context.TODO()).Errorf("Cannot read cache-tag from config: %w", err) + return "", err + } + if cfg.CacheTag != "" { + log.Entry(context.TODO()).Infof("Using cache-tag=%s from config", cfg.CacheTag) + } + return cfg.CacheTag, nil +} + +func GetDetectBuildX(configFile string) bool { + cfg, err := GetConfigForCurrentKubectx(configFile) + if err != nil { + log.Entry(context.TODO()).Errorf("Cannot read detect-buildx option from config: %w", err) + } else if cfg.DetectBuildX != nil { + log.Entry(context.TODO()).Infof("Using detect-buildx=%s from config", *cfg.DetectBuildX) + return *cfg.DetectBuildX + } + return false +} + type GetClusterOpts struct { ConfigFile string DefaultRepo StringOrUndefined diff --git a/pkg/skaffold/docker/auth.go b/pkg/skaffold/docker/auth.go index f2f57745dcf..fec519a6489 100644 --- a/pkg/skaffold/docker/auth.go +++ b/pkg/skaffold/docker/auth.go @@ -64,14 +64,28 @@ type AuthConfigHelper interface { type credsHelper struct{} -func loadDockerConfig() (*configfile.ConfigFile, error) { +func LoadDockerConfig(access bool) (*configfile.ConfigFile, error) { cf, err := config.Load(configDir) + if err == nil && access { + // proper check of config file to detect permissions errors + if _, err = os.Stat(cf.Filename); err == nil { + var file *os.File + file, err = os.Open(cf.Filename) + defer file.Close() + } + if err != nil { + log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %w", configDir, err) + } + } + return cf, err +} + +func loadDockerConfig() (*configfile.ConfigFile, error) { + cf, err := LoadDockerConfig(false) if err != nil { return nil, fmt.Errorf("docker config: %w", err) } - gcp.AutoConfigureGCRCredentialHelper(cf) - return cf, nil } diff --git a/pkg/skaffold/docker/buildx.go b/pkg/skaffold/docker/buildx.go new file mode 100644 index 00000000000..29498322fda --- /dev/null +++ b/pkg/skaffold/docker/buildx.go @@ -0,0 +1,27 @@ +/* +Copyright 2019 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package docker + +func DetectBuildX() bool { + // detect if buildx was installed (docker build then is an alias for docker buildx build): + // https://github.com/docker/buildx/blob/master/commands/install.go + cf, err := LoadDockerConfig(true) + if err == nil && cf.Aliases != nil { + return cf.Aliases["builder"] == "buildx" + } + return false +} diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index 02455ea3495..b448e3b94d8 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -653,6 +653,10 @@ func ToCLIBuildArgs(a *latest.DockerArtifact, evaluatedArgs map[string]*string, args = append(args, "--cache-from", from) } + for _, to := range a.CacheTo { + args = append(args, "--cache-to", to) + } + for _, cliFlag := range a.CliFlags { cliFlag, err := util.ExpandEnvTemplate(cliFlag, env) if err != nil { diff --git a/pkg/skaffold/runner/runcontext/context.go b/pkg/skaffold/runner/runcontext/context.go index cad809bb092..b90558df3ff 100644 --- a/pkg/skaffold/runner/runcontext/context.go +++ b/pkg/skaffold/runner/runcontext/context.go @@ -27,6 +27,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/constants" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker" kubectx "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/context" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" @@ -364,6 +365,17 @@ func (rc *RunContext) EnableGKEARMNodeTolerationInRenderedManifests() bool { return rc.Opts.EnableGKEARMNodeToleration } +func (rc *RunContext) DetectBuildX() bool { + if config.GetDetectBuildX(rc.GlobalConfig()) { + buildx := docker.DetectBuildX() + log.Entry(context.TODO()).Debugf("buildx detection result is %t", buildx) + return buildx + } else { + log.Entry(context.TODO()).Debugf("buildx detection is disabled") + return false + } +} + func (rc *RunContext) DigestSource() string { if rc.Opts.DigestSource != "" { return rc.Opts.DigestSource diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 860efb7f673..3c2cc71867b 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -1589,6 +1589,11 @@ type DockerArtifact struct { // For example: `["golang:1.10.1-alpine3.7", "alpine:3.7"]`. CacheFrom []string `yaml:"cacheFrom,omitempty"` + // CacheTo lists the Docker images used as cache destination. + // If ommited, cacheFrom is used with max mode to export all layers + // For example: `["type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max"]`. + CacheTo []string `yaml:"cacheTo,omitempty"` + // CliFlags are any additional flags to pass to the local daemon during a build. // These flags are only used during a build through the Docker CLI. CliFlags []string `yaml:"cliFlags,omitempty"` From ade44ad4dab66cad84ee5103e55fd2098db7871d Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 13 Jan 2025 19:09:04 -0600 Subject: [PATCH 02/38] fix warnings and import cycle (UT/integration) --- cmd/skaffold/app/cmd/config/set_test.go | 2 +- docs-v2/content/en/schemas/v4beta12.json | 13 +++++++++++++ pkg/skaffold/build/cache/cache.go | 2 +- pkg/skaffold/build/cache/lookup.go | 6 +++--- pkg/skaffold/build/docker/docker.go | 7 +++++-- pkg/skaffold/build/docker/docker_test.go | 4 ++-- pkg/skaffold/build/docker/errors_test.go | 2 +- pkg/skaffold/build/local/types.go | 2 +- pkg/skaffold/config/util.go | 6 +++--- pkg/skaffold/docker/auth.go | 6 ++++-- pkg/skaffold/docker/buildx.go | 2 +- pkg/skaffold/runner/runcontext/context.go | 6 ++---- pkg/skaffold/schema/latest/config.go | 2 +- 13 files changed, 38 insertions(+), 22 deletions(-) diff --git a/cmd/skaffold/app/cmd/config/set_test.go b/cmd/skaffold/app/cmd/config/set_test.go index b34845cfd59..48510a57ebb 100644 --- a/cmd/skaffold/app/cmd/config/set_test.go +++ b/cmd/skaffold/app/cmd/config/set_test.go @@ -415,7 +415,7 @@ func TestGetConfigStructWithIndex(t *testing.T) { description: "survey flag set", cfg: &config.ContextConfig{}, survey: true, - expectedIdx: []int{7}, + expectedIdx: []int{9}, }, { description: "no survey flag set", diff --git a/docs-v2/content/en/schemas/v4beta12.json b/docs-v2/content/en/schemas/v4beta12.json index b8bad3b0334..c30694a2983 100755 --- a/docs-v2/content/en/schemas/v4beta12.json +++ b/docs-v2/content/en/schemas/v4beta12.json @@ -1766,6 +1766,18 @@ "[\"golang:1.10.1-alpine3.7\", \"alpine:3.7\"]" ] }, + "cacheTo": { + "items": { + "type": "string" + }, + "type": "array", + "description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.", + "x-intellij-html-description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.", + "default": "[]", + "examples": [ + "[\"type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max\"]" + ] + }, "cliFlags": { "items": { "type": "string" @@ -1830,6 +1842,7 @@ "network", "addHost", "cacheFrom", + "cacheTo", "cliFlags", "pullParent", "noCache", diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index d29a4a6a995..a05c88aae99 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -121,7 +121,7 @@ func NewCache(ctx context.Context, cfg Config, isLocalImage func(imageName strin } // for backward compatibility, extended build capabilities with BuildKit are disabled by default - buildx := cfg.DetectBuildX() + buildx := cfg.DetectBuildX() && docker.IsBuildXDetected() return &cache{ artifactCache: artifactCache, diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index 346cd99f66d..d7f2fc0d952 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -87,7 +87,7 @@ func (c *cache) lookup(ctx context.Context, out io.Writer, a *latest.Artifact, t } if isLocal, err := c.isLocalImage(a.ImageName); err != nil { - log.Entry(ctx).Debugf("isLocalImage failed %w", err) + log.Entry(ctx).Debugf("isLocalImage failed %v", err) return failed{err} } else if isLocal { return c.lookupLocal(ctx, hash, tag, entry) @@ -128,7 +128,7 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms [] return found{hash: hash} } } else { - log.Entry(ctx).Debugf("RemoteDigest error %w", err) + log.Entry(ctx).Debugf("RemoteDigest error %v", err) } // Image exists remotely with a different tag @@ -162,7 +162,7 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h _, err := c.client.ServerVersion(ctx) load = err == nil if !load { - log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %w", err) + log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %v", err) } } if load { diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index caca5e9b371..455195dcd5e 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -143,6 +143,9 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string var metadata *os.File if b.buildx { metadata, err = os.CreateTemp("", "metadata.json") + if err != nil { + return "", fmt.Errorf("unable to create temp file: %w", err) + } metadata.Close() defer os.Remove(metadata.Name()) args = append(args, "--metadata-file", metadata.Name()) @@ -215,7 +218,7 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT tag, _ := config.GetCacheTag(b.cfg.GlobalConfig()) imgRef, err := docker.ParseReference(artifactTag) if err != nil { - log.Entry(ctx).Errorf("couldn't parse image tag: %w", err) + log.Entry(ctx).Errorf("couldn't parse image tag: %v", err) } else if tag != "" { cacheTag = fmt.Sprintf("%s/%s:%s", imgRef.Repo, cacheRef, tag) } @@ -273,7 +276,7 @@ func getBuildxDigest(ctx context.Context, filename string) (string, error) { return digest, nil } } - log.Entry(ctx).Warnf("No digest found in buildx metadata: %w", err) + log.Entry(ctx).Warnf("No digest found in buildx metadata: %v", err) // if image is not pushed, it could not contain the digest log for debugging: log.Entry(ctx).Debugf("Full buildx metadata: %s", data) return "", err diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go index 509e3a1804f..cfa739128c1 100644 --- a/pkg/skaffold/build/docker/docker_test.go +++ b/pkg/skaffold/build/docker/docker_test.go @@ -187,7 +187,7 @@ func TestDockerCLIBuild(t *testing.T) { } t.Override(&util.OSEnviron, func() []string { return []string{"KEY=VALUE"} }) - builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, mockArtifactResolver{make(map[string]string)}, nil) + builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, false, mockArtifactResolver{make(map[string]string)}, nil) artifact := &latest.Artifact{ Workspace: ".", @@ -268,7 +268,7 @@ func TestDockerCLICheckCacheFromArgs(t *testing.T) { ) t.Override(&util.DefaultExecCommand, mockCmd) - builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, mockArtifactResolver{make(map[string]string)}, nil) + builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, false, mockArtifactResolver{make(map[string]string)}, nil) _, err := builder.Build(context.Background(), io.Discard, &a, test.tag, platform.Matcher{}) t.CheckNoError(err) }) diff --git a/pkg/skaffold/build/docker/errors_test.go b/pkg/skaffold/build/docker/errors_test.go index 32686d38117..1138142c00c 100644 --- a/pkg/skaffold/build/docker/errors_test.go +++ b/pkg/skaffold/build/docker/errors_test.go @@ -60,7 +60,7 @@ Refer https://skaffold.dev/docs/references/yaml/#build-artifacts-docker for deta "docker build . --file "+dockerfilePath+" -t tag", )) t.Override(&docker.DefaultAuthHelper, stubAuth{}) - builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, nil, false, mockArtifactResolver{make(map[string]string)}, nil) + builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, nil, false, false, mockArtifactResolver{make(map[string]string)}, nil) artifact := &latest.Artifact{ ImageName: "test-image", diff --git a/pkg/skaffold/build/local/types.go b/pkg/skaffold/build/local/types.go index 1a4f07407ab..89ddf0f8b7e 100644 --- a/pkg/skaffold/build/local/types.go +++ b/pkg/skaffold/build/local/types.go @@ -106,7 +106,7 @@ func NewBuilder(ctx context.Context, bCtx BuilderContext, buildCfg *latest.Local tryImportMissing := buildCfg.TryImportMissing // for backward compatibility, extended build capabilities with BuildKit are disabled by default - buildx := bCtx.DetectBuildX() + buildx := bCtx.DetectBuildX() && docker.IsBuildXDetected() return &Builder{ local: *buildCfg, diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index e020ee376a3..42ae8ffe083 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -220,7 +220,7 @@ func GetDebugHelpersRegistry(configFile string) (string, error) { func GetCacheTag(configFile string) (string, error) { cfg, err := GetConfigForCurrentKubectx(configFile) if err != nil { - log.Entry(context.TODO()).Errorf("Cannot read cache-tag from config: %w", err) + log.Entry(context.TODO()).Errorf("Cannot read cache-tag from config: %v", err) return "", err } if cfg.CacheTag != "" { @@ -232,9 +232,9 @@ func GetCacheTag(configFile string) (string, error) { func GetDetectBuildX(configFile string) bool { cfg, err := GetConfigForCurrentKubectx(configFile) if err != nil { - log.Entry(context.TODO()).Errorf("Cannot read detect-buildx option from config: %w", err) + log.Entry(context.TODO()).Errorf("Cannot read detect-buildx option from config: %v", err) } else if cfg.DetectBuildX != nil { - log.Entry(context.TODO()).Infof("Using detect-buildx=%s from config", *cfg.DetectBuildX) + log.Entry(context.TODO()).Infof("Using detect-buildx=%t from config", *cfg.DetectBuildX) return *cfg.DetectBuildX } return false diff --git a/pkg/skaffold/docker/auth.go b/pkg/skaffold/docker/auth.go index fec519a6489..8b00e8f593a 100644 --- a/pkg/skaffold/docker/auth.go +++ b/pkg/skaffold/docker/auth.go @@ -71,10 +71,12 @@ func LoadDockerConfig(access bool) (*configfile.ConfigFile, error) { if _, err = os.Stat(cf.Filename); err == nil { var file *os.File file, err = os.Open(cf.Filename) - defer file.Close() + if err == nil { + defer file.Close() + } } if err != nil { - log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %w", configDir, err) + log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %v", configDir, err) } } return cf, err diff --git a/pkg/skaffold/docker/buildx.go b/pkg/skaffold/docker/buildx.go index 29498322fda..784cc9f65ce 100644 --- a/pkg/skaffold/docker/buildx.go +++ b/pkg/skaffold/docker/buildx.go @@ -16,7 +16,7 @@ limitations under the License. package docker -func DetectBuildX() bool { +func IsBuildXDetected() bool { // detect if buildx was installed (docker build then is an alias for docker buildx build): // https://github.com/docker/buildx/blob/master/commands/install.go cf, err := LoadDockerConfig(true) diff --git a/pkg/skaffold/runner/runcontext/context.go b/pkg/skaffold/runner/runcontext/context.go index b90558df3ff..544f48cf494 100644 --- a/pkg/skaffold/runner/runcontext/context.go +++ b/pkg/skaffold/runner/runcontext/context.go @@ -27,7 +27,6 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/constants" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker" kubectx "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/context" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" @@ -367,9 +366,8 @@ func (rc *RunContext) EnableGKEARMNodeTolerationInRenderedManifests() bool { func (rc *RunContext) DetectBuildX() bool { if config.GetDetectBuildX(rc.GlobalConfig()) { - buildx := docker.DetectBuildX() - log.Entry(context.TODO()).Debugf("buildx detection result is %t", buildx) - return buildx + log.Entry(context.TODO()).Debugf("buildx detection is enabled") + return true } else { log.Entry(context.TODO()).Debugf("buildx detection is disabled") return false diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 3c2cc71867b..b102715ec1f 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -1590,7 +1590,7 @@ type DockerArtifact struct { CacheFrom []string `yaml:"cacheFrom,omitempty"` // CacheTo lists the Docker images used as cache destination. - // If ommited, cacheFrom is used with max mode to export all layers + // If omitted, cacheFrom is used with max mode to export all layers. // For example: `["type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max"]`. CacheTo []string `yaml:"cacheTo,omitempty"` From 44fb72ca35a7988d56865d00eb746543a53cc6c5 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Fri, 17 Jan 2025 15:59:00 -0600 Subject: [PATCH 03/38] fix docker build UTs --- pkg/skaffold/build/docker/docker.go | 29 +++++++++---- pkg/skaffold/build/docker/docker_test.go | 54 +++++++++++++++++++++--- pkg/skaffold/build/docker/errors_test.go | 2 +- 3 files changed, 70 insertions(+), 15 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 455195dcd5e..f8845eb600b 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -84,7 +84,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, return "", newBuildError(err, b.cfg) } - if !b.useCLI && b.pushImages { + if !b.useCLI && b.pushImages && !b.buildx { // TODO (tejaldesai) Remove https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/errors/err_map.go#L56 // and instead define a pushErr() method here. return b.localDocker.Push(ctx, out, tag) @@ -140,15 +140,14 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string } // temporary file for buildx metadata containing the image digest: - var metadata *os.File + var metadata string if b.buildx { - metadata, err = os.CreateTemp("", "metadata.json") + metadata, err = getBuildxMetadataFile() if err != nil { return "", fmt.Errorf("unable to create temp file: %w", err) } - metadata.Close() - defer os.Remove(metadata.Name()) - args = append(args, "--metadata-file", metadata.Name()) + defer os.Remove(metadata) + args = append(args, "--metadata-file", metadata) } cmd := exec.CommandContext(ctx, "docker", args...) cmd.Env = append(util.OSEnviron(), b.localDocker.ExtraEnv()...) @@ -175,7 +174,7 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string if !b.buildx { return b.localDocker.ImageID(ctx, opts.Tag) } else { - return getBuildxDigest(ctx, metadata.Name()) + return parseBuildxMetadataFile(ctx, metadata) } } @@ -220,7 +219,7 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT if err != nil { log.Entry(ctx).Errorf("couldn't parse image tag: %v", err) } else if tag != "" { - cacheTag = fmt.Sprintf("%s/%s:%s", imgRef.Repo, cacheRef, tag) + cacheTag = fmt.Sprintf("%s:%s", imgRef.BaseName, tag) } } if !stringslice.Contains(a.DockerArtifact.CacheFrom, cacheRef) { @@ -255,7 +254,19 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT return © } -func getBuildxDigest(ctx context.Context, filename string) (string, error) { +// osCreateTemp allows for replacing metadata for testing purposes +var osCreateTemp = os.CreateTemp + +func getBuildxMetadataFile() (string, error) { + metadata, err := osCreateTemp("", "metadata*.json") + if err != nil { + return "", err + } + metadata.Close() + return metadata.Name(), nil +} + +func parseBuildxMetadataFile(ctx context.Context, filename string) (string, error) { var metadata map[string]interface{} data, err := os.ReadFile(filename) if err == nil { diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go index cfa739128c1..3f6e61da0cd 100644 --- a/pkg/skaffold/build/docker/docker_test.go +++ b/pkg/skaffold/build/docker/docker_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "io" + "os" "path/filepath" "strings" "testing" @@ -38,6 +39,13 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/testutil" ) +var metadata = `{ + "buildx.build.ref": "default/default/8tmdcf9mpy43arexbwej851qz", + "containerimage.config.digest": "sha256:0bb1817818c840e89daa8308168ac33d0d857faed1f6810638cc26169b8d3b13", + "containerimage.digest": "sha256:0bb1817818c840e77afa8308168ac33d0d23adaee1f6810638cc26169b8d3b13", + "image.name": "docker.io/library/test" +}` + func TestDockerCLIBuild(t *testing.T) { tests := []struct { description string @@ -51,6 +59,8 @@ func TestDockerCLIBuild(t *testing.T) { expectedCLIFlags []string // CLI flags expected to be autogenerated. expectedErr error wantDockerCLI bool + buildx bool // buildx detected + daemonless bool // no docker daemon expectedErrCode proto.StatusCode }{ { @@ -79,6 +89,26 @@ func TestDockerCLIBuild(t *testing.T) { expectedCLIFlags: []string{"--load"}, expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, }, + { + description: "buildkit buildx load", + localBuild: latest.LocalBuild{UseBuildkit: util.Ptr(true)}, + wantDockerCLI: true, + buildx: true, + daemonless: false, + imageName: "gcr.io/k8s-skaffold/example:tag", + expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--load", "--metadata-file", "metadata.json"}, + expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, + }, + { + description: "buildkit buildx push", + localBuild: latest.LocalBuild{UseBuildkit: util.Ptr(true), Push: util.Ptr(true)}, + wantDockerCLI: true, + buildx: true, + daemonless: true, + imageName: "gcr.io/k8s-skaffold/example:tag", + expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max", "--push", "--metadata-file", "metadata.json"}, + expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, + }, { description: "cliFlags", cliFlags: []string{"--platform", "linux/amd64"}, @@ -159,6 +189,14 @@ func TestDockerCLIBuild(t *testing.T) { return args, nil }) t.Override(&docker.DefaultAuthHelper, stubAuth{}) + t.Override(&osCreateTemp, func(dir, pattern string) (*os.File, error) { + tmp := t.TempFile("metadata*.json", []byte(metadata)) + os.Rename(tmp, "metadata.json") + return os.Open("metadata.json") + }) + t.Override(&config.GetConfigForCurrentKubectx, func(configFile string) (*config.ContextConfig, error) { + return &config.ContextConfig{CacheTag: "cache"}, nil + }) var mockCmd *testutil.FakeCmd imageName := "tag" @@ -187,7 +225,7 @@ func TestDockerCLIBuild(t *testing.T) { } t.Override(&util.OSEnviron, func() []string { return []string{"KEY=VALUE"} }) - builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, false, mockArtifactResolver{make(map[string]string)}, nil) + builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv, test.daemonless), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, test.buildx, test.buildx && test.localBuild.Push != nil && *test.localBuild.Push, mockArtifactResolver{make(map[string]string)}, nil) artifact := &latest.Artifact{ Workspace: ".", @@ -198,9 +236,15 @@ func TestDockerCLIBuild(t *testing.T) { }, }, } + if test.buildx { + parts := strings.Split(imageName, ":") + artifact.ImageName = parts[0] + artifact.DockerArtifact.CacheFrom = []string{parts[0]} + } - _, err := builder.Build(context.Background(), io.Discard, artifact, imageName, platform.Matcher{}) + digest, err := builder.Build(context.Background(), io.Discard, artifact, imageName, platform.Matcher{}) t.CheckError(test.err != nil, err) + t.CheckTrue(digest != "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") // not empty string hash if mockCmd != nil { t.CheckDeepEqual(1, mockCmd.TimesCalled()) } @@ -268,15 +312,15 @@ func TestDockerCLICheckCacheFromArgs(t *testing.T) { ) t.Override(&util.DefaultExecCommand, mockCmd) - builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, false, mockArtifactResolver{make(map[string]string)}, nil) + builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}, false), mockConfig{}, true, util.Ptr(false), false, false, mockArtifactResolver{make(map[string]string)}, nil) _, err := builder.Build(context.Background(), io.Discard, &a, test.tag, platform.Matcher{}) t.CheckNoError(err) }) } } -func fakeLocalDaemonWithExtraEnv(extraEnv []string) docker.LocalDaemon { - return docker.NewLocalDaemon(&testutil.FakeAPIClient{}, extraEnv, false, nil) +func fakeLocalDaemonWithExtraEnv(extraEnv []string, daemonless bool) docker.LocalDaemon { + return docker.NewLocalDaemon(&testutil.FakeAPIClient{ErrVersion: daemonless}, extraEnv, false, nil) } type mockArtifactResolver struct { diff --git a/pkg/skaffold/build/docker/errors_test.go b/pkg/skaffold/build/docker/errors_test.go index 1138142c00c..c6ece2c2358 100644 --- a/pkg/skaffold/build/docker/errors_test.go +++ b/pkg/skaffold/build/docker/errors_test.go @@ -60,7 +60,7 @@ Refer https://skaffold.dev/docs/references/yaml/#build-artifacts-docker for deta "docker build . --file "+dockerfilePath+" -t tag", )) t.Override(&docker.DefaultAuthHelper, stubAuth{}) - builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, nil, false, false, mockArtifactResolver{make(map[string]string)}, nil) + builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}, false), mockConfig{}, true, nil, false, false, mockArtifactResolver{make(map[string]string)}, nil) artifact := &latest.Artifact{ ImageName: "test-image", From 0611204e359134cb3c5275deba3e74a5071d237c Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Fri, 17 Jan 2025 20:51:49 -0600 Subject: [PATCH 04/38] refactor global config to allow buildx builders --- pkg/skaffold/build/docker/docker.go | 4 ++++ pkg/skaffold/build/docker/docker_test.go | 6 +++--- pkg/skaffold/config/global_config.go | 2 +- pkg/skaffold/config/util.go | 16 ++++++++++------ 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index f8845eb600b..7680691f468 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -139,6 +139,10 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string } } + if b.buildx { + args = append(args, "--builder", config.GetBuildXBuilder(b.cfg.GlobalConfig())) + } + // temporary file for buildx metadata containing the image digest: var metadata string if b.buildx { diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go index 3f6e61da0cd..bac6b66b91f 100644 --- a/pkg/skaffold/build/docker/docker_test.go +++ b/pkg/skaffold/build/docker/docker_test.go @@ -96,7 +96,7 @@ func TestDockerCLIBuild(t *testing.T) { buildx: true, daemonless: false, imageName: "gcr.io/k8s-skaffold/example:tag", - expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--load", "--metadata-file", "metadata.json"}, + expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--load", "--builder", "default", "--metadata-file", "metadata.json"}, expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, }, { @@ -106,7 +106,7 @@ func TestDockerCLIBuild(t *testing.T) { buildx: true, daemonless: true, imageName: "gcr.io/k8s-skaffold/example:tag", - expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max", "--push", "--metadata-file", "metadata.json"}, + expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max", "--push", "--builder", "default", "--metadata-file", "metadata.json"}, expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, }, { @@ -195,7 +195,7 @@ func TestDockerCLIBuild(t *testing.T) { return os.Open("metadata.json") }) t.Override(&config.GetConfigForCurrentKubectx, func(configFile string) (*config.ContextConfig, error) { - return &config.ContextConfig{CacheTag: "cache"}, nil + return &config.ContextConfig{CacheTag: "cache", BuildXBuilder: "default"}, nil }) var mockCmd *testutil.FakeCmd diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go index 4d7a754910e..872e21b222e 100644 --- a/pkg/skaffold/config/global_config.go +++ b/pkg/skaffold/config/global_config.go @@ -34,7 +34,7 @@ type ContextConfig struct { // DebugHelpersRegistry is the registry from which the debug helper images are used. DebugHelpersRegistry string `yaml:"debug-helpers-registry,omitempty"` CacheTag string `yaml:"cache-tag,omitempty"` - DetectBuildX *bool `yaml:"detect-buildx,omitempty"` + BuildXBuilder string `yaml:"buildx-builder,omitempty"` UpdateCheck *bool `yaml:"update-check,omitempty"` Survey *SurveyConfig `yaml:"survey,omitempty"` KindDisableLoad *bool `yaml:"kind-disable-load,omitempty"` diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index 42ae8ffe083..f588fded0d6 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -229,15 +229,19 @@ func GetCacheTag(configFile string) (string, error) { return cfg.CacheTag, nil } -func GetDetectBuildX(configFile string) bool { +func GetBuildXBuilder(configFile string) string { cfg, err := GetConfigForCurrentKubectx(configFile) if err != nil { - log.Entry(context.TODO()).Errorf("Cannot read detect-buildx option from config: %v", err) - } else if cfg.DetectBuildX != nil { - log.Entry(context.TODO()).Infof("Using detect-buildx=%t from config", *cfg.DetectBuildX) - return *cfg.DetectBuildX + log.Entry(context.TODO()).Errorf("Cannot read buildx-builder option from config: %v", err) + } else if cfg.BuildXBuilder != "" { + log.Entry(context.TODO()).Infof("Using buildx-builder=%s from config", cfg.BuildXBuilder) + return cfg.BuildXBuilder } - return false + return "" +} + +func GetDetectBuildX(configFile string) bool { + return GetBuildXBuilder(configFile) != "" } type GetClusterOpts struct { From fd74681fa878d6e992e093525e2ea85162f60ef9 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Fri, 17 Jan 2025 22:55:02 -0600 Subject: [PATCH 05/38] fix windows UT --- pkg/skaffold/build/docker/docker_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go index bac6b66b91f..05f3541810b 100644 --- a/pkg/skaffold/build/docker/docker_test.go +++ b/pkg/skaffold/build/docker/docker_test.go @@ -191,7 +191,7 @@ func TestDockerCLIBuild(t *testing.T) { t.Override(&docker.DefaultAuthHelper, stubAuth{}) t.Override(&osCreateTemp, func(dir, pattern string) (*os.File, error) { tmp := t.TempFile("metadata*.json", []byte(metadata)) - os.Rename(tmp, "metadata.json") + os.Symlink(tmp, "metadata.json") // rename doesn't work on windows (file is still open) return os.Open("metadata.json") }) t.Override(&config.GetConfigForCurrentKubectx, func(configFile string) (*config.ContextConfig, error) { From c608380ca9cea7fb2c1f5ce3874f8f835b79cfb3 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sat, 18 Jan 2025 22:07:14 -0600 Subject: [PATCH 06/38] buildx integration test --- integration/build_test.go | 53 +++++++++++++++++++++++ integration/testdata/buildx/Dockerfile | 12 +++++ integration/testdata/buildx/config | 9 ++++ integration/testdata/buildx/main.go | 14 ++++++ integration/testdata/buildx/skaffold.yaml | 12 +++++ pkg/skaffold/build/docker/docker.go | 2 +- 6 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 integration/testdata/buildx/Dockerfile create mode 100644 integration/testdata/buildx/config create mode 100644 integration/testdata/buildx/main.go create mode 100644 integration/testdata/buildx/skaffold.yaml diff --git a/integration/build_test.go b/integration/build_test.go index eb46951049f..6c9724a2776 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -53,6 +53,12 @@ func TestBuild(t *testing.T) { description: "docker build", dir: "testdata/build", }, + { + setup: setupBuildX, + description: "docker buildx", + args: []string{"--config", "config", "--verbosity=trace"}, + dir: "testdata/buildx", + }, { description: "git tagger", dir: "testdata/tagPolicy", @@ -131,6 +137,7 @@ func TestBuildWithWithPlatform(t *testing.T) { dir string args []string image string + setup func(t *testing.T, workdir string) expectedPlatforms []v1.Platform }{ { @@ -145,11 +152,28 @@ func TestBuildWithWithPlatform(t *testing.T) { args: []string{"--platform", "linux/arm64"}, expectedPlatforms: []v1.Platform{{OS: "linux", Architecture: "arm64"}}, }, + { + setup: setupBuildX, + description: "docker buildx linux/amd64", + dir: "testdata/buildx", + args: []string{"--platform", "linux/amd64", "--cache-artifacts=false", "--config", "config", "--verbosity=trace"}, + expectedPlatforms: []v1.Platform{{OS: "linux", Architecture: "amd64"}}, + }, + { + setup: setupBuildX, + description: "docker buildx linux/arm64", + dir: "testdata/buildx", + args: []string{"--platform", "linux/arm64", "--cache-artifacts=false", "--config", "config", "--verbosity=trace"}, + expectedPlatforms: []v1.Platform{{OS: "linux", Architecture: "arm64"}}, + }, } for _, test := range tests { t.Run(test.description, func(t *testing.T) { MarkIntegrationTest(t, CanRunWithoutGcp) + if test.setup != nil { + test.setup(t, test.dir) + } tmpfile := testutil.TempFile(t, "", []byte{}) args := append(test.args, "--file-output", tmpfile) skaffold.Build(args...).InDir(test.dir).RunOrFail(t) @@ -314,6 +338,35 @@ func setupGitRepo(t *testing.T, dir string) { } } +// setupBuildX sets up a docker buildx builder using buildkit +func setupBuildX(t *testing.T, dir string) { + t.Cleanup(func() { + dockerArgs := [][]string{ + {"buildx", "uninstall"}, + {"buildx", "rm", "buildkit"}, + } + for _, args := range dockerArgs { + cmd := exec.Command("docker", args...) + if buf, err := util.RunCmdOut(context.Background(), cmd); err != nil { + t.Log(string(buf)) + t.Fatal(err) + } + } + }) + + dockerArgs := [][]string{ + {"buildx", "install"}, + {"buildx", "create", "--driver", "docker-container", "--name", "buildkit"}, + } + for _, args := range dockerArgs { + cmd := exec.Command("docker", args...) + if buf, err := util.RunCmdOut(context.Background(), cmd); err != nil { + t.Log(string(buf)) + t.Fatal(err) + } + } +} + // nowInChicago returns the dateTime string as generated by the dateTime tagger func nowInChicago() string { loc, _ := tz.LoadLocation("America/Chicago") diff --git a/integration/testdata/buildx/Dockerfile b/integration/testdata/buildx/Dockerfile new file mode 100644 index 00000000000..970447876b9 --- /dev/null +++ b/integration/testdata/buildx/Dockerfile @@ -0,0 +1,12 @@ +# syntax=docker/dockerfile:1 + +FROM golang:1.23-alpine AS builder + +COPY main.go . +RUN go build -o /app main.go + +FROM alpine:3 + +COPY --from=builder /app . + +CMD ["/app"] diff --git a/integration/testdata/buildx/config b/integration/testdata/buildx/config new file mode 100644 index 00000000000..dcee1db963b --- /dev/null +++ b/integration/testdata/buildx/config @@ -0,0 +1,9 @@ +global: + cache-tag: cache + buildx-builder: buildkit + survey: + last-prompted: "2025-01-19T21:32:47Z" + collect-metrics: true + update: + last-prompted: "2025-01-19T23:10:47Z" +kubeContexts: [] diff --git a/integration/testdata/buildx/main.go b/integration/testdata/buildx/main.go new file mode 100644 index 00000000000..8c3e45267d6 --- /dev/null +++ b/integration/testdata/buildx/main.go @@ -0,0 +1,14 @@ +package main + +import ( + "fmt" + "time" +) + +func main() { + for counter := 0; ; counter++ { + fmt.Println("Hello world!", counter) + + time.Sleep(time.Second * 1) + } +} diff --git a/integration/testdata/buildx/skaffold.yaml b/integration/testdata/buildx/skaffold.yaml new file mode 100644 index 00000000000..bbd8f22a5d2 --- /dev/null +++ b/integration/testdata/buildx/skaffold.yaml @@ -0,0 +1,12 @@ +apiVersion: skaffold/v4beta12 +kind: Config +build: + artifacts: + - image: my-app + docker: + dockerfile: Dockerfile + local: + useBuildkit: true + useDockerCLI: true + tryImportMissing: true + push: false diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 7680691f468..77271618d3c 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -279,7 +279,7 @@ func parseBuildxMetadataFile(ctx context.Context, filename string) (string, erro if err == nil { // avoid panic: interface conversion: interface {} is nil, not string (if keys don't exists) var digest string - if value := metadata["containerimage.digest"]; value != nil { + if value := metadata["containerimage.config.digest"]; value != nil { digest = value.(string) } var name string From 9bafc7c3e9c20f9bcc31913426c2372f1398ce36 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 20 Jan 2025 23:02:58 -0600 Subject: [PATCH 07/38] feat: buildx multiplatform images w/ buildkit Signed-off-by: --- pkg/skaffold/build/builder_mux.go | 7 ++++-- pkg/skaffold/build/docker/docker.go | 35 ++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/pkg/skaffold/build/builder_mux.go b/pkg/skaffold/build/builder_mux.go index 2985ecce973..48243c43566 100644 --- a/pkg/skaffold/build/builder_mux.go +++ b/pkg/skaffold/build/builder_mux.go @@ -38,6 +38,7 @@ type BuilderMux struct { byImageName map[string]PipelineBuilder store ArtifactStore concurrency int + buildx bool cache Cache } @@ -71,7 +72,8 @@ func NewBuilderMux(cfg Config, store ArtifactStore, cache Cache, builder func(p } } concurrency := getConcurrency(pbs, cfg.BuildConcurrency()) - return &BuilderMux{builders: pbs, byImageName: m, store: store, concurrency: concurrency, cache: cache}, nil + buildx := config.GetDetectBuildX(cfg.GlobalConfig()) + return &BuilderMux{builders: pbs, byImageName: m, store: store, concurrency: concurrency, cache: cache, buildx: buildx}, nil } // Build executes the specific image builder for each artifact in the given artifact slice. @@ -106,7 +108,8 @@ func (b *BuilderMux) Build(ctx context.Context, out io.Writer, tags tag.ImageTag } var built string - if platforms.IsMultiPlatform() && !SupportsMultiPlatformBuild(*artifact) { + // buildx creates multiplatform images via buildkit directly + if platforms.IsMultiPlatform() && !SupportsMultiPlatformBuild(*artifact) && !b.buildx { built, err = CreateMultiPlatformImage(ctx, out, artifact, tag, platforms, artifactBuilder) } else { built, err = artifactBuilder(ctx, out, artifact, tag, platforms) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 77271618d3c..4d1aa62d3bd 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -24,6 +24,7 @@ import ( "io" "os" "os/exec" + "strings" v1 "github.com/google/go-containerregistry/pkg/v1" @@ -44,9 +45,13 @@ func (b *Builder) SupportedPlatforms() platform.Matcher { } func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, tag string, matcher platform.Matcher) (string, error) { - var pl v1.Platform - if len(matcher.Platforms) == 1 { - pl = util.ConvertToV1Platform(matcher.Platforms[0]) + var pls []v1.Platform + if len(matcher.Platforms) > 0 { + for _, plat := range matcher.Platforms { + pls = append(pls, util.ConvertToV1Platform(plat)) + } + } else { + pls = append(pls, v1.Platform{}) } a = b.adjustCache(ctx, a, tag) instrumentation.AddAttributesToCurrentSpanFromContext(ctx, map[string]string{ @@ -64,8 +69,10 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, return "", dockerfileNotFound(err, a.ImageName) } - if err := b.pullCacheFromImages(ctx, out, a.ArtifactType.DockerArtifact, pl); err != nil { - return "", cacheFromPullErr(err, a.ImageName) + for _, pl := range pls { + if err := b.pullCacheFromImages(ctx, out, a.ArtifactType.DockerArtifact, pl); err != nil { + return "", cacheFromPullErr(err, a.ImageName) + } } opts := docker.BuildOptions{Tag: tag, Mode: b.cfg.Mode(), ExtraBuildArgs: docker.ResolveDependencyImages(a.Dependencies, b.artifacts, true)} @@ -75,7 +82,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, // we might consider a different approach in the future. // use CLI for cross-platform builds if b.useCLI || (b.useBuildKit != nil && *b.useBuildKit) || len(a.DockerArtifact.CliFlags) > 0 || matcher.IsCrossPlatform() { - imageID, err = b.dockerCLIBuild(ctx, output.GetUnderlyingWriter(out), a.ImageName, a.Workspace, dockerfile, a.ArtifactType.DockerArtifact, opts, pl) + imageID, err = b.dockerCLIBuild(ctx, output.GetUnderlyingWriter(out), a.ImageName, a.Workspace, dockerfile, a.ArtifactType.DockerArtifact, opts, pls) } else { imageID, err = b.localDocker.Build(ctx, out, a.Workspace, a.ImageName, a.ArtifactType.DockerArtifact, opts) } @@ -93,7 +100,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, return imageID, nil } -func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string, workspace string, dockerfilePath string, a *latest.DockerArtifact, opts docker.BuildOptions, pl v1.Platform) (string, error) { +func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string, workspace string, dockerfilePath string, a *latest.DockerArtifact, opts docker.BuildOptions, pls []v1.Platform) (string, error) { args := []string{"build", workspace, "--file", dockerfilePath, "-t", opts.Tag} imgRef, err := docker.ParseReference(opts.Tag) if err != nil { @@ -118,8 +125,14 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string args = append(args, "--force-rm") } - if pl.String() != "" { - args = append(args, "--platform", pl.String()) + var platforms []string + for _, pl := range pls { + if pl.String() != "" { + platforms = append(platforms, pl.String()) + } + } + if len(platforms) > 0 { + args = append(args, "--platform", strings.Join(platforms, ",")) } if b.useBuildKit != nil && *b.useBuildKit { @@ -161,8 +174,8 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string } else { cmd.Env = append(cmd.Env, "DOCKER_BUILDKIT=0") } - } else if pl.String() != "" { // cross-platform builds require buildkit - log.Entry(ctx).Debugf("setting DOCKER_BUILDKIT=1 for docker build for artifact %q since it targets platform %q", name, pl.String()) + } else if len(platforms) > 0 { // cross-platform builds require buildkit + log.Entry(ctx).Debugf("setting DOCKER_BUILDKIT=1 for docker build for artifact %q since it targets platform %q", name, platforms[0]) cmd.Env = append(cmd.Env, "DOCKER_BUILDKIT=1") } cmd.Stdout = out From d95184c49f1a3d86361b07912c5883dfadc7ece0 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 30 Jan 2025 23:41:08 -0600 Subject: [PATCH 08/38] integration test for buildx multiplatform images --- integration/build_test.go | 48 +++++++++++++++++++++++++++++++--- integration/skaffold/helper.go | 4 ++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/integration/build_test.go b/integration/build_test.go index 6c9724a2776..7c744a2c027 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -192,6 +192,8 @@ func TestBuildWithMultiPlatforms(t *testing.T) { dir string args []string image string + setup func(t *testing.T, workdir string) + repo string expectedPlatforms []v1.Platform }{ { @@ -200,14 +202,27 @@ func TestBuildWithMultiPlatforms(t *testing.T) { args: []string{"--platform", "linux/arm64,linux/amd64"}, expectedPlatforms: []v1.Platform{{OS: "linux", Architecture: "arm64"}, {OS: "linux", Architecture: "amd64"}}, }, + { + setup: func(t *testing.T, dir string) { setupBuildX(t, dir); setupRegistry(t, dir) }, + repo: "localhost:5000", + description: "build multiplatform images with buildx", + dir: "testdata/buildx", + args: []string{"--platform", "linux/arm64,linux/amd64", "--cache-artifacts=false", "--config", "config", "--detect-minikube=false", "--push", "--verbosity=trace"}, + expectedPlatforms: []v1.Platform{{OS: "linux", Architecture: "arm64"}, {OS: "linux", Architecture: "amd64"}}, + }, } for _, test := range tests { t.Run(test.description, func(t *testing.T) { - MarkIntegrationTest(t, NeedsGcp) + if test.setup != nil { + MarkIntegrationTest(t, CanRunWithoutGcp) + test.setup(t, test.dir) + } else { + MarkIntegrationTest(t, NeedsGcp) + } tmpfile := testutil.TempFile(t, "", []byte{}) args := append(test.args, "--file-output", tmpfile) - skaffold.Build(args...).InDir(test.dir).RunOrFail(t) + skaffold.Build(args...).WithRepo(test.repo).InDir(test.dir).RunOrFail(t) bytes, err := os.ReadFile(tmpfile) failNowIfError(t, err) buildArtifacts, err := flags.ParseBuildOutput(bytes) @@ -356,7 +371,34 @@ func setupBuildX(t *testing.T, dir string) { dockerArgs := [][]string{ {"buildx", "install"}, - {"buildx", "create", "--driver", "docker-container", "--name", "buildkit"}, + {"buildx", "create", "--driver", "docker-container", "--name", "buildkit", "--driver-opt=network=host"}, + } + for _, args := range dockerArgs { + cmd := exec.Command("docker", args...) + if buf, err := util.RunCmdOut(context.Background(), cmd); err != nil { + t.Log(string(buf)) + t.Fatal(err) + } + } +} + +// setupRegistry deploys docker registry (in localhost, to avoid insecure-registry config) +func setupRegistry(t *testing.T, dir string) { + t.Cleanup(func() { + dockerArgs := [][]string{ + {"rm", "--force", "registry"}, + } + for _, args := range dockerArgs { + cmd := exec.Command("docker", args...) + if buf, err := util.RunCmdOut(context.Background(), cmd); err != nil { + t.Log(string(buf)) + t.Fatal(err) + } + } + }) + + dockerArgs := [][]string{ + {"run", "-d", "-p", "5000:5000", "--name", "registry", "registry"}, } for _, args := range dockerArgs { cmd := exec.Command("docker", args...) diff --git a/integration/skaffold/helper.go b/integration/skaffold/helper.go index 2ac15667c77..2243e665216 100644 --- a/integration/skaffold/helper.go +++ b/integration/skaffold/helper.go @@ -170,7 +170,9 @@ func (b *RunBuilder) WithConfig(configFile string) *RunBuilder { // WithRepo sets the default repository to be used by skaffold. func (b *RunBuilder) WithRepo(repo string) *RunBuilder { - b.repo = repo + if repo != "" { + b.repo = repo + } return b } From 8adcfa106fdefdf283b542315247c5b71bb193e3 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Fri, 31 Jan 2025 00:59:12 -0600 Subject: [PATCH 09/38] fix buildx image digest when pushed to registry --- pkg/skaffold/build/docker/docker.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 4d1aa62d3bd..fe3c31c5f6a 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -293,6 +293,10 @@ func parseBuildxMetadataFile(ctx context.Context, filename string) (string, erro // avoid panic: interface conversion: interface {} is nil, not string (if keys don't exists) var digest string if value := metadata["containerimage.config.digest"]; value != nil { + // image loaded to local docker daemon + digest = value.(string) + } else if value := metadata["containerimage.digest"]; value != nil { + // image pushed to registry digest = value.(string) } var name string From daf2b09e405f02a14f17b319e8f46198d3895cb8 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sun, 2 Feb 2025 14:27:10 -0600 Subject: [PATCH 10/38] fix: "unknown/unknown" architecture/OS in buildx multiplatform images for more info see https://github.com/orgs/community/discussions/45969 tl;dr buildx attestations include additional metadata that confuses registry tools (breaking the integration test) ``` docker buildx imagetools inspect localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... ``` ``` Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.oci.image.index.v1+json Digest: sha256:... Manifests: Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.oci.image.manifest.v1+json Platform: linux/arm64 Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.oci.image.manifest.v1+json Platform: linux/amd64 Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.oci.image.manifest.v1+json Platform: unknown/unknown Annotations: vnd.docker.reference.digest: sha256:... vnd.docker.reference.type: attestation-manifest Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.oci.image.manifest.v1+json Platform: unknown/unknown Annotations: vnd.docker.reference.digest: sha256:... vnd.docker.reference.type: attestation-manifest ``` Using `BUILDX_NO_DEFAULT_ATTESTATIONS=1` fixes the issue, removing the unknown platforms: ``` Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.docker.distribution.manifest.list.v2+json Digest: sha256:... Manifests: Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.docker.distribution.manifest.v2+json Platform: linux/arm64 Name: localhost:5000/my-app:v2.11.1-120-g8adcfa106-dirty@sha256:... MediaType: application/vnd.docker.distribution.manifest.v2+json Platform: linux/amd64 ``` --- pkg/skaffold/build/docker/docker.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index fe3c31c5f6a..bac04b4c80c 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -178,6 +178,11 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string log.Entry(ctx).Debugf("setting DOCKER_BUILDKIT=1 for docker build for artifact %q since it targets platform %q", name, platforms[0]) cmd.Env = append(cmd.Env, "DOCKER_BUILDKIT=1") } + if len(platforms) > 1 && b.buildx { + // avoid "unknown/unknown" architecture/OS caused by buildx default image attestation + log.Entry(ctx).Warnf("setting BUILDX_NO_DEFAULT_ATTESTATIONS=1 for docker buildx for artifact %q since it targets platform %q to avoid unknown/unknown platform issue", name, platforms[0]) + cmd.Env = append(cmd.Env, "BUILDX_NO_DEFAULT_ATTESTATIONS=1") + } cmd.Stdout = out var errBuffer bytes.Buffer From 9073ce009a2598c3aaac72bafbb49b39e93586ea Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sun, 2 Feb 2025 19:33:38 -0600 Subject: [PATCH 11/38] fix actionable error for buildx cross-compile --- pkg/skaffold/build/docker/docker.go | 7 ++++++- pkg/skaffold/build/docker/errors.go | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index bac04b4c80c..05f8e714ef5 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -190,7 +190,12 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string cmd.Stderr = stderr if err := util.RunCmd(ctx, cmd); err != nil { - return "", tryExecFormatErr(fmt.Errorf("running build: %w", err), errBuffer) + if !b.buildx { + err = tryExecFormatErr(fmt.Errorf("running build: %w", err), errBuffer) + } else { + err = tryExecFormatErrBuildX(fmt.Errorf("running build: %w", err), errBuffer) + } + return "", err } if !b.buildx { diff --git a/pkg/skaffold/build/docker/errors.go b/pkg/skaffold/build/docker/errors.go index 54737df8909..74d4d525b99 100644 --- a/pkg/skaffold/build/docker/errors.go +++ b/pkg/skaffold/build/docker/errors.go @@ -170,3 +170,20 @@ func tryExecFormatErr(err error, stdErr bytes.Buffer) error { }, }) } + +func tryExecFormatErrBuildX(err error, stdErr bytes.Buffer) error { + if !execFormatErr.MatchString(stdErr.String()) { + return err + } + return sErrors.NewError(err, + &proto.ActionableErr{ + Message: err.Error(), + ErrCode: proto.StatusCode_BUILD_CROSS_PLATFORM_ERR, + Suggestions: []*proto.Suggestion{ + { + SuggestionCode: proto.SuggestionCode_BUILD_INSTALL_PLATFORM_EMULATORS, + Action: "To run cross-platform builds, use a proper buildx builder. To create and select it, run:\n\n\tdocker buildx create --driver docker-container --name buildkit\n\n\tskaffold config set buildx-builder buildkit\n\nFor more details, see https://docs.docker.com/build/building/multi-platform/", + }, + }, + }) +} From c5eb4e50b91563c357eb08358efa2779f895a2f1 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sun, 2 Feb 2025 21:07:34 -0600 Subject: [PATCH 12/38] docs: buildx design proposal --- .../buildx-transparent-support.md | 284 ++++++++++++++++++ 1 file changed, 284 insertions(+) create mode 100644 docs-v2/design_proposals/buildx-transparent-support.md diff --git a/docs-v2/design_proposals/buildx-transparent-support.md b/docs-v2/design_proposals/buildx-transparent-support.md new file mode 100644 index 00000000000..b1608c086ec --- /dev/null +++ b/docs-v2/design_proposals/buildx-transparent-support.md @@ -0,0 +1,284 @@ +# buildx transparent support + +* Author(s): Mariano Reingart (@reingart) +* Design Shepherd: +* Date: 2025-02-02 +* Status: Draft + +## Objectives + +Transparent local and remote container builds via buildx, a Docker CLI plugin for extended capabilities with BuildKit. + +## Background + +[buildx](https://docs.docker.com/reference/cli/docker/buildx/) is an enhanced container builder using BuildKit that can replace traditional +`docker build` command, with almost the same syntax (transparently). + +This adds several distributed features and advanced capabilities: +* local & remote build-kit builders, either running standalone, in docker containers or Kubernetes clusters +* improved cache support (registry destination, pushing full metadatada and multi-stage layers) +* improved multi-platform image building support (eg. x86_64 and arm64 combined, with emulation or cross-compilation) + +This features are very useful for corporate CI/CD, for example when using GitLab, where the use case requires: +* using ephemeral remote buildkit instances (rootless) +* using remote docker registries for cache, with multi-stage and multi-platform images support +* using a different cache destination tag for flexibility and workflows separation + +The remote BuildKit [rootless](https://github.com/moby/buildkit/blob/master/docs/rootless.md) support is useful in cases +where a privileged docker daemonis not possible or desirable due security policies (eg. untrusted images or review pipelines). +Daemon-less mode is also useful to offload container building from developers notebooks, sharing and reusing remote caches more effectively. + +The buildx command also supports exporting the build cache using `--cache-to`, useful for remote shared caches in distributed use cases. +Beside speed-ups thanks to caching improvements for metadata and multi-stage layers, this could allow different branches +to have different cache destinations (production vs development cache, with different permissions). + +Multi-platform combined image builds are directly supported by buildx, including improved caching of common layers and emulation / cross-compilation. +This could simplify pipelines and provide faster builds of complex codebases. + +References: + +* https://www.docker.com/blog/image-rebase-and-improved-remote-cache-support-in-new-buildkit/ +* https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/ + +## Proposal + +This proposal aims to improve Skaffold's build process by adding transparent support for docker buildx. +The primary goal is to enable users to leverage the advanced features of buildx, such as remote buildkit builders and improved caching, without significant changes to their existing Skaffold configurations. + +New Global Configs: + +* `buildx-builder`: Enables automatic detection of buildx and multiple builder support. +* `cache-tag`: Allows overriding the default cache tagging strategy, useful for managing caches across different branches or environments. + +Skaffold Schema changes: + +* `cacheTo` to specify custom cache destinations, adding `--cache-to` support to the Docker CLI build process when using buildx. + +Build Process Enhancements: + +* The build process now intelligently detects and utilizes buildx if available and configured. +* Reduced dependency on the local Docker daemon when using buildx, enhancing security and flexibility. + +Backward Compatibility: + +All changes are designed to be backward compatible. +If buildx is not detected or used, Skaffold will fall back to the traditional Docker builder. + +This cache behavior is intended to provide transparent user experience, with similar functionality compared to traditional local docker builds, +without additional boilerplate or different configuration / patches for a CI workflow. + +## Design approach + +`docker buildx build` can be configured to execute against a [remote buildkit instance](https://docs.docker.com/build/builders/drivers/remote/). +No docker daemon is necessary for this to work, but also that is supported by default using the [docker driver](https://docs.docker.com/build/builders/drivers/docker/). +Additionally, [docker container driver](https://docs.docker.com/build/builders/drivers/docker-container) +or [kubernetes driver](https://docs.docker.com/build/builders/drivers/kubernetes/) are available too for advanced use cases. + +Since `docker build` and `docker buildx build` essentially share the same options, no major modifications are needed to Skaffold for backward compatibility. + +This proposal implements the logic to detect if buildx is the default builder (looking for an alias in the docker config). +To [set buildx as the default builder](https://github.com/docker/buildx?tab=readme-ov-file#set-buildx-as-the-default-builder), the command `docker buildx install` should be used. + +Then, additional buildx features are availables, like multi-platform support and different cache destinations. + +Cache adjustment is extended via the the `cache-tag`, useful to point to latest or a generic cache tag, instead of the generated one for this build +(via `tagPolicy` that would be useless in most distributed cases, as it can be invalidated by minor changes, specially if using `inputDigest`). + +Multi-platform images now can be built nativelly, without multiplexing the pipeline nor additional steps to stich the different images. + +### User experience + +To use BuildKit transparently, BuildX should be installed as default builder (this creates an alias in docker config): + +``` +docker buildx install +``` + +Then Skaffold should be configured to detect buildx (default builder) and set a generic cache tag: + +``` +skaffold config set -g buildx-builder default +skaffold config set -g cache-tag cache +``` + +Example basic config, this will be sufficient for many users: + +```yaml +apiVersion: skaffold/v4beta13 +build: + artifacts: + - image: my-app-image + context: my-app-image + docker: + dockerfile: Dockerfile + cacheFrom: + - "my-app-image" + cacheTo: + - "my-app-image" + local: + useBuildkit: true + useDockerCLI: true + tryImportMissing: true +``` + +* If no tag is specified for cache, the configured `cache-tag` will be used (in this example my-app-image:cache) +* If `cacheTo` destination is not specified, the `cacheFrom` image name and tag will be adjusted, adding `type=registry,mode=max` (only if push images is enabled) + +Advanced users would prefer to create buildkit instances for multiplatform images, e.g. using the docker container driver: + +``` +docker buildx create --driver docker-container --name local +skaffold config set -g buildx-builder local +``` + +Remote builds are possible, pointing to a remote instance that could be deployed in another host or container: +``` +docker buildx create --name remote --driver remote tcp://buildkitd:2375 +skaffold config set -g buildx-builder remote +``` + +### Errors + +Example for missing builder (`ERROR: no builder "defaultx" found` if skaffold was configured incorrectly): +``` +skaffold build --default-repo localhost:5000 --platform linux/arm64,linux/amd64 --cache-artifacts=false --detect-minikube=false --push +Generating tags... + - my-app -> localhost:5000/my-app:v2.11.1-121-gc703038c9-dirty +Starting build... +Building [my-app]... +Target platforms: [linux/arm64,linux/amd64] +ERROR: no builder "defaultx" found +exit status 1. Docker build ran into internal error. Please retry. +If this keeps happening, please open an issue.. +``` + +Example of improper configuration for multi-platform emulation builds: + +``` +$ /src/out/skaffold build --default-repo localhost:5000 --platform linux/arm64,linux/amd64 --cache-artifacts=false --detect-minikube=false --push +Generating tags... + - my-app -> localhost:5000/my-app:v2.11.1-122-ga0fb3239a-dirty +Starting build... +Building [my-app]... +Target platforms: [linux/arm64,linux/amd64] +#0 building with "default" instance using docker driver + +... + +#19 [linux/arm64 builder 3/3] RUN go build -o /app main.go +#19 0.639 exec /bin/sh: exec format error +#19 ERROR: process "/bin/sh -c go build -o /app main.go" did not complete successfully: exit code: 1 +------ + > [linux/arm64 builder 3/3] RUN go build -o /app main.go: +0.639 exec /bin/sh: exec format error +------ +Dockerfile:6 +-------------------- + 4 | + 5 | COPY main.go . + 6 | >>> RUN go build -o /app main.go + 7 | + 8 | FROM alpine:3 +-------------------- +ERROR: failed to solve: process "/bin/sh -c go build -o /app main.go" did not complete successfully: exit code: 1 +running build: exit status 1. To run cross-platform builds, use a proper buildx builder. To create and select it, run: + + docker buildx create --driver docker-container --name buildkit + + skaffold config set buildx-builder buildkit + +For more details, see https://docs.docker.com/build/building/multi-platform/. + +``` + +This is a corner case, as the default docker builder should not be used for multi-platform builds. +An actionable error was returned with the step-by-step instructions. + +## Implementation plan + +For the minimal viable product (initial PR): + +* Add a new global config buildx-builder to enable buildx detection and support for different builders +* Implements logic to detect buildx is the default builder (via docker config alias) +* Remove dependency on docker daemon if using BuildX (still supported to load images if using minikube or similar) +* Add a new global config cache-tag to override default cache tagging (instead of generated tag) +* Add cacheTo to sakffold schema for the cache destination (optional, default is to use new cacheFrom + cache tag) +* Add --cache-to support in Docker CLI build if using BuildX CLI +* Add multiplatform images building support for buildkit under buildx + +Additional features, error handling and examples can be implemented in the future. + +Future work: include more BuildKit advanced features support like +[multiple build contexts](https://www.docker.com/blog/dockerfiles-now-support-multiple-build-contexts/), +for an improved mono-repo experience for large code-bases. + +Other advanced featues of buildx and buildkit includes [Attestations](https://docs.docker.com/build/metadata/attestations/). +Note: default attestation is disabled for multiplatform builds, as it creates metadata with "unknown/unknown" arch/OS, +causing issues with registry tools an libraries, see [GH discussion](https://github.com/orgs/community/discussions/45969). + +## Release plan + +The buildx support could go through the release stages Alpha -> Beta -> Stable. +This will allow time for community feedback and avoid unnecessary features or rework. + +The following features would be released at each stage: + +**Alpha** + +Implement minimal changes to support buildx (initial PR): +- buildx detection +- cache-to export +- multiplatform images + +**Beta** + +- Implement additional buildx features needed by the community, like multiple context support +- Implement additional actionable errors, if needed +- Update user-facing documentation +- Implement a new buildkit basic example using buildx + +**Stable** + +- Remove custom buildkit example using custom builder +- Implement a new buildkit advanced example using buildx and remote rootless daemon for CI + +## Automated test plan + +New test cases were implemented to cover the new functionality (added to existing test suites): + +1. Unit tests for buildx, similar to docker build. + + * `TestDockerCLIBuild`: "buildkit buildx load", "buildkit buildx push" (including both buildx detection and cache-to) + +2. Integration tests, idem: + + * `TestBuild`: "docker buildx" + * `TestBuildWithWithPlatform`: "docker buildx linux/amd64", "docker buildx linux/arm64" + * `TestBuildWithMultiPlatforms`: "build multiplatform images with buildx" + +3. Add basic and comprehensive buildx examples to the `integration/examples` + directory. + +Note that for multi-platform images and advanced examples, a registry is needed to push the images. +A docker container with a local registry was implemented in the setup of integration tests. +With this approach, all buildx tests can be run locally (not needing GCP nor any other cloud resource like a remote registry). +This avoids modifications to docker daemon config (like insecure-registry exclusions), but it needs a buildkit running within the host network, as both uses localhost. + +## Credits + +This proposal is related to [#8172](https://github.com/GoogleContainerTools/skaffold/pull/8172): "Add buildx option for daemon-less BuildKit support". +Initial code was inspired by by a prior [ebekebe fork](https://github.com/ebekebe/skaffold/commit/1c1fdeb18f4d2847e65e283fba498a14745039af). + +A more general approach was implemented in this initial PR [#9648](https://github.com/GoogleContainerTools/skaffold/pull/9648), +with configurable builders, actionable errors and multi-platform support. + +This actually fixes existing issues like: +* [#5018](https://github.com/GoogleContainerTools/skaffold/issues/5018): "Docker Buildx Integration" +* [#6732](https://github.com/GoogleContainerTools/skaffold/issues/6732): "Support building securely against remote buildkitd" +* [#9197](https://github.com/GoogleContainerTools/skaffold/issues/9197): "Support additional docker buildkit options" + +The fix proposed here uses buildx nativelly, avoiding custom build scripts, like the one in the official example: +[custom-buildx](https://github.com/GoogleContainerTools/skaffold/tree/main/examples/custom-buildx) + +Future work is related to [#2110](https://github.com/GoogleContainerTools/skaffold/issues/2110): "feature request: more control over the docker build context" + From aa143f36d294c77cd95dc5f95476e78cd70b04a0 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sun, 2 Feb 2025 21:47:04 -0600 Subject: [PATCH 13/38] fix: disable metrics and prompts in buildx config --- integration/testdata/buildx/config | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/integration/testdata/buildx/config b/integration/testdata/buildx/config index dcee1db963b..b5e8d04f2c8 100644 --- a/integration/testdata/buildx/config +++ b/integration/testdata/buildx/config @@ -2,8 +2,10 @@ global: cache-tag: cache buildx-builder: buildkit survey: + disable-prompt: true last-prompted: "2025-01-19T21:32:47Z" - collect-metrics: true + collect-metrics: false + update-check: false update: last-prompted: "2025-01-19T23:10:47Z" kubeContexts: [] From df3c188de6956eebb94fc4bdab3fc2fbd4e2bd71 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 3 Feb 2025 20:27:38 -0600 Subject: [PATCH 14/38] chore: fix linter issues --- integration/build_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/build_test.go b/integration/build_test.go index 7c744a2c027..c604181a478 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -354,7 +354,7 @@ func setupGitRepo(t *testing.T, dir string) { } // setupBuildX sets up a docker buildx builder using buildkit -func setupBuildX(t *testing.T, dir string) { +func setupBuildX(t *testing.T, _ string) { t.Cleanup(func() { dockerArgs := [][]string{ {"buildx", "uninstall"}, @@ -383,7 +383,7 @@ func setupBuildX(t *testing.T, dir string) { } // setupRegistry deploys docker registry (in localhost, to avoid insecure-registry config) -func setupRegistry(t *testing.T, dir string) { +func setupRegistry(t *testing.T, _ string) { t.Cleanup(func() { dockerArgs := [][]string{ {"rm", "--force", "registry"}, From 19cc43af32dc01eb890675ff133b7e744532ffab Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Sat, 1 Mar 2025 15:29:34 -0600 Subject: [PATCH 15/38] chore: move schema doc changes to v4beta13 --- docs-v2/content/en/schemas/v4beta12.json | 13 ------------- docs-v2/content/en/schemas/v4beta13.json | 13 +++++++++++++ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/docs-v2/content/en/schemas/v4beta12.json b/docs-v2/content/en/schemas/v4beta12.json index eeb6dd5d73f..9f7232ff241 100755 --- a/docs-v2/content/en/schemas/v4beta12.json +++ b/docs-v2/content/en/schemas/v4beta12.json @@ -1766,18 +1766,6 @@ "[\"golang:1.10.1-alpine3.7\", \"alpine:3.7\"]" ] }, - "cacheTo": { - "items": { - "type": "string" - }, - "type": "array", - "description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.", - "x-intellij-html-description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.", - "default": "[]", - "examples": [ - "[\"type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max\"]" - ] - }, "cliFlags": { "items": { "type": "string" @@ -1842,7 +1830,6 @@ "network", "addHost", "cacheFrom", - "cacheTo", "cliFlags", "pullParent", "noCache", diff --git a/docs-v2/content/en/schemas/v4beta13.json b/docs-v2/content/en/schemas/v4beta13.json index 9f7232ff241..eeb6dd5d73f 100755 --- a/docs-v2/content/en/schemas/v4beta13.json +++ b/docs-v2/content/en/schemas/v4beta13.json @@ -1766,6 +1766,18 @@ "[\"golang:1.10.1-alpine3.7\", \"alpine:3.7\"]" ] }, + "cacheTo": { + "items": { + "type": "string" + }, + "type": "array", + "description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.", + "x-intellij-html-description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.", + "default": "[]", + "examples": [ + "[\"type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max\"]" + ] + }, "cliFlags": { "items": { "type": "string" @@ -1830,6 +1842,7 @@ "network", "addHost", "cacheFrom", + "cacheTo", "cliFlags", "pullParent", "noCache", From 65ea832c3e8189f8720f5fc2e8a91197132f70a1 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 14 Aug 2025 16:54:48 -0500 Subject: [PATCH 16/38] ci: automated release (GitHub action) * build (currently just for linux amd64) & upload artifact + sha256 * create & publish a release (draft or final, depending on git tag/ref) --- .github/workflows/release.yml | 111 ++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 .github/workflows/release.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 00000000000..a20f3e7f143 --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,111 @@ +name: Release Standalone Executables + +# Triggers the workflow on push or pull request events +on: [pull_request] + +concurrency: + group: build-${{ github.event.pull_request.number || github.ref }}-${{github.workflow}} + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + +permissions: + contents: write + +jobs: + build-linux: + runs-on: ubuntu-22.04 + name: Build Linux Executable + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 1.24.2 + id: go + + - name: Build Skaffold from HEAD + run: | + make + echo SKAFFOLD_BINARY=$PWD/out/skaffold >> $GITHUB_ENV + out/skaffold config set --global collect-metrics false + cp out/skaffold skaffold-linux-amd64 + sha256sum skaffold-linux-amd64 > skaffold-linux-amd64.sha256 + + - name: Get Version from Script and save metadata + run: | + VERSION=$($SKAFFOLD_BINARY version) + echo "$VERSION" | tee skaffold.txt + + - name: Upload Linux executable + uses: actions/upload-artifact@v4 + with: + name: skaffold-linux-amd64 + path: | + skaffold-linux-amd64 + skaffold-linux-amd64.sha256 + skaffold.txt + + release: + needs: [build-linux] + runs-on: ubuntu-latest + name: Create Release + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # Fetch all tags and commit history + + - name: Download Linux binary + uses: actions/download-artifact@v4 + with: + name: skaffold-linux-amd64 + path: release + + - name: Get Version from Script metadata + id: get_version + run: | + VERSION=$(cat release/skaffold.txt) + echo "version=$VERSION" >> $GITHUB_ENV + + - name: Determine Release Type + id: determine_release + run: | + if [ "${{ github.ref }}" = "refs/heads/main" ]; then + if git rev-parse "${VERSION}" >/dev/null 2>&1; then + echo "Git tag ${VERSION} already exists. Skipping final release." + echo "final_release=prep" >> $GITHUB_ENV + else + echo "final_release=latest" >> $GITHUB_ENV + fi + else + echo "final_release=draft" >> $GITHUB_ENV + fi + env: + VERSION: ${{ env.version }} + + - name: Create GitHub Release + id: publish_release + run: | + if [ "$FINAL_RELEASE" = "latest" ]; then + RELEASE_NAME="${VERSION}" + PUBLISH="--latest" + else + TIMESTAMP=$(date +'%Y%m%d-%H%M%S') + RELEASE_NAME="${VERSION}-${TIMESTAMP}" + if [ "$FINAL_RELEASE" = "draft" ]; then + PUBLISH="--draft" + else + PUBLISH="--prerelease" + fi + fi + gh release create "$RELEASE_NAME" \ + --title="Release $RELEASE_NAME" \ + --generate-notes \ + release/skaffold-linux-amd64 \ + release/skaffold-linux-amd64.sha256 \ + "$PUBLISH" + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + VERSION: ${{ env.version }} + FINAL_RELEASE: ${{ env.final_release }} From 58a5fe5da4b1ecd357d20cf6e446b98f723b43fa Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 14 Aug 2025 19:06:53 -0500 Subject: [PATCH 17/38] ci: run release on push too --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a20f3e7f143..66c8d3cd67a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,7 +1,7 @@ name: Release Standalone Executables # Triggers the workflow on push or pull request events -on: [pull_request] +on: [push, pull_request] concurrency: group: build-${{ github.event.pull_request.number || github.ref }}-${{github.workflow}} From adf32d5c024c4d6a9ec2df910984df5c2639cb98 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 14 Aug 2025 19:11:46 -0500 Subject: [PATCH 18/38] fix(buildx): export cache as OCI artifact by default Workaround for some registries (AWS ECS/artifactory?) don't fully support non-standard blobs: * https://aws.amazon.com/blogs/containers/announcing-remote-cache-support-in-amazon-ecr-for-buildkit-clients/ * https://jfrog.com/help/r/artifactory-using-docker-buildx-cache-to-and-cache-from-with-artifactory-and-why-max-unique-tags-does-not-apply/artifactory-using-docker-buildx-cache-to-and-cache-from-with-artifactory-and-why-max-unique-tags-does-not-apply --- pkg/skaffold/build/docker/docker.go | 2 +- pkg/skaffold/build/docker/docker_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 6fa66ac7005..d9c6f0d7835 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -259,7 +259,7 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT // add cache destination reference, only if we're pushing to a registry and not given in config if len(a.DockerArtifact.CacheTo) == 0 && b.pushImages { log.Entry(ctx).Debugf("Adjusting cache destination image ref: %s", cacheTag) - ct = append(ct, fmt.Sprintf("type=registry,ref=%s,mode=max", cacheTag)) + ct = append(ct, fmt.Sprintf("type=registry,ref=%s,mode=max,image-manifest=true,oci-mediatypes=true", cacheTag)) } } } else { diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go index 05f3541810b..526a4554709 100644 --- a/pkg/skaffold/build/docker/docker_test.go +++ b/pkg/skaffold/build/docker/docker_test.go @@ -106,7 +106,7 @@ func TestDockerCLIBuild(t *testing.T) { buildx: true, daemonless: true, imageName: "gcr.io/k8s-skaffold/example:tag", - expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max", "--push", "--builder", "default", "--metadata-file", "metadata.json"}, + expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max,image-manifest=true,oci-mediatypes=true", "--push", "--builder", "default", "--metadata-file", "metadata.json"}, expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, }, { From 7192388e601b167b5633cfda7b90718023495d96 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 19 Aug 2025 21:49:54 -0500 Subject: [PATCH 19/38] refactor: configurable cache-repo --- pkg/skaffold/build/docker/docker.go | 16 +++++++++++++++- pkg/skaffold/config/global_config.go | 1 + pkg/skaffold/config/util.go | 12 ++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index d9c6f0d7835..410a5a8cdf8 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -234,14 +234,28 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT // allow this behaviour to be disabled return a } + multiLevel, err := config.GetMultiLevelRepo(b.cfg.GlobalConfig()) + if err != nil { + log.Entry(ctx).Errorf("getting multi-level repo support: %v", err) + } + cacheRepo, err := config.GetCacheRepo(b.cfg.GlobalConfig()) + if err != nil { + log.Entry(ctx).Errorf("getting cache-repo %q: %v", cacheRepo, err) + } if b.buildx { // compute the full cache reference (including registry, preserving tag) tag, _ := config.GetCacheTag(b.cfg.GlobalConfig()) - imgRef, err := docker.ParseReference(artifactTag) + log.Entry(ctx).Debugf("parsing cache ref: %s", cacheRef) + imgRef, err := docker.ParseReference(cacheRef) if err != nil { log.Entry(ctx).Errorf("couldn't parse image tag: %v", err) } else if tag != "" { + log.Entry(ctx).Debugf("using cache image base name: %s and tag: %s", imgRef.BaseName, tag) cacheTag = fmt.Sprintf("%s:%s", imgRef.BaseName, tag) + cacheTag, err = docker.SubstituteDefaultRepoIntoImage(cacheRepo, multiLevel, cacheTag) + if err != nil { + log.Entry(ctx).Errorf("applying cache default repo to %q: %v", tag, err) + } } } if !stringslice.Contains(a.DockerArtifact.CacheFrom, cacheRef) { diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go index 872e21b222e..30810ffe72a 100644 --- a/pkg/skaffold/config/global_config.go +++ b/pkg/skaffold/config/global_config.go @@ -34,6 +34,7 @@ type ContextConfig struct { // DebugHelpersRegistry is the registry from which the debug helper images are used. DebugHelpersRegistry string `yaml:"debug-helpers-registry,omitempty"` CacheTag string `yaml:"cache-tag,omitempty"` + CacheRepo string `yaml:"cache-repo,omitempty"` BuildXBuilder string `yaml:"buildx-builder,omitempty"` UpdateCheck *bool `yaml:"update-check,omitempty"` Survey *SurveyConfig `yaml:"survey,omitempty"` diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index 3f6bdff71dd..67e4527985c 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -229,6 +229,18 @@ func GetCacheTag(configFile string) (string, error) { return cfg.CacheTag, nil } +func GetCacheRepo(configFile string) (string, error) { + cfg, err := GetConfigForCurrentKubectx(configFile) + if err != nil { + log.Entry(context.TODO()).Errorf("Cannot read cache-repo from config: %v", err) + return "", err + } + if cfg.CacheRepo != "" { + log.Entry(context.TODO()).Infof("Using cache-repo=%s from config", cfg.CacheRepo) + } + return cfg.CacheRepo, nil +} + func GetBuildXBuilder(configFile string) string { cfg, err := GetConfigForCurrentKubectx(configFile) if err != nil { From 91b44cd0b6695b2a14d64ad6d5aa35c6f646ec1c Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 21 Aug 2025 20:06:19 -0500 Subject: [PATCH 20/38] refactor: cache src/dest * rewrite cache adjust logic (simplified and removed some magic) * moved out cache ref compute logic * templated env expansion for cacheFrom and cacheTo If no tag is specified in cache source and destination, artifact build tag will be used (if configured cache-tag is not set). This way, the cache tag could be used to override generated tag, useful to direct images to personal or custom dev tag. --- pkg/skaffold/build/docker/docker.go | 111 +++++++++++++++++----------- 1 file changed, 67 insertions(+), 44 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 410a5a8cdf8..665082aca84 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -225,69 +225,92 @@ func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *lat } // adjustCache returns an artifact where any cache references from the artifactImage is changed to the tagged built image name instead. -// Under buildx, if cache-tag is configured, it will be used instead of the generated artifact tag (registry preserved) +// Under buildx, templated cache refs will be evaluated (with image rewriting using the default cache repo and tag) // if no cacheTo was specified in the skaffold yaml, it will add a tagged destination using the same cache source reference func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactTag string) *latest.Artifact { - cacheRef := a.ImageName // lookup value to be replaced - cacheTag := artifactTag // full reference to be used if os.Getenv("SKAFFOLD_DISABLE_DOCKER_CACHE_ADJUSTMENT") != "" { // allow this behaviour to be disabled return a } + if !stringslice.Contains(a.DockerArtifact.CacheFrom, a.ImageName) && !b.buildx { + return a + } + + cf := make([]string, 0, len(a.DockerArtifact.CacheFrom)) + + for _, image := range a.DockerArtifact.CacheFrom { + cacheRef := artifactTag // full reference to be used (backward compatibility) + if b.buildx { + // change cache reference to to the tagged image name (built or given, including registry) + cacheRef = b.computeCacheRefTag(ctx, artifactTag, image) + log.Entry(ctx).Debugf("Adjusting cache source image ref: %s to %s", image, cacheRef) + } + cf = append(cf, cacheRef) + } + + // Create a new copy of CacheTo to modify destinations + ct := make([]string, max(len(a.DockerArtifact.CacheTo), 1)) + if len(a.DockerArtifact.CacheTo) > 0 { + copy(ct, a.DockerArtifact.CacheTo) + } else if len(a.DockerArtifact.CacheFrom) > 0 { + log.Entry(ctx).Infof("Using first cache source as destination: %s", a.DockerArtifact.CacheFrom[0]) + ct[0] = a.DockerArtifact.CacheFrom[0] + } + for i, image := range ct { + if b.buildx && b.pushImages { + cacheRef := b.computeCacheRefTag(ctx, artifactTag, image) + log.Entry(ctx).Debugf("Adjusting cache destination image ref: %s to %s", image, cacheRef) + ct[i] = fmt.Sprintf("type=registry,ref=%s,mode=max,image-manifest=true,oci-mediatypes=true", cacheRef) + } + } + copy := *a + copy.DockerArtifact.CacheFrom = cf + copy.DockerArtifact.CacheTo = ct + return © +} + +func (b *Builder) computeCacheRefTag(ctx context.Context, artifactTag string, cacheRef string) string { multiLevel, err := config.GetMultiLevelRepo(b.cfg.GlobalConfig()) if err != nil { - log.Entry(ctx).Errorf("getting multi-level repo support: %v", err) + log.Entry(ctx).Errorf("Getting multi-level repo support: %v", err) } cacheRepo, err := config.GetCacheRepo(b.cfg.GlobalConfig()) if err != nil { - log.Entry(ctx).Errorf("getting cache-repo %q: %v", cacheRepo, err) + log.Entry(ctx).Errorf("Getting cache-repo %q: %v", cacheRepo, err) } + cacheTag, _ := config.GetCacheTag(b.cfg.GlobalConfig()) if b.buildx { // compute the full cache reference (including registry, preserving tag) - tag, _ := config.GetCacheTag(b.cfg.GlobalConfig()) - log.Entry(ctx).Debugf("parsing cache ref: %s", cacheRef) + imageInfoEnv, err := docker.EnvTags(artifactTag) + if err != nil { + log.Entry(ctx).Errorf("Couldn't build env tags: %v", err) + } + log.Entry(ctx).Debugf("Expanding cache ref env template: %s", cacheRef) + cacheRef, err = util.ExpandEnvTemplate(cacheRef, imageInfoEnv) + if err != nil { + log.Entry(ctx).Errorf("Couldn't expand cache image tag: %v", err) + } + log.Entry(ctx).Infof("parsing expanded cache ref: %s", cacheRef) imgRef, err := docker.ParseReference(cacheRef) if err != nil { - log.Entry(ctx).Errorf("couldn't parse image tag: %v", err) - } else if tag != "" { - log.Entry(ctx).Debugf("using cache image base name: %s and tag: %s", imgRef.BaseName, tag) - cacheTag = fmt.Sprintf("%s:%s", imgRef.BaseName, tag) - cacheTag, err = docker.SubstituteDefaultRepoIntoImage(cacheRepo, multiLevel, cacheTag) - if err != nil { - log.Entry(ctx).Errorf("applying cache default repo to %q: %v", tag, err) - } + log.Entry(ctx).Errorf("Couldn't parse image tag: %v", err) } - } - if !stringslice.Contains(a.DockerArtifact.CacheFrom, cacheRef) { - return a - } - - cf := make([]string, 0, len(a.DockerArtifact.CacheFrom)) - ct := make([]string, 0, len(a.DockerArtifact.CacheTo)) - for _, image := range a.DockerArtifact.CacheFrom { - if image == cacheRef { - // change cache reference to to the tagged image name (built or given, including registry) - log.Entry(ctx).Debugf("Adjusting cache source image ref: %s", cacheTag) - cf = append(cf, cacheTag) - if b.buildx { - // add cache destination reference, only if we're pushing to a registry and not given in config - if len(a.DockerArtifact.CacheTo) == 0 && b.pushImages { - log.Entry(ctx).Debugf("Adjusting cache destination image ref: %s", cacheTag) - ct = append(ct, fmt.Sprintf("type=registry,ref=%s,mode=max,image-manifest=true,oci-mediatypes=true", cacheTag)) - } - } - } else { - cf = append(cf, image) + // determine the cache tag (use explicit tag, configured cache-tag or fallback to image tag) + if imgRef.Tag != "" { + cacheTag = imgRef.Tag + } else if cacheTag == "" { + cacheTag = imageInfoEnv["IMAGE_TAG"] + } + log.Entry(ctx).Debugf("Rebuild cache image base name: %s and tag: %s", imgRef.BaseName, cacheTag) + cacheRef = fmt.Sprintf("%s:%s", imgRef.BaseName, cacheTag) + // sustitute the cache repository (registry): + cacheRef, err = docker.SubstituteDefaultRepoIntoImage(cacheRepo, multiLevel, cacheRef) + if err != nil { + log.Entry(ctx).Errorf("Applying cache default repo failed: %v", err) } } - // just copy any other cache destination given in the config file: - for _, image := range a.DockerArtifact.CacheTo { - ct = append(cf, image) - } - copy := *a - copy.DockerArtifact.CacheFrom = cf - copy.DockerArtifact.CacheTo = ct - return © + log.Entry(ctx).Infof("Computed cache ref: %s", cacheRef) + return cacheRef } // osCreateTemp allows for replacing metadata for testing purposes From 8421dc18a038d54540eb595c3a459a8edf3483c0 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 2 Sep 2025 14:58:06 -0500 Subject: [PATCH 21/38] refactor: cache-tag template expansion New CACHE_TAG env to expand cache from / to references --- pkg/skaffold/build/docker/docker.go | 51 ++++++++++++++++++----------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 665082aca84..b3b25d1d476 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -278,35 +278,48 @@ func (b *Builder) computeCacheRefTag(ctx context.Context, artifactTag string, ca if err != nil { log.Entry(ctx).Errorf("Getting cache-repo %q: %v", cacheRepo, err) } + // build image env and expand cacheTag template: + imageInfoEnv, err := docker.EnvTags(artifactTag) + if err != nil { + log.Entry(ctx).Errorf("Couldn't build env tags: %v", err) + } cacheTag, _ := config.GetCacheTag(b.cfg.GlobalConfig()) + imageInfoEnv["CACHE_TAG"], err = util.ExpandEnvTemplate(cacheTag, imageInfoEnv) + if err != nil { + log.Entry(ctx).Errorf("Couldn't expand default cache-tag: %v", err) + } if b.buildx { // compute the full cache reference (including registry, preserving tag) - imageInfoEnv, err := docker.EnvTags(artifactTag) - if err != nil { - log.Entry(ctx).Errorf("Couldn't build env tags: %v", err) - } log.Entry(ctx).Debugf("Expanding cache ref env template: %s", cacheRef) cacheRef, err = util.ExpandEnvTemplate(cacheRef, imageInfoEnv) if err != nil { log.Entry(ctx).Errorf("Couldn't expand cache image tag: %v", err) } - log.Entry(ctx).Infof("parsing expanded cache ref: %s", cacheRef) + log.Entry(ctx).Infof("Parsing expanded cache ref: %s", cacheRef) imgRef, err := docker.ParseReference(cacheRef) if err != nil { - log.Entry(ctx).Errorf("Couldn't parse image tag: %v", err) - } - // determine the cache tag (use explicit tag, configured cache-tag or fallback to image tag) - if imgRef.Tag != "" { - cacheTag = imgRef.Tag - } else if cacheTag == "" { - cacheTag = imageInfoEnv["IMAGE_TAG"] - } - log.Entry(ctx).Debugf("Rebuild cache image base name: %s and tag: %s", imgRef.BaseName, cacheTag) - cacheRef = fmt.Sprintf("%s:%s", imgRef.BaseName, cacheTag) - // sustitute the cache repository (registry): - cacheRef, err = docker.SubstituteDefaultRepoIntoImage(cacheRepo, multiLevel, cacheRef) - if err != nil { - log.Entry(ctx).Errorf("Applying cache default repo failed: %v", err) + log.Entry(ctx).Errorf("Couldn't parse image tag %s: %v", cacheRef, err) + } else { + // determine the cache tag (use explicit tag, expanded cache-tag or fallback to image tag) + tag := "" + if imgRef.Tag != "" { + tag = imgRef.Tag + } else if cacheTag == "" { + tag = imageInfoEnv["IMAGE_TAG"] + } else { + tag = imageInfoEnv["CACHE_TAG"] + } + if tag == "" { + log.Entry(ctx).Errorf("Invalid empty computed cache-tag") + } + log.Entry(ctx).Debugf("Rewrite cache image base name: %s and tag: %s", imgRef.BaseName, tag) + cacheRef = fmt.Sprintf("%s:%s", imgRef.BaseName, tag) + // sustitute the cache repository (registry): + ref, err := docker.SubstituteDefaultRepoIntoImage(cacheRepo, multiLevel, cacheRef) + if err != nil { + log.Entry(ctx).Errorf("Applying cache default repo failed for '%s': %v", cacheRef, err) + } + cacheRef = ref } } log.Entry(ctx).Infof("Computed cache ref: %s", cacheRef) From 9597f1fb28a19703efb18c1ded2ba91908f5370d Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 2 Sep 2025 17:26:15 -0500 Subject: [PATCH 22/38] refactor: cache-flags config * allow custom parameters to buildx cache-to * disable cache push if not set (to avoid auth errors) Example: ``` cache-flags: - type=registry - mode=max - image-manifest=true - oci-mediatypes=true ``` --- pkg/skaffold/build/docker/docker.go | 33 ++++++++++++++++++---------- pkg/skaffold/config/global_config.go | 1 + pkg/skaffold/config/util.go | 12 ++++++++++ 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index b3b25d1d476..29a5e30a515 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -250,18 +250,29 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT // Create a new copy of CacheTo to modify destinations ct := make([]string, max(len(a.DockerArtifact.CacheTo), 1)) - if len(a.DockerArtifact.CacheTo) > 0 { - copy(ct, a.DockerArtifact.CacheTo) - } else if len(a.DockerArtifact.CacheFrom) > 0 { - log.Entry(ctx).Infof("Using first cache source as destination: %s", a.DockerArtifact.CacheFrom[0]) - ct[0] = a.DockerArtifact.CacheFrom[0] - } - for i, image := range ct { - if b.buildx && b.pushImages { - cacheRef := b.computeCacheRefTag(ctx, artifactTag, image) - log.Entry(ctx).Debugf("Adjusting cache destination image ref: %s to %s", image, cacheRef) - ct[i] = fmt.Sprintf("type=registry,ref=%s,mode=max,image-manifest=true,oci-mediatypes=true", cacheRef) + + // only process cache destination if cache flags are set (to avoid failures on cache push) + cacheFlags, _ := config.GetCacheFlags(b.cfg.GlobalConfig()) + if len(cacheFlags) > 0 { + if len(a.DockerArtifact.CacheTo) > 0 { + copy(ct, a.DockerArtifact.CacheTo) + } else if len(a.DockerArtifact.CacheFrom) > 0 { + log.Entry(ctx).Infof("Using first cache source as destination: %s", a.DockerArtifact.CacheFrom[0]) + ct[0] = a.DockerArtifact.CacheFrom[0] + } + for i, image := range ct { + if b.buildx && b.pushImages { + cacheRef := b.computeCacheRefTag(ctx, artifactTag, image) + log.Entry(ctx).Debugf("Adjusting cache destination image ref: %s to %s", image, cacheRef) + // append a new cache flag with the ref destination: + cacheFlags = append(cacheFlags, fmt.Sprintf("ref=%s", cacheRef)) + // format the flags (comma separated) and it to the new cache-to array + ct[i] = fmt.Sprintf("%s", strings.Join(cacheFlags, ",")) + } } + } else { + log.Entry(ctx).Infof("No cache flags set, skipping cache destination adjustment") + ct = nil } copy := *a copy.DockerArtifact.CacheFrom = cf diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go index 30810ffe72a..ef4a9b7f41e 100644 --- a/pkg/skaffold/config/global_config.go +++ b/pkg/skaffold/config/global_config.go @@ -35,6 +35,7 @@ type ContextConfig struct { DebugHelpersRegistry string `yaml:"debug-helpers-registry,omitempty"` CacheTag string `yaml:"cache-tag,omitempty"` CacheRepo string `yaml:"cache-repo,omitempty"` + CacheFlags []string `yaml:"cache-flags,omitempty"` BuildXBuilder string `yaml:"buildx-builder,omitempty"` UpdateCheck *bool `yaml:"update-check,omitempty"` Survey *SurveyConfig `yaml:"survey,omitempty"` diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index 67e4527985c..a7133d9ef2d 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -241,6 +241,18 @@ func GetCacheRepo(configFile string) (string, error) { return cfg.CacheRepo, nil } +func GetCacheFlags(configFile string) ([]string, error) { + cfg, err := GetConfigForCurrentKubectx(configFile) + if err != nil { + log.Entry(context.TODO()).Errorf("Cannot read cache-flags from config: %v", err) + return nil, err + } + if len(cfg.CacheFlags) > 0 { + log.Entry(context.TODO()).Infof("Using cache-flags=%s from config", cfg.CacheFlags) + } + return cfg.CacheFlags, nil +} + func GetBuildXBuilder(configFile string) string { cfg, err := GetConfigForCurrentKubectx(configFile) if err != nil { From 7b25d671383817558ace25178e9d675f563fcaf1 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 3 Sep 2025 12:13:26 -0500 Subject: [PATCH 23/38] refactor(buildx): log docker build command Use info log-level to avoid debug verbosity --- pkg/skaffold/build/docker/docker.go | 12 ++++++++---- pkg/skaffold/config/util.go | 8 ++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 29a5e30a515..a5b51419289 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -184,6 +184,10 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string stderr := io.MultiWriter(out, &errBuffer) cmd.Stderr = stderr + if b.buildx { + log.Entry(ctx).Infof("Running buildx command: %s", cmd.Args) + } + if err := util.RunCmd(ctx, cmd); err != nil { if !b.buildx { err = tryExecFormatErr(fmt.Errorf("running build: %w", err), errBuffer) @@ -301,12 +305,12 @@ func (b *Builder) computeCacheRefTag(ctx context.Context, artifactTag string, ca } if b.buildx { // compute the full cache reference (including registry, preserving tag) - log.Entry(ctx).Debugf("Expanding cache ref env template: %s", cacheRef) + log.Entry(ctx).Tracef("Expanding cache ref env template: %s", cacheRef) cacheRef, err = util.ExpandEnvTemplate(cacheRef, imageInfoEnv) if err != nil { log.Entry(ctx).Errorf("Couldn't expand cache image tag: %v", err) } - log.Entry(ctx).Infof("Parsing expanded cache ref: %s", cacheRef) + log.Entry(ctx).Tracef("Parsing expanded cache ref: %s", cacheRef) imgRef, err := docker.ParseReference(cacheRef) if err != nil { log.Entry(ctx).Errorf("Couldn't parse image tag %s: %v", cacheRef, err) @@ -323,7 +327,7 @@ func (b *Builder) computeCacheRefTag(ctx context.Context, artifactTag string, ca if tag == "" { log.Entry(ctx).Errorf("Invalid empty computed cache-tag") } - log.Entry(ctx).Debugf("Rewrite cache image base name: %s and tag: %s", imgRef.BaseName, tag) + log.Entry(ctx).Tracef("Rewrite cache image base name: %s and tag: %s", imgRef.BaseName, tag) cacheRef = fmt.Sprintf("%s:%s", imgRef.BaseName, tag) // sustitute the cache repository (registry): ref, err := docker.SubstituteDefaultRepoIntoImage(cacheRepo, multiLevel, cacheRef) @@ -333,7 +337,7 @@ func (b *Builder) computeCacheRefTag(ctx context.Context, artifactTag string, ca cacheRef = ref } } - log.Entry(ctx).Infof("Computed cache ref: %s", cacheRef) + log.Entry(ctx).Tracef("Computed cache ref: %s", cacheRef) return cacheRef } diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index a7133d9ef2d..2bef833c6e2 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -224,7 +224,7 @@ func GetCacheTag(configFile string) (string, error) { return "", err } if cfg.CacheTag != "" { - log.Entry(context.TODO()).Infof("Using cache-tag=%s from config", cfg.CacheTag) + log.Entry(context.TODO()).Debugf("Using cache-tag=%s from config", cfg.CacheTag) } return cfg.CacheTag, nil } @@ -236,7 +236,7 @@ func GetCacheRepo(configFile string) (string, error) { return "", err } if cfg.CacheRepo != "" { - log.Entry(context.TODO()).Infof("Using cache-repo=%s from config", cfg.CacheRepo) + log.Entry(context.TODO()).Debugf("Using cache-repo=%s from config", cfg.CacheRepo) } return cfg.CacheRepo, nil } @@ -248,7 +248,7 @@ func GetCacheFlags(configFile string) ([]string, error) { return nil, err } if len(cfg.CacheFlags) > 0 { - log.Entry(context.TODO()).Infof("Using cache-flags=%s from config", cfg.CacheFlags) + log.Entry(context.TODO()).Debugf("Using cache-flags=%s from config", cfg.CacheFlags) } return cfg.CacheFlags, nil } @@ -258,7 +258,7 @@ func GetBuildXBuilder(configFile string) string { if err != nil { log.Entry(context.TODO()).Errorf("Cannot read buildx-builder option from config: %v", err) } else if cfg.BuildXBuilder != "" { - log.Entry(context.TODO()).Infof("Using buildx-builder=%s from config", cfg.BuildXBuilder) + log.Entry(context.TODO()).Debugf("Using buildx-builder=%s from config", cfg.BuildXBuilder) return cfg.BuildXBuilder } return "" From 2ead20342a74bf6c74ef7b144e9e66e4fb37ebcd Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 3 Sep 2025 20:08:27 -0500 Subject: [PATCH 24/38] fix: ignore incorrectly parsed FROM in tagger #9830 Buildkit parser seems to return 'image:latest' when a FROM has a variable expansion instead of the real image template, like `$BASE` in the following stanza: ``` ARG BASE FROM $BASE AS base ``` This breaks input digest tagger for images that have a dependency to other skaffold build artifacts, as `parseOnbuild` later fails to pull the incorrect/inexistent image. Note that empty images in the from were being ignored, but no this incorrect value. So, as a workaround, this ondition is extended to include and ignore incorrect 'image:latest' values. Actual digest seems not affected, changes in parent base dockerfiles are still computed for the digest. A more complete fix would be inject the proper resolved image refs in `expandBuildArgs`, but this will need to solve the issue in buildkit parser to return the template. Also, probably it will require having pre-computed the previous image tags dependencies. Full error message: ``` generating tag: evaluating custom template component: parsing ONBUILD instructions: retrieving image \"image:latest\": GET https://index.docker.io/v2/library/image/manifests/latest: UNAUTHORIZED: authentication required; [map[Action:pull Class: Name:library/image Type:repository]] ``` --- pkg/skaffold/build/docker/docker.go | 9 +++++---- pkg/skaffold/docker/parse.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index a5b51419289..fe73ad07d53 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -271,7 +271,7 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT // append a new cache flag with the ref destination: cacheFlags = append(cacheFlags, fmt.Sprintf("ref=%s", cacheRef)) // format the flags (comma separated) and it to the new cache-to array - ct[i] = fmt.Sprintf("%s", strings.Join(cacheFlags, ",")) + ct[i] = strings.Join(cacheFlags, ",") } } } else { @@ -317,11 +317,12 @@ func (b *Builder) computeCacheRefTag(ctx context.Context, artifactTag string, ca } else { // determine the cache tag (use explicit tag, expanded cache-tag or fallback to image tag) tag := "" - if imgRef.Tag != "" { + switch { + case imgRef.Tag != "": tag = imgRef.Tag - } else if cacheTag == "" { + case cacheTag == "": tag = imageInfoEnv["IMAGE_TAG"] - } else { + default: tag = imageInfoEnv["CACHE_TAG"] } if tag == "" { diff --git a/pkg/skaffold/docker/parse.go b/pkg/skaffold/docker/parse.go index f1af6c4fc8f..1358aed3907 100644 --- a/pkg/skaffold/docker/parse.go +++ b/pkg/skaffold/docker/parse.go @@ -376,7 +376,7 @@ func expandOnbuildInstructions(ctx context.Context, nodes []*parser.Node, cfg Co var onbuildNodes []*parser.Node if ons, found := onbuildNodesCache[strings.ToLower(from.image)]; found { onbuildNodes = ons - } else if from.image == "" { + } else if from.image == "" || from.image == "image:latest" { // some build args like artifact dependencies are not available until the first build sequence has completed. // skip check if there are unavailable images onbuildNodes = []*parser.Node{} From ce068ebb67379f030d830e6d6475410cf6a79ba0 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 4 Sep 2025 17:18:04 -0500 Subject: [PATCH 25/38] fix: properly try import missing for local registries If using local clusters with a local registry (eg. localhost:32000 with microk8s), tryImport was failing as localhost is not a valid remote registry. The workaround will use a remote registry (default-repo global config) to override the local registry when pulling the image, and then re-tagging. --- pkg/skaffold/build/cache/lookup.go | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index 2866a7c5a61..2475f587bdf 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -20,11 +20,13 @@ import ( "context" "fmt" "io" + "strings" "sync" v1 "github.com/google/go-containerregistry/pkg/v1" specs "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/instrumentation" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" @@ -167,12 +169,41 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h } } if load { + log.Entry(ctx).Tracef("Loading artifact %s", tag) if !c.client.ImageExists(ctx, tag) { log.Entry(ctx).Debugf("Importing artifact %s from docker registry", tag) + orig := tag + if c.buildx && c.cfg.GetCluster().Local { + o := c.cfg.GetCluster().DefaultRepo + old := o.String() + new, _ := config.GetDefaultRepo(c.cfg.GlobalConfig(), nil) + log.Entry(ctx).Tracef("Local cluster registry: %s remote registry: %s", old, new) + // replace old local default-repo with new remote default-repo in the image tag (using substring replace) + if old != "" && new != "" && old != new { + tag = strings.ReplaceAll(tag, old, new) + log.Entry(ctx).Tracef("Rewriting remote artifact %s", tag) + } + } + log.Entry(ctx).Tracef("Importing artifact %s from docker registry", tag) err := c.client.Pull(ctx, io.Discard, tag, pl) if err != nil { + log.Entry(ctx).Warnf("Failed to import artifact %s from docker registry: %v", tag, err) return entry, err } + log.Entry(ctx).Infof("Imported artifact %s from docker registry", tag) + if c.buildx && orig != tag { + // retag the image to the original tag (with old local default-repo) + if err := c.client.Tag(ctx, tag, orig); err != nil { + log.Entry(ctx).Errorf("Failed to retag imported image %s to %s: %v", tag, orig, err) + } else { + log.Entry(ctx).Infof("Retagged imported image %s to %s", tag, orig) + if digest, err := c.client.Push(ctx, io.Discard, orig); err != nil { + log.Entry(ctx).Errorf("Failed to push retagged image %s: %v", orig, err) + } else { + log.Entry(ctx).Tracef("Pushed retagged image %s with digest %s", orig, digest) + } + } + } } else { log.Entry(ctx).Debugf("Importing artifact %s from local docker", tag) } From f7f79be62ce114125f8fa7c7dfef5c7a156a66a4 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 8 Sep 2025 19:23:56 -0500 Subject: [PATCH 26/38] refactor: fix & debug aids for input digest hashing * add relative path if missing (dockerfile in different context, probably related to #8226 and PR #9666) * hash filename after content (so it can be compared with md5sum) * trace messages for debugging These changes are handly in cases where the MD5 hash doesn't match. For example, there could be some transformations in remote builders, like git core.autocrlf changing line endings, that were hard to troubleshoot. --- pkg/skaffold/tag/input_digest.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/tag/input_digest.go b/pkg/skaffold/tag/input_digest.go index d144326aa5f..04a869e3396 100644 --- a/pkg/skaffold/tag/input_digest.go +++ b/pkg/skaffold/tag/input_digest.go @@ -72,6 +72,11 @@ func (t *inputDigestTagger) GenerateTag(ctx context.Context, image latest.Artifa // must sort as hashing is sensitive to the order in which files are processed sort.Strings(srcFiles) for _, d := range srcFiles { + // if the dependency is not an absolute path, we consider it relative to the workspace + // (or fileHasher will fail to find it) + if !filepath.IsAbs(d) { + d = filepath.Join(image.Workspace, d) + } h, err := fileHasher(d, image.Workspace) if err != nil { if os.IsNotExist(err) { @@ -80,6 +85,8 @@ func (t *inputDigestTagger) GenerateTag(ctx context.Context, image latest.Artifa } return "", fmt.Errorf("getting hash for %q: %w", d, err) + } else { + log.Entry(ctx).Tracef("dependency %q hash: %v", d, h) } inputs = append(inputs, h) } @@ -110,7 +117,7 @@ func fileHasher(path string, workspacePath string) (string, error) { if err != nil { pathToHash = path } - h.Write([]byte(pathToHash)) + log.Entry(context.TODO()).Tracef("Hashing file %q %s %s", pathToHash, workspacePath, path) if fi.Mode().IsRegular() { f, err := os.Open(path) @@ -121,6 +128,12 @@ func fileHasher(path string, workspacePath string) (string, error) { if _, err := io.Copy(h, f); err != nil { return "", err } + log.Entry(context.TODO()).Tracef("MD5 content hash for %s: %x", pathToHash, h.Sum(nil)) } + + // include file path in the hash to catch renames + // (after content has been hashed, so it is comparable with other tools like md5sum) + h.Write([]byte(pathToHash)) + return hex.EncodeToString(h.Sum(nil)), nil } From 687a7c023ed407e8ab039b27386e2485dcff8403 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 8 Sep 2025 20:19:23 -0500 Subject: [PATCH 27/38] fix #9830: ignore unresolved parsed FROM in tagger --- pkg/skaffold/docker/parse.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/docker/parse.go b/pkg/skaffold/docker/parse.go index af4229ca1e2..93311e233df 100644 --- a/pkg/skaffold/docker/parse.go +++ b/pkg/skaffold/docker/parse.go @@ -40,6 +40,8 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/proto/v1" ) +const buildkitUnresolvedImagePlaceholder = "image:latest" + type FromTo struct { // From is the relative path (wrt. the skaffold root directory) of the dependency on the host system. From string @@ -386,7 +388,7 @@ func expandOnbuildInstructions(ctx context.Context, nodes []*parser.Node, cfg Co var onbuildNodes []*parser.Node if ons, found := onbuildNodesCache[strings.ToLower(from.image)]; found { onbuildNodes = ons - } else if from.image == "" { + } else if from.image == "" || from.image == buildkitUnresolvedImagePlaceholder { // some build args like artifact dependencies are not available until the first build sequence has completed. // skip check if there are unavailable images onbuildNodes = []*parser.Node{} From b31e5cb3510d21aa05bb5401508d41639c52d367 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 8 Sep 2025 20:40:59 -0500 Subject: [PATCH 28/38] refactor #9830: simple artifact dep example --- examples/simple-artifact-dependency/app/Dockerfile | 9 +++++++-- examples/simple-artifact-dependency/skaffold.yaml | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/examples/simple-artifact-dependency/app/Dockerfile b/examples/simple-artifact-dependency/app/Dockerfile index 57dfed7ce53..1a25f16507c 100644 --- a/examples/simple-artifact-dependency/app/Dockerfile +++ b/examples/simple-artifact-dependency/app/Dockerfile @@ -1,5 +1,6 @@ ARG BASE -FROM golang:1.18 as builder + +FROM golang:1.18 AS builder WORKDIR /code COPY main.go . COPY go.mod . @@ -7,9 +8,13 @@ COPY go.mod . ARG SKAFFOLD_GO_GCFLAGS RUN go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -trimpath -o /app . -FROM $BASE +FROM $BASE AS base + +FROM scratch # Define GOTRACEBACK to mark this container as using the Go language runtime # for `skaffold debug` (https://skaffold.dev/docs/workflows/debug/). ENV GOTRACEBACK=single CMD ["./app"] COPY --from=builder /app . +COPY --from=base hello.txt . + diff --git a/examples/simple-artifact-dependency/skaffold.yaml b/examples/simple-artifact-dependency/skaffold.yaml index 7f4bf90e996..c5dfc73a3e2 100644 --- a/examples/simple-artifact-dependency/skaffold.yaml +++ b/examples/simple-artifact-dependency/skaffold.yaml @@ -9,6 +9,8 @@ build: alias: BASE - image: base context: base + tagPolicy: + inputDigest: {} manifests: rawYaml: - app/k8s-pod.yaml From 73d3f8ab4eb5983522dfa19866bc02927bb286aa Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 9 Sep 2025 12:36:11 -0500 Subject: [PATCH 29/38] ci: manual release exec --- .github/workflows/release.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 66c8d3cd67a..76201d1b1d0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,7 +1,10 @@ name: Release Standalone Executables # Triggers the workflow on push or pull request events -on: [push, pull_request] +on: + push: + pull_request: + workflow_dispatch: concurrency: group: build-${{ github.event.pull_request.number || github.ref }}-${{github.workflow}} From 2d1add2df536920fee0dd3769a6c5d95256e4c4e Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 9 Sep 2025 13:41:53 -0500 Subject: [PATCH 30/38] test: fix config UT --- cmd/skaffold/app/cmd/config/set_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/skaffold/app/cmd/config/set_test.go b/cmd/skaffold/app/cmd/config/set_test.go index 48510a57ebb..a06e0c5a5d2 100644 --- a/cmd/skaffold/app/cmd/config/set_test.go +++ b/cmd/skaffold/app/cmd/config/set_test.go @@ -415,7 +415,7 @@ func TestGetConfigStructWithIndex(t *testing.T) { description: "survey flag set", cfg: &config.ContextConfig{}, survey: true, - expectedIdx: []int{9}, + expectedIdx: []int{11}, }, { description: "no survey flag set", From 595e4d279706cf2899d8ac270bc4072679b61285 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 9 Sep 2025 13:45:13 -0500 Subject: [PATCH 31/38] test: fix UT for input digest hash --- pkg/skaffold/tag/input_digest_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/tag/input_digest_test.go b/pkg/skaffold/tag/input_digest_test.go index dc76985ce38..79f59f6c3e4 100644 --- a/pkg/skaffold/tag/input_digest_test.go +++ b/pkg/skaffold/tag/input_digest_test.go @@ -45,7 +45,7 @@ func TestInputDigest(t *testing.T) { t.RequireNoError(os.WriteFile(file, fileContents1, 0644)) relPathHash, err := fileHasher(file, ".") - t.CheckErrorAndDeepEqual(false, err, "3cced2dec96a8b41b22875686d8941a9", relPathHash) + t.CheckErrorAndDeepEqual(false, err, "f68f0e22ae9dc02857924d257668431c", relPathHash) absPathHash, err := fileHasher(filepath.Join(dir, file), dir) t.CheckErrorAndDeepEqual(false, err, relPathHash, absPathHash) }) @@ -57,7 +57,7 @@ func TestInputDigest(t *testing.T) { t.RequireNoError(os.WriteFile(file2, fileContents1, 0644)) hash1, err := fileHasher(file1, dir1) - t.CheckErrorAndDeepEqual(false, err, "3cced2dec96a8b41b22875686d8941a9", hash1) + t.CheckErrorAndDeepEqual(false, err, "f68f0e22ae9dc02857924d257668431c", hash1) hash2, err := fileHasher(file2, dir2) t.CheckErrorAndDeepEqual(false, err, hash1, hash2) }) From 25faeef110277fbd1a59f76187e5a4579f2a6813 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Tue, 9 Sep 2025 14:10:11 -0500 Subject: [PATCH 32/38] test: fix UT for docker buildx cache-flags --- pkg/skaffold/build/docker/docker_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go index 526a4554709..2d79f04c7a6 100644 --- a/pkg/skaffold/build/docker/docker_test.go +++ b/pkg/skaffold/build/docker/docker_test.go @@ -106,7 +106,7 @@ func TestDockerCLIBuild(t *testing.T) { buildx: true, daemonless: true, imageName: "gcr.io/k8s-skaffold/example:tag", - expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max,image-manifest=true,oci-mediatypes=true", "--push", "--builder", "default", "--metadata-file", "metadata.json"}, + expectedCLIFlags: []string{"--cache-from", "gcr.io/k8s-skaffold/example:cache", "--cache-to", "type=registry,mode=max,image-manifest=true,oci-mediatypes=true,ref=gcr.io/k8s-skaffold/example:cache", "--push", "--builder", "default", "--metadata-file", "metadata.json"}, expectedEnv: []string{"KEY=VALUE", "DOCKER_BUILDKIT=1"}, }, { @@ -195,7 +195,11 @@ func TestDockerCLIBuild(t *testing.T) { return os.Open("metadata.json") }) t.Override(&config.GetConfigForCurrentKubectx, func(configFile string) (*config.ContextConfig, error) { - return &config.ContextConfig{CacheTag: "cache", BuildXBuilder: "default"}, nil + var flags []string + if test.localBuild.Push != nil && *test.localBuild.Push { + flags = append(flags, "type=registry", "mode=max", "image-manifest=true", "oci-mediatypes=true") + } + return &config.ContextConfig{CacheTag: "cache", BuildXBuilder: "default", CacheFlags: flags}, nil }) var mockCmd *testutil.FakeCmd From b7d157a9a4fa11bdce962b9ae323fee100b21b41 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Thu, 18 Sep 2025 19:08:21 -0500 Subject: [PATCH 33/38] fix: avoid verify empty container cmd/args Do not copy container commands or arguments if not specified in skaffold.yaml If not, original command/argument defined in the job will be overwritten with nil. --- pkg/skaffold/verify/k8sjob/verify.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/verify/k8sjob/verify.go b/pkg/skaffold/verify/k8sjob/verify.go index 784ea8f738f..28946a3b9a2 100644 --- a/pkg/skaffold/verify/k8sjob/verify.go +++ b/pkg/skaffold/verify/k8sjob/verify.go @@ -47,6 +47,7 @@ import ( kubernetesclient "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/client" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/loader" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/log" + olog "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/status" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" @@ -213,6 +214,7 @@ func (v *Verifier) createAndRunJob(ctx context.Context, tc latest.VerifyTestCase // This is because the k8s API server can be unresponsive when hit with a large // intitial set of Job CREATE requests if waitErr := wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) { + olog.Entry(context.TODO()).Debugf("Creating verify job in cluster: %+v\n", job) _, err = clientset.BatchV1().Jobs(job.Namespace).Create(ctx, job, metav1.CreateOptions{}) if err != nil { return false, nil @@ -275,10 +277,12 @@ func (v *Verifier) watchJob(ctx context.Context, clientset k8sclient.Interface, pod, ok := event.Object.(*corev1.Pod) if ok { if pod.Status.Phase == corev1.PodSucceeded { + olog.Entry(context.TODO()).Debugf("Verify pod succeeded: %+v\n", pod) // TODO(aaron-prindle) add support for jobs w/ multiple pods in the future break } if pod.Status.Phase == corev1.PodFailed { + olog.Entry(context.TODO()).Debugf("Verify pod failed: %+v\n", pod) failReason := pod.Status.Reason if failReason == "" { failReason = "" @@ -326,6 +330,7 @@ func (v *Verifier) Cleanup(ctx context.Context, out io.Writer, dryRun bool) erro // assumes the job namespace is set and not "" which is the case as createJob // & createJobFromManifestPath set the namespace in the created Job namespace := job.Namespace + olog.Entry(context.TODO()).Debugf("Cleaning up job %q in namespace %q", job.Name, namespace) if err := k8sjobutil.ForceJobDelete(ctx, job.Name, clientset.BatchV1().Jobs(namespace), &v.kubectl); err != nil { // TODO(aaron-prindle): replace with actionable error return errors.Wrap(err, "cleaning up deployed job") @@ -389,13 +394,17 @@ func (v *Verifier) createJobFromManifestPath(jobName string, container latest.Ve job.Name = jobName job.Labels["skaffold.dev/run-id"] = v.labeller.GetRunID() var original corev1.Container + olog.Entry(context.TODO()).Tracef("Lookging for container %s in %+v\n", container.Name, job.Spec.Template.Spec.Containers) for _, c := range job.Spec.Template.Spec.Containers { if c.Name == container.Name { original = c + olog.Entry(context.TODO()).Tracef("Found container %+v\n", c) break } } + olog.Entry(context.TODO()).Tracef("Original containers from manifest: %+v\n", original) patchToK8sContainer(container, &original) + olog.Entry(context.TODO()).Tracef("Patched containers: %+v\n", original) job.Spec.Template.Spec.Containers = []corev1.Container{original} job.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyNever @@ -411,8 +420,12 @@ func (v *Verifier) createJobFromManifestPath(jobName string, container latest.Ve func patchToK8sContainer(container latest.VerifyContainer, dst *corev1.Container) { dst.Image = container.Image - dst.Command = container.Command - dst.Args = container.Args + if container.Command != nil { + dst.Command = container.Command + } + if container.Args != nil { + dst.Args = container.Args + } dst.Name = container.Name for _, e := range container.Env { From 28036fbc4a270ba0dad63bb7b6b87747d8da1186 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 1 Oct 2025 19:07:31 -0500 Subject: [PATCH 34/38] fix: default registry mirror #7368 Workaround to avoid Docker Hub pull rate limits. Skaffold now will parse the name and replace default registry index.docker.io with a configured one. Configuration example: ``` skaffold config set registry-mirror mirror.gcr.io ``` Log with the fix: ``` time="2025-10-01T19:19:41-05:00" level=trace msg="Checking base image python:2.7-alpine for ONBUILD triggers." subtask=-1 task=DevLoop time="2025-10-01T19:19:41-05:00" level=info msg="Using registry-mirror=mirror.gcr.io from config" subtask=-1 task=DevLoop time="2025-10-01T19:19:41-05:00" level=info msg="Using default registry = mirror.gcr.io for reference parsing" subtask=-1 task=DevLoop ... time="2025-10-01T19:20:09-05:00" level=trace msg="--> GET https://mirror.gcr.io/v2/python/manifests/2.7-alpine" ... time="2025-10-01T19:20:09-05:00" level=trace msg="<-- 200 https://mirror.gcr.io/v2/python/manifests/2.7-alpine (215.992197ms)" ``` Previous log broken build (no mirror): ``` getting hash for artifact "...": getting dependencies for "...": parsing ONBUILD instructions: retrieving image "python:2.7-alpine": GET https://index.docker.io/v2/library/python/manifests/2.7-alpine: TOOMANYREQUESTS: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit ``` NOTE: not a full fix as go-containerregistry doesn't support `WithMirror` yet: https://github.com/google/go-containerregistry/issues/1200 --- pkg/skaffold/config/global_config.go | 1 + pkg/skaffold/config/util.go | 13 +++++++++++++ pkg/skaffold/docker/remote.go | 6 ++++++ 3 files changed, 20 insertions(+) diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go index ef4a9b7f41e..1fb6ac65cd5 100644 --- a/pkg/skaffold/config/global_config.go +++ b/pkg/skaffold/config/global_config.go @@ -33,6 +33,7 @@ type ContextConfig struct { InsecureRegistries []string `yaml:"insecure-registries,omitempty"` // DebugHelpersRegistry is the registry from which the debug helper images are used. DebugHelpersRegistry string `yaml:"debug-helpers-registry,omitempty"` + RegistryMirror string `yaml:"registry-mirror,omitempty"` CacheTag string `yaml:"cache-tag,omitempty"` CacheRepo string `yaml:"cache-repo,omitempty"` CacheFlags []string `yaml:"cache-flags,omitempty"` diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go index 2bef833c6e2..4644f1b3e9b 100644 --- a/pkg/skaffold/config/util.go +++ b/pkg/skaffold/config/util.go @@ -217,6 +217,19 @@ func GetDebugHelpersRegistry(configFile string) (string, error) { return constants.DefaultDebugHelpersRegistry, nil } +func GetRegistryMirror(configFile string) (string, error) { + cfg, err := GetConfigForCurrentKubectx(configFile) + if err != nil { + return "", err + } + + if cfg.RegistryMirror != "" { + log.Entry(context.TODO()).Infof("Using registry-mirror=%s from config", cfg.RegistryMirror) + return cfg.RegistryMirror, nil + } + return "", nil +} + func GetCacheTag(configFile string) (string, error) { cfg, err := GetConfigForCurrentKubectx(configFile) if err != nil { diff --git a/pkg/skaffold/docker/remote.go b/pkg/skaffold/docker/remote.go index 1ecefe2f60a..eb98c30bd54 100644 --- a/pkg/skaffold/docker/remote.go +++ b/pkg/skaffold/docker/remote.go @@ -26,6 +26,7 @@ import ( "github.com/google/go-containerregistry/pkg/v1/tarball" specs "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" sErrors "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/errors" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" @@ -145,6 +146,11 @@ func IsInsecure(ref name.Reference, insecureRegistries map[string]bool) bool { } func parseReference(s string, cfg Config, opts ...name.Option) (name.Reference, error) { + mirror, _ := config.GetRegistryMirror(cfg.GlobalConfig()) + if mirror != "" { + log.Entry(context.TODO()).Infof("Using default registry = %s for reference parsing", mirror) + opts = append(opts, name.WithDefaultRegistry(mirror)) + } ref, err := name.ParseReference(s, opts...) if err != nil { return nil, fmt.Errorf("parsing reference %q: %w", s, err) From 63eada2f49c17e98b4982d46de5f9acd5c7b6637 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 1 Oct 2025 19:45:29 -0500 Subject: [PATCH 35/38] ci: generate pre-release on next --- .github/workflows/release.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 76201d1b1d0..c4de2512a78 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -81,6 +81,8 @@ jobs: else echo "final_release=latest" >> $GITHUB_ENV fi + elif [ "${{ github.ref }}" = "refs/heads/next" ]; then + echo "final_release=prep" >> $GITHUB_ENV else echo "final_release=draft" >> $GITHUB_ENV fi From b2427d9f9685dc4dc1be0019bc258631097b31c3 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 1 Oct 2025 19:47:44 -0500 Subject: [PATCH 36/38] test: fix UT for contex-config --- cmd/skaffold/app/cmd/config/set_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/skaffold/app/cmd/config/set_test.go b/cmd/skaffold/app/cmd/config/set_test.go index a06e0c5a5d2..ffcdbfb15ed 100644 --- a/cmd/skaffold/app/cmd/config/set_test.go +++ b/cmd/skaffold/app/cmd/config/set_test.go @@ -415,7 +415,7 @@ func TestGetConfigStructWithIndex(t *testing.T) { description: "survey flag set", cfg: &config.ContextConfig{}, survey: true, - expectedIdx: []int{11}, + expectedIdx: []int{12}, }, { description: "no survey flag set", From d52616a999497f9aa90bc43e5fb036db07f9f91a Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 1 Oct 2025 21:35:48 -0500 Subject: [PATCH 37/38] refactor: reduce log-level & print a warn --- pkg/skaffold/docker/remote.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/docker/remote.go b/pkg/skaffold/docker/remote.go index eb98c30bd54..f80cad746c1 100644 --- a/pkg/skaffold/docker/remote.go +++ b/pkg/skaffold/docker/remote.go @@ -19,6 +19,7 @@ package docker import ( "context" "fmt" + "strings" "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" @@ -146,15 +147,20 @@ func IsInsecure(ref name.Reference, insecureRegistries map[string]bool) bool { } func parseReference(s string, cfg Config, opts ...name.Option) (name.Reference, error) { + // if a mirror was configured, use it as the default registry (instead of docker.io): mirror, _ := config.GetRegistryMirror(cfg.GlobalConfig()) if mirror != "" { - log.Entry(context.TODO()).Infof("Using default registry = %s for reference parsing", mirror) + log.Entry(context.TODO()).Debugf("Using default registry = %s as mirror", mirror) opts = append(opts, name.WithDefaultRegistry(mirror)) } ref, err := name.ParseReference(s, opts...) if err != nil { return nil, fmt.Errorf("parsing reference %q: %w", s, err) } + // if the image was rewritten to use a mirror, warn the user to avoid surprises & troubleshooting: + if mirror != "" && ref.Context().Registry.Name() == mirror && !strings.Contains(s, mirror) && s != "image:latest" { + log.Entry(context.TODO()).Warnf("Rewrote image ref to use %s registry mirror: %q", mirror, ref) + } if IsInsecure(ref, cfg.GetInsecureRegistries()) { ref, err = name.ParseReference(s, name.Insecure) From 8e46fc40bc4f930fa2944719efe0daedf51fda8c Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Wed, 8 Oct 2025 23:18:24 -0500 Subject: [PATCH 38/38] ci: fix latest release --- .github/workflows/release.yml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c4de2512a78..a8a0dfda16f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -74,13 +74,11 @@ jobs: - name: Determine Release Type id: determine_release run: | - if [ "${{ github.ref }}" = "refs/heads/main" ]; then - if git rev-parse "${VERSION}" >/dev/null 2>&1; then - echo "Git tag ${VERSION} already exists. Skipping final release." - echo "final_release=prep" >> $GITHUB_ENV - else - echo "final_release=latest" >> $GITHUB_ENV - fi + if git rev-parse "${VERSION}" >/dev/null 2>&1; then + echo "Git tag ${VERSION} already exists. Skipping final release." + echo "final_release=tag" >> $GITHUB_ENV + elif [ "${{ github.ref }}" = "refs/heads/main" ]; then + echo "final_release=latest" >> $GITHUB_ENV elif [ "${{ github.ref }}" = "refs/heads/next" ]; then echo "final_release=prep" >> $GITHUB_ENV else @@ -95,6 +93,9 @@ jobs: if [ "$FINAL_RELEASE" = "latest" ]; then RELEASE_NAME="${VERSION}" PUBLISH="--latest" + elif [ "$FINAL_RELEASE" = "tag" ]; then + RELEASE_NAME="${VERSION}" + PUBLISH="--verify-tag" else TIMESTAMP=$(date +'%Y%m%d-%H%M%S') RELEASE_NAME="${VERSION}-${TIMESTAMP}"