Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CFE-712: Adds the procedure to configure AWS Load Balancer Operator on STS cluster by using predefined credentials #53666

Merged
merged 1 commit into from Dec 15, 2022

Conversation

xenolinux
Copy link
Contributor

@xenolinux xenolinux commented Dec 12, 2022

Adds the procedure to configure AWS Load Balancer Operator on STS cluster by using predefined credentials

Version(s): 4.12+

Issue: https://issues.redhat.com/browse/CFE-712

Link to docs preview: https://53666--docspreview.netlify.app/openshift-enterprise/latest/networking/aws_load_balancer_operator/installing-albo-sts-cluster.html#nw-installing-albo-on-sts-cluster-predefined-credentials_albo-sts-cluster

QE review:

  • QE has approved this change.

Additional information:

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 12, 2022
@xenolinux xenolinux changed the title Adds the procedure to create AWS Load Balancer Operator on Secure Tok… Adds the procedure to create AWS Load Balancer Operator on STS cluster by using predefined credentials Dec 12, 2022
@xenolinux xenolinux changed the title Adds the procedure to create AWS Load Balancer Operator on STS cluster by using predefined credentials Adds the procedure to configure AWS Load Balancer Operator on STS cluster by using predefined credentials Dec 12, 2022
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Dec 12, 2022

🤖 Updated build preview is available at:
https://53666--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/5369

@xenolinux xenolinux force-pushed the sts-api branch 3 times, most recently from 83640a4 to 4b34300 Compare December 12, 2022 10:11
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 12, 2022
@xenolinux xenolinux force-pushed the sts-api branch 5 times, most recently from 2994707 to 5295746 Compare December 13, 2022 07:20
Comment on lines +10 to +12
.Prerequisites

* You must extract and prepare the `ccoctl` binary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is repeated in each sub chapter of Installing the AWS Load Balancer Operator on Secure Token Service cluster, maybe worth moving it to the very top to reduce the repetitive commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, since this is part of a prerequisites section, we must keep it at the beginning of each chapter. But I can check once the peer-review process begins if we can eliminate the repetitive commands


* You must extract and prepare the `ccoctl` binary.

.Procedure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the first setp we need to retrieve the predefined credentials file, similar to what we did in Bootstraping ... chapter but from another directory:

$ curl --create-dirs -o <path-to-credrequests-dir>/cr.yaml https://raw.githubusercontent.com/openshift/aws-load-balancer-operator/main/hack/controller/controller-credentials-request.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

Comment on lines 20 to 30
$ ccoctl aws create-iam-roles \
--name <name> --region=<aws_region> \
--credentials-requests-dir=hack/controller \
--identity-provider-arn <oidc-arn>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ ccoctl aws create-iam-roles \
--name <name> --region=<aws_region> \
--credentials-requests-dir=hack/controller \
--identity-provider-arn <oidc-arn>
$ ccoctl aws create-iam-roles \
--name <name> --region=<aws_region> \
--credentials-requests-dir=<path-to-credrequests-dir> \
--identity-provider-arn <oidc-arn>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

--credentials-requests-dir=hack/controller \
--identity-provider-arn <oidc-arn>
----
For each `CredentialsRequest` object, the `ccoctl` tool creates an IAM role with a trust policy. The trust policy contains the specified OIDC identity provider, and permissions policy as defined in each `CredentialsRequest` object. The `ccoctl` tool also generates a set of secrets in a `manifests` directory that the AWS Load Balancer Controller uses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't mention these details before, maybe we can skip them this time too. ccoctl details can be discovered in its documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
----

. Create the `aws-load-balancer-controller` resource YAML file, for example, `sample-aws-lb-predefined-creds.yaml`, as follows:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
. Create the `aws-load-balancer-controller` resource YAML file, for example, `sample-aws-lb-predefined-creds.yaml`, as follows:
. Create the `AWSLoadBalancerController` resource YAML file, for example, `sample-aws-lb-manual-creds.yaml`, as follows:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

Comment on lines 57 to 58
spec:
credentials: <secret-name> <3>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
spec:
credentials: <secret-name> <3>
spec:
credentials:
name: <secret-name> <3>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

spec:
credentials: <secret-name> <3>
----
<1> Defines the `aws-load-balancer-controller` resource.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<1> Defines the `aws-load-balancer-controller` resource.
<1> Defines the `AWSLoadBalancerController` resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

Copy link
Contributor

@alebedev87 alebedev87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that we could do some more clarifications. Feel free to rephrase the suggestions, I just wanted to give an idea.

The AWS Load Balancer Operator relies on `CredentialsRequest` to bootstrap the Operator and for each `AWSLoadBalancerController` instance. The AWS Load Balancer Operator waits until the required secrets are created and available. The Cloud Credential Operator does not provision the secrets automatically in the STS cluster. You must set the credentials secrets manually by using the `ccoctl` binary.
The AWS Load Balancer Operator relies on `CredentialsRequest` to bootstrap the Operator and for each `AWSLoadBalancerController` instance. The AWS Load Balancer Operator waits until the required secrets are created and available. The Cloud Credential Operator does not provision the secrets automatically in the STS cluster. You must set the credentials secrets manually by using the `ccoctl` binary.

You can also configure the AWS Load Balancer Operator on the STS cluster by using predefined credentials.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can elaborate on this one because the reason to add the new configuration possibility was driven by the fact that the Cloud Credential Operator is not available in all the clusters. In the GitHub doc we mentioned this briefly. What do you think about something like this:

Suggested change
You can also configure the AWS Load Balancer Operator on the STS cluster by using predefined credentials.
In case the provisioning of the credentials secret should not be done by the Cloud Credential Operator, the `AWSLoadBalancerController` instance can be configured with explicitly specified credentials secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

@@ -3,11 +3,11 @@

:_content-type: PROCEDURE
[id="nw-installing-albo-on-sts-cluster_{context}"]
= Configuring AWS Load Balancer Operator on Secure Token Service cluster
= Configuring AWS Load Balancer Operator on Secure Token Service cluster by using `CredentialsRequest` objects
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
= Configuring AWS Load Balancer Operator on Secure Token Service cluster by using `CredentialsRequest` objects
= Configuring AWS Load Balancer Operator on Secure Token Service cluster by using managed `CredentialsRequest` objects

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe even managed by operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

[id="nw-installing-albo-on-sts-cluster-predefined-credentials_{context}"]
= Configuring the AWS Load Balancer Operator on Secure Token Service cluster by using predefined credentials

You can use the predefined `CredentialsRequest` object by specifying the `spec.credential` field in the AWS load Balancer Controller custom resource (CR).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can use the predefined `CredentialsRequest` object by specifying the `spec.credential` field in the AWS load Balancer Controller custom resource (CR).
The credentials secret can be specified explicitly using the `spec.credentials` field in the AWS Load Balancer Controller custom resource (CR). You can use the predefined controller's `CredentialsRequest` object to fin dout which roles are required by the controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed


.Procedure

. Download the CredentialsRequest custom resource (CR) of the AWS Load Balancer Operator, and create a directory to store it by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
. Download the CredentialsRequest custom resource (CR) of the AWS Load Balancer Operator, and create a directory to store it by running the following command:
. Download the CredentialsRequest custom resource (CR) of the AWS Load Balancer Controller, and create a directory to store it by running the following command:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed


:_content-type: PROCEDURE
[id="nw-installing-albo-on-sts-cluster-predefined-credentials_{context}"]
= Configuring the AWS Load Balancer Operator on Secure Token Service cluster by using predefined credentials
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
= Configuring the AWS Load Balancer Operator on Secure Token Service cluster by using predefined credentials
= Configuring the AWS Load Balancer Operator on Secure Token Service cluster by using explicitly set credentials

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

@xenolinux xenolinux force-pushed the sts-api branch 3 times, most recently from 46b6eff to d02b32a Compare December 13, 2022 09:50
@alebedev87
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022
@xenolinux
Copy link
Contributor Author

@lihongan Please take a look at this PR at your leisure

@lihongan
Copy link

Looks good. Thank you @xenolinux

@xenolinux xenolinux added the peer-review-needed Signifies that the peer review team needs to review this PR label Dec 14, 2022
@GroceryBoyJr
Copy link
Contributor

/label peer-review-in-progress
/remove-label peer-review-needed

@openshift-ci openshift-ci bot added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Dec 14, 2022
@@ -8,12 +8,16 @@ toc::[]

You can install the AWS Load Balancer Operator on the Secure Token Service (STS) cluster.

The AWS Load Balancer Operator relies on `CredentialsRequest` to bootstrap the Operator and for each `AWSLoadBalancerController` instance. The AWS Load Balancer Operator waits until the required secrets are created and available. The Cloud Credential Operator does not provision the secrets automatically in the STS cluster. You must set the credentials secrets manually by using the `ccoctl` binary.
The AWS Load Balancer Operator relies on `CredentialsRequest` to bootstrap the Operator and for each `AWSLoadBalancerController` instance. The AWS Load Balancer Operator waits until the required secrets are created and available. The Cloud Credential Operator does not provision the secrets automatically in the STS cluster. You must set the credentials secrets manually by using the `ccoctl` binary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence "The AWS Load Balancer Operator waits until the required secrets are created and available." is passive voice. The problem is the phrase "are created". A possible rewrite could be: "The AWS Load Balancer Operator requires the secrets to be created and available for correct function."

Copy link
Contributor

@GroceryBoyJr GroceryBoyJr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, Servesha! Only two very small suggestions for change. I am on peer review all week, reach out to me if you would like me to re-review tomorrow or (US) Friday, I am glad to do so.

$ ls manifests/*-credentials.yaml | xargs -I{} oc apply -f {}
----

. Verify that the credentials secret of the controller is created:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Instead of "Verify that the credentials secret of the controller is created:", replace with "Verify the credentials secret has been created for use by the controller:" See https://www.ibm.com/docs/en/ibm-style?topic=grammar-verbs under the heading "Voice" . (Please ping me if you have trouble seeing the IBM style guide.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

@GroceryBoyJr
Copy link
Contributor

/label peer-review-completed
/remove-label peer-review-in-progress

@openshift-ci openshift-ci bot removed the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Dec 14, 2022
@openshift-ci
Copy link

openshift-ci bot commented Dec 14, 2022

@GroceryBoyJr: The label(s) /label peer-review-completed cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, cnv, dev-tools, distributed-tracing, ims, jira/valid-bug, merge-review-in-progress, merge-review-needed, mtc, multi-arch, oadp, peer-review-done, peer-review-in-progress, peer-review-needed, rhacs, rhv, serverless, service-mesh, staff-eng-approved, telco. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label peer-review-completed
/remove-label peer-review-in-progress

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.

@GroceryBoyJr
Copy link
Contributor

/label peer-review-done

@openshift-ci openshift-ci bot added the peer-review-done Signifies that the peer review team has reviewed this PR label Dec 14, 2022
…en Service cluster by using predefined credentials
@xenolinux xenolinux added the merge-review-needed Signifies that the merge review team needs to review this PR label Dec 15, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2022
@openshift-ci
Copy link

openshift-ci bot commented Dec 15, 2022

New changes are detected. LGTM label has been removed.

@snarayan-redhat snarayan-redhat added merge-review-in-progress Signifies that the merge review team is reviewing this PR branch/enterprise-4.12 labels Dec 15, 2022
@snarayan-redhat snarayan-redhat added this to the Planned for 4.12 GA milestone Dec 15, 2022
@snarayan-redhat snarayan-redhat merged commit b78e09e into openshift:main Dec 15, 2022
@snarayan-redhat
Copy link
Contributor

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

@snarayan-redhat: new pull request created: #53868

In response to this:

/cherrypick enterprise-4.12

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.

@xenolinux xenolinux changed the title Adds the procedure to configure AWS Load Balancer Operator on STS cluster by using predefined credentials CFE-712: Adds the procedure to configure AWS Load Balancer Operator on STS cluster by using predefined credentials Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.12 merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants