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
OCPBUGS-2158: operator: add service account issuers to KAS status #1313
OCPBUGS-2158: operator: add service account issuers to KAS status #1313
Conversation
Hello @mfojtik! Some important instructions when contributing to openshift/api: For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard
OR
Who should apply these qe/docs/px labels?
|
/assign @stlaz |
97ec65b
to
ed244af
Compare
LGTM |
|
||
// serviceAccountIssuers tracks history of used service account issuers. | ||
// The item without expiration time represents the currently used service account issuer. | ||
// The other items represents service account issuers that were used previously and are still being trusted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long will the entry be trusted for once it is no longer the present entry?
Where does the configuration for this come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is 24h, and it is hardcoded in the controller. I don't think we want to expose this to users to configure the expiration time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing it would allow users to force remove old entries, but I think we are discussing that elsewhere. Documenting the value is a requirement though IMO
63ca987
to
daae9ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a note about being subject to change otherwise LGTM
906a5ba
to
95bbe01
Compare
serviceAccountIssuers tracks history of used service account issuers. The item without expiration time represents the currently used service account issuer. The other items represents service account issuers that were used previously and are still being trusted. See: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-token-volume-projection xref: https://issues.redhat.com/browse/AUTH-309
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, mfojtik 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:
Approvers can indicate their approval by writing |
/label cherry-pick-approved |
/label qe-approved |
/label docs-approved |
documentation team had been contacted and there will be need to update product documentation once the change is implemented. |
/refresh |
/cherrypick release-4.11 |
@mfojtik: once the present PR merges, I will cherry-pick it on top of release-4.11 in a new PR and assign it to you. In 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 kubernetes/test-infra repository. |
@mfojtik: 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. |
@mfojtik: #1313 failed to apply on top of branch "release-4.11":
In 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 kubernetes/test-infra repository. |
@mfojtik: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-830 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
@mfojtik: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-2158 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
According to the Kubernetes documentation, starting from Kubernetes 1.22, the service-account-issuer flag can be specified multiple times. The first value is then used to generate new tokens and other values are accepted. Using this field can prevent cluster disruptions and allows for smoother reconfiguration of this field.
see: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-token-volume-projection
The status field will allow us to keep track of "used" service account issuers and also expire/prune them.
this is a replacement for: #1309
xref: https://issues.redhat.com/browse/AUTH-309