✨ Rosa roles config implementations #5667

serngawy · 2025-09-15T19:20:26Z

This PR is based on PR 5499 fixing all the comments

Based on proposal #5451
Adding RosaRoleConfig API with implementation. that should create account roles, operator roles, OIDC config and OIDC provider necessary to create ROSA HCP cluster.

Moving RosaMachinePoolAutoScaling definition to ROSAControlPlane to avoid circular dependency.

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

Release note:

Adding Rosa Role Config API and implementation

PanSpagetka

Looks good

serngawy · 2025-09-18T18:44:13Z

/test pull-cluster-api-provider-aws-test

damdo · 2025-09-22T08:11:40Z

/assign @damdo @nrb @richardcase

damdo · 2025-09-23T14:56:11Z

controlplane/rosa/controllers/rosacontrolplane_controller.go

+	rosaRoleConfig := &expinfrav1.ROSARoleConfig{}
+	// Get role configuration from either RosaRoleConfig or direct fields
+	if rosaScope.ControlPlane.Spec.RosaRoleConfigRef != nil {
+		// Get configuration from RosaRoleConfig
+
+		key := client.ObjectKey{
+			Name:      rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name,
+			Namespace: rosaScope.ControlPlane.Namespace,
+		}
+
+		if err := r.Client.Get(ctx, key, rosaRoleConfig); err != nil {
+			if apierrors.IsNotFound(err) {
+				conditions.MarkFalse(rosaScope.ControlPlane,
+					rosacontrolplanev1.ROSARoleConfigReadyCondition,
+					rosacontrolplanev1.ROSARoleConfigNotFoundReason,
+					clusterv1.ConditionSeverityError,
+					"RosaRoleConfig %s/%s not found", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name)
+				rosaScope.Error(err, fmt.Sprintf("RosaRoleConfig %s/%s not found: %s", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name, err.Error()))
+				return ctrl.Result{RequeueAfter: time.Second * 60}, nil
+			}
+			rosaScope.Error(err, fmt.Sprintf("failed to get RosaRoleConfig %s/%s: %s", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name, err.Error()))
+			return ctrl.Result{RequeueAfter: time.Second * 60}, nil
+		}
+
+		// Check if RosaRoleConfig is ready
+		if !conditions.IsTrue(rosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition) {
+			conditions.MarkFalse(rosaScope.ControlPlane,
+				rosacontrolplanev1.ROSARoleConfigReadyCondition,
+				rosacontrolplanev1.ROSARoleConfigNotReadyReason,
+				clusterv1.ConditionSeverityWarning,
+				"RosaRoleConfig %s/%s is not ready", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name)
+			rosaScope.Error(err, fmt.Sprintf("RosaRoleConfig %s/%s is not ready", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name))
+
+			return ctrl.Result{RequeueAfter: time.Second * 60}, nil
+		}
+
+		conditions.MarkTrue(rosaScope.ControlPlane, rosacontrolplanev1.ROSARoleConfigReadyCondition)
+	} else {
+		rosaRoleConfig.Status.OIDCID = rosaScope.ControlPlane.Spec.OIDCID
+		rosaRoleConfig.Status.AccountRolesRef.InstallerRoleARN = rosaScope.ControlPlane.Spec.InstallerRoleARN
+		rosaRoleConfig.Status.AccountRolesRef.SupportRoleARN = rosaScope.ControlPlane.Spec.SupportRoleARN
+		rosaRoleConfig.Status.AccountRolesRef.WorkerRoleARN = rosaScope.ControlPlane.Spec.WorkerRoleARN
+		rosaRoleConfig.Status.OperatorRolesRef = rosaScope.ControlPlane.Spec.RolesRef
+	}
+


I think we could maybe extract this into a specific reconcileRosaRoleConfig function.

damdo · 2025-09-23T14:57:24Z

controlplane/rosa/controllers/rosacontrolplane_controller.go

+					clusterv1.ConditionSeverityError,
+					"RosaRoleConfig %s/%s not found", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name)
+				rosaScope.Error(err, fmt.Sprintf("RosaRoleConfig %s/%s not found: %s", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name, err.Error()))
+				return ctrl.Result{RequeueAfter: time.Second * 60}, nil


Why do we have these RequeueAfter 60s functions all over the place?
Wouldn't erroring normally and retry soon after be ok?

@serngawy These ones are still here I see, any thoughts?

damdo · 2025-09-23T14:58:17Z

exp/api/v1beta2/rosaroleconfig_types.go

+	// UnManaged OIDC Provider type
+	UnManaged OidcProviderType = "UnManaged"


I think Unmanaged might be better.

damdo · 2025-09-23T15:04:08Z

templates/cluster-template-rosa-role-config.yaml

+  credentialsSecretRef:
+    name: rosa-creds-secret
+  rosaRoleConfigRef:
+    name: "${CLUSTER_NAME}-role-config"


There is a missing EndOfFile here

damdo · 2025-09-23T15:05:36Z

exp/controllers/rosaroleconfig_controller.go

+	err = r.setUpRuntime(ctx, scope)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to set up runtime: %w", err)
+	}


I think it would be cleaner to have all these invocations that only return errors as inlined err checks

Suggested change

err = r.setUpRuntime(ctx, scope)

if err != nil {

return ctrl.Result{}, fmt.Errorf("failed to set up runtime: %w", err)

}

if err := r.setUpRuntime(ctx, scope); err != nil {

return ctrl.Result{}, fmt.Errorf("failed to set up runtime: %w", err)

}

k8s-ci-robot · 2025-09-23T21:15:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from nrb. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

damdo · 2025-09-25T07:43:20Z

/label tide/merge-method-squash

damdo

Thanks for addressing some of my comments, still some things left to be addressed but we are looking good! TY

damdo · 2025-09-25T07:45:13Z

exp/controllers/rosaroleconfig_controller.go

+	err = r.reconcileAccountRoles(scope)
+	if err != nil {
+		conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Account Roles failure: %v", err)
+		return ctrl.Result{}, fmt.Errorf("account Roles: %w", err)
+	}


Suggested change

err = r.reconcileAccountRoles(scope)

if err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Account Roles failure: %v", err)

return ctrl.Result{}, fmt.Errorf("account Roles: %w", err)

}

if err := r.reconcileAccountRoles(scope); err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Account Roles failure: %v", err)

return ctrl.Result{}, fmt.Errorf("account Roles: %w", err)

}

damdo · 2025-09-25T07:45:59Z

exp/controllers/rosaroleconfig_controller.go

+	err = r.reconcileOIDC(scope)
+	if err != nil {
+		conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "OIDC Config/provider failure: %v", err)
+		return ctrl.Result{}, fmt.Errorf("oicd Config: %w", err)
+	}


Suggested change

err = r.reconcileOIDC(scope)

if err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "OIDC Config/provider failure: %v", err)

return ctrl.Result{}, fmt.Errorf("oicd Config: %w", err)

}

if err := r.reconcileOIDC(scope); err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "OIDC Config/provider failure: %v", err)

return ctrl.Result{}, fmt.Errorf("oicd Config: %w", err)

}

damdo · 2025-09-25T07:46:15Z

exp/controllers/rosaroleconfig_controller.go

+	err = r.reconcileOperatorRoles(scope)
+	if err != nil {
+		conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Operator Roles failure: %v", err)
+		return ctrl.Result{}, fmt.Errorf("operator Roles: %w", err)
+	}


Suggested change

err = r.reconcileOperatorRoles(scope)

if err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Operator Roles failure: %v", err)

return ctrl.Result{}, fmt.Errorf("operator Roles: %w", err)

}

if err := r.reconcileOperatorRoles(scope); err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Operator Roles failure: %v", err)

return ctrl.Result{}, fmt.Errorf("operator Roles: %w", err)

}

damdo · 2025-09-25T07:46:32Z

exp/controllers/rosaroleconfig_controller.go

+	err := r.deleteOperatorRoles(scope)
+	if err != nil {
+		conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigDeletionFailedReason, clusterv1.ConditionSeverityError, "Failed to delete operator roles: %v", err)
+		return err
+	}


Suggested change

err := r.deleteOperatorRoles(scope)

if err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigDeletionFailedReason, clusterv1.ConditionSeverityError, "Failed to delete operator roles: %v", err)

return err

}

if err := r.deleteOperatorRoles(scope); err != nil {

conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigDeletionFailedReason, clusterv1.ConditionSeverityError, "Failed to delete operator roles: %v", err)

return err

}

damdo · 2025-09-25T07:46:40Z

exp/controllers/rosaroleconfig_controller.go

+	err = r.deleteOIDC(scope)
+	if err != nil {
+		conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigDeletionFailedReason, clusterv1.ConditionSeverityError, "Failed to delete OIDC provider: %v", err)
+		return err
+	}
+
+	err = r.deleteAccountRoles(scope)
+	if err != nil {
+		conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigDeletionFailedReason, clusterv1.ConditionSeverityError, "Failed to delete account roles: %v", err)
+		return err
+	}


Also let's add inline nil check for these

damdo · 2025-09-25T07:47:25Z

exp/controllers/rosaroleconfig_controller.go

+	if err = r.setUpRuntime(ctx, scope); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to set up runtime: %w", err)
+	}


Suggested change

if err = r.setUpRuntime(ctx, scope); err != nil {

return ctrl.Result{}, fmt.Errorf("failed to set up runtime: %w", err)

}

if err := r.setUpRuntime(ctx, scope); err != nil {

return ctrl.Result{}, fmt.Errorf("failed to set up runtime: %w", err)

}

damdo · 2025-09-25T07:48:20Z

controlplane/rosa/controllers/rosacontrolplane_controller.go

+					clusterv1.ConditionSeverityError,
+					"RosaRoleConfig %s/%s not found", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name)
+				rosaScope.Error(err, fmt.Sprintf("RosaRoleConfig %s/%s not found: %s", rosaScope.ControlPlane.Namespace, rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name, err.Error()))
+				return ctrl.Result{RequeueAfter: time.Second * 60}, nil


@serngawy These ones are still here I see, any thoughts?

Signed-off-by: serngawy <[email protected]>

serngawy · 2025-09-25T18:19:15Z

Thanks @damdo , fixed all the err inline nil check AND removed the RequeueAfter 60 (just forget remove it with others)

damdo

Thanks for addressing my comments

damdo

/lgtm

Let's see what others think

k8s-ci-robot · 2025-09-25T18:23:54Z

LGTM label has been added.

Git tree hash: d89f75256d7de8148a3dccb2b6a17b1083baef51

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 15, 2025

k8s-ci-robot requested review from Ankitasw and dlipovetsky September 15, 2025 19:20

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 15, 2025

serngawy mentioned this pull request Sep 15, 2025

✨ Rosa Config implementaiton #5499

Closed

4 tasks

serngawy force-pushed the rosa-roles-implementations branch 3 times, most recently from d9ab817 to ae8dbe6 Compare September 16, 2025 12:59

PanSpagetka approved these changes Sep 16, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2025

serngawy force-pushed the rosa-roles-implementations branch from ae8dbe6 to 1fa491b Compare September 18, 2025 18:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2025

serngawy force-pushed the rosa-roles-implementations branch 2 times, most recently from bf7d5c6 to 4ae9bc3 Compare September 18, 2025 18:17

k8s-ci-robot assigned damdo, nrb and richardcase Sep 22, 2025

damdo reviewed Sep 23, 2025

View reviewed changes

serngawy force-pushed the rosa-roles-implementations branch from 4ae9bc3 to 63d8809 Compare September 23, 2025 21:15

serngawy force-pushed the rosa-roles-implementations branch from 63d8809 to 658a1d2 Compare September 23, 2025 21:25

PanSpagetka added 4 commits September 23, 2025 17:25

Add RosaRoleConfig API and CRD.

11f26ed

Enable partial reconcile of Rosa Operator Roles

d187bc0

Review fixes

38d8117

Add integration tests

8b720d1

Add more tests

34546fb

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 25, 2025

damdo reviewed Sep 25, 2025

View reviewed changes

serngawy force-pushed the rosa-roles-implementations branch from 658a1d2 to c1c0047 Compare September 25, 2025 18:14

Fix comments

c1c0047

Signed-off-by: serngawy <[email protected]>

damdo reviewed Sep 25, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2025

		// UnManaged OIDC Provider type
		UnManaged OidcProviderType = "UnManaged"

✨ Rosa roles config implementations #5667

Are you sure you want to change the base?

✨ Rosa roles config implementations #5667

Conversation

serngawy commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PanSpagetka left a comment

Choose a reason for hiding this comment

Uh oh!

serngawy commented Sep 18, 2025

Uh oh!

damdo commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 23, 2025

Uh oh!

damdo commented Sep 25, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serngawy commented Sep 25, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 25, 2025

Uh oh!

Uh oh!

serngawy commented Sep 15, 2025 •

edited

Loading