From da6c9041672fcd69d69b75ad2287d3a15e6b41dc Mon Sep 17 00:00:00 2001 From: Alexander Greene Date: Thu, 6 May 2021 09:04:26 -0700 Subject: [PATCH] Allow users to specify max ICSP size Problem: It is possible for the `oc adm catalog mirror` command to generate ICSPs that are greater than 1000000 bytes in size. ICSPs that exceed 1000000 bytes are likely to fail when applied to the cluster if the objec already existed in an early state as the `kubectl.kubernetes.io/last-applied-configuration` annotation will likely exceed the 2000000 etcd resource byte limit. Solution: Introduce a the max-icsp-size flag to the `oc adm catalog mirror` command, allowing users to specify the maximum byte size of ICSP files generated by the command. If an ICSP would exceed this limit, create and begin writting mirrors to a new ICSP. The default ICSP limit is 1000000 bytes. --- pkg/cli/admin/catalog/mirror.go | 79 ++++++-- pkg/cli/admin/catalog/mirror_test.go | 287 ++++++++++++++++++++------- 2 files changed, 278 insertions(+), 88 deletions(-) diff --git a/pkg/cli/admin/catalog/mirror.go b/pkg/cli/admin/catalog/mirror.go index 393115ff3e..38aab79e0f 100644 --- a/pkg/cli/admin/catalog/mirror.go +++ b/pkg/cli/admin/catalog/mirror.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "os" "path/filepath" + "strconv" "strings" "text/tabwriter" "time" @@ -71,7 +72,9 @@ var ( `) ) -const IndexLocationLabelKey = "operators.operatorframework.io.index.database.v1" +const ( + IndexLocationLabelKey = "operators.operatorframework.io.index.database.v1" +) func init() { subCommands = append(subCommands, NewMirrorCatalog) @@ -87,6 +90,7 @@ type MirrorCatalogOptions struct { FromFileDir string FileDir string + MaxICSPSize int IcspScope string @@ -145,6 +149,7 @@ func NewMirrorCatalog(f kcmdutil.Factory, streams genericclioptions.IOStreams) * flags.StringVar(&o.FromFileDir, "from-dir", o.FromFileDir, "The directory on disk that file:// images will be read from. Overrides --dir") flags.IntVar(&o.MaxPathComponents, "max-components", 2, "The maximum number of path components allowed in a destination mapping. Example: `quay.io/org/repo` has two path components.") flags.StringVar(&o.IcspScope, "icsp-scope", o.IcspScope, "Scope of registry mirrors in imagecontentsourcepolicy file. Allowed values: repository, registry. Defaults to: repository") + flags.IntVar(&o.MaxICSPSize, "max-icsp-size", 1000000, "The maximum number of bytes for the generated ICSP yaml(s). Defaults to 1000000") return cmd } @@ -382,10 +387,48 @@ func (o *MirrorCatalogOptions) Run() error { fmt.Fprintf(o.IOStreams.ErrOut, "errors during mirroring. the full contents of the catalog may not have been mirrored: %s\n", err.Error()) } - return WriteManifests(o.IOStreams.Out, o.SourceRef, o.DestRef, o.ManifestDir, o.IcspScope, mapping) + return WriteManifests(o.IOStreams.Out, o.SourceRef, o.DestRef, o.ManifestDir, o.IcspScope, o.MaxICSPSize, mapping) +} + +func getRegistryMapping(out io.Writer, icspScope string, mapping map[imagesource.TypedImageReference]imagesource.TypedImageReference) map[string]string { + registryMapping := map[string]string{} + for k, v := range mapping { + if len(v.Ref.ID) == 0 { + fmt.Fprintf(out, "no digest mapping available for %s, skip writing to ImageContentSourcePolicy\n", k) + continue + } + if icspScope == "registry" { + registryMapping[k.Ref.Registry] = v.Ref.Registry + } else { + registryMapping[k.Ref.AsRepository().String()] = v.Ref.AsRepository().String() + } + } + return registryMapping +} + +func generateICSPs(out io.Writer, source string, icspScope string, maxICSPSize int, mapping map[imagesource.TypedImageReference]imagesource.TypedImageReference) ([][]byte, error) { + registryMapping := getRegistryMapping(out, icspScope, mapping) + icsps := [][]byte{} + for i := 0; len(registryMapping) != 0; i++ { + icsp, err := generateICSP(out, source+"-"+strconv.Itoa(i), maxICSPSize, registryMapping) + if err != nil { + return nil, err + } + icsps = append(icsps, icsp) + } + return icsps, nil +} + +func aggregateICSPs(icsps [][]byte) []byte { + aggregation := []byte{} + for _, icsp := range icsps { + aggregation = append(aggregation, []byte("---\n")...) + aggregation = append(aggregation, icsp...) + } + return aggregation } -func WriteManifests(out io.Writer, source, dest imagesource.TypedImageReference, dir, icspScope string, mapping map[imagesource.TypedImageReference]imagesource.TypedImageReference) error { +func WriteManifests(out io.Writer, source, dest imagesource.TypedImageReference, dir, icspScope string, maxICSPSize int, mapping map[imagesource.TypedImageReference]imagesource.TypedImageReference) error { f, err := os.Create(filepath.Join(dir, "mapping.txt")) if err != nil { return err @@ -401,12 +444,12 @@ func WriteManifests(out io.Writer, source, dest imagesource.TypedImageReference, } if dest.Type != imagesource.DestinationFile { - icsp, err := generateICSP(out, source.Ref.Name, icspScope, mapping) + icsps, err := generateICSPs(out, source.Ref.Name, icspScope, maxICSPSize, mapping) if err != nil { return err } - if err := ioutil.WriteFile(filepath.Join(dir, "imageContentSourcePolicy.yaml"), icsp, os.ModePerm); err != nil { + if err := ioutil.WriteFile(filepath.Join(dir, "imageContentSourcePolicy.yaml"), aggregateICSPs(icsps), os.ModePerm); err != nil { return fmt.Errorf("error writing ImageContentSourcePolicy") } @@ -472,7 +515,7 @@ func generateCatalogSource(source imagesource.TypedImageReference, mapping map[i return csExample, nil } -func generateICSP(out io.Writer, name string, icspScope string, mapping map[imagesource.TypedImageReference]imagesource.TypedImageReference) ([]byte, error) { +func generateICSP(out io.Writer, name string, byteLimit int, registryMapping map[string]string) ([]byte, error) { icsp := operatorv1alpha1.ImageContentSourcePolicy{ TypeMeta: metav1.TypeMeta{ APIVersion: operatorv1alpha1.GroupVersion.String(), @@ -485,23 +528,23 @@ func generateICSP(out io.Writer, name string, icspScope string, mapping map[imag }, } - registryMapping := map[string]string{} - for k, v := range mapping { - if len(v.Ref.ID) == 0 { - fmt.Fprintf(out, "no digest mapping available for %s, skip writing to ImageContentSourcePolicy\n", k) - continue - } - if icspScope == "registry" { - registryMapping[k.Ref.Registry] = v.Ref.Registry - } else { - registryMapping[k.Ref.AsRepository().String()] = v.Ref.AsRepository().String() - } - } for key := range registryMapping { icsp.Spec.RepositoryDigestMirrors = append(icsp.Spec.RepositoryDigestMirrors, operatorv1alpha1.RepositoryDigestMirrors{ Source: key, Mirrors: []string{registryMapping[key]}, }) + y, err := yaml.Marshal(icsp) + if err != nil { + return nil, fmt.Errorf("unable to marshal ImageContentSourcePolicy yaml: %v", err) + } + + if len(y) > byteLimit { + if len(icsp.Spec.RepositoryDigestMirrors) > 0 { + icsp.Spec.RepositoryDigestMirrors = icsp.Spec.RepositoryDigestMirrors[:len(icsp.Spec.RepositoryDigestMirrors)-1] + } + break + } + delete(registryMapping, key) } // Create an unstructured object for removing creationTimestamp diff --git a/pkg/cli/admin/catalog/mirror_test.go b/pkg/cli/admin/catalog/mirror_test.go index 7f69b56979..b8b00d24fd 100644 --- a/pkg/cli/admin/catalog/mirror_test.go +++ b/pkg/cli/admin/catalog/mirror_test.go @@ -3,14 +3,18 @@ package catalog import ( "bytes" "fmt" - "github.com/openshift/library-go/pkg/image/reference" - "github.com/openshift/oc/pkg/cli/image/imagesource" "os" "reflect" "strings" "testing" "github.com/google/go-cmp/cmp" + operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1" + "github.com/openshift/library-go/pkg/image/reference" + "github.com/openshift/oc/pkg/cli/image/imagesource" + "gopkg.in/yaml.v2" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" ) func TestWriteToMapping(t *testing.T) { @@ -97,22 +101,20 @@ func TestWriteToMapping(t *testing.T) { } } -func TestGenerateICSP(t *testing.T) { +func TestGetRegistryMapping(t *testing.T) { type args struct { - name string scope string mapping map[imagesource.TypedImageReference]imagesource.TypedImageReference } tests := []struct { - name string - args args - want []byte - wantErr bool + name string + args args + want map[string]string }{ { - name: "src is tagged - skip mirror", + name: "src is tagged - skip mirrors", args: args{ - name: "catalog", + scope: "", mapping: map[imagesource.TypedImageReference]imagesource.TypedImageReference{ mustParseRef(t, "quay.io/halkyonio/operator:v0.1.8"): { Type: imagesource.DestinationRegistry, @@ -126,20 +128,11 @@ func TestGenerateICSP(t *testing.T) { }, }, }, - want: []byte( - `apiVersion: operator.openshift.io/v1alpha1 -kind: ImageContentSourcePolicy -metadata: - name: catalog -spec: - repositoryDigestMirrors: [] -`, - ), + want: map[string]string{}, }, { name: "src is tagged and icsp with registy scope - skip mirror", args: args{ - name: "catalog", scope: "registry", mapping: map[imagesource.TypedImageReference]imagesource.TypedImageReference{ mustParseRef(t, "quay.io/halkyonio/operator:v0.1.8"): { @@ -154,20 +147,11 @@ spec: }, }, }, - want: []byte( - `apiVersion: operator.openshift.io/v1alpha1 -kind: ImageContentSourcePolicy -metadata: - name: catalog -spec: - repositoryDigestMirrors: [] -`, - ), + want: map[string]string{}, }, { name: "src has digest", args: args{ - name: "catalog", mapping: map[imagesource.TypedImageReference]imagesource.TypedImageReference{ mustParseRef(t, "docker.io/strimzi/operator@sha256:d134a9865524c29fcf75bbc4469013bc38d8a15cb5f41acfddb6b9e492f556e4"): { Type: imagesource.DestinationRegistry, @@ -181,23 +165,11 @@ spec: }, }, }, - want: []byte( - `apiVersion: operator.openshift.io/v1alpha1 -kind: ImageContentSourcePolicy -metadata: - name: catalog -spec: - repositoryDigestMirrors: - - mirrors: - - quay.io/olmtest/strimzi-operator - source: docker.io/strimzi/operator -`, - ), + want: map[string]string{"docker.io/strimzi/operator": "quay.io/olmtest/strimzi-operator"}, }, { name: "src has digest and icsp with registry scope", args: args{ - name: "catalog", scope: "registry", mapping: map[imagesource.TypedImageReference]imagesource.TypedImageReference{ mustParseRef(t, "docker.io/strimzi/operator@sha256:d134a9865524c29fcf75bbc4469013bc38d8a15cb5f41acfddb6b9e492f556e4"): { @@ -212,23 +184,11 @@ spec: }, }, }, - want: []byte( - `apiVersion: operator.openshift.io/v1alpha1 -kind: ImageContentSourcePolicy -metadata: - name: catalog -spec: - repositoryDigestMirrors: - - mirrors: - - quay.io - source: docker.io -`, - ), + want: map[string]string{"docker.io": "quay.io"}, }, { name: "multiple", args: args{ - name: "catalog", mapping: map[imagesource.TypedImageReference]imagesource.TypedImageReference{ mustParseRef(t, "docker.io/strimzi/operator@sha256:d134a9865524c29fcf75bbc4469013bc38d8a15cb5f41acfddb6b9e492f556e4"): { Type: imagesource.DestinationRegistry, @@ -252,23 +212,11 @@ spec: }, }, }, - want: []byte( - `apiVersion: operator.openshift.io/v1alpha1 -kind: ImageContentSourcePolicy -metadata: - name: catalog -spec: - repositoryDigestMirrors: - - mirrors: - - quay.io/olmtest/strimzi-operator - source: docker.io/strimzi/operator -`, - ), + want: map[string]string{"docker.io/strimzi/operator": "quay.io/olmtest/strimzi-operator"}, }, { name: "multiple with icsp registry scope", args: args{ - name: "catalog", scope: "registry", mapping: map[imagesource.TypedImageReference]imagesource.TypedImageReference{ mustParseRef(t, "docker.io/strimzi/operator@sha256:d134a9865524c29fcf75bbc4469013bc38d8a15cb5f41acfddb6b9e492f556e4"): { @@ -293,6 +241,134 @@ spec: }, }, }, + want: map[string]string{"docker.io": "quay.io"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := getRegistryMapping(os.Stdout, tt.args.scope, tt.args.mapping) + if len(got) != len(tt.want) { + t.Errorf("Received map length != expected map length") + } + for k := range tt.want { + if got[k] != tt.want[k] { + t.Errorf("Expeced Map DNE actual map %v", got) + } + } + }) + } +} + +func TestGenerateICSP(t *testing.T) { + type args struct { + name string + scope string + mapping map[string]string + } + tests := []struct { + name string + args args + want []byte + wantErr bool + }{ + { + name: "src is tagged - skip mirror", + args: args{ + name: "catalog", + mapping: map[string]string{}, + }, + want: []byte( + `apiVersion: operator.openshift.io/v1alpha1 +kind: ImageContentSourcePolicy +metadata: + name: catalog +spec: + repositoryDigestMirrors: [] +`, + ), + }, + { + name: "src is tagged and icsp with registy scope - skip mirror", + args: args{ + name: "catalog", + scope: "registry", + mapping: map[string]string{}, + }, + want: []byte( + `apiVersion: operator.openshift.io/v1alpha1 +kind: ImageContentSourcePolicy +metadata: + name: catalog +spec: + repositoryDigestMirrors: [] +`, + ), + }, + { + name: "src has digest", + args: args{ + name: "catalog", + mapping: map[string]string{"docker.io/strimzi/operator": "quay.io/olmtest/strimzi-operator"}, + }, + want: []byte( + `apiVersion: operator.openshift.io/v1alpha1 +kind: ImageContentSourcePolicy +metadata: + name: catalog +spec: + repositoryDigestMirrors: + - mirrors: + - quay.io/olmtest/strimzi-operator + source: docker.io/strimzi/operator +`, + ), + }, + { + name: "src has digest and icsp with registry scope", + args: args{ + name: "catalog", + scope: "registry", + mapping: map[string]string{"docker.io": "quay.io"}, + }, + want: []byte( + `apiVersion: operator.openshift.io/v1alpha1 +kind: ImageContentSourcePolicy +metadata: + name: catalog +spec: + repositoryDigestMirrors: + - mirrors: + - quay.io + source: docker.io +`, + ), + }, + { + name: "multiple", + args: args{ + name: "catalog", + mapping: map[string]string{"docker.io/strimzi/operator": "quay.io/olmtest/strimzi-operator"}, + }, + want: []byte( + `apiVersion: operator.openshift.io/v1alpha1 +kind: ImageContentSourcePolicy +metadata: + name: catalog +spec: + repositoryDigestMirrors: + - mirrors: + - quay.io/olmtest/strimzi-operator + source: docker.io/strimzi/operator +`, + ), + }, + { + name: "multiple with icsp registry scope", + args: args{ + name: "catalog", + scope: "registry", + mapping: map[string]string{"docker.io": "quay.io"}, + }, want: []byte( `apiVersion: operator.openshift.io/v1alpha1 kind: ImageContentSourcePolicy @@ -309,7 +385,7 @@ spec: } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := generateICSP(os.Stdout, tt.args.name, tt.args.scope, tt.args.mapping) + got, err := generateICSP(os.Stdout, tt.args.name, 99999999, tt.args.mapping) if (err != nil) != tt.wantErr { t.Errorf("generateICSP() error = %v, wantErr %v", err, tt.wantErr) return @@ -405,6 +481,77 @@ spec: } } +func TestGenerateICSPs(t *testing.T) { + type args struct { + name string + scope string + limit int + } + tests := []struct { + name string + args args + registryMapSize int + }{ + { + name: "Generated ICSPs are smaller than the byte limit", + args: args{ + name: "catalog", + scope: "registry", + limit: 1000, + }, + registryMapSize: 100000, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mapping := map[imagesource.TypedImageReference]imagesource.TypedImageReference{} + for i, byteCount := 0, 0; byteCount < tt.registryMapSize; i++ { + key := fmt.Sprintf("foo-%d", i) + value := fmt.Sprintf("bar-%d", i) + mapping[imagesource.TypedImageReference{Ref: reference.DockerImageReference{Registry: key}}] = imagesource.TypedImageReference{Ref: reference.DockerImageReference{ID: value, Registry: value}} + byteCount += len(key) + len(value) + } + + got, err := generateICSPs(os.Stdout, tt.args.name, tt.args.scope, tt.args.limit, mapping) + if err != nil { + t.Error(err) + return + } + + for _, icsp := range got { + // check that all ICSPs are under ICSP limit + if icspBytes := len(icsp); icspBytes > tt.args.limit { + t.Errorf("ICSP size (%d) exceeded limit (%d)", icspBytes, tt.args.limit) + return + } + // convert Byte array into unstructured object + unstructuredObj := unstructured.Unstructured{Object: map[string]interface{}{}} + err = yaml.Unmarshal(icsp, unstructuredObj.Object) + if err != nil { + t.Error(err) + } + + // convert unstructured object into ICSP + icspObject := &operatorv1alpha1.ImageContentSourcePolicy{} + err = runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObj.Object, &icspObject) + if err != nil { + t.Error(err) + } + + // remove mappings found in ICSP from original mapping + for _, repositoryDigestMirrors := range icspObject.Spec.RepositoryDigestMirrors { + delete(mapping, imagesource.TypedImageReference{Ref: reference.DockerImageReference{Registry: repositoryDigestMirrors.Source}}) + } + } + // ensure that all mappings were seen in ICSPs + if len(mapping) != 0 { + t.Errorf("Not all mappings were found in the generated ICSPs") + return + } + }) + } +} + func ElementsMatch(listA, listB []string) error { aLen := len(listA) bLen := len(listB)