Skip to content

Commit

Permalink
[release-4.13] OCPBUGS-13591,OCPBUGS-13592: Limit the nested reposito…
Browse files Browse the repository at this point in the history 
…ry path while mirroring the images (#635)

* 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

* OCPBUGS-11922: paths not needed in ICSPBuilder interface

---------

Co-authored-by: Luigi Mario Zuccarelli <luzuccar@redhat.com>
  • Loading branch information
@sherine-k @lmzuccarelli
sherine-k and lmzuccarelli committed May 17, 2023
1 parent bee629a commit aee430b
Show file tree
Hide file tree
Showing 8 changed files with 183 additions and 43 deletions.
2 changes: 1 addition & 1 deletion pkg/cli/mirror/manifests.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (b *GenericBuilder) GetMapping(icspScope string, mapping image.TypedImageMa
}

// 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) {
func (o *MirrorOptions) GenerateICSP(icspName, icspScope string, byteLimit int, mapping image.TypedImageMapping, builder ICSPBuilder) (icsps []operatorv1alpha1.ImageContentSourcePolicy, err error) {
registryMapping, err := builder.GetMapping(icspScope, mapping)
if err != nil {
return nil, err
Expand Down
9 changes: 8 additions & 1 deletion pkg/cli/mirror/manifests_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,14 +443,21 @@ 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{}
for ind, sourceImage := range test.sourceImages {
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 {
Expand Down
75 changes: 56 additions & 19 deletions pkg/cli/mirror/mirror.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}

Expand All @@ -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")
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -594,15 +608,15 @@ 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 {
return err
}
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()
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -869,7 +883,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()
Expand All @@ -880,3 +894,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}
}
121 changes: 104 additions & 17 deletions pkg/cli/mirror/mirror_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package mirror
import (
"net/http/httptest"
"net/url"
"strings"
"testing"

"github.com/google/go-containerregistry/pkg/registry"
Expand Down Expand Up @@ -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,
},
},
{
Expand Down Expand Up @@ -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",
},
}

Expand Down Expand Up @@ -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)
})

}
2 changes: 1 addition & 1 deletion pkg/cli/mirror/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down

0 comments on commit aee430b

Please sign in to comment.