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

Implement a canonical way for getting the node name #1098

Closed
fabriziopandini opened this issue Sep 6, 2018 · 25 comments · Fixed by kubernetes/kubernetes#128457
Closed

Implement a canonical way for getting the node name #1098

fabriziopandini opened this issue Sep 6, 2018 · 25 comments · Fixed by kubernetes/kubernetes#128457
Labels
area/HA area/upgrades kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Milestone

Comments

@fabriziopandini
Copy link
Member

fabriziopandini commented Sep 6, 2018

After kubernetes/kubernetes#67944 kubeadm retrives NodeRegistration attributes directly from the node.

The solution implemented for getting the node name uses the authentication info contained in the kubelet.conf file; however in future we want to switch to a more canonical way for doing this e.g. by reading those information from the local copy of the kubelet component config (that should be amended with the node name beforehand 😉)

rif code


EDIT: neolit123 another problem here is that the current error is misleading:

unable to fetch the kubeadm-config ConfigMap: failed to get node registration: failed to get corresponding node: nodes "foo" not found

we should be more descriptive of what is going on; we are parsing the client cert common name and it must be in the system:node:foo format:

openssl x509 -in /var/lib/kubelet/pki/kubelet-client-current.pem  -text -noout | grep ", CN ="
        Subject: O = system:nodes, CN = system:node:controlplane
@fabriziopandini fabriziopandini added area/HA area/upgrades priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 6, 2018
@fabriziopandini fabriziopandini added this to the v1.13 milestone Sep 6, 2018
@fabriziopandini fabriziopandini changed the title Implement a more more canonical way for getting the node name Implement a canonical way for getting the node name Sep 6, 2018
@neolit123 neolit123 modified the milestones: v1.13, v1.14 Nov 21, 2018
@MalloZup
Copy link

MalloZup commented Jan 17, 2019

@fabriziopandini I can have look on this, it is OK for you?

@yagonobre
Copy link
Member

Thanks @MalloZup
/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jan 18, 2019
@neolit123 neolit123 removed the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Feb 3, 2019
@timothysc timothysc modified the milestones: v1.14, Next Feb 7, 2019
@timothysc timothysc added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Feb 7, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 8, 2019
@neolit123 neolit123 added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 8, 2019
@ezzoueidi
Copy link

@MalloZup are you still working on this? If not, I would like to give it a try. :)

@neolit123
Copy link
Member

are you still working on this? If not, I would like to give it a try. :)

@nzoueidi
the issue is not assigned, please go ahead if you'd like.

@xlgao-zju
Copy link

by reading those information from the local copy of the kubelet component config

what if the user do not use the kubelet component config...

@neolit123
Copy link
Member

kubeadm always writes the config file.
in the future the kubelet will only remain with a --config flag and all other flags will be removed, so even users outside kubeadm will be forced to use the config.

@xlgao-zju
Copy link

so even users outside kubeadm will be forced to use the config.

@neolit123 is there any roadmap of this?

@neolit123
Copy link
Member

there is this tracking issue:
kubernetes/kubernetes#86843

@xlgao-zju
Copy link

by reading those information from the local copy of the kubelet component config (that should be amended with the node name beforehand 😉)

I think we need to know the node name at first, then we can amend the copy of kubelet component config with the node name... os, it seems we can not get the node name form kubelet component config, am I right?

@neolit123
Copy link
Member

neolit123 commented Jun 23, 2020

i'd agree.

maybe this should be considered the canonical way:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/phases/join/kubelet.go#L130

@xlgao-zju
Copy link

xlgao-zju commented Jun 24, 2020

this func ( https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/config/cluster.go#L136 ) is mainly used by https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/config/cluster.go#L64 ,and this func will build the NodeRegistration, and will get the name of node from kubelet.config.

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/phases/join/kubelet.go#L130 will use value from the NodeRegistration.

os, is there any other way to get the name of node, besides from parsing the content of kubelet.config?

@neolit123
Copy link
Member

the sequence of events feels wrong in the existing kubeadm logic.

i think the key here is that we need to construct a NodeRegistrationOptions object during upgrades:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/config/cluster.go#L109-L110

kubeadm upgrade should not be doing that.
but i lack the full context here.

@xlgao-zju
Copy link

seems this is used in other cases too, besides kubeadm upgrade:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/config/cluster.go#L64

maybe we should store the node name somewhere. then when we need the node name, we get it directly.

@neolit123
Copy link
Member

neolit123 commented Jun 29, 2020

seems this is used in other cases too, besides kubeadm upgrade:

our logic around node name still feels wrong, because if a "configuration" is fetched from the cluster, then this should be the ClusterConfiguration object (not InitConfiguration), which does not include the node name.

maybe we should store the node name somewhere. then when we need the node name, we get it directly.

one way of doing that would be for kubeadm to write the node name in a file once init / join finishes, but this adds more disk state which is overall undesired.

instead of adding something like that i think we should narrow down / enumerate the cases when a node name is actually needed and also long-term make ClusterConfiguration not include ComponentConfigs and InitConfiguration not include ClusterConfiguration.
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/types.go#L41
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/types.go#L70

@neolit123 neolit123 added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Jun 4, 2024
@kfox1111
Copy link

Was working with using spire for node attestation for kubelet -> kube-apiserver.

Ran into this issue as part of it. doing a kubeadm upgrade plan, it chokes on not being able to find the node name out of the certs in the kubelet kubeconfig, because I'm not using an x509 plugin to authenticate.

Would it be possible to make a command line flag to pass the node name in explicitly?

@kfox1111
Copy link

kfox1111 commented Oct 24, 2024

Or, maybe it could just use the file to grab a token, then use token introspection on it to get the hostname?

kubectl auth whoami -o json

@neolit123
Copy link
Member

neolit123 commented Oct 24, 2024

Ran into this issue as part of it. doing a kubeadm upgrade plan, it chokes on not being able to find the node name out of the certs in the kubelet kubeconfig, because I'm not using an x509 plugin to authenticate.

sorry can you clarify what you mean by "not using an x509 plugin", how is the kubelet authenticating to the apiserver exactly and does it not have a client cert on disk?

Would it be possible to make a command line flag to pass the node name in explicitly?

adding more flags is not ideal.

Or, maybe it could just use the file to grab a token, then use token introspection on it to get the hostname?
kubectl auth whoami -o json

as in setting the active KUBECONFIG to the kubelet client like so:
sudo kubectl auth whoami -o json --kubeconfig /etc/kubernetes/kubelet.conf

and then parsing userInfo from the json?
that would work i guess. what is the client-go source code equivalent of kubectl auth whoami?

@kfox1111
Copy link

kfox1111 commented Oct 24, 2024

sorry can you clarify what you mean by "not using an x509 plugin", how is the kubelet authenticating to the apiserver exactly and does it not have a client cert on disk?

I tweaked /etc/kubernetes/kublet.conf with:

users:
- name: default-auth
  user:
#    client-certificate: /var/lib/kubelet/pki/kubelet-client-current.pem
#    client-key: /var/lib/kubelet/pki/kubelet-client-current.pem
    exec:
      apiVersion: "client.authentication.k8s.io/v1"
      command: "/usr/bin/k8s-spiffe-workload-jwt-exec-auth"
      interactiveMode: Never
      env:
        - name: SPIFFE_ENDPOINT_SOCKET
          value: "unix:///var/run/spire/agent/sockets/main/public/api.sock"
        - name: SPIFFE_JWT_AUDIENCE
          value: "k8s-one"

With it setup this way, I have a node attesting with a hardware tpm against a spire server, and able to get valid jwt tokens for kubelet automatically. No join tokens needed. :) Happy to discuss this setup in detail if you want to know more about it. Or may be worth a discussion at Kubecon, for interested parties.

adding more flags is not ideal.

100%

as in setting the active KUBECONFIG to the kubelet client like so:
sudo kubectl auth whoami -o json --kubeconfig /etc/kubernetes/kubelet.conf

and then parsing userInfo from the json? that would work i guess

Yeah.

what is the client-go source code equivalent of kubectl auth whoami?

Running it with -v=255 it looks like its calling the rest endpoint:

11:38:22.398890  461562 request.go:1212] Request Body: {"kind":"SelfSubjectReview","apiVersion":"authentication.k8s.io/v1","metadata":{"creationTimestamp":null},"status":{"userInfo":{}}}
I1024 11:38:22.398958  461562 round_trippers.go:466] curl -v -XPOST  -H "Accept: application/json, */*" -H "Content-Type: application/json" -H "User-Agent: kubectl/v1.30.5 (linux/arm64) kubernetes/74e84a9" 'https://xxx.xxx.xxx.xxx:6443/apis/authentication.k8s.io/v1/selfsubjectreviews'
I1024 11:38:22.399362  461562 round_trippers.go:510] HTTP Trace: Dial to tcp:xxx.xxx.xxx.xxx:6443 succeed
I1024 11:38:22.413030  461562 round_trippers.go:553] POST https://xxx.xxx.xxx.xxx:6443/apis/authentication.k8s.io/v1/selfsubjectreviews 201 Created in 14 milliseconds
<snip>
I1024 11:38:22.413149  461562 request.go:1212] Response Body: {"kind":"SelfSubjectReview","apiVersion":"authentication.k8s.io/v1","metadata":{"creationTimestamp":"2024-10-24T11:38:22Z"},"status":{"userInfo":{"username":"system:node:node1234","groups":["system:nodes","system:authenticated"]}}}

So, maybe something in https://pkg.go.dev/k8s.io/client-go/kubernetes/typed/authentication/v1 ?

@neolit123
Copy link
Member

Running it with -v=255 it looks like its calling the rest endpoint:

-v=10 is the maximum level, btw.

So, maybe something in https://pkg.go.dev/k8s.io/client-go/kubernetes/typed/authentication/v1 ?

seems like it's SelfSubjectReviews().
thanks for the link and idea.

i guess this can be attempted as the replacement for what kubeadm has now.
one blocker might be if we don't have an active API server at a certain moment where we need to know a node name, but i think there is no such case.

also, would you be able to help us with a PR for that?

@kfox1111
Copy link

This seems to work:

package main

import (
        "log"
        "context"

        "k8s.io/client-go/tools/clientcmd"
        authenticationv1 "k8s.io/api/authentication/v1"
        authv1client "k8s.io/client-go/kubernetes/typed/authentication/v1"
        meta "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func main() {
        config, err := clientcmd.BuildConfigFromFlags("", "/home/kfox/.kube/config")
        if err != nil {
                log.Fatalf("unable to get kubeconfig: %v", err)
        }
        authclient, err := authv1client.NewForConfig(config)
        if err != nil {
                log.Fatalf("Failed to get auth client: %v", err)
        }
        r, err := authclient.SelfSubjectReviews().Create(context.Background(), &authenticationv1.SelfSubjectReview{}, meta.CreateOptions{})
        if err != nil {
                log.Fatalf("Failed to self subject review: %v", err)
        }
        log.Print(r.Status.UserInfo.Username)
}

@neolit123
Copy link
Member

yes, that seems correct.

the problem function is:
getNodeNameFromKubeletConfig()

it seems to be called only here:
https://github.com/kubernetes/kubernetes/blob/8c7160205db67b250dece7cad5c6367debad2642/cmd/kubeadm/app/util/config/cluster.go#L128

which is in a code path that already assumes there is a working apiserver as it has an active API client.
so your idea would work @kfox1111

@kfox1111
Copy link

Should we switch it to always do the self subject call, removing the old code, or only do the call if not x509 as a fallback?

@neolit123
Copy link
Member

neolit123 commented Oct 24, 2024

i think given the circumstances (code paths) the function contents can be replaced to use the new way.
maybe it should be unit tested too, assuming the fake client can handle these self subject reviews.

@neolit123
Copy link
Member

@kfox1111 thinking more about this. the same code might end up being called by kubeadm reset.
please check if that is the case. kubeadm reset is a best effort command so if it ends up being problematic to get the node name we might have to do the old-way -> fallback to new-way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/HA area/upgrades kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants