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

Fix kubeadm file discovery #80675

Merged
merged 3 commits into from Jul 31, 2019

Conversation

@fabriziopandini
Copy link
Member

commented Jul 27, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
#66482 enabled kubeadm join --discovery-file with a discovery file with embedded credentials. However, the provided credentials are used only to get the cluster-info config map, then kubeadm rollbacks to the anonymous user and the workflow exit with an obscure message "kubeadm-config" is forbidden: User "system:anonymous" cannot get resource "configmaps" in API group "" in the namespace "kube-system"

This PR fixes this problem and makes possible to do kubeadm join --discovery-file with a discovery file with embedded credentials; it also implements a better logging and error messages in cases of misconfigurations

Which issue(s) this PR fixes:
Fixes #kubernetes/kubeadm#1687

Special notes for your reviewer:
IMO we should consider this for backporting because the error message is really confusing

Does this PR introduce a user-facing change?:

Fix error in `kubeadm join --discovery-file` when using discovery files with embedded credentials

/sig cluster-lifecycle
/priority critical-urgent
/assign @timothysc
/assign @neolit123

/cc @hickeng

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@fabriziopandini: GitHub didn't allow me to request PR reviews from the following users: hickeng.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind bug

What this PR does / why we need it:
#66482 enabled kubeadm join --discovery-file with a discovery file with embedded credentials. However, the provided credentials are used only to get the cluster-info config map, then kubeadm rollbacks to the anonymous user and the workflow exit with an obscure message "kubeadm-config" is forbidden: User "system:anonymous" cannot get resource "configmaps" in API group "" in the namespace "kube-system"

This PR fixes this problem and makes possible to do kubeadm join --discovery-file with a discovery file with embedded credentials; it also implements a better logging and error messages in cases of misconfigurations

Which issue(s) this PR fixes:
Fixes #kubernetes/kubeadm#1687

Special notes for your reviewer:
IMO we should consider this for backporting because the error message is really confusing

Does this PR introduce a user-facing change?:

Fix error in `kubeadm join --discovery-file` when using discovery files with embedded credentials

/sig cluster-lifecycle
/priority critical-urgent
/assign @timothysc
/assign @neolit123

/cc @hickeng

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.

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:fix-file-discovery branch from a930cd3 to d63e778 Jul 27, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

IMO we should consider this for backporting because the error message is really confusing

+1 for backport to 1.14 and 1.15.

@neolit123
Copy link
Member

left a comment

thanks @fabriziopandini
added mostly some comments about stdout / test names.

cmd/kubeadm/app/util/kubeconfig/kubeconfig_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/kubeconfig/kubeconfig_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/kubeconfig/kubeconfig_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/kubeconfig/kubeconfig_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/kubeconfig/kubeconfig_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/discovery/file/file.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/discovery/file/file.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/discovery/file/file.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/kubeconfig/kubeconfig.go Outdated Show resolved Hide resolved
@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@neolit123 many thanks for the quick review!
comments are addressed and answer to your questions are inline


// Create a new kubeconfig object from the discovery file config, with only the server and the CA cert.
// NB. We do this in order to not pick up other possible misconfigurations in the clusterinfo file
var fileCluster = kubeconfigutil.GetClusterFromKubeConfig(config)

This comment has been minimized.

Copy link
@neolit123

neolit123 Jul 29, 2019

Member

fileCluster :=

This comment has been minimized.

Copy link
@hickeng

hickeng Jul 29, 2019

:= construct looks more inline with existing inline declarations - is there a k8 style guide that makes a definitive statement?

This comment has been minimized.

Copy link
@neolit123

neolit123 Jul 29, 2019

Member

we have this https://github.com/kubernetes/community/blob/master/contributors/guide/coding-conventions.md#code-conventions
which isn't very detailed, so as long as go{vet|lint|fmt} passes changes are tolerated.

fileCluster := is preferred, so we left this one slide.

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

added another comment here: #80675 (comment)

the fix in #80066 seems related.
currently we don't load the CA from disk.

@timothysc
Copy link
Member

left a comment

/lgtm
/approve
/hold
^ for other reviewers, feel free to unhold when ready.
+1 to backport.

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, 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

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@neolit123

the fix in #80066 seems related.

This fix focuses on CA, not on credentials, so they are orthogonal

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

/hold cancel

@hickeng
Copy link

left a comment

I've read through the code change and did not spot any issues - I'm not sufficiently familiar with kubeadm code for that to mean much beyond a cursory check.

I think this API doc needs updating as it's explicitly incorrect once this is merged.

// TLSBootstrapToken is a token used for TLS bootstrapping.
// If .BootstrapToken is set, this field is defaulted to .BootstrapToken.Token, but can be overridden.
// If .File is set, this field must be set in case the KubeConfigFile does not contain any other authentication information

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I think this API doc needs updating as it's explicitly incorrect once this is merged.

// If .File is set, this field must be set in case the KubeConfigFile does not contain any other authentication information

the API comment still seems valid to me. see:
https://github.com/kubernetes/kubernetes/pull/80675/files#diff-fc46de7561d268ec1c3f3722181dd5aeR69

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

/retest

@hickeng

This comment has been minimized.

Copy link

commented Jul 30, 2019

I think this API doc needs updating as it's explicitly incorrect once this is merged.

// If .File is set, this field must be set in case the KubeConfigFile does not contain any other authentication information

the API comment still seems valid to me. see:
https://github.com/kubernetes/kubernetes/pull/80675/files#diff-fc46de7561d268ec1c3f3722181dd5aeR69

// If .File is set, this field must be set in case the KubeConfigFile does not contain any other authentication information

This is currently phrased as an imperative precaution - you must set the bootstrap token just in case the file doesn't contain other authentication information. However with this change, setting the bootstrap token will explicitly prevent the use of other authentication information if present.

This could be rephrased as follows (s/in case/in the case/):

> // If .File is set, this field **must be set** in the case the KubeConfigFile does not contain any other 

I'd suggest a more overt note that token is the primary auth mechanism and will be used in preference of client cert if present.

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/test pull-kubernetes-dependencies

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-kubemark-e2e-gce-big

@olivierlemasle

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

5 similar comments
@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@olivierlemasle

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@olivierlemasle

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@olivierlemasle there is an actual problem with this test job related to the build system.
please see:
https://kubernetes.slack.com/archives/C09QZ4DQB/p1564511430122100

we should retest once that is fixed.

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 30, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 000932d into kubernetes:master Jul 31, 2019

23 checks passed

cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Jul 31, 2019

@Boes-man

This comment has been minimized.

Copy link

commented Aug 1, 2019

Hello, is there a work around until 1.16? Thanks

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

@Boes-man

Hello, is there a work around until 1.16? Thanks

there are backports for 1.15 and 1.14.
for 1.15 not sure it will make it in .2, but maybe .3.

for 1.14 it should make it in the next patch release.

@Boes-man

This comment has been minimized.

Copy link

commented Aug 2, 2019

Awesome! Thanks

k8s-ci-robot added a commit that referenced this pull request Aug 7, 2019

Merge pull request #80729 from neolit123/automated-cherry-pick-of-#80…
…675-origin-release-1.15

Automated cherry pick of #80675: fix-file-discovery

k8s-ci-robot added a commit that referenced this pull request Aug 7, 2019

Merge pull request #80728 from neolit123/automated-cherry-pick-of-#80…
…675-origin-release-1.14

Automated cherry pick of #80675: fix-file-discovery

@fabriziopandini fabriziopandini deleted the fabriziopandini:fix-file-discovery branch Aug 8, 2019

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.