✨ ROSA: Support for OCM service account credentials #5233

mzazrivec · 2024-11-29T20:07:25Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This pull request implements support for OCM (aka api.openshift.com) authentication using service account credentials.

The previous authentication mechanism (known as offline tokens) will be deprecated in few months, therefore we need to implement the alternative way. The offline token autentication is still being supported with my changes, though a deprecation
warning will be printed in the CAPA manager logs.

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:

feat: enable support for service account authentication in ROSA installations

linux-foundation-easycla · 2024-11-29T20:07:32Z

The committers listed above are authorized under a signed CLA.

✅ login: mzazrivec / name: Milan Zázrivec (c504cee)

k8s-ci-robot · 2024-11-29T20:07:34Z

Welcome @mzazrivec!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-29T20:07:35Z

Hi @mzazrivec. 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.

k8s-ci-robot · 2024-12-05T19:03:43Z

[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 stevekuznetsov for approval. For more information see the Kubernetes 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:

docs/book/src/topics/rosa/OWNERS
pkg/rosa/OWNERS

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

serngawy

I believe its better to add a section in doc (OR another md file) show old endUsers how to migrate their current ROSA-HCP clusters from using offline token to service-account. If there is any risk of doing this should be mentioned as well in the doc.

serngawy · 2024-12-06T14:09:14Z

pkg/rosa/client.go

-		ocmAPIUrl = string(secret.Data[ocmAPIURLKey])
-	} else {
-		// fallback to env variables if secrert is not set
-		token = os.Getenv("OCM_TOKEN")


The endUser still should be able to use the os.Getenv to set the token and url for at least 1 releases. The order of getting the cred is 1- ROSAScop-secret 2- rosa-creds-secret 3-os.Getenv
if the endUser still use the os.Env raise warning logs mentioning that OCM_TOKEN & OCM_API_URL will not be used for next release.

serngawy · 2024-12-06T15:05:50Z

docs/book/src/topics/rosa/creating-a-cluster.md

    ```shell
    kubectl create secret generic rosa-creds-secret \
-      --from-literal=ocmToken='eyJhbGciOiJIUzI1NiIsI....' \


ocmToken still valid to be used, keep it in the doc with deprecation note so old users be aware of the migration to service-account.

serngawy · 2024-12-06T15:06:31Z

docs/book/src/topics/rosa/creating-a-cluster.md

-    ```
-
-    and add the following environment variables to the manager container:
-    ```yaml


Same as above, add deprecation Note

serngawy

few comments and please add unit test

serngawy · 2024-12-09T16:46:01Z

pkg/rosa/client.go

+		}
+	}
+
+	if err := rosaScope.Client.Get(ctx, client.ObjectKeyFromObject(secret), secret); err != nil {


This may raise error the secret not found which is okay if the user still using the os.env to set the OCM_TOKEN

serngawy · 2024-12-09T16:48:05Z

pkg/rosa/client.go

+	// Deprecation warning in case SSO offline token was used
+	if token != "" {
+		rosaScope.Info(fmt.Sprintf("Using SSO offline token (%s) is deprecated, use service account credentials instead (%s and %s)",
+			ocmTokenKey, ocmClientIdKey, ocmClientSecretKey))


no need to set those key values , better to add the doc url

serngawy · 2024-12-09T16:48:50Z

pkg/rosa/client.go

+
+		if token != "" {
+			rosaScope.Info(fmt.Sprintf("Defining OCM credentials in environment variable is deprecated, use secret with service account credentials instead (%s and %s)",
+				ocmTokenKey, ocmClientIdKey, ocmClientSecretKey))


no need to set those key values , better to add the doc url

serngawy · 2024-12-09T16:51:30Z

pkg/rosa/client.go

+	ocmClientId = string(secret.Data[ocmClientIdKey])
+	ocmClientSecret = string(secret.Data[ocmClientSecretKey])


no need to define those clientId & clientSecret if the secret is not used

serngawy · 2024-12-09T16:53:28Z

pkg/rosa/client.go

+	}
+
+	if token == "" && (ocmClientId == "" || ocmClientSecret == "") {
+		// Last fall-back is to use OCM_TOKEN & OCM_API_URL environment variables (soon to be deprecated)


please add TODO: note to remove this part of the code next release

serngawy · 2024-12-09T16:59:13Z

docs/book/src/topics/rosa/creating-a-cluster.md

-    Alternatively, you can edit CAPA controller deployment to provide the credentials:
-    ```shell
-    kubectl edit deployment -n capa-system capa-controller-manager
+    Note: to consume the secret without the need to reference it from your `ROSAControlPlane`, name your secret as `default-rosa-creds-secret`.


please mention it must be in the same namespace as capa-controller-manager deployment and better secret naming is rosa-creds-secret

serngawy · 2024-12-09T17:00:52Z

docs/book/src/topics/rosa/creating-a-cluster.md

      - name: OCM_TOKEN
        value: "<token>"
      - name: OCM_API_URL
        value: "https://api.openshift.com" # or https://api.stage.openshift.com
-    ```
+```


please add section , for old user how to migrate from using the offline token to service account.

serngawy · 2024-12-09T17:01:12Z

pkg/rosa/client.go

+	} else { // If the reference to OCM secret wasn't specified in the ROSA control plane, we'll default to a predefined secret name
+		secret = &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "default-rosa-creds-secret",


better name rosa-creds-secret

serngawy · 2024-12-13T21:36:48Z

@faiq @nrb would you add ok-to-test

nrb · 2024-12-16T20:49:23Z

/ok-to-test

serngawy

few comments and please run make lint-fix before you push your code to fix all golang-ci errors

serngawy · 2024-12-16T21:13:27Z

docs/book/src/topics/rosa/creating-a-cluster.md

    ```

-    Alternatively, you can edit CAPA controller deployment to provide the credentials:
+    Note: to consume the secret without the need to reference it from your `ROSAControlPlane`, name your secret as `rosa-creds-secret` and create it in the CAPA manager namespace (usually `capa-system`):


remove ':' (usually capa-system).

serngawy · 2024-12-16T21:19:50Z

docs/book/src/topics/rosa/creating-a-cluster.md

-      env:
+
+### Authentication using SSO offline token (DEPRECATED)
+Instead of the service account credentials you can use SSO offline token, that you can specify either as a secret or specify the offline token


Suggested change

Instead of the service account credentials you can use SSO offline token, that you can specify either as a secret or specify the offline token

The SSO offline token is been deprecated, it is recommended to use the service account as mentioned above. However, for old users you still can visit https://console.redhat.com/openshift/token to retrieve your SSO offline authentication token.

I restructured the text a bit, but I'd rather not call any CAPI users "old". The instructions to go to consoleDot to retrieve the token are part of the text below.

serngawy · 2024-12-16T21:32:51Z

pkg/rosa/client_test.go

+	. "github.com/onsi/gomega"
+)
+
+func createROSAControlPlaneScope(wlSecret, mgrSecret *corev1.Secret, cp *rosacontrolplanev1.ROSAControlPlane) *scope.ROSAControlPlaneScope {


better to create another function named createFakeClientWithObjects and give it the obj (secrets) as param return a fakeClient then use it to createROSAControlPlaneScope

Nah, that would be just one more function created just for tests. Maybe in the future, if we'll need to call it separately, then we can extract it, but I'd rather leave it as it is right now.

I simplified the createROSAControlPlaneScope() function and made it a varadic fn. That way the function is much shorter.

serngawy · 2024-12-16T21:45:53Z

pkg/rosa/client_test.go

+
+	// Test that ocmCredentials() returns error in case none of the secrets has been provided
+	cp.Spec = rosacontrolplanev1.RosaControlPlaneSpec{}
+	rcpScope = createROSAControlPlaneScope(nil, nil, cp)


why do you create a dummy secrets in createROSAControlPlaneScope that you don't need in the test

Yea, I rewrote createROSAControlPlaneScope() as varadic to make this more simple.

serngawy

Almost there, few comments

serngawy · 2024-12-17T23:51:29Z

pkg/rosa/client_test.go

+	cp := createCP("default")
+
+	// Test that ocmCredentials() prefers workload secret to global and environment secrets
+	cp.Spec = rosacontrolplanev1.RosaControlPlaneSpec{


No need for this, the CredentialsSecretRef is already set in the createCP function

serngawy · 2024-12-17T23:55:47Z

pkg/rosa/client_test.go

+
+	// Test that ocmCredentials() prefers global manager secret to environment secret in case workload secret is not specified
+	cp.Spec = rosacontrolplanev1.RosaControlPlaneSpec{}
+	rcpScope = createROSAControlPlaneScope(cp, wlSecret, mgrSecret)


No need to create the wlSecret, no use for it as RosaControlPlaneSpec has no ref for it

serngawy · 2024-12-17T23:59:47Z

pkg/rosa/client_test.go

+	g.Expect(clientSecret).To(Equal(""))
+
+	// Test that ocmCredentials() returns error in case none of the secrets has been provided
+	cp.Spec = rosacontrolplanev1.RosaControlPlaneSpec{}


No need for lines 115 & 116 already same as 104 & 105

serngawy · 2024-12-18T00:00:26Z

docs/book/src/topics/rosa/creating-a-cluster.md

@@ -1,30 +1,83 @@
 # Creating a ROSA cluster

 ## Permissions
-CAPA controller requires an API token in order to be able to provision ROSA clusters:
+### Authentication using service account credentials


ROSA-HCP not ROSA

I'm assuming you meant to write that comment one line below.

Not saying I cannot change it, but ...

$ git checkout main $ git grep ROSA docs/book/src/topics/rosa/creating-a-cluster.md docs/book/src/topics/rosa/creating-a-cluster.md:# Creating a ROSA cluster docs/book/src/topics/rosa/creating-a-cluster.md:CAPA controller requires service account credentials to be able to provision ROSA clusters: docs/book/src/topics/rosa/creating-a-cluster.md:1. Create a new kubernetes secret with the service account credentials to be referenced later by `ROSAControlPlane` docs/book/src/topics/rosa/creating-a-cluster.md: Note: to consume the secret without the need to reference it from your `ROSAControlPlane`, name your secret as `rosa-creds-secret` and create it in the CAPA manager namespace (usually `capa-system`) docs/book/src/topics/rosa/creating-a-cluster.md:1. Create a credentials secret within the target namespace with the token to be referenced later by `ROSAControlePlane` docs/book/src/topics/rosa/creating-a-cluster.md:Follow the guide [here](https://docs.aws.amazon.com/ROSA/latest/userguide/getting-started-hcp.html) up until [Step 3](https://docs.aws.amazon.com/ROSA/latest/userguide/getting-started-hcp.html#getting-started-hcp-step-3) docs/book/src/topics/rosa/creating-a-cluster.md:Once Step 3 is done, you will be ready to proceed with creating a ROSA cluster using cluster-api. docs/book/src/topics/rosa/creating-a-cluster.md:1. Render the cluster manifest using the ROSA cluster template: docs/book/src/topics/rosa/creating-a-cluster.md:1. If a credentials secret was created earlier, edit `ROSAControlPlane` to reference it: docs/book/src/topics/rosa/creating-a-cluster.md: kind: ROSAControlPlane docs/book/src/topics/rosa/creating-a-cluster.md: kind: ROSAControlPlane docs/book/src/topics/rosa/creating-a-cluster.md:see [ROSAControlPlane CRD Reference](https://cluster-api-aws.sigs.k8s.io/crd/#controlplane.cluster.x-k8s.io/v1beta2.ROSAControlPlane) for all possible configurations.

most of our doc distinguish between ROSA (classic) and ROSA with HCP like the aws doc here so better to mention it as "ROSA-HCP" or "ROSA with HCP".

Yep. As I said, I can change it. My point was that the .md document in question uses just ROSA. No HCP, no Hypershift.

Okay then, lets do it in another PR change all docs at once.
Would you create an issue mentioning that we need to change ROSA to ROSA-HCP in docs so we don't forget it.

serngawy · 2024-12-18T00:04:36Z

pkg/rosa/client_test.go

+	"sigs.k8s.io/cluster-api-provider-aws/v2/util/system"
+)
+
+func createROSAControlPlaneScope(cp *rosacontrolplanev1.ROSAControlPlane, secrets ...*corev1.Secret) *scope.ROSAControlPlaneScope {


Change the name then, if you want to keep this function as is. ROSAControlPlaneScope doesn't required secrets to be created. Better naming like creaeROSAControlPlanScopWithSecrets

Umm, OK :-)

serngawy · 2024-12-18T14:34:03Z

docs/book/src/topics/rosa/creating-a-cluster.md

@@ -1,30 +1,83 @@
 # Creating a ROSA cluster

 ## Permissions
-CAPA controller requires an API token in order to be able to provision ROSA clusters:
+### Authentication using service account credentials


most of our doc distinguish between ROSA (classic) and ROSA with HCP like the aws doc here so better to mention it as "ROSA-HCP" or "ROSA with HCP".

serngawy · 2024-12-18T15:13:19Z

@nrb @faiq would you review & lgtm this PR

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 29, 2024

k8s-ci-robot added needs-priority cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 29, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 29, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 29, 2024

k8s-ci-robot requested review from faiq and nrb November 29, 2024 20:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 3, 2024

mzazrivec force-pushed the support_for_service_accounts_in_rosa_hcp branch from d51f93b to 6c7f244 Compare December 3, 2024 13:21

mzazrivec changed the title ~~Draft: ROSA: Support for OCM service account credentials~~ Draft: ✨ ROSA: Support for OCM service account credentials Dec 3, 2024

mzazrivec force-pushed the support_for_service_accounts_in_rosa_hcp branch from 6c7f244 to 1be3f7a Compare December 5, 2024 19:03

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2024

mzazrivec changed the title ~~Draft: ✨ ROSA: Support for OCM service account credentials~~ ✨ ROSA: Support for OCM service account credentials Dec 6, 2024

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 6, 2024

serngawy suggested changes Dec 6, 2024

View reviewed changes

mzazrivec force-pushed the support_for_service_accounts_in_rosa_hcp branch from 1be3f7a to 4cea03f Compare December 9, 2024 14:38

serngawy suggested changes Dec 9, 2024

View reviewed changes

serngawy mentioned this pull request Dec 9, 2024

🌱Add initial Rosa machine pool integration tests #5214

Open

5 tasks

mzazrivec force-pushed the support_for_service_accounts_in_rosa_hcp branch from 4cea03f to 71fcc0c Compare December 13, 2024 18:52

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2024

serngawy suggested changes Dec 16, 2024

View reviewed changes

mzazrivec force-pushed the support_for_service_accounts_in_rosa_hcp branch from 71fcc0c to e0a57f4 Compare December 17, 2024 19:09

serngawy suggested changes Dec 18, 2024

View reviewed changes

ROSA: Support for OCM service account credentials

c504cee

mzazrivec force-pushed the support_for_service_accounts_in_rosa_hcp branch from e0a57f4 to c504cee Compare December 18, 2024 13:34

serngawy approved these changes Dec 18, 2024

View reviewed changes

		ocmClientId = string(secret.Data[ocmClientIdKey])
		ocmClientSecret = string(secret.Data[ocmClientSecretKey])

	Instead of the service account credentials you can use SSO offline token, that you can specify either as a secret or specify the offline token
	The SSO offline token is been deprecated, it is recommended to use the service account as mentioned above. However, for old users you still can visit https://console.redhat.com/openshift/token to retrieve your SSO offline authentication token.

✨ ROSA: Support for OCM service account credentials #5233

Are you sure you want to change the base?

✨ ROSA: Support for OCM service account credentials #5233

Conversation

mzazrivec commented Nov 29, 2024 • edited Loading

linux-foundation-easycla bot commented Nov 29, 2024 • edited Loading

k8s-ci-robot commented Nov 29, 2024

k8s-ci-robot commented Nov 29, 2024

k8s-ci-robot commented Dec 5, 2024

serngawy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serngawy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serngawy commented Dec 13, 2024 • edited Loading

nrb commented Dec 16, 2024

serngawy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serngawy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serngawy commented Dec 18, 2024

mzazrivec commented Nov 29, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 29, 2024 •

edited

Loading

serngawy commented Dec 13, 2024 •

edited

Loading