Skip to content

Commit

Permalink
fix: make IMAGE_TAG available in buildArgs when used in docker `FRO…
Browse files Browse the repository at this point in the history
…M` (#9664)

* provide a docker.ImageReference in dependency resolution step

* pass nils

* other plumbing

* fix: add back out io.Writer field

* fix bugs

* fix lookup and hash tests

* fix tag test

* fix skaffold/runner

* fix skaffold/graph

* remove the tags field where its not needed

* make fixes to test

* fix tests

* fix another typo

* add license boilerplate

* add 2025 as a valid year

* chore:add integration test

* format go file

* fix: lint and test

* fix test

* fix:lint

* add debug output

* add test action for debugging

* add test

* add test

* add test

* add test

* add test

* use the default context when building the image

* fix imports

* fix: flaky helm deploy unit test by making the order of helm command calls consistent

* fix lint

* add unit tests and fix existing tests

* remove xtra file

* fix lint

* test that the build arg used as part of a filename is also replaced

* use stub instead of mock for test objects

* use empty string in place of tag because hash_test unit tests mock SingleArtifactDependencies so the value doesn't matter

* add integration test to ensure that the build int BUILD args can be used as part of a file

* fix imports, remove github action workflow used for debugging

* fix lint

* use one function everywhere in the codebase to generate the image info tags

* Replace use of EvalBuildArgs with EvalBuildArgsWithEnv since they invoke the same function

* add unit test for hash package

* add test for TestCheckArtifacts

---------

Co-authored-by: Abe Winter <[email protected]>
  • Loading branch information
alphanota and abe-winter authored Jan 23, 2025
1 parent 4aeb393 commit 93e325b
Show file tree
Hide file tree
Showing 40 changed files with 659 additions and 226 deletions.
13 changes: 11 additions & 2 deletions cmd/skaffold/app/cmd/diagnose.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ import (

"github.com/spf13/cobra"

deployutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/util"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/diagnose"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/parser"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
schemaUtil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tags"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/version"
Expand Down Expand Up @@ -106,13 +108,20 @@ func printArtifactDiagnostics(ctx context.Context, out io.Writer, configs []sche
if err != nil {
return fmt.Errorf("getting run context: %w", err)
}
tagger, err := tag.NewTaggerMux(runCtx)
if err != nil {
return fmt.Errorf("getting tagger: %w", err)
}
imageTags, err := deployutil.ImageTags(ctx, runCtx, tagger, out, runCtx.Artifacts())
if err != nil {
return fmt.Errorf("getting tags: %w", err)
}
for _, c := range configs {
config := c.(*latest.SkaffoldConfig)
fmt.Fprintln(out, "Skaffold version:", version.Get().GitCommit)
fmt.Fprintln(out, "Configuration version:", config.APIVersion)
fmt.Fprintln(out, "Number of artifacts:", len(config.Build.Artifacts))

if err := diagnose.CheckArtifacts(ctx, runCtx, out); err != nil {
if err := diagnose.CheckArtifacts(ctx, runCtx, imageTags, out); err != nil {
return fmt.Errorf("running diagnostic on artifacts: %w", err)
}

Expand Down
2 changes: 1 addition & 1 deletion hack/boilerplate/boilerplate.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def get_regexs():
# Search for "YEAR" which exists in the boilerplate, but shouldn't in the real thing
regexs["year"] = re.compile( 'YEAR' )
# dates can be 2018, company holder names can be anything
regexs["date"] = re.compile( '(2019|2020|2021|2022|2023|2024)' )
regexs["date"] = re.compile( '(2019|2020|2021|2022|2023|2024|2025)' )
# strip // +build \n\n build constraints
regexs["go_build_constraints"] = re.compile(r"^(// \+build.*\n)+\n", re.MULTILINE)
# strip //go:build \n build constraints (for go1.17 and higher)
Expand Down
49 changes: 49 additions & 0 deletions integration/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
v1 "github.com/opencontainers/image-spec/specs-go/v1"
"k8s.io/apimachinery/pkg/util/wait"

"github.com/GoogleContainerTools/skaffold/v2/cmd/skaffold/app/flags"
"github.com/GoogleContainerTools/skaffold/v2/integration/skaffold"
Expand Down Expand Up @@ -331,3 +332,51 @@ func failNowIfError(t Fataler, err error) {
t.Fatal(err)
}
}

func TestRunWithDockerAndBuildArgs(t *testing.T) {
tests := []struct {
description string
projectDir string
skaffoldArgs []string
dockerRunArgs []string
wantOutput string
}{
{
description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to Docker build as build args",
projectDir: "testdata/docker-run-with-build-args/artifact-with-dependency",
skaffoldArgs: []string{"--kube-context", "default"},
dockerRunArgs: []string{"run", "child:latest"},
wantOutput: "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest",
},
{
description: "IMAGE_TAG can be used as a part of a filename in the Dockerfile",
projectDir: "testdata/docker-run-with-build-args/single-artifact",
skaffoldArgs: []string{"--kube-context", "default"},
dockerRunArgs: []string{"run", "example:latest"},
wantOutput: "HELLO WORLD",
},
}

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
defer skaffold.Delete().InDir(test.projectDir).Run(t.T)
if err := skaffold.Build(test.skaffoldArgs...).InDir(test.projectDir).Run(t.T); err != nil {
t.Errorf("skaffold build args: %v working directory:%s returned unexpected error: %v", test.skaffoldArgs, test.projectDir, err)
}

got := ""

err := wait.PollImmediate(time.Millisecond*500, 1*time.Minute, func() (bool, error) {
out, _ := exec.Command("docker", test.dockerRunArgs...).Output()
t.Logf("Output:[%s]\n", out)
got = strings.Trim(string(out), " \n")
return got == test.wantOutput, nil
})

if err != nil {
t.Errorf("docker run produced incorrect output, got:[%s], want:[%s], err: %v", got, test.wantOutput, err)
}
failNowIfError(t, err)
})
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
FROM golang:1.23 as builder
WORKDIR /code
COPY main.go .
COPY go.mod .
# `skaffold debug` sets SKAFFOLD_GO_GCFLAGS to disable compiler optimizations
ARG SKAFFOLD_GO_GCFLAGS
ARG IMAGE_REPO
ARG IMAGE_NAME
ARG IMAGE_TAG
RUN go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -ldflags="-X main.ImageRepo=${IMAGE_REPO} -X main.ImageName=${IMAGE_NAME} -X main.ImageTag=${IMAGE_TAG}" -trimpath -o /app main.go

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module github.com/GoogleContainerTools/skaffold/examples/docker-deploy-with-build-args/base

go 1.23
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package main

import (
"fmt"
)

var ImageRepo = "unknown"
var ImageTag = "unknown"
var ImageName = "unknown"

func main() {
output := fmt.Sprintf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s\n", ImageRepo, ImageName, ImageTag)
fmt.Println(output)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
ARG BASE
FROM $BASE as parent
FROM alpine
COPY --from=parent /app .
CMD ["./app"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
apiVersion: skaffold/v4beta12
kind: Config
build:
tagPolicy:
sha256: {}
local:
push: false
useDockerCLI: true
artifacts:
- image: gcr.io/k8s-skaffold/skaffold
context: base
docker:
dockerfile: Dockerfile
noCache: true
buildArgs:
IMAGE_REPO: '{{.IMAGE_REPO}}'
IMAGE_NAME: '{{.IMAGE_NAME}}'
IMAGE_TAG: '{{.IMAGE_TAG}}'
- image: child
requires:
- image: gcr.io/k8s-skaffold/skaffold
alias: BASE
context: child
docker:
dockerfile: Dockerfile
noCache: true
deploy:
docker:
images: [child]
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM busybox
ARG IMAGE_TAG
RUN echo $IMAGE_TAG
COPY "script-${IMAGE_TAG}.sh" script.sh
CMD ["/bin/sh","script.sh"]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
echo "HELLO WORLD"
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
apiVersion: skaffold/v4beta12
kind: Config
build:
tagPolicy:
sha256: {}
local:
push: false
useDockerCLI: true
artifacts:
- image: example
docker:
dockerfile: Dockerfile
noCache: true
buildArgs:
IMAGE_TAG: '{{.IMAGE_TAG}}'
deploy:
docker:
images: [example]
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, tag string) ([]string, error)

type Config interface {
docker.Config
Expand Down
30 changes: 18 additions & 12 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, io.Writer, *latest.Artifact, platform.Resolver) (string, error)
hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, 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, out io.Writer, a *latest.Artifact, platforms platform.Resolver) (string, error) {
func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Resolver, tag 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, out, a, platforms.GetPlatforms(a.ImageName))
hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName), tag)
if err != nil {
endTrace(instrumentation.TraceEndError(err))
return "", err
}
hashes := []string{hash}
for _, dep := range sortedDependencies(a, h.artifacts) {
depHash, err := h.hash(ctx, out, dep, platforms)
depHash, err := h.hash(ctx, out, dep, platforms, tag)
if err != nil {
endTrace(instrumentation.TraceEndError(err))
return "", err
Expand All @@ -94,15 +94,15 @@ func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest.
return encode(hashes)
}

func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher) (string, error) {
func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher, tag string) (string, error) {
return h.syncStore.Exec(a.ImageName,
func() (string, error) {
return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms)
return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms, tag)
})
}

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

// Append the artifact's configuration
Expand All @@ -113,7 +113,7 @@ func singleArtifactHash(ctx context.Context, out io.Writer, depLister Dependency
inputs = append(inputs, config)

// Append the digest of each input file
deps, err := depLister(ctx, a)
deps, err := depLister(ctx, a, tag)
if err != nil {
return "", fmt.Errorf("getting dependencies for %q: %w", a.ImageName, err)
}
Expand All @@ -133,7 +133,7 @@ func singleArtifactHash(ctx context.Context, out io.Writer, depLister Dependency
}

// add build args for the artifact if specified
args, err := hashBuildArgs(out, a, mode)
args, err := hashBuildArgs(out, a, tag, mode)
if err != nil {
return "", fmt.Errorf("hashing build args: %w", err)
}
Expand Down Expand Up @@ -171,16 +171,22 @@ func artifactConfig(a *latest.Artifact) (string, error) {
return string(buf), nil
}

func hashBuildArgs(out io.Writer, artifact *latest.Artifact, mode config.RunMode) ([]string, error) {
func hashBuildArgs(out io.Writer, artifact *latest.Artifact, tag string, mode config.RunMode) ([]string, error) {
// only one of args or env is ever populated
var args map[string]*string
var env map[string]string
var err error

envTags, evalErr := docker.EnvTags(tag)
if evalErr != nil {
return nil, fmt.Errorf("unable to create build args: %w", err)
}

switch {
case artifact.DockerArtifact != nil:
args, err = docker.EvalBuildArgs(mode, artifact.Workspace, artifact.DockerArtifact.DockerfilePath, artifact.DockerArtifact.BuildArgs, nil)
args, err = docker.EvalBuildArgsWithEnv(mode, artifact.Workspace, artifact.DockerArtifact.DockerfilePath, artifact.DockerArtifact.BuildArgs, nil, envTags)
case artifact.KanikoArtifact != nil:
args, err = docker.EvalBuildArgs(mode, kaniko.GetContext(artifact.KanikoArtifact, artifact.Workspace), artifact.KanikoArtifact.DockerfilePath, artifact.KanikoArtifact.BuildArgs, nil)
args, err = docker.EvalBuildArgsWithEnv(mode, kaniko.GetContext(artifact.KanikoArtifact, artifact.Workspace), artifact.KanikoArtifact.DockerfilePath, artifact.KanikoArtifact.BuildArgs, nil, envTags)
case artifact.BuildpackArtifact != nil:
env, err = buildpacks.GetEnv(out, artifact, mode)
case artifact.CustomArtifact != nil && artifact.CustomArtifact.Dependencies.Dockerfile != nil:
Expand Down
Loading

0 comments on commit 93e325b

Please sign in to comment.