Merge pull request #2513 from openshift-cherrypick-robot/cherry-pick-…

…2512-to-release_1.2.46

[release_1.2.46] OCM-11467 | fix: A few more bug fixes for registry config

cmd/create/cluster/cmd.go

@@ -3204,15 +3204,18 @@ func run(cmd *cobra.Command, _ []string) {
 	}
 	if clusterRegistryConfigArgs != nil {
 		allowedRegistries, blockedRegistries, insecureRegistries,
-			additionalTrustedCa, allowedRegistriesForImport := clusterregistryconfig.GetClusterRegistryConfigArgs(
+			additionalTrustedCa, allowedRegistriesForImport,
+			platformAllowlist := clusterregistryconfig.GetClusterRegistryConfigArgs(
 			clusterRegistryConfigArgs)
 		clusterConfig.AllowedRegistries = allowedRegistries
 		clusterConfig.BlockedRegistries = blockedRegistries
 		clusterConfig.InsecureRegistries = insecureRegistries
+		clusterConfig.PlatformAllowlist = platformAllowlist
+
 		if additionalTrustedCa != "" {
 			ca, err := clusterregistryconfig.BuildAdditionalTrustedCAFromInputFile(additionalTrustedCa)
 			if err != nil {
-				r.Reporter.Errorf("%s", err)
+				r.Reporter.Errorf("Failed to build the additional trusted ca from file %s, got error: %s", additionalTrustedCa, err)
 				os.Exit(1)
 			}
 			clusterConfig.AdditionalTrustedCa = ca

cmd/describe/cluster/cmd.go

@@ -497,11 +497,16 @@ func run(cmd *cobra.Command, argv []string) {
 				strings.Join(cluster.AWS().AdditionalAllowedPrincipals(), ","))
 		}
 		if cluster.RegistryConfig() != nil {
-			registryConfigOutput, err := getClusterRegistryConfig(cluster, r.OCMClient)
-			if err != nil {
-				r.Reporter.Errorf("Failed to get cluster registry config for cluster '%s': %v", clusterKey, err)
-				os.Exit(1)
+			var allowlist *cmv1.RegistryAllowlist
+			if cluster.RegistryConfig().PlatformAllowlist().ID() != "" {
+				allowlist, err = r.OCMClient.GetAllowlist(cluster.RegistryConfig().PlatformAllowlist().ID())
+				if err != nil {
+					r.Reporter.Errorf("Failed to get allowlist with id '%s': %v",
+						cluster.RegistryConfig().PlatformAllowlist().ID(), err)
+					os.Exit(1)
+				}
 			}
+			registryConfigOutput := getClusterRegistryConfig(cluster, allowlist)
 			str = fmt.Sprintf("%s"+
 				"Registry Configuration:\n"+
 				"%s\n", str, registryConfigOutput)
@@ -848,7 +853,7 @@ func getAuditLogForwardingStatus(cluster *cmv1.Cluster) string {
 	return auditLogForwardingStatus
 }
 
-func getClusterRegistryConfig(cluster *cmv1.Cluster, client *ocm.Client) (string, error) {
+func getClusterRegistryConfig(cluster *cmv1.Cluster, allowlist *cmv1.RegistryAllowlist) string {
 	var output string
 	if cluster.RegistryConfig().RegistrySources() != nil {
 		registryResources := cluster.RegistryConfig().RegistrySources()
@@ -876,23 +881,27 @@ func getClusterRegistryConfig(cluster *cmv1.Cluster, client *ocm.Client) (string
 		allowedRegistriesForImport := cluster.RegistryConfig().AllowedRegistriesForImport()
 		for _, registry := range allowedRegistriesForImport {
 			output = fmt.Sprintf("%s"+
-				"    - Domain Name: %s\n    - Insecure: %s\n", output,
+				"    - Domain Name:          %s\n    - Insecure:             %s\n", output,
 				registry.DomainName(), strconv.FormatBool(registry.Insecure()))
 		}
 	}
-	if cluster.RegistryConfig().PlatformAllowlist().ID() != "" {
-		allowlist, err := client.GetAllowlist(cluster.RegistryConfig().PlatformAllowlist().ID())
-		if err != nil {
-			return output, err
-		}
-
+	if cluster.RegistryConfig().PlatformAllowlist().ID() != "" && allowlist != nil {
 		output = fmt.Sprintf("%s"+
 			" - Platform Allowlist:      %s\n"+
 			"    - Registries:           %s\n", output,
 			cluster.RegistryConfig().PlatformAllowlist().ID(), strings.Join(allowlist.Registries(), ","))
 	}
-
-	return output, nil
+	if cluster.RegistryConfig().AdditionalTrustedCa() != nil {
+		output = fmt.Sprintf("%s"+
+			" - Additional Trusted CA:         \n", output,
+		)
+		for registry := range cluster.RegistryConfig().AdditionalTrustedCa() {
+			output = fmt.Sprintf("%s"+
+				"    - %s: REDACTED\n", output,
+				registry)
+		}
+	}
+	return output
 }
 
 func getExternalAuthConfigStatus(cluster *cmv1.Cluster) string {

cmd/describe/cluster/cmd_test.go

@@ -9,9 +9,6 @@ import (
 	. "github.com/onsi/ginkgo/v2/dsl/table"
 	. "github.com/onsi/gomega"
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
-
-	"github.com/openshift/rosa/pkg/logging"
-	"github.com/openshift/rosa/pkg/ocm"
 )
 
 const (
@@ -140,17 +137,11 @@ var _ = Describe("Cluster description", Ordered, func() {
 })
 
 var _ = Describe("getClusterRegistryConfig", func() {
-	var client *ocm.Client
-	BeforeEach(func() {
-		// todo this test expects and uses a real ocm client
-		// disabling the test until we can mock this to run in prow
-		Skip("disabling test until ocm client is mocked")
-		c, err := ocm.NewClient().Logger(logging.NewLogger()).Build()
-		Expect(err).NotTo(HaveOccurred())
-		client = c
-	})
 	It("Should return expected output", func() {
-		mockCluster, err := cmv1.NewCluster().RegistryConfig(cmv1.NewClusterRegistryConfig().
+		mockCa := make(map[string]string)
+		mockCa["registry.io"] = "-----BEGIN CERTIFICATE-----\nlalala\n-----END CERTIFICATE-----\n"
+		mockCa["registry.io2"] = "-----BEGIN CERTIFICATE-----\nlalala\n-----END CERTIFICATE-----\n"
+		mockCluster, err := cmv1.NewCluster().RegistryConfig(cmv1.NewClusterRegistryConfig().AdditionalTrustedCa(mockCa).
 			RegistrySources(cmv1.NewRegistrySources().
 				AllowedRegistries([]string{"allow1.com", "allow2.com"}...).
 				InsecureRegistries([]string{"insecure1.com", "insecure2.com"}...).
@@ -159,15 +150,22 @@ var _ = Describe("getClusterRegistryConfig", func() {
 				DomainName("quay.io").Insecure(true)).
 			PlatformAllowlist(cmv1.NewRegistryAllowlist().ID("test-id"))).Build()
 		Expect(err).NotTo(HaveOccurred())
-		output, err := getClusterRegistryConfig(mockCluster, client)
+
+		mockAllowlist, err := cmv1.NewRegistryAllowlist().ID("test-id").
+			Registries([]string{"registry1.io", "registry2.io"}...).Build()
 		Expect(err).NotTo(HaveOccurred())
+		output := getClusterRegistryConfig(mockCluster, mockAllowlist)
 		expectedOutput := " - Allowed Registries:      allow1.com,allow2.com\n" +
 			" - Blocked Registries:      block1.com,block2.com\n" +
 			" - Insecure Registries:     insecure1.com,insecure2.com\n" +
 			" - Allowed Registries for Import:         \n" +
-			"    - Domain Name: quay.io\n" +
-			"    - Insecure: true\n" +
-			" - Platform Allowlist:      test-id\n"
+			"    - Domain Name:          quay.io\n" +
+			"    - Insecure:             true\n" +
+			" - Platform Allowlist:      test-id\n" +
+			"    - Registries:           registry1.io,registry2.io\n" +
+			" - Additional Trusted CA:         \n" +
+			"    - registry.io: REDACTED\n" +
+			"    - registry.io2: REDACTED\n"
 		Expect(output).To(Equal(expectedOutput))
 	})
 })

cmd/edit/cluster/cmd.go

@@ -639,22 +639,30 @@ func run(cmd *cobra.Command, _ []string) {
 		os.Exit(1)
 	}
 	if clusterRegistryConfigArgs != nil {
-		prompt := "Changing any registry related parameter will trigger a rollout across all machinepools " +
-			"(all machinepool nodes will be recreated, following pod draining from each node). Do you want to proceed?"
-		if !confirm.ConfirmRaw(prompt) {
-			r.Reporter.Warnf("You have not changed any registry configuration -- exiting.")
-			os.Exit(0)
-		}
 		allowedRegistries, blockedRegistries, insecureRegistries,
-			additionalTrustedCa, allowedRegistriesForImport := clusterregistryconfig.GetClusterRegistryConfigArgs(
+			additionalTrustedCa, allowedRegistriesForImport,
+			platformAllowlist := clusterregistryconfig.GetClusterRegistryConfigArgs(
 			clusterRegistryConfigArgs)
+
+		// prompt for a warning if any registry config field is set
+		if allowedRegistries != nil || blockedRegistries != nil || insecureRegistries != nil ||
+			additionalTrustedCa != "" || allowedRegistriesForImport != "" || platformAllowlist != "" {
+			prompt := "Changing any registry related parameter will trigger a rollout across all machinepools " +
+				"(all machinepool nodes will be recreated, following pod draining from each node). Do you want to proceed?"
+			if !confirm.ConfirmRaw(prompt) {
+				r.Reporter.Warnf("You have not changed any registry configuration -- exiting.")
+				os.Exit(0)
+			}
+		}
+
 		clusterConfig.AllowedRegistries = allowedRegistries
 		clusterConfig.BlockedRegistries = blockedRegistries
 		clusterConfig.InsecureRegistries = insecureRegistries
+		clusterConfig.PlatformAllowlist = platformAllowlist
 		if additionalTrustedCa != "" {
 			ca, err := clusterregistryconfig.BuildAdditionalTrustedCAFromInputFile(additionalTrustedCa)
 			if err != nil {
-				r.Reporter.Errorf("%s", err)
+				r.Reporter.Errorf("Failed to build the additional trusted ca from file %s, got error: %s", additionalTrustedCa, err)
 				os.Exit(1)
 			}
 			clusterConfig.AdditionalTrustedCa = ca

pkg/clusterregistryconfig/flags.go

@@ -87,15 +87,18 @@ func AddClusterRegistryConfigFlags(cmd *cobra.Command) *ClusterRegistryConfigArg
 }
 
 func GetClusterRegistryConfigArgs(args *ClusterRegistryConfigArgs) (
-	[]string, []string, []string, string, string) {
+	[]string, []string, []string, string, string, string) {
 	return args.allowedRegistries, args.blockedRegistries,
-		args.insecureRegistries, args.additionalTrustedCa, args.allowedRegistriesForImport
+		args.insecureRegistries, args.additionalTrustedCa, args.allowedRegistriesForImport,
+		args.platformAllowlist
 }
 
 func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 	args *ClusterRegistryConfigArgs, isHostedCP bool, cluster *cmv1.Cluster) (
 	*ClusterRegistryConfigArgs, error) {
 
+	var allowedRegistries []string
+
 	if !isHostedCP {
 		if IsClusterRegistryConfigSetViaCLI(cmd) {
 			return nil, fmt.Errorf("Setting the registry config is only supported for hosted clusters")
@@ -105,10 +108,6 @@ func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 
 	result := &ClusterRegistryConfigArgs{}
 
-	if args.allowedRegistries != nil && args.blockedRegistries != nil {
-		return nil, fmt.Errorf("Allowed registries and blocked registries are mutually exclusive fields")
-	}
-
 	result.allowedRegistries = args.allowedRegistries
 	result.insecureRegistries = args.insecureRegistries
 	result.blockedRegistries = args.blockedRegistries
@@ -147,7 +146,7 @@ func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 		enableRegistriesConfig = true
 	}
 
-	if !enableRegistriesConfig && interactive.Enabled() && isHostedCP {
+	if interactive.Enabled() && isHostedCP {
 		updateRegistriesConfigValue, err := interactive.GetBool(interactive.Input{
 			Question: "Enable registries config",
 			Default:  enableRegistriesConfig,
@@ -158,9 +157,12 @@ func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 		enableRegistriesConfig = updateRegistriesConfigValue
 	}
 
+	isBlockedRegistryNotSet := result.blockedRegistries == nil || strings.Join(result.blockedRegistries, ",") == ""
+	isAllowedRegistryNotSet := result.allowedRegistries == nil || strings.Join(result.allowedRegistries, ",") == ""
+
 	if enableRegistriesConfig && interactive.Enabled() {
 		// Allowed registries and blocked registries are mutually exclusive
-		if result.blockedRegistries == nil {
+		if isBlockedRegistryNotSet {
 			allowedRegistriesInputs, err := interactive.GetString(interactive.Input{
 				Question: "Allowed Registries",
 				Help:     cmd.Lookup(allowedRegistriesFlag).Usage,
@@ -169,10 +171,22 @@ func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 			if err != nil {
 				return nil, fmt.Errorf("Expected a comma-separated list of allowed registries: %s", err)
 			}
-			result.allowedRegistries = helper.HandleEmptyStringOnSlice(strings.Split(allowedRegistriesInputs, ","))
+			allowedRegistries = helper.HandleEmptyStringOnSlice(strings.Split(allowedRegistriesInputs, ","))
+
+			if len(allowedRegistries) > 0 {
+				// received double quotes from the user. need to remove the existing value
+				if len(allowedRegistries) == 1 && allowedRegistries[0] == input.DoubleQuotesToRemove {
+					allowedRegistries[0] = ""
+				}
+			}
+			result.allowedRegistries = allowedRegistries
+			isAllowedRegistryNotSet = result.allowedRegistries == nil || strings.Join(result.allowedRegistries, ",") == ""
+		} else {
+			// if blocked registries is set, remove allowed registries
+			result.allowedRegistries = []string{}
 		}
 
-		if result.allowedRegistries == nil {
+		if isAllowedRegistryNotSet {
 			blockedRegistriesInputs, err := interactive.GetString(interactive.Input{
 				Question: "Blocked Registries",
 				Help:     cmd.Lookup(blockedRegistriesFlag).Usage,
@@ -182,6 +196,10 @@ func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 				return nil, fmt.Errorf("Expected a comma-separated list of blocked registries: %s", err)
 			}
 			result.blockedRegistries = helper.HandleEmptyStringOnSlice(strings.Split(blockedRegistriesInputs, ","))
+			isBlockedRegistryNotSet = result.blockedRegistries == nil || strings.Join(result.blockedRegistries, ",") == ""
+		} else {
+			// if allowed registries is set, remove blocked registries
+			result.blockedRegistries = []string{}
 		}
 
 		insecureRegistriesInputs, err := interactive.GetString(interactive.Input{
@@ -219,6 +237,10 @@ func GetClusterRegistryConfigOptions(cmd *pflag.FlagSet,
 		return nil, fmt.Errorf("Expected valid allowed registries for import values: %v", err)
 	}
 
+	if !isBlockedRegistryNotSet && !isAllowedRegistryNotSet {
+		return nil, fmt.Errorf("Allowed registries and blocked registries are mutually exclusive fields")
+	}
+
 	return result, nil
 }
 
@@ -287,8 +309,8 @@ func BuildAdditionalTrustedCAFromInputFile(specPath string) (map[string]string,
 	for k, v := range specJson {
 		form[k], ok = v.(string)
 		if !ok {
-			return nil, fmt.Errorf(
-				"the value for key '%s' is '%v'. Expect value to be string, got %T", k, v, v)
+			return nil, fmt.Errorf("expected a valid value for 'additional_trusted_ca'. " +
+				"Should be in a <registry>:<boolean> format.")
 		}
 	}
 

pkg/clusterregistryconfig/flags_test.go

@@ -47,7 +47,7 @@ var _ = Describe("Cluster Registry Config tests", func() {
 			_, err := BuildAdditionalTrustedCAFromInputFile(caPath)
 			Expect(
 				err,
-			).To(MatchError("the value for key 'userRegistry.io' is 'true'. Expect value to be string, got bool"))
+			).To(MatchError("expected a valid value for 'additional_trusted_ca'. Should be in a <registry>:<boolean> format."))
 
 		})
 	})