Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@imjasonh @tekton-robot
imjasonh authored and tekton-robot committed Jan 4, 2022
1 parent eb953aa commit 2e9c68c
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 38 deletions.
30 changes: 11 additions & 19 deletions pkg/pod/entrypoint_lookup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ import (
// EntrypointCache looks up an image's entrypoint (command) in a container
// image registry, possibly using the given service account's credentials.
type EntrypointCache interface {
// Get the Image data for the given image reference. If the value is
// get the Image data for the given image reference. If the value is
// not found in the cache, it will be fetched from the image registry,
// possibly using K8s service account imagePullSecrets.
//
// It also returns the digest associated with the given reference. If
// the reference referred to an index, the returned digest will be the
// index's digest, not any platform-specific image contained by the
// index.
Get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string) (*imageData, error)
get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string) (*imageData, error)
}

// imageData contains information looked up about an image or multi-platform image index.
Expand Down Expand Up @@ -72,7 +72,7 @@ func resolveEntrypoints(ctx context.Context, cache EntrypointCache, namespace, s
id = cid
} else {
// Look it up for real.
lid, err := cache.Get(ctx, ref, namespace, serviceAccountName)
lid, err := cache.get(ctx, ref, namespace, serviceAccountName)
if err != nil {
return nil, err
}
Expand All @@ -85,23 +85,15 @@ func resolveEntrypoints(ctx context.Context, cache EntrypointCache, namespace, s
// Resolve the original reference to a reference by digest.
steps[i].Image = ref.Context().Digest(id.digest.String()).String()

if len(id.commands) == 1 {
// Promote the single found command to the step's command.
for _, v := range id.commands {
steps[i].Command = v
break
}
} else {
// Encode the map of platform->command to JSON and pass it via env var.
b, err := json.Marshal(id.commands)
if err != nil {
return nil, err
}
steps[i].Env = append(steps[i].Env, corev1.EnvVar{
Name: "TEKTON_PLATFORM_COMMANDS",
Value: string(b),
})
// Encode the map of platform->command to JSON and pass it via env var.
b, err := json.Marshal(id.commands)
if err != nil {
return nil, err
}
steps[i].Env = append(steps[i].Env, corev1.EnvVar{
Name: "TEKTON_PLATFORM_COMMANDS",
Value: string(b),
})
}
return steps, nil
}
32 changes: 21 additions & 11 deletions pkg/pod/entrypoint_lookup_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"fmt"

"github.com/containerd/containerd/platforms"
"github.com/google/go-containerregistry/pkg/authn"
"github.com/google/go-containerregistry/pkg/authn/k8schain"
"github.com/google/go-containerregistry/pkg/name"
v1 "github.com/google/go-containerregistry/pkg/v1"
Expand Down Expand Up @@ -57,7 +56,7 @@ func NewEntrypointCache(kubeclient kubernetes.Interface) (EntrypointCache, error
// It also returns the digest associated with the given reference. If the
// reference referred to an index, the returned digest will be the index's
// digest, not any platform-specific image contained by the index.
func (e *entrypointCache) Get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string) (*imageData, error) {
func (e *entrypointCache) get(ctx context.Context, ref name.Reference, namespace, serviceAccountName string) (*imageData, error) {
// If image is specified by digest, check the local cache.
if digest, ok := ref.(name.Digest); ok {
if id, ok := e.lru.Get(digest.String()); ok {
Expand All @@ -73,15 +72,14 @@ func (e *entrypointCache) Get(ctx context.Context, ref name.Reference, namespace
if err != nil {
return nil, fmt.Errorf("error creating k8schain: %v", err)
}
mkc := authn.NewMultiKeychain(kc)

desc, err := remote.Get(ref, remote.WithAuthFromKeychain(mkc))
desc, err := remote.Get(ref, remote.WithAuthFromKeychain(kc))
if err != nil {
return nil, err
}

// Check the cache for this ref@digest, in case we've seen it before.
// This saves looking up each continuent image's commands if we've seen
// This saves looking up each constinuent image's commands if we've seen
// the multi-platform image before.
refByDigest := ref.Context().Digest(desc.Digest.String()).String()
if id, ok := e.lru.Get(refByDigest); ok {
Expand All @@ -98,11 +96,11 @@ func (e *entrypointCache) Get(ctx context.Context, ref name.Reference, namespace
if err != nil {
return nil, err
}
ep, err := getCommand(img)
ep, plat, err := imageInfo(img)
if err != nil {
return nil, err
}
id.commands["only-platform"] = ep
id.commands[plat] = ep
case desc.MediaType.IsIndex():
idx, err := desc.ImageIndex()
if err != nil {
Expand All @@ -129,7 +127,7 @@ func (e *entrypointCache) Get(ctx context.Context, ref name.Reference, namespace
if err != nil {
return nil, err
}
id.commands[plat], err = getCommand(img)
id.commands[plat], _, err = imageInfo(img)
if err != nil {
return nil, err
}
Expand All @@ -144,14 +142,26 @@ func (e *entrypointCache) Get(ctx context.Context, ref name.Reference, namespace
return id, nil
}

func getCommand(img v1.Image) ([]string, error) {
func imageInfo(img v1.Image) (cmd []string, platform string, err error) {
cf, err := img.ConfigFile()
if err != nil {
return nil, err
return nil, "", err
}
ep := cf.Config.Entrypoint
if len(ep) == 0 {
ep = cf.Config.Cmd
}
return ep, nil

plat := platforms.Format(specs.Platform{
OS: cf.OS,
Architecture: cf.Architecture,
// A single image's config metadata doesn't include the CPU
// architecture variant, but we'll assume this is okay since
// the runtime node's image selection will also select the same
// image. This will only be a problem if the image is a
// single-platform image that happens to specify a variant, and
// the runtime node it gets assigned to has a value for
// runtime.GOARM.
})
return ep, plat, nil
}
25 changes: 17 additions & 8 deletions pkg/pod/entrypoint_lookup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestResolveEntrypoints(t *testing.T) {
id := &imageData{
digest: dig,
commands: map[string][]string{
"*": {"my", "entrypoint"},
"only-plat": {"my", "entrypoint"},
},
}

Expand Down Expand Up @@ -103,19 +103,28 @@ func TestResolveEntrypoints(t *testing.T) {
}, {
// The step that didn't specify a command had its digest and
// entrypoint looked up in the registry, and cached.
Image: "gcr.io/my/image@" + dig.String(),
Command: []string{"my", "entrypoint"},
Image: "gcr.io/my/image@" + dig.String(),
Env: []corev1.EnvVar{{
Name: "TEKTON_PLATFORM_COMMANDS",
Value: `{"only-plat":["my","entrypoint"]}`,
}},
}, {
// The step that didn't specify command was looked up in the
// registry (by digest), and cached.
Image: "gcr.io/my/image@" + dig.String(),
Command: []string{"my", "entrypoint"},
Image: "gcr.io/my/image@" + dig.String(),
Env: []corev1.EnvVar{{
Name: "TEKTON_PLATFORM_COMMANDS",
Value: `{"only-plat":["my","entrypoint"]}`,
}},
}, {
// The other step that didn't specify command or digest was
// resolved from the *local* cache, without hitting the remote
// registry again.
Image: "gcr.io/my/image@" + dig.String(),
Command: []string{"my", "entrypoint"},
Image: "gcr.io/my/image@" + dig.String(),
Env: []corev1.EnvVar{{
Name: "TEKTON_PLATFORM_COMMANDS",
Value: `{"only-plat":["my","entrypoint"]}`,
}},
}, {
Image: "reg.io/multi/arch@" + dig.String(),
Env: []corev1.EnvVar{{
Expand All @@ -134,7 +143,7 @@ type data struct {
seen bool // Whether the image has been looked up before.
}

func (f fakeCache) Get(ctx context.Context, ref name.Reference, _, _ string) (*imageData, error) {
func (f fakeCache) get(ctx context.Context, ref name.Reference, _, _ string) (*imageData, error) {
if d, ok := ref.(name.Digest); ok {
if data, found := f[d.String()]; found {
return data.id, nil
Expand Down

0 comments on commit 2e9c68c

Please sign in to comment.