From 5aa24005e5b2020d50bb37497be6cc870d1b5b0b Mon Sep 17 00:00:00 2001 From: Luigi Mario Zuccarelli Date: Wed, 10 May 2023 16:17:04 +0200 Subject: [PATCH] OCPBUGS-11922,OCPBUGS-11910: Limit the nested repository path while mirroring the images (#623) * OCPBUGS-11922: Limit the nested repository path while mirroring the images * Fix issues with ICSP not containing the modified mirrors based on --max-nested-paths --------- Co-authored-by: Sherine Khoury --- pkg/cli/mirror/manifests.go | 12 +-- pkg/cli/mirror/manifests_test.go | 9 ++- pkg/cli/mirror/mirror.go | 75 ++++++++++++++----- pkg/cli/mirror/mirror_test.go | 121 ++++++++++++++++++++++++++----- pkg/cli/mirror/operator.go | 2 +- pkg/cli/mirror/options.go | 2 +- pkg/image/mapping.go | 13 +++- pkg/image/mapping_test.go | 2 +- 8 files changed, 188 insertions(+), 48 deletions(-) diff --git a/pkg/cli/mirror/manifests.go b/pkg/cli/mirror/manifests.go index 6707b788b..83ab51802 100644 --- a/pkg/cli/mirror/manifests.go +++ b/pkg/cli/mirror/manifests.go @@ -38,7 +38,7 @@ var icspTypeMeta = metav1.TypeMeta{ // ICSPBuilder defines methods for generating ICSPs type ICSPBuilder interface { New(string, int) operatorv1alpha1.ImageContentSourcePolicy - GetMapping(string, image.TypedImageMapping) (map[string]string, error) + GetMapping(int, string, image.TypedImageMapping) (map[string]string, error) } var _ ICSPBuilder = &ReleaseBuilder{} @@ -58,7 +58,7 @@ func (b *ReleaseBuilder) New(icspName string, icspCount int) operatorv1alpha1.Im } } -func (b *ReleaseBuilder) GetMapping(_ string, mapping image.TypedImageMapping) (map[string]string, error) { +func (b *ReleaseBuilder) GetMapping(paths int, _ string, mapping image.TypedImageMapping) (map[string]string, error) { // Scope is set to repository for release because // they are mirrored as different repo names by // release planner @@ -83,7 +83,7 @@ func (b *OperatorBuilder) New(icspName string, icspCount int) operatorv1alpha1.I } } -func (b *OperatorBuilder) GetMapping(icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { +func (b *OperatorBuilder) GetMapping(paths int, icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { return getRegistryMapping(icspScope, mapping) } @@ -104,13 +104,13 @@ func (b *GenericBuilder) New(icspName string, icspCount int) operatorv1alpha1.Im } } -func (b *GenericBuilder) GetMapping(icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { +func (b *GenericBuilder) GetMapping(paths int, icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { return getRegistryMapping(icspScope, mapping) } // GenerateICSP will generate ImageContentSourcePolicy objects based on image mapping and an ICSPBuilder -func GenerateICSP(icspName, icspScope string, byteLimit int, mapping image.TypedImageMapping, builder ICSPBuilder) (icsps []operatorv1alpha1.ImageContentSourcePolicy, err error) { - registryMapping, err := builder.GetMapping(icspScope, mapping) +func (o *MirrorOptions) GenerateICSP(icspName, icspScope string, byteLimit int, mapping image.TypedImageMapping, builder ICSPBuilder) (icsps []operatorv1alpha1.ImageContentSourcePolicy, err error) { + registryMapping, err := builder.GetMapping(o.MaxNestedPaths, icspScope, mapping) if err != nil { return nil, err } diff --git a/pkg/cli/mirror/manifests_test.go b/pkg/cli/mirror/manifests_test.go index 2c766900d..b534e34a8 100644 --- a/pkg/cli/mirror/manifests_test.go +++ b/pkg/cli/mirror/manifests_test.go @@ -443,6 +443,13 @@ func TestICSPGeneration(t *testing.T) { }, }} + // test processNestedPaths + o := &MirrorOptions{ + MaxNestedPaths: 0, + ToMirror: "localhost:5000", + UserNamespace: "ocpbugs-11922/mirror-release", + } + for _, test := range tests { t.Run(test.name, func(t *testing.T) { mapping := image.TypedImageMapping{} @@ -450,7 +457,7 @@ func TestICSPGeneration(t *testing.T) { mapping[sourceImage] = test.destImages[ind] } - icsps, err := GenerateICSP("test", test.icspScope, test.icspSizeLimit, mapping, test.typ) + icsps, err := o.GenerateICSP("test", test.icspScope, test.icspSizeLimit, mapping, test.typ) if test.err != "" { require.EqualError(t, err, test.err) } else { diff --git a/pkg/cli/mirror/mirror.go b/pkg/cli/mirror/mirror.go index 3b804deae..85fbcf14d 100644 --- a/pkg/cli/mirror/mirror.go +++ b/pkg/cli/mirror/mirror.go @@ -202,10 +202,9 @@ func checkDockerReference(mirror imagesource.TypedImageReference, nested int) er return fmt.Errorf("destination registry must consist of registry host and namespace(s) only, and must not include an image tag or ID") } - depth := strings.Split(strings.Join([]string{mirror.Ref.Namespace, mirror.Ref.Name}, "/"), "/") - if len(depth) > nested { - destination := strings.Join([]string{mirror.Ref.Registry, mirror.Ref.Namespace, mirror.Ref.Name}, "/") - return fmt.Errorf("the max-nested-paths value (%d) for %s exceeds the registry mirror paths setting (some registries limit the nested paths)", len(depth), destination) + depth := strings.Split(mirror.Ref.RepositoryName(), "/") + if nested > 0 && (len(depth) >= nested) { + return fmt.Errorf("the max-nested-paths value (%d) must be strictly higher than the number of path-components in the destination %s - try increasing the value", nested, mirror.Ref.RepositoryName()) } return nil } @@ -445,10 +444,19 @@ func (o *MirrorOptions) mirrorMappings(cfg v1alpha2.ImageSetConfiguration, image Type: srcRef.Type, } - dstTIR := imagesource.TypedImageReference{ - Ref: dstRef.Ref, - Type: dstRef.Type, + // OCPBUGS-11922 + dstTIR := o.processNestedPaths(&dstRef) + // Updating the original map - which will later be used to generate ICSP + images[srcRef] = image.TypedImage{ + Category: dstRef.Category, + ImageFormat: dstRef.ImageFormat, + TypedImageReference: image.TypedImageReference{ + Type: dstRef.Type, + Ref: dstTIR.Ref, + OCIFBCPath: dstRef.OCIFBCPath, + }, } + // Updating mappings which will be used for mirroring the images mappings = append(mappings, mirror.Mapping{ Source: srcTIR, Destination: dstTIR, @@ -489,7 +497,7 @@ func (o *MirrorOptions) newMirrorImageOptions(insecure bool) (*mirror.MirrorImag func (o *MirrorOptions) generateResults(mapping image.TypedImageMapping, dir string) error { mappingResultsPath := filepath.Join(dir, mappingFile) - if err := writeMappingFile(mappingResultsPath, mapping); err != nil { + if err := o.writeMappingFile(mappingResultsPath, mapping); err != nil { return err } @@ -499,8 +507,8 @@ func (o *MirrorOptions) generateResults(mapping image.TypedImageMapping, dir str generic := image.ByCategory(mapping, v1alpha2.TypeGeneric) operator := image.ByCategory(mapping, v1alpha2.TypeOperatorBundle, v1alpha2.TypeOperatorRelatedImage) - getICSP := func(mapping image.TypedImageMapping, name string, builder ICSPBuilder) error { - icsps, err := GenerateICSP(name, namespaceICSPScope, icspSizeLimit, mapping, builder) + getICSP := func(mapping image.TypedImageMapping, name string, scope string, builder ICSPBuilder) error { + icsps, err := o.GenerateICSP(name, scope, icspSizeLimit, mapping, builder) if err != nil { return fmt.Errorf("error generating ICSP manifests") } @@ -535,14 +543,20 @@ func (o *MirrorOptions) generateResults(mapping image.TypedImageMapping, dir str } } - if err := getICSP(releases, "release", &ReleaseBuilder{}); err != nil { + if err := getICSP(releases, "release", namespaceICSPScope, &ReleaseBuilder{}); err != nil { return err } - if err := getICSP(generic, "generic", &GenericBuilder{}); err != nil { + if err := getICSP(generic, "generic", namespaceICSPScope, &GenericBuilder{}); err != nil { return err } - if err := getICSP(operator, "operator", &OperatorBuilder{}); err != nil { - return err + if o.MaxNestedPaths > 0 { + if err := getICSP(operator, "operator", repositoryICSPScope, &OperatorBuilder{}); err != nil { + return err + } + } else { + if err := getICSP(operator, "operator", namespaceICSPScope, &OperatorBuilder{}); err != nil { + return err + } } return WriteICSPs(dir, allICSPs) @@ -594,7 +608,7 @@ func (o *MirrorOptions) processAssociationErrors(errs []error) error { return nil } -func writeMappingFile(mappingPath string, mapping image.TypedImageMapping) error { +func (o *MirrorOptions) writeMappingFile(mappingPath string, mapping image.TypedImageMapping) error { path := filepath.Clean(mappingPath) mappingFile, err := os.Create(path) if err != nil { @@ -602,7 +616,7 @@ func writeMappingFile(mappingPath string, mapping image.TypedImageMapping) error } defer mappingFile.Close() klog.Infof("Writing image mapping to %s", mappingPath) - if err := image.WriteImageMapping(mapping, mappingFile); err != nil { + if err := image.WriteImageMapping(o.MaxNestedPaths, mapping, mappingFile); err != nil { return err } return mappingFile.Sync() @@ -666,7 +680,7 @@ func (o *MirrorOptions) mirrorToMirrorWrapper(ctx context.Context, cfg v1alpha2. } if o.DryRun { - if err := writeMappingFile(mappingPath, mapping); err != nil { + if err := o.writeMappingFile(mappingPath, mapping); err != nil { return err } if err := o.outputPruneImagePlan(ctx, prevAssociations, prunedAssociations); err != nil { @@ -816,7 +830,7 @@ func (o *MirrorOptions) mirrorToDiskWrapper(ctx context.Context, cfg v1alpha2.Im mappingPath := filepath.Join(o.Dir, mappingFile) if o.DryRun { - if err := writeMappingFile(mappingPath, mapping); err != nil { + if err := o.writeMappingFile(mappingPath, mapping); err != nil { return err } return cleanup() @@ -877,7 +891,7 @@ func (o *MirrorOptions) diskToMirrorWrapper(ctx context.Context, cleanup cleanup mappingPath := filepath.Join(o.Dir, mappingFile) if o.DryRun { - if err := writeMappingFile(mappingPath, mapping); err != nil { + if err := o.writeMappingFile(mappingPath, mapping); err != nil { return err } return cleanup() @@ -888,3 +902,26 @@ func (o *MirrorOptions) diskToMirrorWrapper(ctx context.Context, cleanup cleanup } return nil } + +func (o *MirrorOptions) processNestedPaths(ref *image.TypedImage) imagesource.TypedImageReference { + + if o.MaxNestedPaths > 0 { + dir := ref.Ref + full := dir.RepositoryName() + + pathComponents := strings.Split(full, "/") + if o.MaxNestedPaths > 0 && len(pathComponents) > o.MaxNestedPaths { + lastPathComponent := strings.Join(pathComponents[o.MaxNestedPaths-1:], "-") + newPathComponents := pathComponents[:o.MaxNestedPaths-1] + newRef := dir // reinitializing newRef from dir (so that we don't loose id and tag) + newRef.Namespace = strings.Join(newPathComponents, "/") + newRef.Name = lastPathComponent + return imagesource.TypedImageReference{Ref: newRef, Type: ref.Type} + } else { + // return original - no changes + return imagesource.TypedImageReference{Ref: ref.Ref, Type: ref.Type} + } + } + // return original - no changes + return imagesource.TypedImageReference{Ref: ref.Ref, Type: ref.Type} +} diff --git a/pkg/cli/mirror/mirror_test.go b/pkg/cli/mirror/mirror_test.go index a8d10b4db..cc2dcaede 100644 --- a/pkg/cli/mirror/mirror_test.go +++ b/pkg/cli/mirror/mirror_test.go @@ -3,6 +3,7 @@ package mirror import ( "net/http/httptest" "net/url" + "strings" "testing" "github.com/google/go-containerregistry/pkg/registry" @@ -75,77 +76,77 @@ func TestMirrorComplete(t *testing.T) { { name: "Valid/RegDest", args: []string{"docker://reg.com"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "reg.com", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/LocalhostRegDest", args: []string{"docker://localhost"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "localhost", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/FqdnRegPortDest", args: []string{"docker://reg.com:5000"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "reg.com:5000", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/LocalhostRegPortDest", args: []string{"docker://localhost:5000"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "localhost:5000", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/RegNamespace", args: []string{"docker://reg.com/foo/bar"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "reg.com", UserNamespace: "foo/bar", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/LocalhostRegNamespace", args: []string{"docker://localhost/foo/bar"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "localhost", UserNamespace: "foo/bar", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/NonFqdnRegPortNamespace", args: []string{"docker://reg:5000/foo"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "reg:5000", UserNamespace: "foo", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { name: "Valid/NonFqdnRegPortNamespaceName", args: []string{"docker://reg:5000/foo/bar"}, - opts: &MirrorOptions{MaxNestedPaths: 3}, + opts: &MirrorOptions{MaxNestedPaths: 0}, expOpts: &MirrorOptions{ ToMirror: "reg:5000", UserNamespace: "foo/bar", - MaxNestedPaths: 3, + MaxNestedPaths: 0, }, }, { @@ -227,7 +228,7 @@ func TestMirrorComplete(t *testing.T) { name: "Invalid/ExceedsNestedPathsLength", args: []string{"docker://reg.com/foo/bar/baz"}, opts: &MirrorOptions{MaxNestedPaths: 2}, - expError: "the max-nested-paths value (3) for reg.com/foo/bar/baz exceeds the registry mirror paths setting (some registries limit the nested paths)", + expError: "the max-nested-paths value (2) must be strictly higher than the number of path-components in the destination foo/bar/baz - try increasing the value", }, } @@ -534,3 +535,89 @@ func TestRemovePreviouslyMirrored(t *testing.T) { }) } } + +func TestNestedPaths(t *testing.T) { + + // test processNestedPaths + o := &MirrorOptions{ + MaxNestedPaths: 2, + ToMirror: "localhost:5000", + UserNamespace: "ocpbugs-11922/mirror-release", + } + + img := &image.TypedImage{ + TypedImageReference: image.TypedImageReference{ + Ref: reference.DockerImageReference{ + Name: "ci-cd/gitlab-runner-ubi-images/gitlab-runner-helper-ocp", + Namespace: "ocpbugs-11922/mirror-release/gitlab-org", + Registry: "localhost:5000", + }, + }, + } + + t.Run("Testing processNestedPaths (2) : should fail", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.NotEqual(t, "localhost:5000/ocpbugs-11922/mirror-release/gitlab-org-ci-cd-gitlab-runner-ubi-images-gitlab-runner-helper-ocp", dst) + }) + o.MaxNestedPaths = 3 + t.Run("Testing processNestedPaths (3) : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release/gitlab-org-ci-cd-gitlab-runner-ubi-images-gitlab-runner-helper-ocp", dst) + }) + + o.MaxNestedPaths = 4 + t.Run("Testing processNestedPaths (4) : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release/gitlab-org/ci-cd-gitlab-runner-ubi-images-gitlab-runner-helper-ocp", dst) + }) + + o.MaxNestedPaths = 5 + t.Run("Testing processNestedPaths (5) : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release/gitlab-org/ci-cd/gitlab-runner-ubi-images-gitlab-runner-helper-ocp", dst) + }) + + o.MaxNestedPaths = 6 + t.Run("Testing processNestedPaths (6) : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release/gitlab-org/ci-cd/gitlab-runner-ubi-images/gitlab-runner-helper-ocp", dst) + }) + + // change image + img = &image.TypedImage{ + TypedImageReference: image.TypedImageReference{ + Ref: reference.DockerImageReference{ + Name: "openshift4/ose-kube-rbac-proxy", + Namespace: "ocpbugs-11922/mirror-release", + Registry: "localhost:5000", + }, + }, + } + + o.MaxNestedPaths = 2 + t.Run("Testing processNestedPaths (2) new image : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release-openshift4-ose-kube-rbac-proxy", dst) + }) + + o.MaxNestedPaths = 5 + t.Run("Testing processNestedPaths (5) new image : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release/openshift4/ose-kube-rbac-proxy", dst) + }) + + o.MaxNestedPaths = 10 + t.Run("Testing processNestedPaths (10) new image : should pass", func(t *testing.T) { + res := o.processNestedPaths(img) + dst := strings.Join([]string{res.Ref.Registry, res.Ref.Namespace, res.Ref.Name}, "/") + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release/openshift4/ose-kube-rbac-proxy", dst) + }) + +} diff --git a/pkg/cli/mirror/operator.go b/pkg/cli/mirror/operator.go index 20d3a7ee1..a742c6b8f 100644 --- a/pkg/cli/mirror/operator.go +++ b/pkg/cli/mirror/operator.go @@ -441,7 +441,7 @@ func (o *OperatorOptions) plan(ctx context.Context, dc *declcfg.DeclarativeConfi } } - if err := writeMappingFile(mappingFile, result); err != nil { + if err := o.writeMappingFile(mappingFile, result); err != nil { return nil, err } } diff --git a/pkg/cli/mirror/options.go b/pkg/cli/mirror/options.go index 16653757d..251ec269b 100644 --- a/pkg/cli/mirror/options.go +++ b/pkg/cli/mirror/options.go @@ -73,8 +73,8 @@ func (o *MirrorOptions) BindFlags(fs *pflag.FlagSet) { fs.StringVar(&o.OCIRegistriesConfig, "oci-registries-config", o.OCIRegistriesConfig, "Registries config file location (used only with --use-oci-feature flag)") fs.BoolVar(&o.OCIInsecureSignaturePolicy, "oci-insecure-signature-policy", o.OCIInsecureSignaturePolicy, "If set, OCI catalog push will not try to push signatures") fs.BoolVar(&o.SkipPruning, "skip-pruning", o.SkipPruning, "If set, will disable pruning globally") - fs.IntVar(&o.MaxNestedPaths, "max-nested-paths", 2, "Number of nested paths, for destination registries that limit nested paths") fs.MarkDeprecated("use-oci-feature", "Use --include-local-oci-catalogs instead") + fs.IntVar(&o.MaxNestedPaths, "max-nested-paths", 0, "Number of nested paths, for destination registries that limit nested paths") } func (o *MirrorOptions) init() { diff --git a/pkg/image/mapping.go b/pkg/image/mapping.go index c2a06f76b..751c429c8 100644 --- a/pkg/image/mapping.go +++ b/pkg/image/mapping.go @@ -161,14 +161,23 @@ func ReadImageMapping(mappingsPath, separator string, typ v1alpha2.ImageType) (T } // WriteImageMapping writes key map k/v to an io.Writer. -func WriteImageMapping(m TypedImageMapping, output io.Writer) error { +func WriteImageMapping(nestedPaths int, m TypedImageMapping, output io.Writer) error { + var strFrom, strTo string for fromStr, toStr := range m { // Prefer tag over id for mapping file for // compatability with `oc image mirror`. if toStr.Ref.Tag != "" { toStr.Ref.ID = "" } - _, err := output.Write([]byte(fmt.Sprintf("%s=%s\n", fromStr.Ref.Exact(), toStr.Ref.Exact()))) + // OCPBUGS-11922 + if nestedPaths > 0 { + strFrom = fromStr.Ref.String() + strTo = toStr.Ref.String() + } else { + strFrom = fromStr.Ref.Exact() + strTo = toStr.Ref.Exact() + } + _, err := output.Write([]byte(fmt.Sprintf("%s=%s\n", strFrom, strTo))) if err != nil { return err } diff --git a/pkg/image/mapping_test.go b/pkg/image/mapping_test.go index 37147f707..423f88c50 100644 --- a/pkg/image/mapping_test.go +++ b/pkg/image/mapping_test.go @@ -374,7 +374,7 @@ func TestWriteImageMapping(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { output := new(strings.Builder) - err := WriteImageMapping(test.mapping, output) + err := WriteImageMapping(0, test.mapping, output) if test.err != "" { require.EqualError(t, err, test.err) } else {