fix node lifecycle controller panic when conditionType ready is nil

[fusida]

What type of PR is this?

/kind bug

What this PR does / why we need it:

there are some network plugins and operators that will patch the node conditions, if they use merge-patch by mistake may cause the node conditions to be replaced unexpectedly.
when node .status.conditions[].ready is not exist, the node lifecycle controller may panic.
the kube controller manager error:

E0119 17:48:21.362092       1 runtime.go:78] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 8148 [running]:
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.logPanic(0x41ed3c0, 0x74f3f20)
  /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0x95
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
  /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0x86
panic(0x41ed3c0, 0x74f3f20)
  /usr/local/go/src/runtime/panic.go:965 +0x1b9
k8s.io/kubernetes/pkg/controller/nodelifecycle.(*Controller).doNoExecuteTaintingPass.func1(0xc0ae9a9920, 0x19, 0x3eac400, 0xc0c17d2140, 0xc162af370d35209e, 0x11fa57b3b4c, 0x75333e0, 0xc162af370d35209e, 0x11fa57b3b4c,0x75333e0, ...)
  /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/controller/nodelifecycle/node_lifecycle_controller.go:685 +0x2e3
k8s.io/kubernetes/pkg/controller/nodelifecycle/scheduler.(*RateLimitedTimedQueue).Try(0xc06c39b3c0, 0xc038623dd8)
  /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/controller/nodelifecycle/scheduler/rate_limited_queue.go:249 +0x255
k8s.io/kubernetes/pkg/controller/nodelifecycle.(*Controller).doNoExecuteTaintingPass(0xc00162c900)
  /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/controller/nodelifecycle/node_lifecycle_controller.go:671 +0x13a
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0xc0c4f7e250)

Which issue(s) this PR fixes:

Fixes #
condition may be nil and condition.Status will panic:

 _, condition := controllerutil.GetNodeCondition(&node.Status, v1.NodeReady)
// Because we want to mimic NodeStatus.Condition["Ready"] we make "unreachable" and "not ready" taints mutually exclusive.
taintToAdd := v1.Taint{}
oppositeTaint := v1.Taint{}
switch condition.Status { 

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix node lifecycle controller panic when conditionType ready is been patch nil by mistake

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @fusida. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

[kannon92]

/ok-to-test

Can you add a unit test or integration test that covers this bug? I expect it to not have any issues now.

[kannon92]

Sounds like a bug.

/triage accepted
/priority important-longterm

[kannon92]

/priority important-soon

[kannon92]

Can you share which k8s version you got this in? And does this look to be an regression?

[fusida]

Can you share which k8s version you got this in? And does this look to be an regression?

it happened 1.22.15, but he code still unchange in 1.28

patch node conditions ready nil, and stop kubelet report lease, then the kcm set node notready will meet this problem

[fusida]

/remove-area apiserver
/remove-area cloudprovider
/remove-area code-generation
/remove-area dependency
/remove-area kube-proxy
/remove-area kubeadm
/remove-area kubectl

[k8s-ci-robot]

@fusida: Those labels are not set on the issue: area/codegeneration

In response to this:

/remove-area apiserver
/remove-area cloudprovider
/remove-area codegeneration
/remove-area dependency
/remove-area kube-proxy
/remove-area kubeadm
/remove-area kubectl

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.

[fusida]

/remove-area code-generation

[fusida]

/retest

[fusida]

@fusida please remove the second commit, is unrelated and the same function is used with the same signature in staging/src/k8s.io/component-helpers/node/util/conditions.go

You can do a follow up and try to consolidate these functions, but that will require other reviewers I prefer to not mix things here, changing function signatures

I have removed the second commit.

[aojea]

/lgtm
/approve
@fusida you need to fix the release note, it seems you have added it to the other existing block

/assign @liggitt

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 9869f19b0ec8fa8f8a52dec0b39f066aa8a8864e

[fusida]

@aojea done

[liggitt]

it's definitely good not to panic, but is retrying correct, or should the absence of the Ready condition be treated as presence with Unknown status?

[aojea]

My understanding is that it should never be nil and it happens because an external force action, then other controller in the kubelet should reconcile it, so retrying waiting for the kubelet (IIRC) seems a good option

[liggitt]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, fusida, liggitt

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:

• pkg/controller/nodelifecycle/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sftim]

Changelog suggestion

-fix node lifecycle controller panic when conditionType ready is been patch nil by mistake
+Fixed a panic in the node lifecycle controller when a `Ready` condition was incorrectly patched to be null.