Skip to content

Commit c99f988

Browse files
committed
Review fixes
1 parent b43ea50 commit c99f988

14 files changed

+239
-78
lines changed

config/crd/bases/controlplane.cluster.x-k8s.io_rosacontrolplanes.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,15 @@ spec:
759759
[\n\t\t{\n\t\t\t\"Effect\": \"Allow\",\n\t\t\t\"Action\": [\n\t\t\t\t\"ec2:AttachVolume\",\n\t\t\t\t\"ec2:CreateSnapshot\",\n\t\t\t\t\"ec2:CreateTags\",\n\t\t\t\t\"ec2:CreateVolume\",\n\t\t\t\t\"ec2:DeleteSnapshot\",\n\t\t\t\t\"ec2:DeleteTags\",\n\t\t\t\t\"ec2:DeleteVolume\",\n\t\t\t\t\"ec2:DescribeInstances\",\n\t\t\t\t\"ec2:DescribeSnapshots\",\n\t\t\t\t\"ec2:DescribeTags\",\n\t\t\t\t\"ec2:DescribeVolumes\",\n\t\t\t\t\"ec2:DescribeVolumesModifications\",\n\t\t\t\t\"ec2:DetachVolume\",\n\t\t\t\t\"ec2:ModifyVolume\"\n\t\t\t],\n\t\t\t\"Resource\":
760760
\"*\"\n\t\t}\n\t]\n}"
761761
type: string
762+
required:
763+
- controlPlaneOperatorARN
764+
- imageRegistryARN
765+
- ingressARN
766+
- kmsProviderARN
767+
- kubeCloudControllerARN
768+
- networkARN
769+
- nodePoolManagementARN
770+
- storageARN
762771
type: object
763772
rosaClusterName:
764773
description: |-

config/crd/bases/infrastructure.cluster.x-k8s.io_rosaroleconfigs.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,15 @@ spec:
410410
[\n\t\t{\n\t\t\t\"Effect\": \"Allow\",\n\t\t\t\"Action\": [\n\t\t\t\t\"ec2:AttachVolume\",\n\t\t\t\t\"ec2:CreateSnapshot\",\n\t\t\t\t\"ec2:CreateTags\",\n\t\t\t\t\"ec2:CreateVolume\",\n\t\t\t\t\"ec2:DeleteSnapshot\",\n\t\t\t\t\"ec2:DeleteTags\",\n\t\t\t\t\"ec2:DeleteVolume\",\n\t\t\t\t\"ec2:DescribeInstances\",\n\t\t\t\t\"ec2:DescribeSnapshots\",\n\t\t\t\t\"ec2:DescribeTags\",\n\t\t\t\t\"ec2:DescribeVolumes\",\n\t\t\t\t\"ec2:DescribeVolumesModifications\",\n\t\t\t\t\"ec2:DetachVolume\",\n\t\t\t\t\"ec2:ModifyVolume\"\n\t\t\t],\n\t\t\t\"Resource\":
411411
\"*\"\n\t\t}\n\t]\n}"
412412
type: string
413+
required:
414+
- controlPlaneOperatorARN
415+
- imageRegistryARN
416+
- ingressARN
417+
- kmsProviderARN
418+
- kubeCloudControllerARN
419+
- networkARN
420+
- nodePoolManagementARN
421+
- storageARN
413422
type: object
414423
type: object
415424
type: object

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,7 @@ type AWSRolesRef struct {
414414
// }
415415
// ]
416416
// }
417-
IngressARN string `json:"ingressARN,omitempty"`
417+
IngressARN string `json:"ingressARN"`
418418

419419
// ImageRegistryARN is an ARN value referencing a role appropriate for the Image Registry Operator.
420420
//
@@ -449,7 +449,7 @@ type AWSRolesRef struct {
449449
// }
450450
// ]
451451
// }
452-
ImageRegistryARN string `json:"imageRegistryARN,omitempty"`
452+
ImageRegistryARN string `json:"imageRegistryARN"`
453453

454454
// StorageARN is an ARN value referencing a role appropriate for the Storage Operator.
455455
//
@@ -480,7 +480,7 @@ type AWSRolesRef struct {
480480
// }
481481
// ]
482482
// }
483-
StorageARN string `json:"storageARN,omitempty"`
483+
StorageARN string `json:"storageARN"`
484484

485485
// NetworkARN is an ARN value referencing a role appropriate for the Network Operator.
486486
//
@@ -506,7 +506,7 @@ type AWSRolesRef struct {
506506
// }
507507
// ]
508508
// }
509-
NetworkARN string `json:"networkARN,omitempty"`
509+
NetworkARN string `json:"networkARN"`
510510

511511
// KubeCloudControllerARN is an ARN value referencing a role appropriate for the KCM/KCC.
512512
// Source: https://cloud-provider-aws.sigs.k8s.io/prerequisites/#iam-policies
@@ -584,7 +584,7 @@ type AWSRolesRef struct {
584584
// ]
585585
// }
586586
// +immutable
587-
KubeCloudControllerARN string `json:"kubeCloudControllerARN,omitempty"`
587+
KubeCloudControllerARN string `json:"kubeCloudControllerARN"`
588588

589589
// NodePoolManagementARN is an ARN value referencing a role appropriate for the CAPI Controller.
590590
//
@@ -697,7 +697,7 @@ type AWSRolesRef struct {
697697
// }
698698
//
699699
// +immutable
700-
NodePoolManagementARN string `json:"nodePoolManagementARN,omitempty"`
700+
NodePoolManagementARN string `json:"nodePoolManagementARN"`
701701

702702
// ControlPlaneOperatorARN is an ARN value referencing a role appropriate for the Control Plane Operator.
703703
//
@@ -737,8 +737,8 @@ type AWSRolesRef struct {
737737
// ]
738738
// }
739739
// +immutable
740-
ControlPlaneOperatorARN string `json:"controlPlaneOperatorARN,omitempty"`
741-
KMSProviderARN string `json:"kmsProviderARN,omitempty"`
740+
ControlPlaneOperatorARN string `json:"controlPlaneOperatorARN"`
741+
KMSProviderARN string `json:"kmsProviderARN"`
742742
}
743743

744744
// RosaControlPlaneStatus defines the observed state of ROSAControlPlane.

controlplane/rosa/api/v1beta2/rosacontrolplane_webhook.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
/*
2+
Copyright 2023 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package v1beta2
218

319
import (

exp/api/v1beta2/rosaroleconfig_types.go

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright The Kubernetes Authors.
2+
Copyright 2025 The Kubernetes Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -25,9 +25,6 @@ import (
2525
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
2626
)
2727

28-
// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
29-
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.
30-
3128
// ROSARoleConfigSpec defines the desired state of ROSARoleConfig
3229
type ROSARoleConfigSpec struct {
3330
AccountRoleConfig AccountRoleConfig `json:"accountRoleConfig"`
@@ -158,6 +155,25 @@ const (
158155
RosaRoleConfigCreatedReason = "Created"
159156
)
160157

158+
const (
159+
// IngressOperatorARNSuffix is the suffix for the ingress operator role.
160+
IngressOperatorARNSuffix = "-openshift-ingress-operator-cloud-credentials"
161+
// ImageRegistryARNSuffix is the suffix for the image registry operator role.
162+
ImageRegistryARNSuffix = "-openshift-image-registry-installer-cloud-credentials"
163+
// StorageARNSuffix is the suffix for the storage operator role.
164+
StorageARNSuffix = "-openshift-cluster-csi-drivers-ebs-cloud-credentials"
165+
// NetworkARNSuffix is the suffix for the network operator role.
166+
NetworkARNSuffix = "-openshift-cloud-network-config-controller-cloud-credentials"
167+
// KubeCloudControllerARNSuffix is the suffix for the kube cloud controller role.
168+
KubeCloudControllerARNSuffix = "-kube-system-kube-controller-manager"
169+
// NodePoolManagementARNSuffix is the suffix for the node pool management role.
170+
NodePoolManagementARNSuffix = "-kube-system-capa-controller-manager"
171+
// ControlPlaneOperatorARNSuffix is the suffix for the control plane operator role.
172+
ControlPlaneOperatorARNSuffix = "-kube-system-control-plane-operator"
173+
// KMSProviderARNSuffix is the suffix for the kms provider role.
174+
KMSProviderARNSuffix = "-kube-system-kms-provider"
175+
)
176+
161177
// SetConditions sets the conditions of the ROSARoleConfig.
162178
func (r *ROSARoleConfig) SetConditions(conditions clusterv1.Conditions) {
163179
r.Status.Conditions = conditions
@@ -168,6 +184,11 @@ func (r *ROSARoleConfig) GetConditions() clusterv1.Conditions {
168184
return r.Status.Conditions
169185
}
170186

187+
// IsSharedVPC checks if the shared VPC config is set.
188+
func (s SharedVPCConfig) IsSharedVPC() bool {
189+
return s.VPCEndpointRoleARN != "" && s.RouteRoleARN != ""
190+
}
191+
171192
func init() {
172193
SchemeBuilder.Register(&ROSARoleConfig{}, &ROSARoleConfigList{})
173194
}

exp/controllers/rosamachinepool_controller.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
/*
2+
Copyright 2023 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package controllers
218

319
import (

exp/controllers/rosaroleconfig_controller.go

Lines changed: 47 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright The Kubernetes Authors.
2+
Copyright 2025 The Kubernetes Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -63,7 +63,6 @@ type ROSARoleConfigReconciler struct {
6363
client.Client
6464
Log logr.Logger
6565
Scheme *runtime.Scheme
66-
Endpoints []scope.ServiceEndpoint
6766
WatchFilterValue string
6867
NewStsClient func(cloud.ScopeUsage, cloud.Session, logger.Wrapper, runtime.Object) stsiface.STSClient
6968
NewOCMClient func(ctx context.Context, scope rosa.OCMSecretsRetriever) (rosa.OCMClient, error)
@@ -93,6 +92,7 @@ func (r *ROSARoleConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
9392
if apierrors.IsNotFound(err) {
9493
return ctrl.Result{}, nil
9594
}
95+
log.Error(err, "Failed to get ROSARoleConfig")
9696
return ctrl.Result{Requeue: true}, nil
9797
}
9898

@@ -101,7 +101,6 @@ func (r *ROSARoleConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
101101
Client: r.Client,
102102
RosaRoleConfig: roleConfig,
103103
ControllerName: "rosaroleconfig",
104-
Endpoints: r.Endpoints,
105104
Logger: log,
106105
})
107106

@@ -113,7 +112,7 @@ func (r *ROSARoleConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
113112
defer func() {
114113
conditions.SetSummary(scope.RosaRoleConfig, conditions.WithConditions(expinfrav1.RosaRoleConfigReadyCondition), conditions.WithStepCounter())
115114

116-
if err := scope.Close(); err != nil {
115+
if err := scope.PatchObject(); err != nil {
117116
reterr = errors.Join(reterr, err)
118117
}
119118
}()
@@ -134,9 +133,7 @@ func (r *ROSARoleConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
134133
}
135134

136135
if controllerutil.AddFinalizer(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigFinalizer) {
137-
if err := scope.PatchObject(); err != nil {
138-
return ctrl.Result{}, err
139-
}
136+
return ctrl.Result{}, err
140137
}
141138

142139
err = r.createAccountRoles(ctx, roleConfig, scope, ocmClient)
@@ -260,7 +257,6 @@ func (r *ROSARoleConfigReconciler) createOperatorRoles(ctx context.Context, role
260257
version := roleConfig.Spec.AccountRoleConfig.Version
261258
hostedCp := true
262259
forcePolicyCreation := true
263-
isSharedVpc := config.SharedVPCConfig.VPCEndpointRoleARN != "" && config.SharedVPCConfig.RouteRoleARN != ""
264260

265261
operatorRoles, err := runtime.AWSClient.ListOperatorRoles(version, "", config.Prefix)
266262

@@ -270,28 +266,35 @@ func (r *ROSARoleConfigReconciler) createOperatorRoles(ctx context.Context, role
270266

271267
for _, roles := range operatorRoles {
272268
for _, role := range roles {
273-
if role.RoleName == fmt.Sprintf("%s-openshift-ingress-operator-cloud-credentials", config.Prefix) {
269+
roleSuffix := strings.TrimPrefix(role.RoleName, config.Prefix)
270+
if roleSuffix == role.RoleName {
271+
continue
272+
}
273+
switch roleSuffix {
274+
case expinfrav1.IngressOperatorARNSuffix:
274275
scope.RosaRoleConfig.Status.OperatorRolesRef.IngressARN = role.RoleARN
275-
} else if role.RoleName == fmt.Sprintf("%s-openshift-image-registry-installer-cloud-credentials", config.Prefix) {
276+
case expinfrav1.ImageRegistryARNSuffix:
276277
scope.RosaRoleConfig.Status.OperatorRolesRef.ImageRegistryARN = role.RoleARN
277-
} else if role.RoleName == fmt.Sprintf("%s-openshift-cluster-csi-drivers-ebs-cloud-credentials", config.Prefix) {
278+
case expinfrav1.StorageARNSuffix:
278279
scope.RosaRoleConfig.Status.OperatorRolesRef.StorageARN = role.RoleARN
279-
} else if role.RoleName == fmt.Sprintf("%s-openshift-cloud-network-config-controller-cloud-credentials", config.Prefix) {
280+
case expinfrav1.NetworkARNSuffix:
280281
scope.RosaRoleConfig.Status.OperatorRolesRef.NetworkARN = role.RoleARN
281-
} else if role.RoleName == fmt.Sprintf("%s-kube-system-kube-controller-manager", config.Prefix) {
282+
case expinfrav1.KubeCloudControllerARNSuffix:
282283
scope.RosaRoleConfig.Status.OperatorRolesRef.KubeCloudControllerARN = role.RoleARN
283-
} else if role.RoleName == fmt.Sprintf("%s-kube-system-capa-controller-manager", config.Prefix) {
284+
case expinfrav1.NodePoolManagementARNSuffix:
284285
scope.RosaRoleConfig.Status.OperatorRolesRef.NodePoolManagementARN = role.RoleARN
285-
} else if role.RoleName == fmt.Sprintf("%s-kube-system-control-plane-operator", config.Prefix) {
286+
case expinfrav1.ControlPlaneOperatorARNSuffix:
286287
scope.RosaRoleConfig.Status.OperatorRolesRef.ControlPlaneOperatorARN = role.RoleARN
287-
} else if role.RoleName == fmt.Sprintf("%s-kube-system-kms-provider", config.Prefix) {
288+
case expinfrav1.KMSProviderARNSuffix:
288289
scope.RosaRoleConfig.Status.OperatorRolesRef.KMSProviderARN = role.RoleARN
289290
}
290291
}
291292
}
292293

293294
if !r.operatorRolesReady(&scope.RosaRoleConfig.Status.OperatorRolesRef) {
294-
err = operatorroles.CreateOperatorRoles(runtime, ocm.Production, config.PermissionsBoundaryARN, interactive.ModeAuto, policies, version, isSharedVpc, config.Prefix, hostedCp, installerRoleArn, forcePolicyCreation,
295+
// not all operator roles are set, operator roles are not ready yet.
296+
r.clearOperatorRolesRef(&scope.RosaRoleConfig.Status.OperatorRolesRef)
297+
err = operatorroles.CreateOperatorRoles(runtime, ocm.Production, config.PermissionsBoundaryARN, interactive.ModeAuto, policies, version, config.SharedVPCConfig.IsSharedVPC(), config.Prefix, hostedCp, installerRoleArn, forcePolicyCreation,
295298
oidcConfigID, config.SharedVPCConfig.RouteRoleARN, ocm.DefaultChannelGroup, config.SharedVPCConfig.VPCEndpointRoleARN)
296299
return err
297300
}
@@ -301,33 +304,21 @@ func (r *ROSARoleConfigReconciler) createOperatorRoles(ctx context.Context, role
301304

302305
func (r *ROSARoleConfigReconciler) reconcileOIDCConfig(roleConfig *expinfrav1.ROSARoleConfig, scope *scope.RosaRoleConfigScope, ocmClient *ocm.Client) error {
303306
if scope.RosaRoleConfig.Status.OIDCID != "" {
307+
oidcConfig, err := ocmClient.GetOidcConfig(scope.RosaRoleConfig.Status.OIDCID)
308+
if err != nil || oidcConfig == nil {
309+
return fmt.Errorf("failed to get OIDC config: %w", err)
310+
}
304311
return nil
305312
}
306313
if roleConfig.Spec.OperatorRoleConfig.OIDCID != "" {
307314
scope.RosaRoleConfig.Status.OIDCID = roleConfig.Spec.OperatorRoleConfig.OIDCID
308315
return nil
309316
}
310-
// Try to get OIDC UUID from some operator role policy document.
311-
roleName := fmt.Sprintf("%s-openshift-ingress-operator-cloud-credentials", roleConfig.Spec.OperatorRoleConfig.Prefix)
312-
roleDetails, err := scope.IAMClient().GetRole(context.TODO(), &iamv2.GetRoleInput{
313-
RoleName: &roleName,
314-
})
315-
if err != nil {
316-
return r.createOIDCConfig(scope, ocmClient)
317-
}
318-
oidcID, err := r.GetOIDCIDFromOperatorRole(scope, roleDetails)
319-
if err != nil {
320-
return r.createOIDCConfig(scope, ocmClient)
321-
}
322-
scope.RosaRoleConfig.Status.OIDCID = oidcID
323-
return nil
317+
318+
return r.createOIDCConfig(scope, ocmClient)
324319
}
325320

326321
func (r *ROSARoleConfigReconciler) createOIDCProvider(scope *scope.RosaRoleConfigScope, ocmClient *ocm.Client) error {
327-
if scope.RosaRoleConfig.Status.OIDCProviderARN != "" {
328-
return nil
329-
}
330-
331322
var err error
332323
oidcID := scope.RosaRoleConfig.Status.OIDCID
333324
if oidcID == "" {
@@ -423,8 +414,7 @@ func (r *ROSARoleConfigReconciler) createAccountRoles(ctx context.Context, roleC
423414
}
424415

425416
managedPolicies := true
426-
isSharedVpc := config.SharedVPCConfig.VPCEndpointRoleARN != "" && config.SharedVPCConfig.RouteRoleARN != ""
427-
err := accountroles.CreateHCPRoles(runtime, config.Prefix, managedPolicies, config.PermissionsBoundaryARN, ocm.Production, policies, config.Version, config.Path, isSharedVpc, config.SharedVPCConfig.RouteRoleARN, config.SharedVPCConfig.VPCEndpointRoleARN)
417+
err := accountroles.CreateHCPRoles(runtime, config.Prefix, managedPolicies, config.PermissionsBoundaryARN, ocm.Production, policies, config.Version, config.Path, config.SharedVPCConfig.IsSharedVPC(), config.SharedVPCConfig.RouteRoleARN, config.SharedVPCConfig.VPCEndpointRoleARN)
428418
return err
429419
}
430420

@@ -471,23 +461,16 @@ func (r *ROSARoleConfigReconciler) deleteAccountRoles(ocmClient *ocm.Client, aws
471461
return err
472462
}
473463

474-
var err2, err3 error
475464
if canDeleteRole(clusters, roles.InstallerRoleARN) {
476-
err = awsClient.DeleteAccountRole(strings.Split(roles.InstallerRoleARN, "/")[1], config.Prefix, true, deleteHcpSharedVpcPolicies)
465+
err = errors.Join(err, awsClient.DeleteAccountRole(strings.Split(roles.InstallerRoleARN, "/")[1], config.Prefix, true, deleteHcpSharedVpcPolicies))
477466
}
478467
if canDeleteRole(clusters, roles.WorkerRoleARN) {
479-
err2 = awsClient.DeleteAccountRole(strings.Split(roles.WorkerRoleARN, "/")[1], config.Prefix, true, deleteHcpSharedVpcPolicies)
468+
err = errors.Join(err, awsClient.DeleteAccountRole(strings.Split(roles.WorkerRoleARN, "/")[1], config.Prefix, true, deleteHcpSharedVpcPolicies))
480469
}
481470
if canDeleteRole(clusters, roles.SupportRoleARN) {
482-
err3 = awsClient.DeleteAccountRole(strings.Split(roles.SupportRoleARN, "/")[1], config.Prefix, true, deleteHcpSharedVpcPolicies)
483-
}
484-
if err != nil {
485-
return err
486-
}
487-
if err2 != nil {
488-
return err2
471+
err = errors.Join(err, awsClient.DeleteAccountRole(strings.Split(roles.SupportRoleARN, "/")[1], config.Prefix, true, deleteHcpSharedVpcPolicies))
489472
}
490-
return err3
473+
return err
491474
}
492475

493476
func (r *ROSARoleConfigReconciler) deleteOIDCProvider(ocmClient *ocm.Client, awsClient aws.Client, oidcConfigID string) error {
@@ -661,3 +644,19 @@ func (r *ROSARoleConfigReconciler) GetOIDCIDFromOperatorRole(scope *scope.RosaRo
661644

662645
return "", fmt.Errorf("cant extract oidc uuid from the %s policy document", *roleDetails.Role.RoleName)
663646
}
647+
648+
// clearOperatorRolesRef clears all field values in the OperatorRolesRef by setting them to empty strings.
649+
func (r ROSARoleConfigReconciler) clearOperatorRolesRef(operatorRolesRef *v1beta2.AWSRolesRef) {
650+
if operatorRolesRef == nil {
651+
return
652+
}
653+
654+
operatorRolesRef.IngressARN = ""
655+
operatorRolesRef.ImageRegistryARN = ""
656+
operatorRolesRef.StorageARN = ""
657+
operatorRolesRef.NetworkARN = ""
658+
operatorRolesRef.KubeCloudControllerARN = ""
659+
operatorRolesRef.NodePoolManagementARN = ""
660+
operatorRolesRef.ControlPlaneOperatorARN = ""
661+
operatorRolesRef.KMSProviderARN = ""
662+
}

0 commit comments

Comments
 (0)