Skip to content

OSDOCS#7581,7582: Add token auth via CCO for Operator authors (AWS STS)#64808

Merged
adellape merged 1 commit intoopenshift:mainfrom
adellape:sts
Oct 24, 2023
Merged

OSDOCS#7581,7582: Add token auth via CCO for Operator authors (AWS STS)#64808
adellape merged 1 commit intoopenshift:mainfrom
adellape:sts

Conversation

@adellape
Copy link
Copy Markdown
Contributor

@adellape adellape commented Sep 15, 2023

https://issues.redhat.com/browse/OSDOCS-7581 (Concept)
https://issues.redhat.com/browse/OSDOCS-7582 (Procedure)

4.14 only

Preview:

TODO:

@openshift-ci openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 15, 2023
@adellape adellape force-pushed the sts branch 4 times, most recently from afe878c to 24fc761 Compare September 15, 2023 21:58
@adellape adellape changed the title OSDOCS#7582: Add tokenized auth for Operators via CCO [WIP] OSDOCS#7582: Add tokenized auth for Operators via CCO Sep 15, 2023
@openshift-ci 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 15, 2023
@adellape adellape force-pushed the sts branch 5 times, most recently from 7761cf1 to a4279dd Compare September 18, 2023 15:22
@ocpdocs-previewbot
Copy link
Copy Markdown

ocpdocs-previewbot commented Sep 18, 2023

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

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

@adellape adellape force-pushed the sts branch 7 times, most recently from 1df809b to 3187deb Compare September 18, 2023 16:58
@openshift-ci openshift-ci Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2023
@adellape adellape changed the title [WIP] OSDOCS#7582: Add tokenized auth for Operators via CCO OSDOCS#7582: Add tokenized auth for Operators via CCO Sep 18, 2023
@openshift-ci 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 Sep 18, 2023
@adellape adellape changed the title OSDOCS#7582: Add tokenized auth for Operators via CCO OSDOCS#7581,7582: Add tokenized auth for Operators via CCO Sep 18, 2023
@adellape adellape added this to the Planned for 4.14 GA milestone Sep 18, 2023
@adellape adellape force-pushed the sts branch 2 times, most recently from 3221224 to 86eff83 Compare September 18, 2023 21:02
@openshift-ci openshift-ci Bot removed the peer-review-needed Signifies that the peer review team needs to review this PR label Sep 29, 2023
@nalhadef
Copy link
Copy Markdown
Contributor

Hi! I had a few comments. My biggest difficulty: every time the word "multitenancy" appeared, I read it as "multi-lieutenancy" which confused me as it didn't make sense. :-)

@nalhadef
Copy link
Copy Markdown
Contributor

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

@openshift-ci openshift-ci Bot added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Sep 29, 2023
<11> The `resources` parameter defines resource constraints for all the containers in the pod created by OLM.
<12> The `nodeSelector` parameter defines a `NodeSelector` for the pod created by OLM.

. If the cluster is in STS mode, include the following fields in the `Subscription` object:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it help to spell STS out?

Comment thread modules/osdk-cco-aws-sts-alt.adoc Outdated
* Use the CCO utility (`ccoctl`) to generate the `Secret` YAML object from the `CredentialsRequest` object
* Apply the `Secret` object to the cluster in the appropriate namespace

The Operator still must be able to consume the resulting secret to communicate with cloud APIs. Because in this case the secret will be created by the user before the Operator is installed, the Operator can do either of the following:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replace "will be created" with "is created."

From the Red Hat supplemental style guide:
image

Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
webIdentityTokenPath := "/var/run/secrets/openshift/serviceaccount/token"
----

.. Ensure you have a `CredentialsRequest` object, if one does not already exist, ready to be patched and applied. For example:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Delete "if one does not already exist" and it's accompanying commas. The "ensure" portion of this sentence carries the same meaning, rending these extra words as merely extra words. This deletion also brings the opening and closing phrases together, strengthening the message.

@adellape
Copy link
Copy Markdown
Contributor Author

adellape commented Oct 2, 2023

@fxierh Applied additional changes per suggestions in a separate commit: 463575d (#64808)

Could you PTAL to verify, as well as see my remaining question in comment reply: #64808 (comment) ?

Thank you!

Copy link
Copy Markdown

@gallettilance gallettilance left a comment

Choose a reason for hiding this comment

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

just realize the credReq used as an example is outdated

Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
ServiceAccountNames: []string{
"<service_account_name>",
},
CloudTokenPath: "",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it the whole var CredentialsRequestTemplate that needs addressing or just the CloudTokenPath line here?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2023
Comment thread modules/osdk-cco-aws-sts-enabling.adoc
Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
@adellape
Copy link
Copy Markdown
Contributor Author

adellape commented Oct 9, 2023

@fxierh @gallettilance Thank you for helping get these in shape. I've attempted to interpret/combine your latest comments into some changes but I don't believe I've fully addressed Lance's #64808 (review) about the outdated credReq. I've added a few clarification questions in-line or in replies.

PTAL at the latest changes, which are in an additional separate commit: 117ddcd (#64808) (edit: have squashed)

@adellape
Copy link
Copy Markdown
Contributor Author

adellape commented Oct 13, 2023

Also @fxierh @gallettilance - Release notes for this feature are here: #66133

Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
case <-timeout:
// timeout is exceeded, return an error
return nil, fmt.Errorf("timed out waiting for secret %s in namespace %s", name, namespace)
// add to this error with a pointer to instructions for following a manual path to a Secret that will work on STS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this comment for the line that follows ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I assumed the line before it; adjusted the indentation for the comment to look more like the others.

Comment thread modules/osdk-cco-aws-sts-enabling.adoc Outdated
Comment on lines +208 to +209
// apply credentialsRequest on install
credReq := credreq.CredentialsRequestTemplate
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there an indentation issue here ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adjusted.

@fxierh
Copy link
Copy Markdown

fxierh commented Oct 16, 2023

LGTM for the rest.

@adellape adellape changed the title OSDOCS#7581,7582: Add token auth via CCO for Operator authors OSDOCS#7581,7582: Add token auth via CCO for Operator authors (AWS STS) Oct 16, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2023
@adellape adellape merged commit 393a273 into openshift:main Oct 24, 2023
@adellape adellape deleted the sts branch October 24, 2023 16:14
@adellape
Copy link
Copy Markdown
Contributor Author

/cherrypick enterprise-4.14

@openshift-cherrypick-robot
Copy link
Copy Markdown

@adellape: new pull request created: #66818

Details

In response to this:

/cherrypick enterprise-4.14

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.14 peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.