✨ Rosa Config implementaiton #5499

PanSpagetka · 2025-05-21T13:37:12Z

Based on proposal #5451
Adding RosaRoleConfig API with implementation. that should create Account/Operator roles and OIDC config/provider necessary to create ROSA cluster.

We need to move RosaMachinePoolAutoScaling definition to controlplane, because otherwise there would be 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:

k8s-ci-robot · 2025-05-21T13:37:15Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2025-05-21T13:37:18Z

[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 assign neolit123 for approval. 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

k8s-ci-robot · 2025-05-21T13:37:22Z

Hi @PanSpagetka. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

serngawy · 2025-06-17T19:36:21Z

Dockerfile

@@ -28,12 +28,17 @@ WORKDIR /workspace
 # Copy the Go Modules manifests
 COPY go.mod go.mod
 COPY go.sum go.sum
+COPY ./rosa /workspace/rosa


why are we adding this ? what is the rosa file

serngawy · 2025-06-17T19:36:38Z

Dockerfile

 # Cache deps before building and copying source so that we don't need to re-download as much
 # and so that source changes don't invalidate our downloaded layer
 RUN  --mount=type=cache,target=/root/.local/share/golang \
     --mount=type=cache,target=/go/pkg/mod \
     go mod download

+# RUN go mod download


no need for this line

serngawy · 2025-06-17T19:37:35Z

PROJECT

can you just add the RosaRoleConfig item without changing the order of other items

serngawy · 2025-06-17T19:38:28Z

config/default/tmp.yaml

why do we need this file ?

serngawy · 2025-06-17T19:38:54Z

config/crd/kustomization.yaml

@@ -38,6 +39,7 @@ patchesStrategicMerge:
 - patches/webhook_in_awsmanagedcontrolplanes.yaml
 - patches/webhook_in_eksconfigs.yaml
 - patches/webhook_in_eksconfigtemplates.yaml
+#- patches/webhook_in_rosaroleconfigs.yaml


I believe you need to uncomment this line

serngawy · 2025-06-17T19:46:49Z

exp/controllers/rosaroleconfig_controller.go

+		}
+	}
+
+	if scope.RosaRoleConfig.Status.OIDCID == "" {


I believe you should check/set the RosaRoleConfig condition first , then get the oidc using OCM client if it is not exist then create it.
Same applied for account-roles and operator-roles

serngawy · 2025-06-17T19:47:52Z

exp/controllers/rosaroleconfig_controller.go

+		}
+	}
+
+	err = r.deleteOperatorRoles(ocmClient, awsClient, scope.RosaRoleConfig.Spec.AccountRoleConfig.Prefix)


Do we need to delete the operator roles before the oidc-provider ?

Nope, a changed it so it matches reverse creation order.

I guess the order here is not correct, deleting the operatrRoles first then oidc-provider then odic-config ?

serngawy · 2025-06-17T19:49:45Z

exp/controllers/rosaroleconfig_controller.go

+	return ocmClient.DeleteOidcConfig(oidcConfigID)
+}
+
+type reporter struct {


please move this to another file , better to be under pkg/.../rosa

PanSpagetka · 2025-08-05T12:30:24Z

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

PanSpagetka · 2025-08-05T14:16:00Z

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

serngawy · 2025-08-06T18:52:07Z

exp/api/v1beta2/rosaroleconfig_types.go

+	// +optional
+	// +immutable
+	SharedVPCConfig SharedVPCConfig `json:"sharedVPCConfig,omitempty"`
+	// OIDCID is the ID of the OIDC config that will be used to create the operator roles.


Suggested change

// OIDCID is the ID of the OIDC config that will be used to create the operator roles.

// OIDCID is the ID of the OIDC config that will be used to create the operator roles. A managed OIDC-provider will be created if the OIDCID not specified

serngawy · 2025-08-06T18:52:38Z

exp/api/v1beta2/rosaroleconfig_types.go

+	OperatorRoleConfig OperatorRoleConfig            `json:"operatorRoleConfig"`
+	OIDCConfig         OIDCConfig                    `json:"oidcConfig"`
+	IdentityRef        *infrav1.AWSIdentityReference `json:"identityRef,omitempty"`
+	Region             string                        `json:"region,omitempty"`


as we discussed no need for region

serngawy · 2025-08-06T19:00:36Z

exp/controllers/rosaroleconfig_controller.go

+	}
+
+	oidcID := scope.RosaRoleConfig.Status.OIDCID
+	err = r.deleteOIDCProvider(ocmClient, awsClient, oidcID)


We don't delete the oidc-provider if the user set it under the spec.operatorRole.OIDCConfig

PanSpagetka · 2025-08-07T14:35:54Z

/retest-required

PanSpagetka · 2025-08-08T05:48:40Z

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

PanSpagetka · 2025-08-08T06:34:08Z

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

PanSpagetka · 2025-08-08T07:37:20Z

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

serngawy · 2025-08-11T13:56:48Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

+	// If specified, the roles and OIDC configuration will be taken from the referenced RosaRoleConfig instead of the direct fields.
+	//
+	// +optional
+	RosaRoleConfigRef *corev1.LocalObjectReference `json:"rosaRoleConfigRef,omitempty"`


The OIDC is immutable , so validation should be applied (could be in the webhook update ) that RosaRoleConfigRef cannot be change if the cluster is provisioned

serngawy · 2025-08-11T14:10:59Z

exp/controllers/rosaroleconfig_controller.go

+		}
+	}
+
+	err = r.deleteOperatorRoles(ocmClient, awsClient, scope.RosaRoleConfig.Spec.AccountRoleConfig.Prefix)


I guess the order here is not correct, deleting the operatrRoles first then oidc-provider then odic-config ?

serngawy · 2025-08-11T14:25:00Z

exp/controllers/rosaroleconfig_controller.go

+	if len(operatorRoles) > 0 {
+		for _, roles := range operatorRoles {
+			for _, role := range roles {
+				if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-ingress-operator-cloud-credentials", config.Prefix)) {


why strings.contain not exact string equal by name ?

serngawy · 2025-08-11T14:30:10Z

exp/controllers/rosaroleconfig_controller.go

+				}
+			}
+		}
+	} else {


The logic here is not correct , what if one Role is missing not all of them ? we create 8 operator Roles, we should check every single one

No, if there is one role missing we don't create 8 roles because you can't have 2 roles with same name. And in rosa cli we handle roles creation as a batch operation, so we are consistent with that.

that is not correct , rosa lib will re-create the missing operator roles if there are any. You can reproduce this scenario with rosa-cli

serngawy · 2025-08-12T11:01:44Z

exp/controllers/rosaroleconfig_controller.go

+				if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-ingress-operator-cloud-credentials", config.Prefix)) {
+					scope.RosaRoleConfig.Status.OperatorRolesRef.IngressARN = role.RoleARN
+				}
+				if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-image-registry-installer-cloud-credentials", config.Prefix)) {


Suggested change

if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-image-registry-installer-cloud-credentials", config.Prefix)) {

else if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-image-registry-installer-cloud-credentials", config.Prefix)) {

better to set else if with the following if(s) check

better to have the RoleNames in list/map so easier for you to check the roles count and existence

No having roles names in list doesn't help, every role has its own field so every condition needs to be spelled out.

PanSpagetka · 2025-08-14T06:45:09Z

/retest-required

PanSpagetka · 2025-08-14T07:31:49Z

/retest-required

serngawy

Thanks @PanSpagetka, sounds good in general we can add more test in future PRs

serngawy · 2025-08-14T17:43:03Z

Please add release note , mention adding Rosa RoleConfig API

k8s-ci-robot · 2025-08-15T11:54:14Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

nrb

Some questions and style suggestions.

nrb · 2025-08-15T21:29:24Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

@@ -714,8 +737,8 @@ type AWSRolesRef struct {
 	//	]
 	// }
 	// +immutable
-	ControlPlaneOperatorARN string `json:"controlPlaneOperatorARN"`
-	KMSProviderARN          string `json:"kmsProviderARN"`
+	ControlPlaneOperatorARN string `json:"controlPlaneOperatorARN,omitempty"`


Why are these fields being updated with omitempty?

nrb · 2025-08-15T21:35:21Z

controlplane/rosa/api/v1beta2/rosacontrolplane_webhook.go

@@ -179,6 +183,29 @@ func (r *ROSAControlPlane) validateExternalAuthProviders() *field.Error {
 	return nil
 }

+func (r *ROSAControlPlane) validateRosaRoleConfig() *field.Error {
+	hasRosaRoleConfigRef := r.Spec.RosaRoleConfigRef != nil
+	hasAnyDirectRoleFields := r.Spec.OIDCID != "" || r.Spec.InstallerRoleARN != "" || r.Spec.SupportRoleARN != "" || r.Spec.WorkerRoleARN != "" ||


True, but you don't know which field is missing without checking all of them.

I can see why this information is being moved into a RoleConfig, because checking all these fields individually is tricky.

nrb · 2025-08-15T21:36:40Z

controlplane/rosa/api/v1beta2/rosacontrolplane_webhook.go

@@ -179,6 +186,58 @@ func (r *ROSAControlPlane) validateExternalAuthProviders() *field.Error {
 	return nil
 }

+func (r *ROSAControlPlane) validateRosaRoleConfig() *field.Error {
+	hasAnyDirectRoleFields := r.Spec.OIDCID != "" || r.Spec.InstallerRoleARN != "" || r.Spec.SupportRoleARN != "" || r.Spec.WorkerRoleARN != "" ||
+		r.Spec.RolesRef.IngressARN != "" || r.Spec.RolesRef.ImageRegistryARN != "" || r.Spec.RolesRef.StorageARN != "" ||


For the sake of clarity, perhaps we could use a r.Spec.RolesRef == nil check to demonstrate that there is no RolesRef at all, without checking every field within it.

We can look at the specific RolesRef fields themselves further down in the function, since up here we're just looking for a high level conflict.

nrb · 2025-08-15T21:44:42Z

controlplane/rosa/api/v1beta2/rosacontrolplane_webhook.go

+	if r.Spec.WorkerRoleARN == "" {
+		return field.Invalid(field.NewPath("spec.workerRoleARN"), r.Spec.WorkerRoleARN, "must be specified")
+	}
+	if r.Spec.RolesRef.IngressARN == "" {


Probably also want to make sure that r.Spec.RolesRef isn't nil at some point, possibly up above where my previous comment is.

Another suggestion would be to say if you reach this point in the code, you know all the previous fields aren't "", because the function would have returned.

Knowing that, we could say that at this point if r.Spec.RolesRef is not nil, then the user has specified all the direct fields and a RolesRef, triggering the mutual exclusion.

In terms of clarity, I think it would also be worth having a couple helper functions for checking the direct fields and the RolesRef fields; it keeps the conditionals grouped into smaller chunks.

nrb · 2025-08-15T21:46:49Z

exp/api/v1beta2/rosaroleconfig_types.go

+// EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
+// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.


Suggested change

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!

// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

nrb · 2025-08-15T22:25:47Z

pkg/cloud/scope/rosaroleconfig.go

+	"context"
+
+	awsv2 "github.com/aws/aws-sdk-go-v2/aws"
+	iamv2 "github.com/aws/aws-sdk-go-v2/service/iam"


Could import these directly without an alias now that there's no reference to the older SDK.

nrb · 2025-08-15T22:26:51Z

pkg/cloud/scope/rosaroleconfig.go

+// PatchObject persists the RosaRoleConfig configuration and status.
+func (s *RosaRoleConfigScope) PatchObject() error {
+	return s.patchHelper.Patch(
+		context.TODO(),


Suggested change

context.TODO(),

context.Background(),

nrb · 2025-08-15T22:27:06Z

pkg/cloud/scope/rosaroleconfig.go

+}
+
+// Close closes the current scope persisting the RosaRoleConfig configuration and status.
+func (s *RosaRoleConfigScope) Close() error {


Why have this wrapper function?

nrb · 2025-08-15T22:29:13Z

pkg/rosa/helpers.go

+}
+
+// Debugf prints a debug message with the given format and arguments.
+func (r *Reporter) Debugf(format string, args ...interface{}) {


These functions just swallow the input? Are you trying to hide the output?

nrb · 2025-08-15T22:29:40Z

go.mod

@@ -1,6 +1,6 @@
 module sigs.k8s.io/cluster-api-provider-aws/v2

-go 1.23.0
+go 1.23.1


Leave this at 1.23.0.

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 21, 2025

k8s-ci-robot requested review from Ankitasw and serngawy May 21, 2025 13:37

k8s-ci-robot added needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2025

PanSpagetka marked this pull request as draft May 21, 2025 13:37

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2025

PanSpagetka force-pushed the rosa-roles-implementations branch from 11dac0b to 6056618 Compare May 28, 2025 13:43

serngawy mentioned this pull request May 29, 2025

✨ Add RosaRoles config #5473

Closed

5 tasks

PanSpagetka force-pushed the rosa-roles-implementations branch 6 times, most recently from 1587db4 to 9121ec2 Compare June 9, 2025 13:40

PanSpagetka force-pushed the rosa-roles-implementations branch 3 times, most recently from 07b73a3 to 097252f Compare June 16, 2025 11:47

serngawy suggested changes Jun 17, 2025

View reviewed changes

PanSpagetka force-pushed the rosa-roles-implementations branch from 097252f to 0c9fa93 Compare June 18, 2025 11:40

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

PanSpagetka force-pushed the rosa-roles-implementations branch 3 times, most recently from b3aded3 to 23fa4cb Compare June 24, 2025 11:29

PanSpagetka force-pushed the rosa-roles-implementations branch from 23fa4cb to 2515be2 Compare July 2, 2025 08:09

PanSpagetka force-pushed the rosa-roles-implementations branch from fedd967 to a428b51 Compare August 5, 2025 11:12

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

PanSpagetka force-pushed the rosa-roles-implementations branch from a428b51 to c2e82ca Compare August 5, 2025 11:31

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

PanSpagetka force-pushed the rosa-roles-implementations branch 3 times, most recently from 4761247 to fe144c5 Compare August 5, 2025 12:13

serngawy suggested changes Aug 6, 2025

View reviewed changes

PanSpagetka force-pushed the rosa-roles-implementations branch from fe144c5 to 60be2ee Compare August 7, 2025 12:02

PanSpagetka force-pushed the rosa-roles-implementations branch from 60be2ee to bc8e7af Compare August 8, 2025 06:44

serngawy suggested changes Aug 11, 2025

View reviewed changes

serngawy suggested changes Aug 12, 2025

View reviewed changes

PanSpagetka force-pushed the rosa-roles-implementations branch from 0cbe365 to b3e19fc Compare August 14, 2025 07:32

PanSpagetka added 3 commits August 14, 2025 12:53

Add RosaRoleConfig API and CRD.

e03734b

Enable partial reconcile of Rosa Operator Roles

8060b46

Use session v2 API

54f21ce

PanSpagetka force-pushed the rosa-roles-implementations branch from b3e19fc to 54f21ce Compare August 14, 2025 11:05

serngawy approved these changes Aug 14, 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 Aug 15, 2025

nrb requested changes Aug 15, 2025

View reviewed changes

	// OIDCID is the ID of the OIDC config that will be used to create the operator roles.
	// OIDCID is the ID of the OIDC config that will be used to create the operator roles. A managed OIDC-provider will be created if the OIDCID not specified

	if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-image-registry-installer-cloud-credentials", config.Prefix)) {
	else if strings.Contains(role.RoleName, fmt.Sprintf("%s-openshift-image-registry-installer-cloud-credentials", config.Prefix)) {

		// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
		// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

✨ Rosa Config implementaiton #5499

Are you sure you want to change the base?

✨ Rosa Config implementaiton #5499

Conversation

PanSpagetka commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 21, 2025

Uh oh!

k8s-ci-robot commented May 21, 2025

Uh oh!

k8s-ci-robot commented May 21, 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!

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

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!

PanSpagetka commented Aug 5, 2025

Uh oh!

PanSpagetka commented Aug 5, 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!

PanSpagetka commented Aug 7, 2025

Uh oh!

PanSpagetka commented Aug 8, 2025

Uh oh!

PanSpagetka commented Aug 8, 2025

Uh oh!

PanSpagetka commented Aug 8, 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!

PanSpagetka commented Aug 14, 2025

Uh oh!

PanSpagetka commented Aug 14, 2025

Uh oh!

serngawy left a comment

Choose a reason for hiding this comment

Uh oh!

serngawy commented Aug 14, 2025

Uh oh!

PanSpagetka commented May 21, 2025 •

edited

Loading

serngawy Jun 17, 2025 •

edited

Loading