From 22719471f5417410abac88dbe3ace779a383aa5b Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Wed, 5 Jun 2024 09:52:44 -0400 Subject: [PATCH] OCM-8315 | feat: add additional allowed principals config for hcp clusters Signed-off-by: Mike Ng --- cmd/create/cluster/cmd.go | 47 +++++++++++++++++++++-- cmd/describe/cluster/cmd.go | 6 +++ cmd/describe/cluster/cmd_test.go | 16 ++++++-- cmd/edit/cluster/cmd.go | 65 ++++++++++++++++++++++++++++++- pkg/helper/roles/helpers.go | 16 ++++++++ pkg/helper/roles/helpers_test.go | 66 ++++++++++++++++++++++++++++++++ pkg/ocm/clusters.go | 24 ++++++++---- 7 files changed, 226 insertions(+), 14 deletions(-) create mode 100644 pkg/helper/roles/helpers_test.go diff --git a/cmd/create/cluster/cmd.go b/cmd/create/cluster/cmd.go index 13901b6426..8d0ec77005 100644 --- a/cmd/create/cluster/cmd.go +++ b/cmd/create/cluster/cmd.go @@ -180,9 +180,10 @@ var args struct { tags []string // Hypershift options: - hostedClusterEnabled bool - billingAccount string - noCni bool + hostedClusterEnabled bool + billingAccount string + noCni bool + additionalAllowedPrincipals []string // Cluster Admin createAdminUser bool @@ -455,6 +456,15 @@ func initFlags(cmd *cobra.Command) { "A file contains a PEM-encoded X.509 certificate bundle that will be "+ "added to the nodes' trusted certificate store.") + flags.StringSliceVar( + &args.additionalAllowedPrincipals, + "additional-allowed-principals", + nil, + "A comma-separated list of additional allowed principal ARNs "+ + "to be added to the Hosted Control Plane's VPC Endpoint Service to enable additional "+ + "VPC Endpoint connection requests to be automatically accepted.", + ) + flags.BoolVar(&args.enableCustomerManagedKey, "enable-customer-managed-key", false, @@ -2852,6 +2862,31 @@ func run(cmd *cobra.Command, _ []string) { os.Exit(1) } + // Additional Allowed Principals + if cmd.Flags().Changed("additional-allowed-principals") && !isHostedCP { + r.Reporter.Errorf("Additional Allowed Principals is supported only for Hosted Control Planes") + os.Exit(1) + } + additionalAllowedPrincipals := args.additionalAllowedPrincipals + if isHostedCP && interactive.Enabled() { + aapInputs, err := interactive.GetString(interactive.Input{ + Question: "Additional Allowed Principal ARNs", + Help: cmd.Flags().Lookup("additional-allowed-principals").Usage, + Default: strings.Join(additionalAllowedPrincipals, ","), + }) + if err != nil { + r.Reporter.Errorf("Expected a valid value for Additional Allowed Principal ARNs: %s", err) + os.Exit(1) + } + additionalAllowedPrincipals = helper.HandleEmptyStringOnSlice(strings.Split(aapInputs, ",")) + } + if len(additionalAllowedPrincipals) > 0 { + if err := roles.ValidateAdditionalAllowedPrincipals(additionalAllowedPrincipals); err != nil { + r.Reporter.Errorf(err.Error()) + os.Exit(1) + } + } + // Audit Log Forwarding auditLogRoleARN := args.AuditLogRoleARN @@ -3100,6 +3135,7 @@ func run(cmd *cobra.Command, _ []string) { AdditionalComputeSecurityGroupIds: additionalComputeSecurityGroupIds, AdditionalInfraSecurityGroupIds: additionalInfraSecurityGroupIds, AdditionalControlPlaneSecurityGroupIds: additionalControlPlaneSecurityGroupIds, + AdditionalAllowedPrincipals: additionalAllowedPrincipals, } if httpTokens != "" { @@ -3896,6 +3932,11 @@ func buildCommand(spec ocm.Spec, operatorRolesPrefix string, command += " --no-cni" } + if len(spec.AdditionalAllowedPrincipals) > 0 { + command += fmt.Sprintf(" --additional-allowed-principals %s", + strings.Join(spec.AdditionalAllowedPrincipals, ",")) + } + for _, p := range properties { command += fmt.Sprintf(" --properties \"%s\"", p) } diff --git a/cmd/describe/cluster/cmd.go b/cmd/describe/cluster/cmd.go index 820b14a01b..fbfed31bea 100644 --- a/cmd/describe/cluster/cmd.go +++ b/cmd/describe/cluster/cmd.go @@ -483,6 +483,12 @@ func run(cmd *cobra.Command, argv []string) { str = fmt.Sprintf("%s"+ "Audit Log Role ARN: %s\n", str, cluster.AWS().AuditLog().RoleArn()) } + if len(cluster.AWS().AdditionalAllowedPrincipals()) > 0 { + // Omitted the 'Allowed' due to formatting + str = fmt.Sprintf("%s"+ + "Additional Principals: %s\n", str, + strings.Join(cluster.AWS().AdditionalAllowedPrincipals(), ",")) + } } if cluster.Status().State() == cmv1.ClusterStateError { diff --git a/cmd/describe/cluster/cmd_test.go b/cmd/describe/cluster/cmd_test.go index 514898f221..ab6112cac8 100644 --- a/cmd/describe/cluster/cmd_test.go +++ b/cmd/describe/cluster/cmd_test.go @@ -23,6 +23,8 @@ var ( `{"displayName":"displayname","id":"bar","kind":"Cluster","name":"foo"}`) expectClusterWithExternalAuthConfig = []byte( `{"displayName":"displayname","external_auth_config":{"enabled":true},"kind":"Cluster"}`) + expectClusterWithAap = []byte( + `{"aws":{"additional_allowed_principals":["foobar"]},"displayName":"displayname","kind":"Cluster"}`) expectClusterWithNameAndValueAndUpgradeInformation = []byte( `{"displayName":"displayname","id":"bar","kind":"Cluster","name":"foo","scheduledUpgrade":{"nextRun":"` + now.Format("2006-01-02 15:04 MST") + `","state":"` + state + `","version":"` + @@ -32,9 +34,9 @@ var ( now.Format("2006-01-02 15:04 MST") + `","state":"` + state + `","version":"` + version + `"}}`) - clusterWithNameAndID, emptyCluster, clusterWithExternalAuthConfig *cmv1.Cluster - emptyUpgradePolicy, upgradePolicyWithVersionAndNextRun *cmv1.UpgradePolicy - emptyUpgradeState, upgradePolicyWithState *cmv1.UpgradePolicyState + clusterWithNameAndID, emptyCluster, clusterWithExternalAuthConfig, clusterWithAap *cmv1.Cluster + emptyUpgradePolicy, upgradePolicyWithVersionAndNextRun *cmv1.UpgradePolicy + emptyUpgradeState, upgradePolicyWithState *cmv1.UpgradePolicyState berr error ) @@ -46,6 +48,9 @@ var _ = BeforeSuite(func() { externalAuthConfig := cmv1.NewExternalAuthConfig().Enabled(true) clusterWithExternalAuthConfig, berr = cmv1.NewCluster().ExternalAuthConfig(externalAuthConfig).Build() Expect(berr).NotTo(HaveOccurred()) + additionalAllowedPrincipals := cmv1.NewAWS().AdditionalAllowedPrincipals("foobar") + clusterWithAap, berr = cmv1.NewCluster().AWS(additionalAllowedPrincipals).Build() + Expect(berr).NotTo(HaveOccurred()) emptyUpgradePolicy, berr = cmv1.NewUpgradePolicy().Build() Expect(berr).NotTo(HaveOccurred()) emptyUpgradeState, berr = cmv1.NewUpgradePolicyState().Build() @@ -108,6 +113,11 @@ var _ = Describe("Cluster description", Ordered, func() { func() *cmv1.Cluster { return clusterWithExternalAuthConfig }, func() *cmv1.UpgradePolicy { return emptyUpgradePolicy }, func() *cmv1.UpgradePolicyState { return nil }, expectClusterWithExternalAuthConfig, nil), + + Entry("Prints cluster information with the additional allowed principals", + func() *cmv1.Cluster { return clusterWithAap }, + func() *cmv1.UpgradePolicy { return emptyUpgradePolicy }, + func() *cmv1.UpgradePolicyState { return nil }, expectClusterWithAap, nil), ) }) }) diff --git a/cmd/edit/cluster/cmd.go b/cmd/edit/cluster/cmd.go index b3505a96e6..54d4ea5579 100644 --- a/cmd/edit/cluster/cmd.go +++ b/cmd/edit/cluster/cmd.go @@ -28,6 +28,7 @@ import ( "github.com/openshift/rosa/pkg/aws" "github.com/openshift/rosa/pkg/helper" + "github.com/openshift/rosa/pkg/helper/roles" "github.com/openshift/rosa/pkg/input" "github.com/openshift/rosa/pkg/interactive" "github.com/openshift/rosa/pkg/interactive/confirm" @@ -53,6 +54,9 @@ var args struct { // Audit log forwarding auditLogRoleARN string + + // Other options + additionalAllowedPrincipals []string } var Cmd = &cobra.Command{ @@ -147,6 +151,15 @@ func init() { "", "The ARN of the role that is used to forward audit logs to AWS CloudWatch.", ) + + flags.StringSliceVar( + &args.additionalAllowedPrincipals, + "additional-allowed-principals", + nil, + "A comma-separated list of additional allowed principal ARNs "+ + "to be added to the Hosted Control Plane's VPC Endpoint Service to enable additional "+ + "VPC Endpoint connection requests to be automatically accepted.", + ) } func run(cmd *cobra.Command, _ []string) { @@ -160,7 +173,7 @@ func run(cmd *cobra.Command, _ []string) { changedFlags := false for _, flag := range []string{"expiration-time", "expiration", "private", "disable-workload-monitoring", "http-proxy", "https-proxy", "no-proxy", - "additional-trust-bundle-file", "audit-log-arn"} { + "additional-trust-bundle-file", "additional-allowed-principals", "audit-log-arn"} { if cmd.Flags().Changed(flag) { changedFlags = true } @@ -230,6 +243,11 @@ func run(cmd *cobra.Command, _ []string) { os.Exit(1) } + var additionalAllowedPrincipals []string + if cmd.Flags().Changed("additional-allowed-principals") { + additionalAllowedPrincipals = args.additionalAllowedPrincipals + } + var private *bool var privateValue bool if cmd.Flags().Changed("private") { @@ -500,6 +518,47 @@ func run(cmd *cobra.Command, _ []string) { } } + /******* AdditionalAllowedPrincipals *******/ + updateAdditionalAllowedPrincipals := false + if additionalAllowedPrincipals != nil { + updateAdditionalAllowedPrincipals = true + } + if !updateAdditionalAllowedPrincipals && additionalAllowedPrincipals == nil && + interactive.Enabled() { + updateAdditionalAllowedPrincipalsValue, err := interactive.GetBool(interactive.Input{ + Question: "Update additional allowed principals", + Default: updateAdditionalAllowedPrincipals, + }) + if err != nil { + r.Reporter.Errorf("Expected a valid update-additional-allowed-principals value: %s", err) + os.Exit(1) + } + updateAdditionalAllowedPrincipals = updateAdditionalAllowedPrincipalsValue + } + if updateAdditionalAllowedPrincipals && interactive.Enabled() { + aapInputs, err := interactive.GetString(interactive.Input{ + Question: "Additional Allowed Principal ARNs", + Help: cmd.Flags().Lookup("additional-allowed-principals").Usage, + Default: cluster.AWS().AdditionalAllowedPrincipals(), + }) + if err != nil { + r.Reporter.Errorf("Expected a valid value for Additional Allowed Principal ARNs: %s", err) + os.Exit(1) + } + additionalAllowedPrincipals = helper.HandleEmptyStringOnSlice(strings.Split(aapInputs, ",")) + } + if len(additionalAllowedPrincipals) > 0 { + if len(additionalAllowedPrincipals) == 1 && + additionalAllowedPrincipals[0] == input.DoubleQuotesToRemove { + additionalAllowedPrincipals = []string{} + } else { + if err := roles.ValidateAdditionalAllowedPrincipals(additionalAllowedPrincipals); err != nil { + r.Reporter.Errorf(err.Error()) + os.Exit(1) + } + } + } + // Audit Log Forwarding auditLogRole, err := setAuditLogForwarding(r, cmd, cluster, args.auditLogRoleARN) if err != nil { @@ -549,6 +608,10 @@ func run(cmd *cobra.Command, _ []string) { } } + if additionalAllowedPrincipals != nil { + clusterConfig.AdditionalAllowedPrincipals = additionalAllowedPrincipals + } + if auditLogRole != nil { clusterConfig.AuditLogRoleARN = new(string) *clusterConfig.AuditLogRoleARN = *auditLogRole diff --git a/pkg/helper/roles/helpers.go b/pkg/helper/roles/helpers.go index 7810160746..76cc58d033 100644 --- a/pkg/helper/roles/helpers.go +++ b/pkg/helper/roles/helpers.go @@ -281,3 +281,19 @@ func upgradeMissingOperatorRole(missingRoles map[string]*cmv1.STSOperator, clust } return nil } + +func ValidateAdditionalAllowedPrincipals(aapARNs []string) error { + duplicate, found := aws.HasDuplicates(aapARNs) + if found { + return fmt.Errorf("Invalid additional allowed principals list, duplicate key '%s' found", + duplicate) + } + for _, aap := range aapARNs { + err := aws.ARNValidator(aap) + if err != nil { + return fmt.Errorf("Expected valid ARNs for additional allowed principals list: %s", + err) + } + } + return nil +} diff --git a/pkg/helper/roles/helpers_test.go b/pkg/helper/roles/helpers_test.go new file mode 100644 index 0000000000..246949d1ae --- /dev/null +++ b/pkg/helper/roles/helpers_test.go @@ -0,0 +1,66 @@ +/* +Copyright (c) 2024 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package roles + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestRolesHelper(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Roles Helper Suite") +} + +var _ = Describe("Roles Helper", func() { + Context("Validate Additional Allowed Principals", func() { + It("should pass when valid ARNs", func() { + aapARNs := []string{ + "arn:aws:iam::123456789012:role/role1", + "arn:aws:iam::123456789012:role/role2", + } + err := ValidateAdditionalAllowedPrincipals(aapARNs) + Expect(err).ShouldNot(HaveOccurred()) + }) + It("should error when containing duplicate ARNs", func() { + aapARNs := []string{ + "arn:aws:iam::123456789012:role/role1", + "arn:aws:iam::123456789012:role/role2", + "arn:aws:iam::123456789012:role/role1", + } + err := ValidateAdditionalAllowedPrincipals(aapARNs) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal( + "Invalid additional allowed principals list, " + + "duplicate key 'arn:aws:iam::123456789012:role/role1' found")) + }) + + It("should error when contain invalid ARN", func() { + aapARNs := []string{ + "arn:aws:iam::123456789012:role/role1", + "foobar", + } + err := ValidateAdditionalAllowedPrincipals(aapARNs) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring( + "Expected valid ARNs for additional allowed principals list")) + }) + + }) +}) diff --git a/pkg/ocm/clusters.go b/pkg/ocm/clusters.go index 552d8e2d4c..4b18b5dd08 100644 --- a/pkg/ocm/clusters.go +++ b/pkg/ocm/clusters.go @@ -141,9 +141,10 @@ type Spec struct { AdditionalTrustBundle *string // HyperShift options: - Hypershift Hypershift - BillingAccount string - NoCni bool + Hypershift Hypershift + BillingAccount string + NoCni bool + AdditionalAllowedPrincipals []string // Audit Log Forwarding AuditLogRoleARN *string @@ -628,11 +629,16 @@ func (c *Client) UpdateCluster(clusterKey string, creator *aws.Creator, config S clusterBuilder.Hypershift(hyperShiftBuilder) } - // Edit audit log role arn - if config.AuditLogRoleARN != nil { + if config.AuditLogRoleARN != nil || config.AdditionalAllowedPrincipals != nil { awsBuilder := cmv1.NewAWS() - auditLogBuiler := cmv1.NewAuditLog().RoleArn(*config.AuditLogRoleARN) - awsBuilder = awsBuilder.AuditLog(auditLogBuiler) + if config.AdditionalAllowedPrincipals != nil { + awsBuilder = awsBuilder.AdditionalAllowedPrincipals(config.AdditionalAllowedPrincipals...) + } + // Edit audit log role arn + if config.AuditLogRoleARN != nil { + auditLogBuiler := cmv1.NewAuditLog().RoleArn(*config.AuditLogRoleARN) + awsBuilder = awsBuilder.AuditLog(auditLogBuiler) + } clusterBuilder.AWS(awsBuilder) } @@ -887,6 +893,10 @@ func (c *Client) createClusterSpec(config Spec) (*cmv1.Cluster, error) { awsBuilder = awsBuilder.AdditionalControlPlaneSecurityGroupIds(config.AdditionalControlPlaneSecurityGroupIds...) } + if len(config.AdditionalAllowedPrincipals) > 0 { + awsBuilder = awsBuilder.AdditionalAllowedPrincipals(config.AdditionalAllowedPrincipals...) + } + if config.SubnetIds != nil { awsBuilder = awsBuilder.SubnetIDs(config.SubnetIds...) }