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.

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

Kubeadm: check health status of all control plane components in wait-control-plane phase for inti #119598

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 26 additions & 2 deletions cmd/kubeadm/app/cmd/phases/init/waitcontrolplane.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ import (
clientset "k8s.io/client-go/kubernetes"

"k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow"
"k8s.io/kubernetes/cmd/kubeadm/app/features"
"k8s.io/kubernetes/cmd/kubeadm/app/phases/controlplane"

"k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient"
dryrunutil "k8s.io/kubernetes/cmd/kubeadm/app/util/dryrun"
)
Expand Down Expand Up @@ -93,6 +96,28 @@ func runWaitControlPlanePhase(c workflow.RunData) error {
" from directory %q\n",
data.ManifestDir())

waitForControlPlaneComponentsFunc := func() error {
waiter.SetTimeout(data.Cfg().Timeouts.ControlPlaneComponentHealthCheck.Duration)
return waiter.WaitForAPI()
}

clusterConfig := data.Cfg().ClusterConfiguration
if features.Enabled(clusterConfig.FeatureGates, features.WaitForAllControlPlaneComponents) {
waitForControlPlaneComponentsFunc = func() error {
if data.DryRun() {
fmt.Println("[dryrun] Would wait for the control plane components to be ready")
return nil
}

return controlplane.WaitForControlPlaneComponents(
controlplane.ControlPlaneComponents,
data.Cfg().Timeouts.ControlPlaneComponentHealthCheck.Duration,
data.ManifestDir(),
data.KubeletDir(),
clusterConfig.CertificatesDir)
}
}

handleError := func(err error) error {
context := struct {
Error string
Expand All @@ -111,8 +136,7 @@ func runWaitControlPlanePhase(c workflow.RunData) error {
return handleError(err)
}

waiter.SetTimeout(data.Cfg().Timeouts.ControlPlaneComponentHealthCheck.Duration)
if err := waiter.WaitForAPI(); err != nil {
if err := waitForControlPlaneComponentsFunc(); err != nil {
return handleError(err)
}

Expand Down
44 changes: 44 additions & 0 deletions cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow"
cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
"k8s.io/kubernetes/cmd/kubeadm/app/features"
"k8s.io/kubernetes/cmd/kubeadm/app/phases/controlplane"
etcdphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/etcd"
markcontrolplanephase "k8s.io/kubernetes/cmd/kubeadm/app/phases/markcontrolplane"
etcdutil "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd"
Expand Down Expand Up @@ -71,6 +73,7 @@ func NewControlPlaneJoinPhase() workflow.Phase {
newEtcdLocalSubphase(),
newUpdateStatusSubphase(),
newMarkControlPlaneSubphase(),
newWaitControlPlaneComponentsSubphase(),
},
}
}
Expand Down Expand Up @@ -108,6 +111,14 @@ func newMarkControlPlaneSubphase() workflow.Phase {
}
}

func newWaitControlPlaneComponentsSubphase() workflow.Phase {
return workflow.Phase{
Name: "wait-control-plane-components",
Hidden: true,
Copy link
Member

Choose a reason for hiding this comment

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

a new hidden phases seems ok.
i guess on join we are not calling WaitForAPI or have a dedicated phase to wait for the apiserver to come up?

Copy link
Author

Choose a reason for hiding this comment

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

a new hidden phases seems ok. i guess on join we are not calling WaitForAPI or have a dedicated phase to wait for the apiserver to come up?

Do you mean we need to add legacy WaitForAPI code when this feature gate is disabled in this hidden phase

Copy link
Member

Choose a reason for hiding this comment

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

no, i'm just trying to remember what happens on join.
i guess we are not even calling WaitForAPI on join, so adding the new hidden phase seems OK.
later if we decide we can make it non-hidden.

Run: runWaitControlPlaneComponentsPhase,
}
}

func runEtcdPhase(c workflow.RunData) error {
data, ok := c.(JoinData)
if !ok {
Expand Down Expand Up @@ -202,3 +213,36 @@ func runMarkControlPlanePhase(c workflow.RunData) error {

return nil
}

func runWaitControlPlaneComponentsPhase(c workflow.RunData) error {
data, ok := c.(JoinData)
if !ok {
return errors.New("control-plane-join phase invoked with an invalid data struct")
}

if data.Cfg().ControlPlane == nil {
return nil
}

initCfg, err := data.InitCfg()
if err != nil {
return err
}

if features.Enabled(initCfg.FeatureGates, features.WaitForAllControlPlaneComponents) {
fmt.Println("[wait-control-plane] Waiting for the kubelet to boot up control plane components")
timeout := initCfg.Timeouts.ControlPlaneComponentHealthCheck.Duration
err := controlplane.WaitForControlPlaneComponents(
controlplane.ControlPlaneComponents,
timeout,
data.ManifestDir(),
data.KubeletDir(),
initCfg.CertificatesDir)
if err != nil {
return err
}

}

return nil
}
3 changes: 3 additions & 0 deletions cmd/kubeadm/app/features/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const (
EtcdLearnerMode = "EtcdLearnerMode"
// UpgradeAddonsBeforeControlPlane is expected to be in deprecated in v1.28 and will be removed in future release
UpgradeAddonsBeforeControlPlane = "UpgradeAddonsBeforeControlPlane"
// WaitForAllControlPlaneComponents is expected to be in v1.29
WaitForAllControlPlaneComponents = "WaitForAllControlPlaneComponents"
)

// InitFeatureGates are the default feature gates for the init command
Expand All @@ -52,6 +54,7 @@ var InitFeatureGates = FeatureList{
FeatureSpec: featuregate.FeatureSpec{Default: false, PreRelease: featuregate.Deprecated},
DeprecationMessage: "The UpgradeAddonsBeforeControlPlane feature gate is deprecated and will be removed in a future release.",
},
WaitForAllControlPlaneComponents: {FeatureSpec: featuregate.FeatureSpec{Default: false}},
}

// Feature represents a feature being gated
Expand Down
183 changes: 183 additions & 0 deletions cmd/kubeadm/app/phases/controlplane/components.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
/*
Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controlplane

import (
"context"
"encoding/json"
"fmt"
"io"
"os"
"path/filepath"
"time"

"github.com/pkg/errors"
"gopkg.in/yaml.v2"

v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/rest"
"k8s.io/klog/v2"
kubeletconfig "k8s.io/kubelet/config/v1beta1"

kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
"k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig"
"k8s.io/kubernetes/cmd/kubeadm/app/util/staticpod"
)

// ControlPlaneComponents contains all components in control plane
var ControlPlaneComponents = []string{
kubeadmconstants.KubeAPIServer,
kubeadmconstants.KubeControllerManager,
kubeadmconstants.KubeScheduler,
}
Comment on lines +42 to +47
Copy link
Member

Choose a reason for hiding this comment

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

let's make this a function instead that returns a private list:

func ControlPlaneComponents() []string {
...
}


type component struct {
name string
labels map[string]string
touched bool
}

// WaitForControlPlaneComponents wait for control plane component to be ready by check pod status returned by kubelet
func WaitForControlPlaneComponents(componentNames []string, timeout time.Duration, manifestDir, kubeletDir, certificatesDir string) error {
certFile := filepath.Join(certificatesDir, kubeadmconstants.APIServerKubeletClientCertName)
keyFile := filepath.Join(certificatesDir, kubeadmconstants.APIServerKubeletClientKeyName)
Comment on lines +57 to +58
Copy link
Member

Choose a reason for hiding this comment

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

can we use the cert and key for the kubelet client stored in /etc/kubernetes/kubelet.conf (they link to files under /var/lib/kubelet...)?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean file /var/lib/kubelet/pki/kubelet-client-current.pem ?
When I try to run cmd:

curl -k --cert /var/lib/kubelet/pki/kubelet-client-current.pem --key /var/lib/kubelet/pki/kubelet-client-current.pem https://127.0.0.1:10250/pods

got the following result:
Forbidden (user=system:node:localhost.localdomain, verb=get, resource=nodes, subresource=proxy)

By the way, why cert and key of apiserver-kubelet-client is not good to fetch pods info from kubelet?

Copy link
Member

Choose a reason for hiding this comment

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

i was wondering if we can just use the less privileged kubelet cert. apparently not. let's continue with the apiserver cert for now.

thanks


client, err := rest.HTTPClientFor(&rest.Config{
TLSClientConfig: rest.TLSClientConfig{
CertFile: certFile,
KeyFile: keyFile,
Insecure: true,
},
})
if err != nil {
return errors.Wrap(err, "failed to create kubelet client")
}

kubeletEndpoint, err := getKubeletEndpoint(filepath.Join(kubeletDir, kubeadmconstants.KubeletConfigurationFileName))
if err != nil {
return errors.Wrap(err, "failed to get kubelet endpoint")
}

components := make([]*component, len(componentNames))
for i, name := range componentNames {
labels, err := getComponentLabels(name, manifestDir)
if err != nil {
return errors.Wrapf(err, "failed to get pod labels of %s component", name)
}

components[i] = &component{name, labels, false}
}

return wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, false, func(ctx context.Context) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

can we use the same retry interval as the legacy waiter for API server:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/apiclient/wait.go#L82
or it's too short?

klog.V(1).Infoln("[control-plane] polling status of control plane components...")

resp, err := client.Get(kubeletEndpoint)
if err != nil {
fmt.Printf("[kubelet client] Error getting pods [%v]\n", err)
Copy link
Member

Choose a reason for hiding this comment

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

instead of printing these errors we can store the last error as a lastError and return it?

err := wait.PollUntilContextTimeout...

if err != nil {
    return lastError
}

return false, nil
}

defer resp.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

unless i'm mistaken the linter in this repo will complain that the return value is not checked.

Copy link
Author

@tommas1988 tommas1988 Feb 5, 2024

Choose a reason for hiding this comment

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

unless i'm mistaken the linter in this repo will complain that the return value is not checked.

Yes, the linter did complaint with this, but it is not a required check, and i also check some old code that did not check the returned err with closing a response body, so I just levea is as it is.

I will check this returned error in new commit


data, err := io.ReadAll(resp.Body)
if err != nil {
fmt.Printf("[kubelet client] Error reading pods from response body [%v]\n", err)
return false, nil
}

pods := &v1.PodList{}
if err := json.Unmarshal(data, pods); err != nil {
fmt.Printf("[kubelet client] Error parsing pods from response body: %q\n", data)
return false, nil
}

for _, comp := range components {
labels := comp.labels
match_pod:
for _, pod := range pods.Items {
podLabels := pod.ObjectMeta.Labels
for key, value := range labels {
if podLabels[key] != value {
continue match_pod
}
}

comp.touched = true
Copy link
Member

Choose a reason for hiding this comment

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

on a quick look it's not obvious why we the code is tracking labels and touched state. can you elaborate why status.Ready is not enough?

Copy link
Author

Choose a reason for hiding this comment

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

on a quick look it's not obvious why we the code is tracking labels and touched state. can you elaborate why status.Ready is not enough?

component.touched: bool is dealing with the case: when pods info returned from kubelet cannot be found with this component.labels.
I will add some comments on component struct

Copy link
Member

Choose a reason for hiding this comment

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

ok, i will check the comments later.
currently, i still don't get why we need to the labels and touched flags.


for _, status := range pod.Status.ContainerStatuses {
if !status.Ready {
klog.V(1).Infof("[control-plane] component: %s is not ready\n", comp.name)
return false, nil
}
}

klog.V(1).Infof("[control-plane] component: %s is ready\n", comp.name)
}
}

for _, comp := range components {
if !comp.touched {
fmt.Printf("[kubelet client] Couldn`t find pod for component: %s with labels: [%v]\n", comp.name, comp.labels)
return false, nil
}
}

return true, nil
})
}

func getKubeletEndpoint(configFile string) (string, error) {
config := &kubeletconfig.KubeletConfiguration{}

data, err := os.ReadFile(configFile)
if err != nil {
return "", err
}

if err := yaml.Unmarshal(data, config); err != nil {
return "", err
}

if config.Authorization.Mode == kubeletconfig.KubeletAuthorizationModeWebhook {
// make sure cluster admins role binding is created, thus request to kubelet will pass server authorization
if _, err := kubeconfig.EnsureAdminClusterRoleBinding(kubeadmconstants.KubernetesDir, nil); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

isn't the RB already created at this point?
it is one of the first things that happen during kubeadm init and for joining CP nodes it should be there already.

Copy link
Author

Choose a reason for hiding this comment

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

isn't the RB already created at this point? it is one of the first things that happen during kubeadm init and for joining CP nodes it should be there already.

This RB is created in function initData.Client() during kubeadm init, and initData.Client() is ever not called till WaitControlPlanePhase is reached

Copy link
Member

@neolit123 neolit123 Feb 4, 2024

Choose a reason for hiding this comment

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

if the CRB is required earlier perhaps we can call a _ = initData.Client() instead of calling kubeconfig.EnsureAdminClusterRoleBinding. that is because we want this Client() function to set the right vars / state. getKubeletEndpoint() should just return the endpoint and not perform CRB creation.

in runWaitControlPlanePhase there is a client, err := data.ClientWithoutBootstrap(), but this client doesn't have permissions.

but, in this discussion we talked about using the apiserver client cert/key
https://github.com/kubernetes/kubernetes/pull/119598/files#r1430504568
and the code in this PR is still using it. i'm not sure i understand why we need to bootstrap the admin.conf client?

i.e.

// make sure cluster admins role binding is created, thus request to kubelet will pass server authorization

kubeconfig.EnsureAdminClusterRoleBinding() ensures that the kubeadm:cluster-admins group (which admin.conf is part of) is grated the cluster-admin privilege. it seems unrelated to getting the pods with the apiserver client cert/key. that user should already be able to get the pods from the kubelet, no?

Copy link
Author

Choose a reason for hiding this comment

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

but, in this discussion we talked about using the apiserver client cert/key
https://github.com/kubernetes/kubernetes/pull/119598/files#r1430504568
and the code in this PR is still using it. i'm not sure i understand why we need to bootstrap the admin.conf client?

previously waitconrolpane,go use data.Client() to get client which is changed by this commit.

kubeconfig.EnsureAdminClusterRoleBinding() ensures that the kubeadm:cluster-admins group (which admin.conf is part of) is grated the cluster-admin privilege. it seems unrelated to getting the pods with the apiserver client cert/key. that user should already be able to get the pods from the kubelet, no?

kubelet server will install auth filter with Authorization configuration, and when Authorization.Mode = Webhook (which is default), kubelet server will create SubjectAccessReview to authorize requests to kubelet.

if the CRB is required earlier perhaps we can call a _ = initData.Client() instead of calling kubeconfig.EnsureAdminClusterRoleBinding. that is because we want this Client() function to set the right vars / state. getKubeletEndpoint() should just return the endpoint and not perform CRB creation.

Why put this code in getKubeletEndpoint() is only use _ = initData.Client() is not clear why request to kubelet will be failed without kubeadm:cluster-admins RB is created (I did spend some time to find why request to kubelet return athorize error).

Now, I need to revert client, err := data.ClientWithoutBootstrap() to client, err := data.Client() or _ = data.Client() when this feature gate is enabled in waitcontrolplane?

Copy link
Member

Choose a reason for hiding this comment

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

Now, I need to revert client, err := data.ClientWithoutBootstrap() to client, err := data.Client() or _ = data.Client() when this feature gate is enabled in waitcontrolplane?

there might be a "chicken and egg" problem, because data.ClientWithoutBootstrap was used because data.Client() does not work yet. i.e. the CRB cannot be performed due to apiserver pod not ready. you can try it, of course.

kubelet server will install auth filter with Authorization configuration, and when Authorization.Mode = Webhook (which is default), kubelet server will create SubjectAccessReview to authorize requests to kubelet.

i see, so it's because of this:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/certs/certlist.go#L294
apiserver-kubelet-client needs the admin credentials, to get pods or do other actions.

if it's possible to create the Client() earlier that might be best, but if it's not, perhaps we can:

  • during init use the credentials from super-admin.conf
  • during join just use admin.conf or apiserver-kubelet-client. at that point the CRB should already exist.

return "", err
}
}

kubeletPort := config.Port
if kubeletPort == 0 {
kubeletPort = kubeadmconstants.KubeletPort
}

return fmt.Sprintf("https://127.0.0.1:%d/pods", kubeletPort), nil
}

func getComponentLabels(component string, manifestDir string) (map[string]string, error) {
pod, err := staticpod.ReadStaticPodFromDisk(kubeadmconstants.GetStaticPodFilepath(component, manifestDir))
if err != nil {
return nil, err
}

labels := pod.ObjectMeta.Labels
if labels == nil {
return nil, errors.New("Empty labels")
}

return labels, nil
}