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(TKC-2188): use global.defaultRegistry for Test Workflows #5644

Merged
Merged
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
1 change: 1 addition & 0 deletions cmd/tcl/testworkflow-toolkit/spawn/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ func CreateBaseMachine() expressions.Machine {
"namespace": env.Namespace(),
"defaultRegistry": env.Config().System.DefaultRegistry,

"images.defaultRegistry": env.Config().System.DefaultRegistry,
"images.init": env.Config().Images.Init,
"images.toolkit": env.Config().Images.Toolkit,
"images.persistence.enabled": strconv.FormatBool(env.Config().Images.InspectorPersistenceEnabled),
Expand Down
34 changes: 33 additions & 1 deletion pkg/imageinspector/inspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"path/filepath"
"regexp"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand All @@ -18,6 +19,10 @@ type inspector struct {
storage []Storage
}

var (
ImageWithRegistryRe = regexp.MustCompile(`^[^/]+\.[^/]+/`)
)

func NewInspector(defaultRegistry string, infoFetcher InfoFetcher, secretFetcher SecretFetcher, storage ...Storage) Inspector {
return &inspector{
defaultRegistry: defaultRegistry,
Expand All @@ -27,7 +32,7 @@ func NewInspector(defaultRegistry string, infoFetcher InfoFetcher, secretFetcher
}
}

func (i *inspector) get(ctx context.Context, registry, image string) *Info {
func (i *inspector) rawGet(ctx context.Context, registry, image string) *Info {
for _, s := range i.storage {
v, err := s.Get(ctx, RequestBase{Registry: registry, Image: image})
if err != nil && !errors.Is(err, context.Canceled) {
Expand All @@ -40,6 +45,16 @@ func (i *inspector) get(ctx context.Context, registry, image string) *Info {
return nil
}

func (i *inspector) get(ctx context.Context, registry, image string) *Info {
if resolvedName := i.ResolveName(registry, image); resolvedName != image {
v := i.rawGet(ctx, "", resolvedName)
if v != nil {
return v
}
}
return i.rawGet(ctx, registry, image)
}

func (i *inspector) fetch(ctx context.Context, registry, image string, pullSecretNames []string) (*Info, error) {
// Fetch the secrets
secrets := make([]corev1.Secret, len(pullSecretNames))
Expand Down Expand Up @@ -68,13 +83,30 @@ func (i *inspector) save(ctx context.Context, registry, image string, info *Info
if info == nil {
return
}
if resolvedName := i.ResolveName(registry, image); resolvedName != image {
registry = ""
image = resolvedName
}
for _, s := range i.storage {
if err := s.Store(ctx, RequestBase{Registry: registry, Image: image}, *info); err != nil {
log.DefaultLogger.Warnw("error while saving image details in the cache", "registry", registry, "image", image, "error", err)
}
}
}

func (i *inspector) ResolveName(registry, image string) string {
if ImageWithRegistryRe.MatchString(image) {
return image
}
if registry == "" {
registry = i.defaultRegistry
}
if registry == "" {
return image
}
return fmt.Sprintf("%s/%s", registry, image)
}

func (i *inspector) Inspect(ctx context.Context, registry, image string, pullPolicy corev1.PullPolicy, pullSecretNames []string) (*Info, error) {
// Load from cache
if pullPolicy != corev1.PullAlways {
Expand Down
74 changes: 67 additions & 7 deletions pkg/imageinspector/inspector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,20 @@ func TestInspectorInspect(t *testing.T) {
secrets := NewMockSecretFetcher(ctrl)
storage1 := NewMockStorageWithTransfer(ctrl)
storage2 := NewMockStorageWithTransfer(ctrl)
inspector := NewInspector("default", infos, secrets, storage1, storage2)
inspector := NewInspector("default.io", infos, secrets, storage1, storage2)

sec := corev1.Secret{StringData: map[string]string{"foo": "bar"}}
req := RequestBase{Registry: "regname", Image: "imgname"}
req := RequestBase{Registry: "regname.io", Image: "imgname"}
resolvedReq := RequestBase{Image: "regname.io/imgname"}
storage1.EXPECT().Get(gomock.Any(), resolvedReq).Return(nil, nil)
storage2.EXPECT().Get(gomock.Any(), resolvedReq).Return(nil, nil)
storage1.EXPECT().Get(gomock.Any(), req).Return(nil, nil)
storage2.EXPECT().Get(gomock.Any(), req).Return(nil, nil)
secrets.EXPECT().Get(gomock.Any(), "secname").Return(&sec, nil)
infos.EXPECT().Fetch(gomock.Any(), req.Registry, req.Image, []corev1.Secret{sec}).Return(&info1, nil)

storage1.EXPECT().Store(gomock.Any(), req, info1).Return(nil)
storage2.EXPECT().Store(gomock.Any(), req, info1).Return(nil)
storage1.EXPECT().Store(gomock.Any(), resolvedReq, info1).Return(nil)
storage2.EXPECT().Store(gomock.Any(), resolvedReq, info1).Return(nil)

v, err := inspector.Inspect(context.Background(), req.Registry, req.Image, corev1.PullIfNotPresent, []string{"secname"})
assert.NoError(t, err)
Expand All @@ -42,10 +45,11 @@ func TestInspectorInspectWithCache(t *testing.T) {
secrets := NewMockSecretFetcher(ctrl)
storage1 := NewMockStorageWithTransfer(ctrl)
storage2 := NewMockStorageWithTransfer(ctrl)
inspector := NewInspector("default", infos, secrets, storage1, storage2)
inspector := NewInspector("default.io", infos, secrets, storage1, storage2)

req := RequestBase{Registry: "regname", Image: "imgname"}
storage1.EXPECT().Get(gomock.Any(), req).Return(&info1, nil)
req := RequestBase{Registry: "regname.io", Image: "imgname"}
resolvedReq := RequestBase{Image: "regname.io/imgname"}
storage1.EXPECT().Get(gomock.Any(), resolvedReq).Return(&info1, nil)

v, err := inspector.Inspect(context.Background(), req.Registry, req.Image, corev1.PullIfNotPresent, []string{"secname"})
assert.NoError(t, err)
Expand All @@ -54,3 +58,59 @@ func TestInspectorInspectWithCache(t *testing.T) {
// Wait until asynchronous storage will be done
<-time.After(10 * time.Millisecond)
}

func TestInspector_ResolveName_NoDefault_NoOverride(t *testing.T) {
ctrl := gomock.NewController(t)
infos := NewMockInfoFetcher(ctrl)
secrets := NewMockSecretFetcher(ctrl)
inspector := NewInspector("", infos, secrets)

assert.Equal(t, "image:1.2.3", inspector.ResolveName("", "image:1.2.3"))
assert.Equal(t, "repo/image:1.2.3", inspector.ResolveName("", "repo/image:1.2.3"))
assert.Equal(t, "docker.io/image:1.2.3", inspector.ResolveName("", "docker.io/image:1.2.3"))
assert.Equal(t, "ghcr.io/image:1.2.3", inspector.ResolveName("", "ghcr.io/image:1.2.3"))
assert.Equal(t, "docker.io/repo/image:1.2.3", inspector.ResolveName("", "docker.io/repo/image:1.2.3"))
assert.Equal(t, "ghcr.io/repo/image:1.2.3", inspector.ResolveName("", "ghcr.io/repo/image:1.2.3"))
}

func TestInspector_ResolveName_Default_NoOverride(t *testing.T) {
ctrl := gomock.NewController(t)
infos := NewMockInfoFetcher(ctrl)
secrets := NewMockSecretFetcher(ctrl)
inspector := NewInspector("default.io", infos, secrets)

assert.Equal(t, "default.io/image:1.2.3", inspector.ResolveName("", "image:1.2.3"))
assert.Equal(t, "default.io/repo/image:1.2.3", inspector.ResolveName("", "repo/image:1.2.3"))
assert.Equal(t, "docker.io/image:1.2.3", inspector.ResolveName("", "docker.io/image:1.2.3"))
assert.Equal(t, "ghcr.io/image:1.2.3", inspector.ResolveName("", "ghcr.io/image:1.2.3"))
assert.Equal(t, "docker.io/repo/image:1.2.3", inspector.ResolveName("", "docker.io/repo/image:1.2.3"))
assert.Equal(t, "ghcr.io/repo/image:1.2.3", inspector.ResolveName("", "ghcr.io/repo/image:1.2.3"))
}

func TestInspector_ResolveName_NoDefault_Override(t *testing.T) {
ctrl := gomock.NewController(t)
infos := NewMockInfoFetcher(ctrl)
secrets := NewMockSecretFetcher(ctrl)
inspector := NewInspector("", infos, secrets)

assert.Equal(t, "default.io/image:1.2.3", inspector.ResolveName("default.io", "image:1.2.3"))
assert.Equal(t, "default.io/repo/image:1.2.3", inspector.ResolveName("default.io", "repo/image:1.2.3"))
assert.Equal(t, "docker.io/image:1.2.3", inspector.ResolveName("default.io", "docker.io/image:1.2.3"))
assert.Equal(t, "ghcr.io/image:1.2.3", inspector.ResolveName("default.io", "ghcr.io/image:1.2.3"))
assert.Equal(t, "docker.io/repo/image:1.2.3", inspector.ResolveName("default.io", "docker.io/repo/image:1.2.3"))
assert.Equal(t, "ghcr.io/repo/image:1.2.3", inspector.ResolveName("default.io", "ghcr.io/repo/image:1.2.3"))
}

func TestInspector_ResolveName_Default_Override(t *testing.T) {
ctrl := gomock.NewController(t)
infos := NewMockInfoFetcher(ctrl)
secrets := NewMockSecretFetcher(ctrl)
inspector := NewInspector("default.io", infos, secrets)

assert.Equal(t, "default.io/image:1.2.3", inspector.ResolveName("default.io", "image:1.2.3"))
assert.Equal(t, "default.io/repo/image:1.2.3", inspector.ResolveName("default.io", "repo/image:1.2.3"))
assert.Equal(t, "docker.io/image:1.2.3", inspector.ResolveName("default.io", "docker.io/image:1.2.3"))
assert.Equal(t, "ghcr.io/image:1.2.3", inspector.ResolveName("default.io", "ghcr.io/image:1.2.3"))
assert.Equal(t, "docker.io/repo/image:1.2.3", inspector.ResolveName("default.io", "docker.io/repo/image:1.2.3"))
assert.Equal(t, "ghcr.io/repo/image:1.2.3", inspector.ResolveName("default.io", "ghcr.io/repo/image:1.2.3"))
}
14 changes: 14 additions & 0 deletions pkg/imageinspector/mock_inspector.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/imageinspector/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
//go:generate mockgen -destination=./mock_inspector.go -package=imageinspector "github.com/kubeshop/testkube/pkg/imageinspector" Inspector
type Inspector interface {
Inspect(ctx context.Context, registry, image string, pullPolicy corev1.PullPolicy, pullSecretNames []string) (*Info, error)
ResolveName(registry, image string) string
}

type StorageTransfer interface {
Expand Down
1 change: 1 addition & 0 deletions pkg/testworkflows/testworkflowexecutor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ func (e *executor) Execute(ctx context.Context, workflow testworkflowsv1.TestWor
"clusterId": e.clusterID,
"cdeventsTarget": os.Getenv("CDEVENTS_TARGET"),

"images.defaultRegistry": e.defaultRegistry,
"images.init": constants.DefaultInitImage,
"images.toolkit": constants.DefaultToolkitImage,
"images.persistence.enabled": strconv.FormatBool(e.enableImageDataPersistentCache),
Expand Down
13 changes: 9 additions & 4 deletions pkg/testworkflows/testworkflowprocessor/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ type ContainerMutations[T any] interface {
SetSecurityContext(sc *corev1.SecurityContext) T

ApplyCR(cr *testworkflowsv1.ContainerConfig) T
ApplyImageData(image *imageinspector.Info) error
ApplyImageData(image *imageinspector.Info, resolvedImageName string) error
EnableToolkit(ref string) T
}

Expand Down Expand Up @@ -385,7 +385,7 @@ func (c *container) ToKubernetesTemplate() (corev1.Container, error) {
}, nil
}

func (c *container) ApplyImageData(image *imageinspector.Info) error {
func (c *container) ApplyImageData(image *imageinspector.Info, resolvedImageName string) error {
if image == nil {
return nil
}
Expand All @@ -396,8 +396,12 @@ func (c *container) ApplyImageData(image *imageinspector.Info) error {
if err != nil {
return err
}
if len(c.Command()) == 0 {
args := c.Args()
command := c.Command()
args := c.Args()
if resolvedImageName != "" && c.Image() != resolvedImageName {
c.SetImage(resolvedImageName)
}
if len(command) == 0 {
c.SetCommand(image.Entrypoint...)
if len(args) == 0 {
c.SetArgs(image.Cmd...)
Expand Down Expand Up @@ -429,6 +433,7 @@ func (c *container) EnableToolkit(ref string) Container {
"TK_EXR": "{{resource.root}}",
"TK_FS": "{{resource.fsPrefix}}",
"TK_SA": "{{internal.serviceaccount.default}}",
"TK_R": "{{internal.images.defaultRegistry}}",
"TK_DASH": "{{internal.dashboard.url}}",
"TK_API": "{{internal.api.url}}",
"TK_CLU": "{{internal.clusterId}}",
Expand Down
5 changes: 3 additions & 2 deletions pkg/testworkflows/testworkflowprocessor/containerstage.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ func (s *containerStage) Flatten() []Stage {
return []Stage{s}
}

func (s *containerStage) ApplyImages(images map[string]*imageinspector.Info) error {
return s.container.ApplyImageData(images[s.container.Image()])
func (s *containerStage) ApplyImages(images map[string]*imageinspector.Info, imageNameResolutions map[string]string) error {
originalImageName := s.container.Image()
return s.container.ApplyImageData(images[originalImageName], imageNameResolutions[originalImageName])
}

func (s *containerStage) Resolve(m ...expressions.Machine) error {
Expand Down
4 changes: 2 additions & 2 deletions pkg/testworkflows/testworkflowprocessor/groupstage.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,9 @@ func (s *groupStage) Add(stages ...Stage) GroupStage {
return s
}

func (s *groupStage) ApplyImages(images map[string]*imageinspector.Info) error {
func (s *groupStage) ApplyImages(images map[string]*imageinspector.Info, imageNameResolutions map[string]string) error {
for i := range s.children {
err := s.children[i].ApplyImages(images)
err := s.children[i].ApplyImages(images, imageNameResolutions)
if err != nil {
return errors.Wrap(err, "applying image data")
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/testworkflows/testworkflowprocessor/mock_container.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions pkg/testworkflows/testworkflowprocessor/mock_stage.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ func (*dummyInspector) Inspect(ctx context.Context, registry, image string, pull
return &imageinspector.Info{}, nil
}

func (*dummyInspector) ResolveName(registry, image string) string {
return image
}

var (
ins = &dummyInspector{}
proc = NewPro(ins)
Expand Down
4 changes: 3 additions & 1 deletion pkg/testworkflows/testworkflowprocessor/processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,16 @@ func (p *processor) Bundle(ctx context.Context, workflow *testworkflowsv1.TestWo
// Load the image details
imageNames := root.GetImages()
images := make(map[string]*imageinspector.Info)
imageNameResolutions := map[string]string{}
for image := range imageNames {
info, err := p.inspector.Inspect(ctx, "", image, corev1.PullIfNotPresent, pullSecretNames)
imageNameResolutions[image] = p.inspector.ResolveName("", image)
if err != nil {
return nil, fmt.Errorf("resolving image error: %s: %s", image, err.Error())
}
images[image] = info
}
err = root.ApplyImages(images)
err = root.ApplyImages(images, imageNameResolutions)
if err != nil {
return nil, errors.Wrap(err, "applying image data")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/testworkflows/testworkflowprocessor/stage.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ type Stage interface {
Resolve(m ...expressions.Machine) error
ContainerStages() []ContainerStage
GetImages() map[string]struct{}
ApplyImages(images map[string]*imageinspector.Info) error
ApplyImages(images map[string]*imageinspector.Info, imageNameResolutions map[string]string) error
Flatten() []Stage
}
Loading