NE-1381: API - add STSIAMRoleARN field #113

alebedev87 · 2023-09-27T11:46:27Z

Context
According to Tokenized Cloud Auth Enablement For Operators EP CCO can now provision secrets on STS clusters too. However this EP is meant for the operators only, the operands are supposed to be managed via their APIs.

Goal
The goal of this PR is to add a new field for the Role ARN which is supposed to be added to CredentialsRequest CR created by the operator for its operand (AWS LoadBalancer Controller).

openshift-ci-robot · 2023-09-27T11:46:30Z

@alebedev87: This pull request references NE-1381 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Context
According to Tokenized Cloud Auth Enablement For Operators EP CCO can now provision secrets on STS clusters too. The EP is dedicated to the operator's credentials, the operands are supposed to be managed via their APIs.

Goal
The goal of this PR is to add a new field for the Role ARN which is supposed to be added to CredentialsRequest CR created by the operator for its operand (AWS LoadBalancer controller).

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/test-infra repository.

openshift-ci-robot · 2023-09-27T12:17:14Z

@alebedev87: This pull request references NE-1381 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Context
According to Tokenized Cloud Auth Enablement For Operators EP CCO can now provision secrets on STS clusters too. The EP is dedicated to the operator's credentials, the operands are supposed to be managed via their APIs.

Goal
The goal of this PR is to add a new field for the Role ARN which is supposed to be added to CredentialsRequest CR created by the operator for its operand (AWS LoadBalancer controller).

Commits

NE-1381: API - add STSIAMRoleARN field: API change

STSIAMRoleARN usage example: API usage example

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/test-infra repository.

JoelSpeed · 2023-09-27T12:30:35Z

api/v1/awsloadbalancercontroller_types.go

+	// This field won't have any effect if credentials have already been provided through the `Credentials` field,
+	// as a request for credentials from the Cloud Credentials Operator will not be triggered.


You could enforce this validation using a CEL validation rule

CEL validation is added.

api/v1/awsloadbalancercontroller_types.go

JoelSpeed · 2023-09-27T12:33:23Z

api/v1/awsloadbalancercontroller_types.go

+	//
+	// +kubebuilder:validation:Optional
+	// +optional
+	CredentialsRequestConfig *AWSLoadBalancerCredentialsRequestConfig `json:"credentialsRequestConfig,omitempty"`


So this allows me to configure a credentials request that is created by the load balancer type? Or to refer to an existing credentials request?

This is to configure a credentials request that is created for the managed load balancer controller. The credentials request is defined by the operator. The new field is the only moving part in it for the moment.

openshift-ci-robot · 2023-10-13T12:48:40Z

@alebedev87: This pull request references NE-1381 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Context
According to Tokenized Cloud Auth Enablement For Operators EP CCO can now provision secrets on STS clusters too. The EP is dedicated to the operator's credentials, the operands are supposed to be managed via their APIs.

Goal
The goal of this PR is to add a new field for the Role ARN which is supposed to be added to CredentialsRequest CR created by the operator for its operand (AWS LoadBalancer Controller).

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/test-infra repository.

alebedev87 · 2023-10-17T12:11:24Z

Needs openshift/release#44017 to make the rosa e2e work.

alebedev87 · 2023-10-18T10:49:00Z

/test e2e-aws-proxy-operator

openshift-ci-robot · 2023-10-18T13:24:14Z

@alebedev87: This pull request references NE-1381 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Context
According to Tokenized Cloud Auth Enablement For Operators EP CCO can now provision secrets on STS clusters too. However this EP is meant for the operators only, the operands are supposed to be managed via their APIs.

Goal
The goal of this PR is to add a new field for the Role ARN which is supposed to be added to CredentialsRequest CR created by the operator for its operand (AWS LoadBalancer Controller).

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/test-infra repository.

alebedev87 · 2023-10-18T18:07:32Z

/test e2e-aws-rosa-operator

alebedev87 · 2023-10-18T18:39:54Z

ROSA cluster provisioning is blocked by openshift/rosa#1548.

candita · 2023-10-18T19:31:36Z

/assign @gcs278

alebedev87 · 2023-10-19T13:30:20Z

Docs updated to truncate the role name used by ccoctl. This should help avoiding the problems when controller and operator roles names turn to be same due to the limitation of 64 chars.

chenz4027 · 2023-10-19T13:46:32Z

/test e2e-aws-rosa-operator

alebedev87 · 2023-10-19T15:25:11Z

/test e2e-aws-rosa-operator

gcs278

Looks great, mostly nit picks.

docs/install.md

gcs278 · 2023-10-19T16:40:41Z

docs/install.md

tangential nit I think the names of these documents are confusing.

docs/prerequisities.md:

For STS, shows you how to create IAM Role and then install ALBO

VPC and Subnets

docs/install.md

For STS, assumes ALBO is already installed, and shows you how to create the AWSLoadBalancerController with STS IAM Role ARN.

Nothing for non-STS

prerequisties.md documents STS install, but install.md documents how to configure an ALBC instance (only for STS).

My thoughts:

It shouldn't be prerequisites.md if we describe how to install ALBO on an STS cluster, that should be install.md

install.md isn't showing how to install, it's showing how to finish configuring ALBC (post-install?)

install.md lacks showing how to install non-STS clusters

I think these suggestions are out of scope of this PR, so I don't want to hold you to it right now, but what do you think about someone addressing this as a bug in another PR? Do you agree something seems funky with organization?

Do you agree something seems funky with organization?

Yes, I agree. The 3 docs (prerequisistes, install, tutorial) overlap and mislead sometimes. It's a problem of this repository from the very beginning. Many reshuffles were done with different success rates.

I did another one to exclude the overlap, feel free to leave comments.

I like it. It feels like it flows a bit better.

docs/install.md

gcs278 · 2023-10-19T16:56:25Z

docs/install.md

-
-2. For `AWSLoadBalancerController` CR the **aws-load-balancer-operator** creates a `CredentialsRequest` named `aws-load-balancer-controller-cluster` in the `openshift-cloud-credential-operator` namespace. Extract and save the created `CredentialsRequest` in a directory:
+#### Option 1. Using `ccoctl`
+The dedicated controller's `CredentialsRequest` is maintained in [hack/controller](../hack/controller/) directory of this repository.


nit I think this information is good, but arguably shouldn't it be added to prerequisites.md to be symmetrical? These two documents seem to describe very similar things, and this statement also applies there.

Also, do you think it should be an absolute link, [hack/controller](/hack/controller/), to be more resilient to moving docs around?

nit I think this information is good, but arguably shouldn't it be added to prerequisites.md to be symmetrical? These two documents seem to describe very similar things, and this statement also applies there.

Both docs are aligned now.

Also, do you think it should be an absolute link, hack/controller, to be more resilient to moving docs around?

The path you proposed doesn't seem to be 100% absolute, it's still relative to the repository. I'm fine with it but does this syntax work?

Ah sorry. I was wrong about how github markdown works. You'd need to do the full URL in order to make it absolute, and that feels wrong. Seems like relative paths are preferred anyways. mkdocs/mkdocs#192

docs/install.md

docs/tutorial.md

test/e2e/operator_test.go

gcs278 · 2023-10-19T17:25:02Z

test/e2e/util.go

-// withCredSecretIf adds the credentials secret only if the given condition is true.
-func (b *albcBuilder) withCredSecretIf(condition bool, name string) *albcBuilder {
+// withRoleARNIf adds given role ARN to the credentials request config if the given condition is true.
+func (b *albcBuilder) withRoleARNIf(condition bool, roleARN string) *albcBuilder {


So I tried to run E2E in my STS cluster, non-ROSA, but this logic won't allow me to provide a roleARN manually, right?

I feel like we should support directly provide ENV while not in a Rosa cluster, otherwise I can't test your changes manually in my STS cluster.

There is a dedicated README chapter for the manual testing on a ROSA cluster. But I believe it should work on any STS cluster too. I think I should add ALBO_E2E_CONTROLLER_ROLE_ARN variable in there and maybe make it more generic: change the chapter name and add new platform toALBO_E2E_PLATFORM envvar.

Does it sound like what you were looking for?

Ah okay, I didn't realize the platform was controlled by a ENV Variables, I thought it was looking at some sort of cluster config object. Yea I suppose I could say I'm "ROSA" to test, but that does feel a little funky because I'd be lying.

I think your suggestion makes sense. If I provide ALBO_E2E_CONTROLLER_ROLE_ARN, I feel like it should use that in ALBC creation regardless of my platform, but at the same time, I don't think I should have to set ALBO_E2E_PLATFORM to ROSA.

Ah okay, I didn't realize the platform was controlled by a ENV Variables, I thought it was looking at some sort of cluster config object.

I don't know any reliable way of how to detect whether your container runs on a cluster which uses STS credentials. Infrastructure resource doesn't have any info about it. CCO in manual mode != STS. We could check the format of the contents of the cluster operator's secrets but that seems to be a little too much. Overall, it's a good question which needs to be digged in. I'll try to follow up on it.

I tried to address the comment in this commit. Let me know if it's still unclear.

Yea your approach is fine.

chenz4027 · 2023-10-19T17:35:06Z

@alebedev87 It passes the provision tests. My fix was in and it no longer blocks that step :)

INFO[2023-10-19T15:39:55Z] Running step e2e-aws-rosa-operator-rosa-cluster-provision. 
INFO[2023-10-19T16:10:28Z] Step e2e-aws-rosa-operator-rosa-cluster-provision succeeded after 30m32s.

alebedev87 · 2023-10-19T17:54:30Z

Thanks @chenz4027! The provisioning is back indeed.

alebedev87 · 2023-10-22T22:59:56Z

/test e2e-aws-operator

alebedev87 · 2023-10-23T07:24:22Z

Problems pulling openshift/node image faced:

                "containerStatuses": [
                    {
                        "image": "openshift/origin-node",
                        "imageID": "",
                        "lastState": {},
                        "name": "echo",
                        "ready": false,
                        "restartCount": 0,
                        "started": false,
                        "state": {
                            "waiting": {
                                "message": "Back-off pulling image \"openshift/origin-node\"",
                                "reason": "ImagePullBackOff"
                            }
                        }
                    }
                ],

Since it's pulled from docker.io, I decided to replace it with openshift/tools images from the internal registry, similar to what was done in the cluster ingress operator.

alebedev87 · 2023-10-23T07:25:34Z

Another ROSA provisioning issue: openshift/release#44633.

gcs278

Thanks for the updates, I like the reformatting.

docs/install.md

gcs278 · 2023-10-23T17:23:21Z

docs/install.md

-In an STS cluster,  the **cloud-credential-operator** does not automatically provision secrets for `CredentialsRequest`s, 
-so the cluster-admin must provision them manually.
-IAM role and policies must also be provisioned manually.
+## STS clusters


nit This is install.md, but only shows how to install STS clusters. Is it worth adding a Non-STS Cluster section for completeness?

Non-STS clusters chapter added.

gcs278 · 2023-10-23T17:34:35Z

docs/install.md

-1. Extract and prepare the `ccoctl` binary as described in [Configuring the Cloud Credential Operator utility](https://docs.openshift.com/container-platform/4.13/authentication/managing_cloud_provider_credentials/cco-mode-sts.html#cco-ccoctl-configuring_cco-mode-sts).
-
-2. For `AWSLoadBalancerController` CR the **aws-load-balancer-operator** creates a `CredentialsRequest` named `aws-load-balancer-controller-cluster` in the `openshift-cloud-credential-operator` namespace. Extract and save the created `CredentialsRequest` in a directory:
+The operator can be installed using [the OperatorHub web UI](https://docs.openshift.com/container-platform/latest/operators/understanding/olm-understanding-operatorhub.html) or by running the following commands:


When installing from the UI, don't you also need to apply:

config: env: - name: ROLEARN value: "${ROLEARN}"

to the subscription object? How is this done when you install it via the UI? I'm confused, because looking at https://github.com/alebedev87/aws-load-balancer-operator/blob/ne-1381-api-rolearn/bundle/manifests/aws-load-balancer-operator.clusterserviceversion.yaml, I followed to https://docs.openshift.com/container-platform/4.13/networking/aws_load_balancer_operator/installing-albo-sts-cluster.html#nw-bootstra-albo-on-sts-cluster_albo-sts-cluster and that didn't tell me how to include the ROLEARN env? Or is that automatic somehow when using the Web UI?

The OperatorHub's web UI is supposed to be updated with a new input box for the role ARN. A screenshot of how this is supposed to look like can be found in How To STS doc. Let me say this explicitly in our markdown.

gcs278 · 2023-10-23T18:44:35Z

docs/install.md

I like it. It feels like it flows a bit better.

docs/install.md

README.md

docs/tutorial.md

gcs278 · 2023-10-23T19:29:09Z

test/e2e/operator_test.go

@@ -1111,6 +1178,25 @@ func TestAWSLoadBalancerControllerWithExternalTypeNLBAndNonStandardPort(t *testi
 	}
 }

+// TestAWSLoadBalancerControllerOpenAPIValidation tests validations added to AWSLoadBalancerController CRD.
+func TestAWSLoadBalancerControllerOpenAPIValidation(t *testing.T) {


I think this test is a good addition, but future looking, are you familiar with the API integration test suite? https://github.com/openshift/api/tree/master/tests#readme. You can create API tests like this in YAML files. Not sure how easy it would be to port that over here.

Interesting. That may be useful to test the openAPI validations indeed. The setup may be a little heavy for this concrete PR but if the API will grow in complexity we can think of it.

…troller

alebedev87 · 2023-10-24T15:04:57Z

ROSA provisions 4.13.x clusters which don't have CredentialsRequest CRD updated with STSIAMRoleARN field used by the code introduced in this PR. The support starts from 4.14.0.
Therefore, as of now the ROSA e2e job cannot work with the new STS flow. This commit aims at fixing the gap temporarily.

gcs278 · 2023-10-24T17:02:38Z

Thanks for working through these reviews with me. Hopefully CI will sort itself out.
/approve
/lgtm

openshift-ci · 2023-10-24T17:03:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gcs278]

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

alebedev87 · 2023-10-24T21:32:13Z

/test e2e-aws-rosa-operator

openshift-ci · 2023-10-24T23:44:41Z

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

kowen-rh · 2023-10-25T10:11:00Z

/label docs-approved

alebedev87 · 2023-10-25T13:04:21Z

I edited Kevin's comment from docs-approved to /label docs-approved but it was not enough for the bot.

/label docs-approved

CFields651 · 2023-10-25T23:42:43Z

/label px-approved

ShudiLi · 2023-10-26T07:03:12Z

The ALBO can be created on ROSA STS with secrets created by ccoctl or the AWS CLI.
/qe-approved
thanks.

alebedev87 · 2023-10-26T07:46:53Z

/label qe-approved

I believe that's what Shudi wanted to say 😉.

openshift-ci-robot · 2023-10-26T07:47:44Z

@alebedev87: This pull request references NE-1381 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Context
According to Tokenized Cloud Auth Enablement For Operators EP CCO can now provision secrets on STS clusters too. However this EP is meant for the operators only, the operands are supposed to be managed via their APIs.

Goal
The goal of this PR is to add a new field for the Role ARN which is supposed to be added to CredentialsRequest CR created by the operator for its operand (AWS LoadBalancer Controller).

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/test-infra repository.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 27, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2023

openshift-ci bot requested review from gcs278 and miheer September 27, 2023 11:53

JoelSpeed reviewed Sep 27, 2023

View reviewed changes

alebedev87 force-pushed the ne-1381-api-rolearn branch from 36d4061 to 731de07 Compare September 27, 2023 14:23

alebedev87 force-pushed the ne-1381-api-rolearn branch from 7c2022d to 6a8130a Compare October 13, 2023 09:18

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2023

alebedev87 force-pushed the ne-1381-api-rolearn branch from 6a8130a to f1a1b7c Compare October 13, 2023 09:46

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2023

alebedev87 changed the title ~~[WIP] NE-1381: API - add STSIAMRoleARN field~~ NE-1381: API - add STSIAMRoleARN field Oct 16, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2023

alebedev87 mentioned this pull request Oct 17, 2023

OCPBUGS-19757: bump controller iam policy to v2.4.7 #114

Merged

openshift-ci bot assigned gcs278 Oct 18, 2023

alebedev87 force-pushed the ne-1381-api-rolearn branch from eb71d9d to 7548094 Compare October 19, 2023 13:28

gcs278 reviewed Oct 19, 2023

View reviewed changes

alebedev87 force-pushed the ne-1381-api-rolearn branch 3 times, most recently from d7185cc to b9932fb Compare October 23, 2023 15:45

gcs278 reviewed Oct 23, 2023

View reviewed changes

alebedev87 force-pushed the ne-1381-api-rolearn branch from b9932fb to c081ccc Compare October 24, 2023 11:02

alebedev87 added 3 commits October 24, 2023 16:43

NE-1381: API - add STSIAMRoleARN field

6ed0632

e2e: create test namespace earlier to avoid race with pull secret con…

e2e98f4

…troller

e2e: ensure controller's secret created using credentialsrequest

e5e259f

alebedev87 force-pushed the ne-1381-api-rolearn branch from c081ccc to e5e259f Compare October 24, 2023 14:44

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Oct 25, 2023

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Oct 25, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Oct 26, 2023

openshift-ci bot merged commit f84896d into openshift:main Oct 26, 2023

alebedev87 mentioned this pull request Jan 25, 2024

Secret Creation for ROSA - Non STS Cluster #121

Closed

		// This field won't have any effect if credentials have already been provided through the `Credentials` field,
		// as a request for credentials from the Cloud Credentials Operator will not be triggered.

NE-1381: API - add STSIAMRoleARN field #113

NE-1381: API - add STSIAMRoleARN field #113

Conversation

alebedev87 commented Sep 27, 2023 • edited Loading

openshift-ci-robot commented Sep 27, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Sep 27, 2023 • edited by openshift-ci bot Loading

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

openshift-ci-robot commented Oct 13, 2023 • edited by openshift-ci bot Loading

alebedev87 commented Oct 17, 2023 • edited Loading

alebedev87 commented Oct 18, 2023

openshift-ci-robot commented Oct 18, 2023 • edited by openshift-ci bot Loading

alebedev87 commented Oct 18, 2023

alebedev87 commented Oct 18, 2023

candita commented Oct 18, 2023

alebedev87 commented Oct 19, 2023

chenz4027 commented Oct 19, 2023

alebedev87 commented Oct 19, 2023

gcs278 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 Oct 20, 2023 • edited Loading

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

chenz4027 commented Oct 19, 2023 • edited Loading

alebedev87 commented Oct 19, 2023

alebedev87 commented Oct 22, 2023

alebedev87 commented Oct 23, 2023

alebedev87 commented Oct 23, 2023

gcs278 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

alebedev87 commented Oct 24, 2023

gcs278 commented Oct 24, 2023

openshift-ci bot commented Oct 24, 2023

alebedev87 commented Oct 24, 2023

openshift-ci bot commented Oct 24, 2023

kowen-rh commented Oct 25, 2023 • edited by alebedev87 Loading

alebedev87 commented Oct 25, 2023

CFields651 commented Oct 25, 2023

ShudiLi commented Oct 26, 2023

alebedev87 commented Oct 26, 2023

openshift-ci-robot commented Oct 26, 2023 • edited by openshift-ci bot Loading

alebedev87 commented Sep 27, 2023 •

edited

Loading

openshift-ci-robot commented Sep 27, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 27, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 13, 2023 •

edited by openshift-ci bot

Loading

alebedev87 commented Oct 17, 2023 •

edited

Loading

openshift-ci-robot commented Oct 18, 2023 •

edited by openshift-ci bot

Loading

alebedev87 Oct 23, 2023 •

edited

Loading

alebedev87 Oct 20, 2023 •

edited

Loading

chenz4027 commented Oct 19, 2023 •

edited

Loading

kowen-rh commented Oct 25, 2023 •

edited by alebedev87

Loading

openshift-ci-robot commented Oct 26, 2023 •

edited by openshift-ci bot

Loading