CCO-770: Docs: consolidate and simply short-term token workflows#986
CCO-770: Docs: consolidate and simply short-term token workflows#986jstuever wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jstuever: This pull request references CCO-770 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.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstuever The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughConsolidates provider-specific short-lived credential guides into a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@jstuever: This pull request references CCO-770 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.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
README.md (1)
240-240: Consider adding .md extension to documentation link for clarity.While GitHub renders both formats, including the
.mdextension is more explicit and aligns with other documentation links in the repository.📋 Suggested improvement
-[Documentation](./docs/short-term-credentials) +[Documentation](./docs/short-term-credentials.md)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 240, Update the markdown link in README.md so it explicitly references the documentation file with the .md extension; replace the current reference "[Documentation](./docs/short-term-credentials)" with a link that includes the extension (e.g., "[Documentation](./docs/short-term-credentials.md)") to match other docs links and improve clarity.docs/short-term-credentials.md (1)
280-283: Replace hardcoded directory names with placeholders.The migration workflow uses hardcoded directory names like "newstscluster" and "output_dir" which are inconsistent with the placeholder pattern used elsewhere (e.g.,
<aws_infra_name>). Users need to substitute these with their actual values, so placeholders would be clearer.📋 Suggested improvement
At line 280-283:
- OIDC_ISSUER_URL=`awk '/serviceAccountIssuer/ { print $2 }' newstscluster/manifests/cluster-authentication-02-config.yaml` + OIDC_ISSUER_URL=`awk '/serviceAccountIssuer/ { print $2 }' <output_dir>/manifests/cluster-authentication-02-config.yaml`At line 307:
- $ find ./newstscluster/manifests -iname "*yaml" -print0 | xargs -I {} -0 -t oc replace -f {} + $ find ./<output_dir>/manifests -iname "*yaml" -print0 | xargs -I {} -0 -t oc replace -f {}At line 315:
oc apply -f output_dir/manifests/azure-ad-pod-identity-webhook-config.yaml + oc apply -f <output_dir>/manifests/azure-ad-pod-identity-webhook-config.yamlConsider adding a note earlier in the migration section to define
<output_dir>as the directory created byccoctlcommands.Also applies to: 307-308, 315-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` around lines 280 - 283, The docs use hardcoded directory names ("newstscluster", "output_dir") in code blocks (e.g., the awk/oc commands) which should be replaced with descriptive placeholders; update occurrences of newstscluster and output_dir to a consistent placeholder like <output_dir> or <aws_infra_name> in the awk line and the oc patch command, and add a short note earlier in the migration section that defines <output_dir> as the directory created by the ccoctl commands so readers know what to substitute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/short-term-credentials.md`:
- Line 45: Update the phrasing to use the hyphenated compound adjective
"short-lived" wherever "short lived" appears (e.g., the sentence "The token is a
projected ServiceAccount into the Pod, and is short lived for an hour after
which it is refreshed." should read "...and is short-lived for an hour...").
Also fix the related instance referenced (line 101) and ensure "Google" is
capitalized correctly where noted. Search for occurrences of "short lived" in
the document and replace with "short-lived" and correct the proper noun
capitalization.
- Line 427: Fix the spelling typo in the documentation sentence that currently
reads "Delete the cloud resurces using the
[ccoctl](ccoctl.md#deleting-resources) tool..." by changing "resurces" to
"resources" so the sentence reads "Delete the cloud resources using the
[ccoctl](ccoctl.md#deleting-resources) tool...". Ensure the corrected word
appears wherever the misspelling occurs in docs/short-term-credentials.md.
- Line 291: Change the typo in the sentence "Restart all nodes to fresh the
service accounts on all pods. This *will* take a while." to use "refresh"
instead of "fresh" so it reads "Restart all nodes to refresh the service
accounts on all pods. This *will* take a while."; update that exact sentence in
the docs/short-term-credentials.md content.
- Line 23: Update the documentation text to capitalize "Google" wherever it
appears as a proper noun; specifically replace the phrase "google client" with
"Google client" in the sentence that begins "The google client shares the OIDC
token..." so the proper noun is correctly capitalized.
- Line 232: Replace the misspelled token "CredentialsCrequests" with the correct
"CredentialsRequests" wherever it appears in the documentation text (e.g., in
the line "Extract CredentialsCrequests from the cluster's release image.").
Ensure the corrected spelling is used consistently across the document so
references to CredentialsRequests match other docs and any code/entity names.
- Line 3: The overview paragraph contains two spelling mistakes: replace
"porpose" with "purpose" and "utlize" with "utilize" in the sentence describing
how the cloud provider trusts Kubernetes ServiceAccount tokens (look for the
occurrences of "porpose" and "utlize" in the ServiceAccount token overview
paragraph). Make those two simple word fixes so the sentence reads correctly
while leaving all other text unchanged.
---
Nitpick comments:
In `@docs/short-term-credentials.md`:
- Around line 280-283: The docs use hardcoded directory names ("newstscluster",
"output_dir") in code blocks (e.g., the awk/oc commands) which should be
replaced with descriptive placeholders; update occurrences of newstscluster and
output_dir to a consistent placeholder like <output_dir> or <aws_infra_name> in
the awk line and the oc patch command, and add a short note earlier in the
migration section that defines <output_dir> as the directory created by the
ccoctl commands so readers know what to substitute.
In `@README.md`:
- Line 240: Update the markdown link in README.md so it explicitly references
the documentation file with the .md extension; replace the current reference
"[Documentation](./docs/short-term-credentials)" with a link that includes the
extension (e.g., "[Documentation](./docs/short-term-credentials.md)") to match
other docs links and improve clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8be839d5-adf8-4cd5-9869-230b8dd0ee69
📒 Files selected for processing (5)
README.mddocs/azure_workload_identity.mddocs/gcp_workload_identity.mddocs/short-term-credentials.mddocs/sts.md
💤 Files with no reviewable changes (3)
- docs/azure_workload_identity.md
- docs/gcp_workload_identity.md
- docs/sts.md
|
@jstuever: This pull request references CCO-770 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.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
LOL. I forgot to do a spellcheck pass, and CodeRabbit called me out on it. |
|
/cc @patrickdillon |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/short-term-credentials.md (1)
3-3:⚠️ Potential issue | 🟡 MinorFix spelling error: "utlize" should be "utilize".
The word "utlize" on line 3 is misspelled and should be "utilize".
📝 Proposed fix
-OpenShift can be configured to use short-term credentials for components with [AWS Security Token Service](https://docs.aws.amazon.com/STS/latest/APIReference/welcome.html), [Azure AD Workload Identity](https://azure.github.io/azure-workload-identity/docs/), or [GCP Workload Identity Federation](https://docs.cloud.google.com/iam/docs/workload-identity-federation). In this configuration, the cloud provider is configured to trust the Kubernetes ServiceAccount tokens signed by the OpenShift cluster for the purpose of authenticating to specific roles or service accounts. A component such as a cluster operator can utlize the signed ServiceAccount token mounted in the Pod to gain access to the cloud provider via the role or service account that was specifically configured for the component. This enables fine-grained control of permissions for each component. This process also automates requesting and refreshing of credentials. The ServiceAccount token used by a Pod is rotated by the kubelet on the node where Pod is running when the Pod's ServiceAccount token is approaching expiry. ServiceAccount tokens are valid for one hour. +OpenShift can be configured to use short-term credentials for components with [AWS Security Token Service](https://docs.aws.amazon.com/STS/latest/APIReference/welcome.html), [Azure AD Workload Identity](https://azure.github.io/azure-workload-identity/docs/), or [GCP Workload Identity Federation](https://docs.cloud.google.com/iam/docs/workload-identity-federation). In this configuration, the cloud provider is configured to trust the Kubernetes ServiceAccount tokens signed by the OpenShift cluster for the purpose of authenticating to specific roles or service accounts. A component such as a cluster operator can utilize the signed ServiceAccount token mounted in the Pod to gain access to the cloud provider via the role or service account that was specifically configured for the component. This enables fine-grained control of permissions for each component. This process also automates requesting and refreshing of credentials. The ServiceAccount token used by a Pod is rotated by the kubelet on the node where Pod is running when the Pod's ServiceAccount token is approaching expiry. ServiceAccount tokens are valid for one hour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` at line 3, Fix the spelling error in the documentation by replacing the misspelled word "utlize" with the correct "utilize" in the sentence that begins "A component such as a cluster operator can utlize the signed ServiceAccount token mounted in the Pod..." inside the short-term credentials doc (update the phrase in the paragraph describing ServiceAccount token usage).
🧹 Nitpick comments (3)
docs/short-term-credentials.md (3)
226-228: Specify language for code fence.The code block should specify a language identifier (e.g.,
bash) for proper syntax highlighting.📝 Proposed fix
- ``` + ```bash ./openshift-install create cluster ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` around lines 226 - 228, The fenced code block containing "./openshift-install create cluster" is missing a language identifier; update the triple backticks so the fence specifies a language (e.g., add "bash" after the opening ```), so the block becomes ```bash before the line "./openshift-install create cluster" to enable proper syntax highlighting.
343-343: Hyphenate compound adjective "post-install".The heading uses "Post install/migrate" but the compound adjective "post-install" should be hyphenated when modifying a noun.
📝 Proposed fix
-### Post install/migrate verification +### Post-install/migrate verification🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` at line 343, Update the heading "Post install/migrate verification" to hyphenate the compound adjective—change it to "Post-install/migrate verification" so the adjective correctly modifies the noun; locate the heading text in docs/short-term-credentials.md (the line containing the "Post install/migrate verification" header) and replace it accordingly.
170-170: Use descriptive link text instead of "here".The link text "here" is not descriptive. Consider using more specific text that describes the destination, such as "technical details on private S3 buckets with CloudFront".
📝 Proposed fix
- * Create the S3 bucket (with public read-only access). You can alternatively use a private S3 bucket with a CloudFront Distribution providing access to the OIDC config by including the --create-private-s3-bucket parameter. More information on the technical details [here](./sts-private-bucket.md) + * Create the S3 bucket (with public read-only access). You can alternatively use a private S3 bucket with a CloudFront Distribution providing access to the OIDC config by including the --create-private-s3-bucket parameter. More information on the [technical details for private S3 buckets](./sts-private-bucket.md)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` at line 170, Replace the non-descriptive link text "here" in the sentence that mentions "--create-private-s3-bucket" with a descriptive phrase like "technical details on private S3 buckets with CloudFront" (or similar) so the link clearly indicates the destination; update the link text in docs/short-term-credentials.md for the line containing the bucket/CloudFront explanation to use the new descriptive text while keeping the same URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/short-term-credentials.md`:
- Line 280: Replace inconsistent directory names in the example commands so they
all use a single placeholder (e.g., <output_dir>) instead of mixing
"newstscluster" and "output_dir"; specifically update the OIDC_ISSUER_URL
assignment line that references
newstscluster/manifests/cluster-authentication-02-config.yaml and any other
occurrences (the lines referencing "newstscluster" and "output_dir") to use the
same placeholder, and add a brief note instructing users to substitute their
actual ccoctl output directory if needed.
---
Duplicate comments:
In `@docs/short-term-credentials.md`:
- Line 3: Fix the spelling error in the documentation by replacing the
misspelled word "utlize" with the correct "utilize" in the sentence that begins
"A component such as a cluster operator can utlize the signed ServiceAccount
token mounted in the Pod..." inside the short-term credentials doc (update the
phrase in the paragraph describing ServiceAccount token usage).
---
Nitpick comments:
In `@docs/short-term-credentials.md`:
- Around line 226-228: The fenced code block containing "./openshift-install
create cluster" is missing a language identifier; update the triple backticks so
the fence specifies a language (e.g., add "bash" after the opening ```), so the
block becomes ```bash before the line "./openshift-install create cluster" to
enable proper syntax highlighting.
- Line 343: Update the heading "Post install/migrate verification" to hyphenate
the compound adjective—change it to "Post-install/migrate verification" so the
adjective correctly modifies the noun; locate the heading text in
docs/short-term-credentials.md (the line containing the "Post install/migrate
verification" header) and replace it accordingly.
- Line 170: Replace the non-descriptive link text "here" in the sentence that
mentions "--create-private-s3-bucket" with a descriptive phrase like "technical
details on private S3 buckets with CloudFront" (or similar) so the link clearly
indicates the destination; update the link text in
docs/short-term-credentials.md for the line containing the bucket/CloudFront
explanation to use the new descriptive text while keeping the same URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82b31e26-8d71-4a42-947f-5a9148859de9
📒 Files selected for processing (5)
README.mddocs/azure_workload_identity.mddocs/gcp_workload_identity.mddocs/short-term-credentials.mddocs/sts.md
💤 Files with no reviewable changes (3)
- docs/azure_workload_identity.md
- docs/sts.md
- docs/gcp_workload_identity.md
Previously, the documentation for the short-term token workflows was split into three docs, one for each supported cloud. They were also more complicated than they needed to be. This caused unnecessary burden on both the maintainers of the repo as well as users attempting to follow them. This change combines the three docs into a single workflow. This reduces the maintenance burden on repo maintainers. It also simplifies the installation workflows by having the oc extract use the --included and --install-config flags in addition to outputting the manifests directly into the installation folder directly
|
@jstuever: This pull request references CCO-770 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.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/short-term-credentials.md (1)
3-3:⚠️ Potential issue | 🟡 MinorFix spelling error: "utlize" should be "utilize".
The word is misspelled in the sentence describing how cluster operators use ServiceAccount tokens.
📝 Proposed fix
-OpenShift can be configured to use short-term credentials for components with [AWS Security Token Service](https://docs.aws.amazon.com/STS/latest/APIReference/welcome.html), [Azure AD Workload Identity](https://azure.github.io/azure-workload-identity/docs/), or [GCP Workload Identity Federation](https://docs.cloud.google.com/iam/docs/workload-identity-federation). In this configuration, the cloud provider is configured to trust the Kubernetes ServiceAccount tokens signed by the OpenShift cluster for the purpose of authenticating to specific roles or service accounts. A component such as a cluster operator can utlize the signed ServiceAccount token mounted in the Pod to gain access to the cloud provider via the role or service account that was specifically configured for the component. This enables fine-grained control of permissions for each component. This process also automates requesting and refreshing of credentials. The ServiceAccount token used by a Pod is rotated by the kubelet on the node where Pod is running when the Pod's ServiceAccount token is approaching expiry. ServiceAccount tokens are valid for one hour. +OpenShift can be configured to use short-term credentials for components with [AWS Security Token Service](https://docs.aws.amazon.com/STS/latest/APIReference/welcome.html), [Azure AD Workload Identity](https://azure.github.io/azure-workload-identity/docs/), or [GCP Workload Identity Federation](https://docs.cloud.google.com/iam/docs/workload-identity-federation). In this configuration, the cloud provider is configured to trust the Kubernetes ServiceAccount tokens signed by the OpenShift cluster for the purpose of authenticating to specific roles or service accounts. A component such as a cluster operator can utilize the signed ServiceAccount token mounted in the Pod to gain access to the cloud provider via the role or service account that was specifically configured for the component. This enables fine-grained control of permissions for each component. This process also automates requesting and refreshing of credentials. The ServiceAccount token used by a Pod is rotated by the kubelet on the node where Pod is running when the Pod's ServiceAccount token is approaching expiry. ServiceAccount tokens are valid for one hour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` at line 3, Replace the misspelled word "utlize" with "utilize" in the paragraph that describes how a component such as a cluster operator uses the signed ServiceAccount token (the sentence starting "A component such as a cluster operator can utlize the signed ServiceAccount token..."); update that single word only so the sentence reads "can utilize the signed ServiceAccount token" without other changes.
🧹 Nitpick comments (1)
docs/short-term-credentials.md (1)
343-343: Consider hyphenating compound adjective.The heading "Post install/migrate verification" uses "Post install/migrate" as a compound adjective. Standard style guides recommend hyphenating such phrases: "Post-install/migrate verification" or "Post-install/post-migrate verification".
📝 Proposed fix
-### Post install/migrate verification +### Post-install/migrate verification🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/short-term-credentials.md` at line 343, Update the heading "Post install/migrate verification" to use hyphenation for the compound adjective; change it to either "Post-install/migrate verification" or, for clarity, "Post-install/post-migrate verification" so the phrase is grammatically correct and follows style guides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/short-term-credentials.md`:
- Line 3: Replace the misspelled word "utlize" with "utilize" in the paragraph
that describes how a component such as a cluster operator uses the signed
ServiceAccount token (the sentence starting "A component such as a cluster
operator can utlize the signed ServiceAccount token..."); update that single
word only so the sentence reads "can utilize the signed ServiceAccount token"
without other changes.
---
Nitpick comments:
In `@docs/short-term-credentials.md`:
- Line 343: Update the heading "Post install/migrate verification" to use
hyphenation for the compound adjective; change it to either
"Post-install/migrate verification" or, for clarity, "Post-install/post-migrate
verification" so the phrase is grammatically correct and follows style guides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1cadf5b-4bc2-4eb3-87b2-2158c4f24c11
📒 Files selected for processing (5)
README.mddocs/azure_workload_identity.mddocs/gcp_workload_identity.mddocs/short-term-credentials.mddocs/sts.md
💤 Files with no reviewable changes (3)
- docs/azure_workload_identity.md
- docs/gcp_workload_identity.md
- docs/sts.md
|
@jstuever: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
Previously, the documentation for the short-term token workflows was split into three docs, one for each supported cloud. They were also more complicated than they needed to be. This caused unnecessary burden on both the maintainers of the repo as well as users attempting to follow them.
This change combines the three docs into a single workflow. This reduces the maintenance burden on repo maintainers. It also simplifies the installation workflows by having the oc extract use the --included and --install-config flags in addition to outputting the manifests directly into the installation folder directly
Summary by CodeRabbit