Bug 1906880: operator: remove odicdiscoveryendpoint controller #276
Conversation
|
fyi @deads2k |
|
@joelddiaz I guess the two options are 1) this PR or 2) we disable the controller for 4.7 and re-enable for 4.8 @marun did say that 4.8 should be able to handle this gracefully. However, my understanding is that it would require a complete restart of the cluster. Thus 4.8 cluster install time would regress if we didn't set the |
My understanding is that for 4.8 the goal is to remove the requirement to require a restart on issuer change. The apiserver would be updated to support multiple issuers to ensure that previously issued tokens would continue to be accepted until expiry. That would allow an issuer change without risk of breakage. cc: @stlaz |
|
@dgoodwin i'm fine with the PR (even after 4.8 gains the ability to gracefully handle the issuer change). With STS we will already require that these pieces be set up pre-installation anyway. WDYT? |
|
I largely have to defer to you folks, but if you and Seth are on board then no objections from me, I'd say ship it. |
I think the TL;DR for what this means is that using the AWS pod identity stuff will require some extra steps going forward as CCO will not be performing this one step. |
|
/cc @derekwaynecarr |
|
/lgtm We need a bug though and will need a release note, cc @jeana-redhat @sjenning could you fire a draft note into this PR or maybe the bug once it exists that Jeana can work with? Or I can give it a shot. |
openshift-ci-robot
added
lgtm
|
as discussed: upgrade from 4.6 -> 4.7 is not impacted /lgtm |
|
so this will be a known issue in the 4.7 RNs? just keep me posted 👍
@derekwaynecarr do you mean internal docs or user docs? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, dgoodwin, sjenning 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
What about upgrade from 4.5 to 4.6? Or is this not enabled by default for 4.6? |
sjenning
changed the title
openshift-ci-robot
added
bugzilla/severity-high
|
@sjenning: This pull request references Bugzilla bug 1906880, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
@marun It is enabled by default in 4.6. I think we should backport this to 4.6 z-stream asap. That way new 4.6 clusters and clusters upgrading from 4.5 to latest 4.6.z have the same behavior as 4.7 clusters. It will only be clusters that have already installed/upgraded to a 4.6.0-4.6.9ish that will have |
|
@sjenning: All pull requests linked via external trackers have merged: Bugzilla bug 1906880 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. |
Can we put in 4.6 an update to this controller to undo the changes back to the internal issuer URL? (so that all clusters would have the internal issuer?) |
|
@joelddiaz we can't do that without causing the issue we are avoiding here; invalidating all the TokenRequest API acquired tokens in the the cluster. |
|
I now understand that it's too late for this, and that we're only finding out now because the first bound token users have appeared. |
|
/cherry-pick release-4.6 |
|
@sjenning: #276 failed to apply on top of branch "release-4.6": 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. |
Since the last external consumer was removed in 6ea240c (operator: remove odicdiscoveryendpoint controller, 2020-12-08, openshift#276), we no longer need to make this helper part of the package's public API. Make it internal so folks are less likely to abuse it. Machines are generally supposed to avoid looking at the message property anyway [1], so we only expect to care about it for things like unit tests. [1]: https://github.com/openshift/api/blob/be1be0e89115702f8b508d351c4f5c9a16e5ae95/config/v1/types_cluster_operator.go#L136
implements https://issues.redhat.com/browse/CO-1317
@marun @dgoodwin @joelddiaz