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

OSDOCS-4272: Updating for service account issuer behavior change #51791

Merged
merged 1 commit into from Oct 31, 2022

Conversation

bergerhoffer
Copy link
Contributor

@bergerhoffer bergerhoffer commented Oct 18, 2022

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 18, 2022
@bergerhoffer bergerhoffer added this to the Continuous Release milestone Oct 18, 2022
@bergerhoffer
Copy link
Contributor Author

Note: this PR is probably going to need to be cherry picked carefully - so that it makes it to 4.9/4.10/4.11 at the same time that the code change makes it in to the applicable z-stream

@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Oct 18, 2022

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

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

@bergerhoffer
Copy link
Contributor Author

bergerhoffer commented Oct 18, 2022

@mfojtik @stlaz Here's the PR at least for the part, updating this procedure to say the old issuer is valid for 24 hours, and removing the steps to manually restart pods.

Let me know:

  1. If we want to add the procedure to instantly revoke into the product docs, and
  2. Which 4.9, 4.10, and 4.11 z-stream this change will be making it into, so that we can make sure it gets mentioned in each of the z-stream release notes. Do we have separate bugs/Jiras for each of these versions?

Preview: https://51791--docspreview.netlify.app/openshift-enterprise/latest/authentication/bound-service-account-tokens.html#bound-sa-tokens-configuring_bound-service-account-tokens

====
If you update the `serviceAccountIssuer` field and there are bound tokens already in use, all bound tokens with the previous issuer value will be invalidated. Unless the holder of a bound token has explicit support for a change in issuer, the holder will not request a new bound token until pods have been restarted.

If necessary, you can manually restart all pods in the cluster so that the holder will request a new bound token. Before doing this, wait for a new revision of the Kubernetes API server pods to roll out with your service account issuer changes.
Copy link
Member

Choose a reason for hiding this comment

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

If necessary, you can manually restart all pods in the cluster so that the holder will request a new bound token. Before doing this, wait for a new revision of the Kubernetes API server pods to roll out with your service account issuer changes.

I would keep this section here to ensure we indicate that workloads that use bound tokens might need to restart to pick up a new bound token.

/cc @stlaz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfojtik I thought you had said that we didn't need the steps to manually restart all pods? Are you saying you want me to keep this statement in the note, and also bring back those steps in the procedure?

Copy link
Member

Choose a reason for hiding this comment

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

@bergerhoffer "if necessary" is the culprit here... we want to make sure that if somebody runs a workload that uses a bound token for something, that workload is aware the service account changed. If the workload is smart, it always reads it from the disk in a container (and it will be updated there)... if the workload is not smart, they read it into memory and it might need a restart to update.

with this sentence, I want to ensure that we cover the "not so-smart" workloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Sentence + steps put back as requested :)

@openshift-ci openshift-ci bot requested a review from stlaz October 20, 2022 11:47
@openshift-ci openshift-ci bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 20, 2022
@bergerhoffer
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Oct 20, 2022
@JoeAldinger
Copy link
Contributor

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

@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 Oct 20, 2022
Copy link
Contributor

@JoeAldinger JoeAldinger left a comment

Choose a reason for hiding this comment

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

/remove-label peer-review-in-progress
/label peer-review-done
/lgtm
@opayne1 , @Wiharris, and I will coordinate on getting this merged at the correct time. Tagged them so the RNs team is aware. Please thanks, all.

@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 Oct 20, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2022
@bergerhoffer
Copy link
Contributor Author

Thanks @JoeAldinger! I believe this will be making the z-streams next week (4.11.11 and 4.10.39, not sure about 4.9 yet). But I will confirm for sure as soon as I know!

@bergerhoffer
Copy link
Contributor Author

@JoeAldinger @opayne1 Okay, these all look to be making it for next week, so:

  • 4.11.11 on Monday 10/31
  • 4.10.39 on Tuesday 11/1
  • 4.9.51 on Wednesday 11/2

So we'll need to cherry pick this PR to the appropriate version on the appropriate day. And also will need to add a blurb to each of the z-stream release notes (blurb is in this gdoc).

I can check back in each of these days to make sure we're good, but let me know if there's anything else I can do. Thanks for your help with this!

@opayne1
Copy link
Contributor

opayne1 commented Oct 25, 2022

Thank you @bergerhoffer! We will keep you updated.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2022
@xingxingxia
Copy link
Contributor

/lgtm

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

Since the first z-stream is releasing today, I am merging this and will set up each CP with the date and z-stream PR it needs to be merged with.

@bergerhoffer bergerhoffer merged commit 7b78452 into openshift:main Oct 31, 2022
@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.12

@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.11

@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.10

@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.9

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #52295

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.

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #52296

In response to this:

/cherrypick enterprise-4.11

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-cherrypick-robot

@bergerhoffer: new pull request created: #52297

In response to this:

/cherrypick enterprise-4.10

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-cherrypick-robot

@bergerhoffer: new pull request created: #52298

In response to this:

/cherrypick enterprise-4.9

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.9 branch/enterprise-4.10 branch/enterprise-4.11 branch/enterprise-4.12 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants