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.

Sign up for GitHub

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

Jump to bottom

Fix EnsureAdminClusterRoleBindingImpl error handling #122893

Merged
merged 1 commit into from
Jan 21, 2024
Merged

Fix EnsureAdminClusterRoleBindingImpl error handling #122893

merged 1 commit into from
Jan 21, 2024

Conversation

danwinship
Copy link
Contributor

@danwinship danwinship commented Jan 20, 2024 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

EnsureAdminClusterRoleBindingImpl assumed that the client Create() call would return nil on error, but that's not how the real clients work; only the fake clients work that way. 😖

Noticed while trying to fix that fact, #122892 (which this PR blocks). With fake client code that uses the real client semantics, the unit tests fail with:

--- FAIL: TestEnsureAdminClusterRoleBindingImpl (0.00s)
    --- FAIL: TestEnsureAdminClusterRoleBindingImpl/admin.conf:_CRB_already_exists,_use_the_admin.conf_client (0.00s)
        kubeconfig_test.go:938: expected error: true, got false, error: <nil>
FAIL
FAIL	k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig	0.103s
FAIL

FWIW the unit test probably would have caught this bug in one of the other sub-tests if it checked the error text rather than just checking "any error" vs "no error"... (Though I recognize that people have differing opinions on whether unit tests should check for specific error text...)

Which issue(s) this PR fixes:

none

Does this PR introduce a user-facing change?

probably? Also, the code is new in 1.29 but I don't know if that makes it /kind regression or if it's a bug in a new feature

/assign @neolit123

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 20, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 20, 2024
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 20, 2024
neolit123
neolit123 reviewed Jan 20, 2024
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

EnsureAdminClusterRoleBindingImpl assumed that the client Create() call would return nil on error, but that's not how the real clients work; only the fake clients work that way. 😖

yikes.

probably? Also, the code is new in 1.29 but I don't know if that makes it /kind regression or if it's a bug in a new feature

i think we can go without a release note, but this seems like it should be backported to 1.29. it's new code in 1.29, so no /kind regression. please send the cherry pick if you could, i will be OOO but i can lgtm/approve,

FWIW the unit test probably would have caught this bug in one of the other sub-tests if it checked the error text rather than just checking "any error" vs "no error"... (Though I recognize that people have differing opinions on whether unit tests should check for specific error text...)

returning always nil on error seems fine to me.
no unit tests updates are needed here in this PR?

/approve

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go
@@ -642,6 +642,7 @@ func EnsureAdminClusterRoleBindingImpl(ctx context.Context, adminClient, superAd
clusterRoleBinding,
metav1.CreateOptions{},
); err != nil {
crbResult = nil
Copy link
Member

@neolit123 neolit123 Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this almost feels like a bug or a "miss-design" in client-go.
in go stdlib commonly if err != nil, everything else is nil or 0 value.

returning a zeroed struct is not so common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah; I would have changed the real clients to return nil rather than changing the fake clients to return non-nil but that would presumably break real code...

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, neolit123

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2024
@neolit123
Copy link
Member

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 20, 2024
@danwinship
Fix EnsureAdminClusterRoleBindingImpl error handling
b18caee 
The code assumed Create() returned nil on error, but that's only true
for the fake clients in unit tests.
@danwinship danwinship force-pushed the kubeadm-rolebinding-failure branch from c34349b to b18caee Compare January 21, 2024 15:54
@danwinship
Copy link
Contributor Author

danwinship commented Jan 21, 2024 •
edited
Loading

no unit tests updates are needed here in this PR?

No, because we're just going from "crbResult is nil because FakeClient.Create() returned nil on error" to "crbResult is nil because we explicitly set it to nil ourselves (and also FakeClient.Create() returned nil anyway but we're pretending it didn't)".

@danwinship danwinship mentioned this pull request Jan 21, 2024
Merged
neolit123
neolit123 reviewed Jan 21, 2024
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2f65984294686bd80386d1f0aee44ab003832fdf

@k8s-ci-robot k8s-ci-robot merged commit a07b1aa into kubernetes:master Jan 21, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Jan 21, 2024
@danwinship danwinship deleted the kubeadm-rolebinding-failure branch January 21, 2024 23:05
@danwinship danwinship mentioned this pull request Jan 21, 2024
Closed
This was referenced Jan 22, 2024
Closed
Closed
Merged
@danwinship danwinship mentioned this pull request Jan 22, 2024
Merged
@npolshakova npolshakova mentioned this pull request Mar 27, 2024
Closed
@npolshakova npolshakova mentioned this pull request Sep 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@SataQiu SataQiu Awaiting requested review from SataQiu

Assignees

@neolit123 neolit123

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.
3 participants
@danwinship @k8s-ci-robot @neolit123