-
Notifications
You must be signed in to change notification settings - Fork 40.1k
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
Fix to previous EnsureAdminClusterRoleBindingImpl fix #122906
Fix to previous EnsureAdminClusterRoleBindingImpl fix #122906
Conversation
The previous fix changed the behavior of EnsureAdminClusterRoleBindingImpl under the assumption that the unit test was correct and the real-world behavior was wrong, but in fact, the real-world behavior was already correct, and the unit test was expecting the wrong result because of the difference in behavior between real and fake clients.
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
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.
lgtm
The fix seems valid to me as well
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, pacoxu 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 |
#122901 is another way to fix it(not sure which is better) |
/lgtm To make the CI green. /assign @neolit123 @SataQiu |
LGTM label has been added. Git tree hash: 6ffb09c05032df0a7d48aa1623fd16480f23fd22
|
What type of PR is this?
/kind bug
/kind cleanup
/kind failing-test
What this PR does / why we need it:
Fix to #122893; The previous fix changed the behavior of
EnsureAdminClusterRoleBindingImpl
under the assumption that the unit test was correct and the real-world behavior was wrong, but in fact, the real-world behavior was already correct, and the unit test was expecting the wrong result because of the difference in behavior between real and fake clients.Comparing the original state, the post-122893 state, and the new state with this commit:
Create()
succeeds:crbResult
would contain the result of theCreate
(line 640), and socrbResult != nil
would be true and we'dreturn adminClient, nil
(line 672-673)crbExists
would be set totrue
(line 661), socrbExists
would be true and we'd returnadminClient, nil
(line 669-670)Create()
fails withapierrors.IsForbidden()
crbResult
would get set tonil
(line 653), socrbResult != nil
would be false (line 672) and we'd try thesuperAdminClient
crbClient
gets set tonil
on line 648crbExists
never gets set totrue
, socrbExists
would befalse
on line 669 and we'd try thesuperAdminClient
Create()
fails withapierrors.IsAlreadyExists()
crbResult
is nil, so thecrbResult != nil
check at line 672 fails so we fall through to maybe try with thesuperAdminClient
, which will fail whether or notsuperAdminClient
is set; This is not the correct behavior, but the"admin.conf: CRB already exists, use the admin.conf client"
test hadexpectedError: true
anyway! 🙁crbResult
is an empty CRB object, so thecrbResult != nil
check at line 672 succeeds and we return theadminClient
, which is what was supposed to happencrbResult
gets set to nil (line 648), so we fall through at line 671 rather than returning theadminClient
. This caused kubeadm-kinder-upgrade-*-1-29-latest failed for existing ClusterRoleBinding kubeadm#3000crbExists
gets set to true (line 653), so the check at line 669 succeeds and we return theadminClient
, fixing the bugCreate()
fails with any other error and eventually times out:crbResult
isnil
in CI and non-nil
in the real world, but it doesn't matter because, we see thaterr != nil
and returnlastError
(line 667-668)crbResult
isnil
either way, but again, we returnlastError
before checking itcrbExists
isfasl
either way, but again, we returnlastError
before checking itSo I think this should be correct...
Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#3000
Does this PR introduce a user-facing change?
/assign @neolit123 @pacoxu