From f3c891f3b32d81d4d4e4b1f3e50a1134bad5145a Mon Sep 17 00:00:00 2001 From: Sherine Khoury Date: Wed, 10 May 2023 09:59:00 +0200 Subject: [PATCH] Fix issues with ICSP not containing the modified mirrors based on --max-nested-paths --- pkg/cli/mirror/manifests.go | 24 ++++-------- pkg/cli/mirror/mirror.go | 74 +++++++++++++++++------------------ pkg/cli/mirror/mirror_test.go | 2 +- 3 files changed, 44 insertions(+), 56 deletions(-) diff --git a/pkg/cli/mirror/manifests.go b/pkg/cli/mirror/manifests.go index 2abc85f2c..83ab51802 100644 --- a/pkg/cli/mirror/manifests.go +++ b/pkg/cli/mirror/manifests.go @@ -62,7 +62,7 @@ func (b *ReleaseBuilder) GetMapping(paths int, _ string, mapping image.TypedImag // Scope is set to repository for release because // they are mirrored as different repo names by // release planner - return getRegistryMapping(paths, repositoryICSPScope, mapping) + return getRegistryMapping(repositoryICSPScope, mapping) } var _ ICSPBuilder = &OperatorBuilder{} @@ -84,7 +84,7 @@ func (b *OperatorBuilder) New(icspName string, icspCount int) operatorv1alpha1.I } func (b *OperatorBuilder) GetMapping(paths int, icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { - return getRegistryMapping(paths, icspScope, mapping) + return getRegistryMapping(icspScope, mapping) } var _ ICSPBuilder = &GenericBuilder{} @@ -105,7 +105,7 @@ func (b *GenericBuilder) New(icspName string, icspCount int) operatorv1alpha1.Im } func (b *GenericBuilder) GetMapping(paths int, icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { - return getRegistryMapping(paths, icspScope, mapping) + return getRegistryMapping(icspScope, mapping) } // GenerateICSP will generate ImageContentSourcePolicy objects based on image mapping and an ICSPBuilder @@ -160,7 +160,7 @@ func aggregateICSPs(icsps [][]byte) []byte { return aggregation } -func getRegistryMapping(paths int, icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { +func getRegistryMapping(icspScope string, mapping image.TypedImageMapping) (map[string]string, error) { registryMapping := map[string]string{} for k, v := range mapping { if len(v.Ref.ID) == 0 { @@ -179,18 +179,10 @@ func getRegistryMapping(paths int, icspScope string, mapping image.TypedImageMap case icspScope == repositoryICSPScope: registryMapping[k.Ref.AsRepository().String()] = v.Ref.AsRepository().String() case icspScope == namespaceICSPScope: - // OCPBUGS-11922 - if paths > 0 { - source := path.Join(imgRegistry, imgNamespace, k.Ref.Name) - reg, org, repo, _, _ := v1alpha2.ParseImageReference(v.Ref.String()) - dest := path.Join(reg, org, repo) - registryMapping[source] = dest - } else { - source := path.Join(imgRegistry, imgNamespace) - reg, org, _, _, _ := v1alpha2.ParseImageReference(v.Ref.AsRepository().Exact()) - dest := path.Join(reg, org) - registryMapping[source] = dest - } + source := path.Join(imgRegistry, imgNamespace) + reg, org, _, _, _ := v1alpha2.ParseImageReference(v.Ref.AsRepository().Exact()) + dest := path.Join(reg, org) + registryMapping[source] = dest default: return registryMapping, fmt.Errorf("invalid ICSP scope %s", icspScope) } diff --git a/pkg/cli/mirror/mirror.go b/pkg/cli/mirror/mirror.go index 4f6ed6dee..bd5dbec72 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") } - destination := strings.Join([]string{mirror.Ref.Registry, mirror.Ref.RepositoryName()}, "/") - depth := strings.Split(destination, "/") + depth := strings.Split(mirror.Ref.RepositoryName(), "/") if nested > 0 && (len(depth) >= nested) { - return fmt.Errorf("the max-nested-paths value (%d) for %s must be higher than (registry + namespace) - try increasing the value", len(depth), destination) + 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", len(depth), mirror.Ref.RepositoryName()) } return nil } @@ -447,7 +446,17 @@ func (o *MirrorOptions) mirrorMappings(cfg v1alpha2.ImageSetConfiguration, image // 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, @@ -498,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 := o.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") } @@ -534,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) @@ -892,35 +907,16 @@ func (o *MirrorOptions) processNestedPaths(ref *image.TypedImage) imagesource.Ty if o.MaxNestedPaths > 0 { dir := ref.Ref - full := strings.Join([]string{dir.Registry, dir.Namespace, dir.Name}, "/") - splitUserNS := strings.Split(o.UserNamespace, "/") - // find the path length for registry & usernamespace - newIndex := o.MaxNestedPaths - (len(splitUserNS) + 1) - // if less than 0 will cause a panic - if newIndex >= 0 { - // restore the original usernamespace - dir.Namespace = o.UserNamespace - // split the full reference from the usernamespace - name := strings.Split(full, o.UserNamespace) - // use the second part to split the paths - newName := strings.Split(strings.Trim(name[1], "/"), "/") - // no need to replace - if newIndex >= len(newName) { - dir.Name = strings.Trim(name[1], "/") - return imagesource.TypedImageReference{Ref: dir, Type: ref.Type} - } - // replace all '/' with '-' from the index (MaxNestedPaths - (len(usernamespace) + 1)) - replacedName := strings.ReplaceAll(strings.Join(newName[newIndex:], "/"), "/", "-") - preFix := strings.Join(newName[:newIndex], "/") - // we should never really see this condition - // MaxNestedPaths should always be greater than len(split(registry + usernamespace) - // i.e o.MaxNestedPaths > strings.Split(registry + usernamespace) - if newIndex == 0 && len(preFix) == 0 { - dir.Name = strings.Trim(replacedName, "/") - } else { - dir.Name = strings.Join([]string{preFix, replacedName}, "/") - } - return imagesource.TypedImageReference{Ref: dir, Type: ref.Type} + 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} diff --git a/pkg/cli/mirror/mirror_test.go b/pkg/cli/mirror/mirror_test.go index 54073df4e..f72c39fea 100644 --- a/pkg/cli/mirror/mirror_test.go +++ b/pkg/cli/mirror/mirror_test.go @@ -603,7 +603,7 @@ func TestNestedPaths(t *testing.T) { 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) + require.Equal(t, "localhost:5000/ocpbugs-11922/mirror-release-openshift4-ose-kube-rbac-proxy", dst) }) o.MaxNestedPaths = 5