ARO-13398, HOSTEDCP-1972: hypershift-operator: allow opting into pull secret rotation #5307
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@stevekuznetsov: This pull request references ARO-13398 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.19.0" version, but no target version was set. This pull request references HOSTEDCP-1972 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.19.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stevekuznetsov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
needs-ok-to-test
|
Hi @stevekuznetsov. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
stevekuznetsov
force-pushed
the
skuznets/watch-pull-secrets
branch
from
02ebd49 to
8f2bea6
Compare
Please note that this change won't trigger a rollout either as it is now. Only changing the ref will. So this PR will be no-op from ux pov atm. Can we include at commit in the PR to implement the inplace rollout? |
|
@enxebre ah right - I can update the hash in this PR as well - WDYT? Also could you please |
Unfortunately that's a no go because a non backward compatible change in how we build the hash would transparently cause a global rollout for the whole cluster fleet as soon as this change gets shipped in to a management cluster, which we don't want to. /ok-to-test |
openshift-ci
bot
added
ok-to-test
|
@enxebre of course - that's the reason I wanted a dual-sided accept. New field in |
stevekuznetsov
force-pushed
the
skuznets/watch-pull-secrets
branch
2 times, most recently
from
ad3f5d0 to
48e2919
Compare
| description: |- | ||
| Name of the referent. | ||
| This field is effectively required, but due to backwards compatibility is | ||
| allowed to be empty. Instances of this type with an empty value here are |
There was a problem hiding this comment.
This is technically backwards incompat as well, unclear - would have been invalid anyway?
There was a problem hiding this comment.
(I couldn't embed corev1.LocalObjectReference as it somehow broke the applyconfiggenerator)
| @@ -373,11 +373,16 @@ func (r *NodePoolReconciler) token(ctx context.Context, hcluster *hyperv1.Hosted | |||
| return nil, fmt.Errorf("failed to look up release image metadata: %w", err) | |||
| } | |||
|
|
|||
| pullSecret, err := r.getPullSecret(ctx, hcluster) | |||
There was a problem hiding this comment.
The sheer number of times we do lookups for all sorts of data in this reconciler even when a parent func has the context is pretty wild. This all hits listers so it's pretty cheap but it certainly makes it hard to grok the data flow and introduces lots of opportunities for races, incoherent cache access, etc
openshift-ci
bot
added
area/cli
stevekuznetsov
force-pushed
the
skuznets/watch-pull-secrets
branch
from
48e2919 to
e9a8974
Compare
|
I think I need another |
|
Nvm, was just impatient for Prow |
stevekuznetsov
force-pushed
the
skuznets/watch-pull-secrets
branch
from
e9a8974 to
70137b6
Compare
|
e2e failing with That's my new condition ... how was this working in the past with invalid pull credentials? huh? |
stevekuznetsov
force-pushed
the
skuznets/watch-pull-secrets
branch
from
70137b6 to
0c446f4
Compare
There is no technical reason that we cannot react to changes to referenced secrets for pull credentials; allow users to opt into this behavior by labelling their secret. Signed-off-by: Steve Kuznetsov <[email protected]>
stevekuznetsov
force-pushed
the
skuznets/watch-pull-secrets
branch
from
0c446f4 to
28208f3
Compare
|
@enxebre ready for review, the previous e2e was a known flake |
|
@stevekuznetsov: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
There is no technical reason that we cannot react to changes to referenced secrets for pull credentials; allow users to opt into this behavior by labelling their secret.
A follow-up will make this change not cause node rollout.