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 join the cluster with pre-existing client certificate if provided #66482

Merged
merged 1 commit into from Jul 27, 2018

Conversation

@dixudx
Copy link
Member

dixudx commented Jul 22, 2018

What this PR does / why we need it:
support kubeadm join with a pre-existing client certificate

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#945

Special notes for your reviewer:
/cc @luxas @timothysc @kubernetes/sig-cluster-lifecycle-pr-reviews

Release note:

kubeadm now can join the cluster with pre-existing client certificate if provided

@k8s-ci-robot k8s-ci-robot requested review from luxas and timothysc Jul 22, 2018

@dixudx dixudx force-pushed the dixudx:kubeadm_use_existing_config branch 2 times, most recently from bc23ef9 to a1ed359 Jul 22, 2018

@dixudx

This comment has been minimized.

Copy link
Member Author

dixudx commented Jul 23, 2018

/retest

clusterinfo.CertificateAuthorityData,
cfg.TLSBootstrapToken,
), nil
if len(cfg.TLSBootstrapToken) != 0 {

This comment has been minimized.

@neolit123

neolit123 Jul 23, 2018

Member
if len(cfg.TLSBootstrapToken) == 0 {
	config, nil
}
...

to avoid indentation for the bellow block.


// extractFromFile tries to read the pem file
func extractFromFile(filename string) ([]byte, error) {
glog.V(3).Infof("Reading config file %q", filename)

This comment has been minimized.

@neolit123

neolit123 Jul 23, 2018

Member

don't we have the generic "open file" -> return []byte, error function already?
if not, i think this should go in utils - e.g. readFileBytes()?

this would avoid the imports here too:

+	"io/ioutil"
+	"os"
+
+	"github.com/golang/glog"

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

In other places we just use ioutil.ReadFile for this. I think that's sufficient here as well which removes the need for this func.

@luxas
Copy link
Member

luxas left a comment

Great start 👏!

// GetValidatedClusterInfoObject returns a validated Cluster object that specifies where the cluster is and the CA cert to trust
func GetValidatedClusterInfoObject(cfg *kubeadmapi.JoinConfiguration) (*clientcmdapi.Cluster, error) {
// GetValidatedConfigInfoObject returns a validated Cluster object that specifies where the cluster is and the CA cert to trust
func GetValidatedConfigInfoObject(cfg *kubeadmapi.JoinConfiguration) (*clientcmdapi.Config, error) {

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

I'd rename this to DiscoverValidatedKubeConfig or just RunDiscovery here and for all sub-dirs so it's clear this returns a KubeConfig object (not any Config), and that this is the discovery process.


// extractFromFile tries to read the pem file
func extractFromFile(filename string) ([]byte, error) {
glog.V(3).Infof("Reading config file %q", filename)

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

In other places we just use ioutil.ReadFile for this. I think that's sufficient here as well which removes the need for this func.

}
authInfo.ClientKeyData = clientKey
}
}

// Create a new kubeconfig object from the given, just copy over the server and the CA cert

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

...and optional extra client certs

@@ -86,21 +122,21 @@ func ValidateClusterInfo(clusterinfo *clientcmdapi.Config, clustername string) (
return true, nil
})

// If we couldn't fetch the cluster-info ConfigMap, just return the cluster-info object the user provided

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

this should not be changed, this is still called cluster-info, we just append one more alternative.

"", // no user provided
defaultCluster.CertificateAuthorityData,
)
}

client, err := kubeconfigutil.ToClientSet(configFromClusterInfo)

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

You should/can now remove this code that looks up the cluster info ConfigMap, not needed anymore. It could make sense to do that in a separate PR that would be merged first, but I don't have a strong preference here.

var authInfo *clientcmdapi.AuthInfo
if config.Contexts[config.CurrentContext] != nil {
// load pre-existing client certificates
authInfo = config.AuthInfos[config.Contexts[config.CurrentContext].AuthInfo]

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

After this, one more line and if block that checks for authInfo != nil

defaultCluster := kubeconfigutil.GetClusterFromKubeConfig(config)

var authInfo *clientcmdapi.AuthInfo
if config.Contexts[config.CurrentContext] != nil {

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

and len(config.AuthInfos) > 0

return nil, err
}
authInfo.ClientKeyData = clientKey
}

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

You can just move this code over here and return what was found:

if len(authInfo.ClientCertificateData) == 0 || len(authInfo.ClientKeyData) == 0 {
    return nil, fmt.Errorf("couldn't read authentication info from the given kubeconfig file")
}
return configFromClusterInfo = kubeconfigutil.CreateWithCerts(...)
authInfo.ClientCertificateData,
)
} else {
configFromClusterInfo = kubeconfigutil.CreateBasic(

This comment has been minimized.

@luxas

luxas Jul 23, 2018

Member

this can be left as-is, but just return it directly here.

@timothysc
Copy link
Member

timothysc left a comment

/approve

Other can lgtm when ready.

@dixudx dixudx force-pushed the dixudx:kubeadm_use_existing_config branch from a1ed359 to 997a612 Jul 26, 2018

@dixudx

This comment has been minimized.

Copy link
Member Author

dixudx commented Jul 26, 2018

/test pull-kubernetes-e2e-kops-aws

@dixudx

This comment has been minimized.

Copy link
Member Author

dixudx commented Jul 27, 2018

/retest

@dixudx

This comment has been minimized.

Copy link
Member Author

dixudx commented Jul 27, 2018

@neolit123 @stealthybox PTAL. Needs your lgtm. Thanks.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jul 27, 2018

/lgtm
thank you @dixudx

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 27, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, neolit123, timothysc

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-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 27, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jul 27, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 27, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f7641e8 into kubernetes:master Jul 27, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation dixudx authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@dixudx dixudx deleted the dixudx:kubeadm_use_existing_config branch Jul 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.