Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make IMAGE_TAG available in buildArgs when used in docker FROM #9450

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/skaffold/build/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type cache struct {
}

// DependencyLister fetches a list of dependencies for an artifact
type DependencyLister func(ctx context.Context, artifact *latest.Artifact) ([]string, error)
type DependencyLister func(ctx context.Context, artifact *latest.Artifact, tags map[string]string) ([]string, error)

type Config interface {
docker.Config
Expand Down
16 changes: 8 additions & 8 deletions pkg/skaffold/build/cache/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ var (
)

type artifactHasher interface {
hash(context.Context, *latest.Artifact, platform.Resolver) (string, error)
hash(context.Context, *latest.Artifact, platform.Resolver, map[string]string) (string, error)
}

type artifactHasherImpl struct {
Expand All @@ -67,20 +67,20 @@ func newArtifactHasher(artifacts graph.ArtifactGraph, lister DependencyLister, m
}
}

func (h *artifactHasherImpl) hash(ctx context.Context, a *latest.Artifact, platforms platform.Resolver) (string, error) {
func (h *artifactHasherImpl) hash(ctx context.Context, a *latest.Artifact, platforms platform.Resolver, tags map[string]string) (string, error) {
ctx, endTrace := instrumentation.StartTrace(ctx, "hash_GenerateHashOneArtifact", map[string]string{
"ImageName": instrumentation.PII(a.ImageName),
})
defer endTrace()

hash, err := h.safeHash(ctx, a, platforms.GetPlatforms(a.ImageName))
hash, err := h.safeHash(ctx, a, platforms.GetPlatforms(a.ImageName), tags)
if err != nil {
endTrace(instrumentation.TraceEndError(err))
return "", err
}
hashes := []string{hash}
for _, dep := range sortedDependencies(a, h.artifacts) {
depHash, err := h.hash(ctx, dep, platforms)
depHash, err := h.hash(ctx, dep, platforms, tags)
if err != nil {
endTrace(instrumentation.TraceEndError(err))
return "", err
Expand All @@ -94,15 +94,15 @@ func (h *artifactHasherImpl) hash(ctx context.Context, a *latest.Artifact, platf
return encode(hashes)
}

func (h *artifactHasherImpl) safeHash(ctx context.Context, a *latest.Artifact, platforms platform.Matcher) (string, error) {
func (h *artifactHasherImpl) safeHash(ctx context.Context, a *latest.Artifact, platforms platform.Matcher, tags map[string]string) (string, error) {
return h.syncStore.Exec(a.ImageName,
func() (string, error) {
return singleArtifactHash(ctx, h.lister, a, h.mode, platforms)
return singleArtifactHash(ctx, h.lister, a, h.mode, platforms, tags)
})
}

// singleArtifactHash calculates the hash for a single artifact, and ignores its required artifacts.
func singleArtifactHash(ctx context.Context, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher) (string, error) {
func singleArtifactHash(ctx context.Context, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher, tags map[string]string) (string, error) {
var inputs []string

// Append the artifact's configuration
Expand All @@ -113,7 +113,7 @@ func singleArtifactHash(ctx context.Context, depLister DependencyLister, a *late
inputs = append(inputs, config)

// Append the digest of each input file
deps, err := depLister(ctx, a)
deps, err := depLister(ctx, a, tags)
if err != nil {
return "", fmt.Errorf("getting dependencies for %q: %w", a.ImageName, err)
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/skaffold/build/cache/lookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, platfor

i := i
go func() {
details[i] = c.lookup(ctx, artifacts[i], tags[artifacts[i].ImageName], platforms, h)
details[i] = c.lookup(ctx, artifacts[i], tags, platforms, h)
wg.Done()
}()
}
Expand All @@ -57,13 +57,14 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, platfor
return details
}

func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, platforms platform.Resolver, h artifactHasher) cacheDetails {
func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tags map[string]string, platforms platform.Resolver, h artifactHasher) cacheDetails {
tag := tags[a.ImageName]
ctx, endTrace := instrumentation.StartTrace(ctx, "lookup_CacheLookupOneArtifact", map[string]string{
"ImageName": instrumentation.PII(a.ImageName),
})
defer endTrace()

hash, err := h.hash(ctx, a, platforms)
hash, err := h.hash(ctx, a, platforms, tags)
if err != nil {
return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/build/gcb/cloud_build.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer
})
}

dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact)
dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil)
if err != nil {
return "", sErrors.NewErrorWithStatusCode(&proto.ActionableErr{
ErrCode: proto.StatusCode_BUILD_GCB_GET_DEPENDENCY_ERR,
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/diagnose/diagnose.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg Config)
start := time.Now()
g := graph.ToArtifactGraph(cfg.Artifacts())
sourceDependencies := graph.NewSourceDependenciesCache(cfg, nil, g)
paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a)
paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a, nil)
return timeutil.Humanize(time.Since(start)), paths, err
}

Expand Down
29 changes: 23 additions & 6 deletions pkg/skaffold/graph/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type SourceDependenciesCache interface {
TransitiveArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error)
// SingleArtifactDependencies returns the source dependencies for only the target artifact.
// The result (even if an error) is cached so that the function is evaluated only once for every artifact. The cache is reset before the start of the next devloop.
SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error)
SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tags map[string]string) ([]string, error)
// Reset removes the cached source dependencies for all artifacts
Reset()
}
Expand All @@ -65,7 +65,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont
})
defer endTrace()

deps, err := r.SingleArtifactDependencies(ctx, a)
deps, err := r.SingleArtifactDependencies(ctx, a, nil)
if err != nil {
endTrace(instrumentation.TraceEndError(err))
return nil, err
Expand All @@ -81,14 +81,14 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont
return deps, nil
}

func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) {
func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tags map[string]string) ([]string, error) {
ctx, endTrace := instrumentation.StartTrace(ctx, "SingleArtifactDependencies", map[string]string{
"ArtifactName": instrumentation.PII(a.ImageName),
})
defer endTrace()

res, err := r.cache.Exec(a.ImageName, func() ([]string, error) {
return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver)
return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver, tags)
})
if err != nil {
endTrace(instrumentation.TraceEndError(err))
Expand All @@ -104,8 +104,20 @@ func (r *dependencyResolverImpl) Reset() {
r.cache = util.NewSyncStore[[]string]()
}

func quickMakeEnvTags(tag string) (map[string]string, error) {
imgRef, err := docker.ParseReference(tag)
if err != nil {
return nil, fmt.Errorf("couldn't parse image tag %s %w", tag, err)
}
return map[string]string{
"IMAGE_REPO": imgRef.Repo,
"IMAGE_NAME": imgRef.Name,
"IMAGE_TAG": imgRef.Tag,
}, nil
}

// sourceDependenciesForArtifact returns the build dependencies for the current artifact.
func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver) ([]string, error) {
func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, tags map[string]string) ([]string, error) {
var (
paths []string
err error
Expand All @@ -118,7 +130,12 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg
// For single build scenarios like `build` and `run`, it is called for the cache hash calculations which are already handled in `artifactHasher`.
// For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`)
deps := docker.ResolveDependencyImages(a.Dependencies, r, false)
args, evalErr := docker.EvalBuildArgs(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps)
var envTags map[string]string
envTags, err = quickMakeEnvTags(tags[a.ImageName])
if err != nil {
return nil, err
}
args, evalErr := docker.EvalBuildArgsWithEnv(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps, envTags)
if evalErr != nil {
return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/runner/new.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold
return nil, fmt.Errorf("creating actiosn runner: %w", err)
}

depLister := func(ctx context.Context, artifact *latest.Artifact) ([]string, error) {
depLister := func(ctx context.Context, artifact *latest.Artifact, tags map[string]string) ([]string, error) {
ctx, endTrace := instrumentation.StartTrace(ctx, "NewForConfig_depLister")
defer endTrace()

buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact)
buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, tags)
if err != nil {
endTrace(instrumentation.TraceEndError(err))
return nil, err
Expand Down