From 18ff7de9ae885b3b824a4c549b620a933419d36b Mon Sep 17 00:00:00 2001 From: Cesar Wong Date: Tue, 5 Sep 2023 17:51:57 -0400 Subject: [PATCH] Use MCO and CCO image references when looking up mappings Currently, we unconditionally use an image mapping from the management cluster if a mapping exists for ocp-release-dev or ocp/release. When the individual images do not use those registries, the wrong mapping is used. This commit uses the individual images from the payload to look for any image mappings that may exist on the management cluster. --- .../ignitionserver/ignitionserver.go | 86 ++++++++----------- .../ignitionserver/ignitionserver_test.go | 53 +++++++----- 2 files changed, 65 insertions(+), 74 deletions(-) diff --git a/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver.go b/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver.go index 496080537c..406293917b 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver.go +++ b/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "net" - "strings" configv1 "github.com/openshift/api/config/v1" routev1 "github.com/openshift/api/route/v1" @@ -186,53 +185,40 @@ func ReconcileIgnitionServer(ctx context.Context, if configOperatorImage == "" { return fmt.Errorf("cluster-config-operator image not found in payload images") } + machineConfigOperatorImage := componentImages["machine-config-operator"] + if machineConfigOperatorImage == "" { + return fmt.Errorf("machine-config-operator image not found in payload images") + } servingCertSecretName := ignitionserver.IgnitionServingCertSecret("").Name if hcp.Spec.Platform.Type != hyperv1.IBMCloudPlatform { servingCertSecretName = manifests.IgnitionServerCertSecret("").Name } - var ( - icsFound bool - sourceRef reference.DockerImageReference - mirrorImage string - ) - - if openShiftRegistryOverrides != "=" && len(openShiftRegistryOverrides) > 0 { - var err error - // Ignition server cannot handle ImageContentSourcePolicies or ImageDigestMachineSet, so we need to - // fill the registryOverrides in the case of disconnected environments - mirrorImage = lookupDisconnectedRegistry(ctx, openShiftRegistryOverrides) - if len(mirrorImage) > 0 { - sourceRef, err = reference.Parse(mirrorImage) - if err != nil { - return fmt.Errorf("failed to parse private registry hosted control plane image reference %q: %w", mirrorImage, err) - } - icsFound = true - } + // Determine if we need to override the machine config operator and cluster config operator + // images based on image mappings present in management cluster. + ocpRegistryMapping := util.ConvertImageRegistryOverrideStringToMap(openShiftRegistryOverrides) + overrideConfigOperatorImage, err := lookupMappedImage(ocpRegistryMapping, configOperatorImage) + if err != nil { + return err + } + overrideMachineConfigOperatorImage, err := lookupMappedImage(ocpRegistryMapping, machineConfigOperatorImage) + if err != nil { + return err } - if icsFound { - privateRegistry := sourceRef.Registry - - if !strings.HasPrefix(componentImages["machine-config-operator"], privateRegistry) { - ocpReleaseMcoImage := componentImages["machine-config-operator"] - mcoSha, err := reference.Parse(ocpReleaseMcoImage) - if err != nil { - return fmt.Errorf("failed to parse machine-config-operator image reference %q: %w", ocpReleaseMcoImage, err) - } - registryOverrides[ocpReleaseMcoImage] = fmt.Sprintf("%s@%s", mirrorImage, mcoSha.ID) + imageOverrides := map[string]string{} + for k, v := range registryOverrides { + if k != "" { + imageOverrides[k] = v } + } - if !strings.HasPrefix(componentImages["cluster-config-operator"], privateRegistry) { - ocpReleaseCCOImage := componentImages["cluster-config-operator"] - ccoSha, err := reference.Parse(ocpReleaseCCOImage) - if err != nil { - return fmt.Errorf("failed to parse cluster-config-operator image reference %s: %w", ocpReleaseCCOImage, err) - } - registryOverrides[ocpReleaseCCOImage] = fmt.Sprintf("%s@%s", mirrorImage, ccoSha.ID) - } + if overrideConfigOperatorImage != configOperatorImage { + imageOverrides[configOperatorImage] = overrideConfigOperatorImage + } - delete(registryOverrides, "") + if overrideMachineConfigOperatorImage != machineConfigOperatorImage { + imageOverrides[machineConfigOperatorImage] = overrideMachineConfigOperatorImage } ignitionServerDeployment := ignitionserver.Deployment(controlPlaneNamespace) @@ -244,7 +230,7 @@ func ReconcileIgnitionServer(ctx context.Context, hcp, defaultIngressDomain, hasHealthzHandler, - registryOverrides, + imageOverrides, openShiftRegistryOverrides, managementClusterHasCapabilitySecurityContextConstraint, ignitionServerLabels, @@ -896,18 +882,16 @@ cp /tmp/manifests/99_feature-gate.yaml %[1]s/99_feature-gate.yaml return fmt.Sprintf(script, workDir, featureGateYAML) } -func lookupDisconnectedRegistry(ctx context.Context, strOcpOverrides string) string { - ocpOverrides := strings.Split(strOcpOverrides, ",") - for _, entry := range ocpOverrides { - sourceImage := strings.Split(entry, "=")[0] - mirrorImage := strings.Split(entry, "=")[1] - - if strings.Contains(sourceImage, "openshift-release-dev") || strings.Contains(sourceImage, "ocp/release") { - // Production releases: 'openshift-release-dev' - // Nightly releases: 'ocp/release' - return mirrorImage +func lookupMappedImage(ocpOverrides map[string][]string, image string) (string, error) { + ref, err := reference.Parse(image) + if err != nil { + return "", fmt.Errorf("failed to parse image (%s): %w", image, err) + } + for source, replacements := range ocpOverrides { + if ref.AsRepository().String() == source { + newRef := fmt.Sprintf("%s@%s", replacements[0], ref.ID) + return newRef, nil } } - - return "" + return image, nil } diff --git a/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver_test.go b/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver_test.go index 863b2c4ea9..73e24498fd 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/ignitionserver/ignitionserver_test.go @@ -1,8 +1,6 @@ package ignitionserver import ( - "context" - "reflect" "testing" . "github.com/onsi/gomega" @@ -143,37 +141,46 @@ func TestReconcileIgnitionServerServiceRoute(t *testing.T) { } } -func TestLookupDisconnectedRegistry(t *testing.T) { - type args struct { - ctx context.Context - ocpOverrides string +func TestLookupMappedImage(t *testing.T) { + + ocpOverrides := map[string][]string{ + "quay.io/jparrill": {"registry.hypershiftbm.lab:5000/jparrill"}, + "quay.io/karmab": {"registry.hypershiftbm.lab:5000/karmab"}, + "quay.io/mavazque": {"registry.hypershiftbm.lab:5000/mavazque"}, + "quay.io/ocp-release": {"registry.hypershiftbm.lab:5000/openshift/release-images"}, + "quay.io/openshift-release-dev/ocp-v4.0-art-dev": {"registry.hypershiftbm.lab:5000/openshift/release"}, + "registry.access.redhat.com/openshift4/ose-oauth-proxy": {"registry.hypershiftbm.lab:5000/openshift4/ose-oauth-proxy"}, + "registry.redhat.io/lvms4": {"registry.hypershiftbm.lab:5000/lvms4"}, + "registry.redhat.io/multicluster-engine": {"registry.hypershiftbm.lab:5000/acm-d"}, + "registry.redhat.io/openshift4": {"registry.hypershiftbm.lab:5000/openshift4"}, + "registry.redhat.io/rhacm2": {"registry.hypershiftbm.lab:5000/acm-d"}, + "registry.redhat.io/rhel8": {"registry.hypershiftbm.lab:5000/rhel8"}, } + tests := []struct { - name string - args args - want string + name string + in string + expect string }{ { - name: "given an wellformed openshiftRegistryOverrides string, it should return the mirrorImage pointing to the private registry", - args: args{ - ctx: context.TODO(), - ocpOverrides: "quay.io/jparrill=registry.hypershiftbm.lab:5000/jparrill,quay.io/karmab=registry.hypershiftbm.lab:5000/karmab,quay.io/mavazque=registry.hypershiftbm.lab:5000/mavazque,quay.io/ocp-release=registry.hypershiftbm.lab:5000/openshift/release-images,quay.io/openshift-release-dev/ocp-v4.0-art-dev=registry.hypershiftbm.lab:5000/openshift/release,registry.access.redhat.com/openshift4/ose-oauth-proxy=registry.hypershiftbm.lab:5000/openshift4/ose-oauth-proxy,registry.redhat.io/lvms4=registry.hypershiftbm.lab:5000/lvms4,registry.redhat.io/multicluster-engine=registry.hypershiftbm.lab:5000/acm-d,registry.redhat.io/openshift4=registry.hypershiftbm.lab:5000/openshift4,registry.redhat.io/openshift4=registry.hypershiftbm.lab:5000/openshift4,registry.redhat.io/rhacm2=registry.hypershiftbm.lab:5000/acm-d,registry.redhat.io/rhel8=registry.hypershiftbm.lab:5000/rhel8", - }, - want: "registry.hypershiftbm.lab:5000/openshift/release", + name: "return the mirrorImage if it exists in the ocp overrides", + in: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:34acb17e45b80267919ea6a2f0e2b21e3f87db3c1e2443d2b3ac9afc0455ae07", + expect: "registry.hypershiftbm.lab:5000/openshift/release@sha256:34acb17e45b80267919ea6a2f0e2b21e3f87db3c1e2443d2b3ac9afc0455ae07", }, { - name: "given an wellformed openshiftRegistryOverrides string without ocpRelease entry, it should return an empty slice", - args: args{ - ctx: context.TODO(), - ocpOverrides: "quay.io/jparrill=registry.hypershiftbm.lab:5000/jparrill,quay.io/karmab=registry.hypershiftbm.lab:5000/karmab,quay.io/mavazque=registry.hypershiftbm.lab:5000/mavazque,registry.access.redhat.com/openshift4/ose-oauth-proxy=registry.hypershiftbm.lab:5000/openshift4/ose-oauth-proxy,registry.redhat.io/lvms4=registry.hypershiftbm.lab:5000/lvms4,registry.redhat.io/multicluster-engine=registry.hypershiftbm.lab:5000/acm-d,registry.redhat.io/openshift4=registry.hypershiftbm.lab:5000/openshift4,registry.redhat.io/openshift4=registry.hypershiftbm.lab:5000/openshift4,registry.redhat.io/rhacm2=registry.hypershiftbm.lab:5000/acm-d,registry.redhat.io/rhel8=registry.hypershiftbm.lab:5000/rhel8", - }, - want: "", + name: "return the same image if not in the overrides", + in: "registry.ci.openshift.org/ocp/4.14-2023-09-05-120503@sha256:34acb17e45b80267919ea6a2f0e2b21e3f87db3c1e2443d2b3ac9afc0455ae07", + expect: "registry.ci.openshift.org/ocp/4.14-2023-09-05-120503@sha256:34acb17e45b80267919ea6a2f0e2b21e3f87db3c1e2443d2b3ac9afc0455ae07", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := lookupDisconnectedRegistry(tt.args.ctx, tt.args.ocpOverrides); !reflect.DeepEqual(got, tt.want) { - t.Errorf("lookupDisconnectedRegistry() = %v, want %v", got, tt.want) + got, err := lookupMappedImage(ocpOverrides, tt.in) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if got != tt.expect { + t.Errorf("Wanted: %s, Got: %s", tt.expect, got) } }) }