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: Fetching kube-proxy's config map is now optional #82248

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@rosti
Copy link
Member

commented Sep 2, 2019

What type of PR is this?
/kind design

What this PR does / why we need it:

Whenever kubeadm needs to fetch its configuration from the cluster, it gets
the component configuration of all supported components (currently only kubelet
and kube-proxy). However, kube-proxy is deemed an optional component and its
installation may be skipped (by skipping the addon/kube-proxy phase on init).
When kube-proxy's installation is skipped, its config map is not created and
all kubeadm operations, that fetch the config from the cluster, are bound to
fail with "not found" or "forbidden" (because of missing RBAC rules) errors.

To fix this issue, we have to ignore the 403 and 404 errors, returned on an
attempt to fetch kube-proxy's component config from the cluster.
The GetFromKubeProxyConfigMap function now supports returning nil for both
error and object to indicate just such a case.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1733

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-longterm
/assign @neolit123
/assign @fabriziopandini

Does this PR introduce a user-facing change?:

kubeadm: Allow users to skip the kube-proxy init addon phase during init and still be able to join a cluster and perform some other minor operations (but not upgrade).

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


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rosti

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

@rosti rosti force-pushed the rosti:proxyless branch from 76c771d to 2ce7fba Sep 2, 2019
Copy link
Member

left a comment

@rosti
Thanks for this PR.
I don't want to block on this, but as per comment in the issue, I'm in favour of a different approach that makes explicit what the user want instead of guessing cluster condition/ignoring arbitrary errors.

The approach I'm suggesting requires API changes, but we are at the beginning of a new cycle and there is room to make this effort enabler/coordinated with the addon project integration, which is the long term answer to the issue this PR aims to fix

@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

the requests have been way too many.
i think it doesn't cost us much to turn these errors into warnings.
if an API change or addons are the correct fix long term, we can revert this.

@rosti

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

Ok, let's deliberate on this on today's office hours. I think, that you both have a point here and we need to find the common path.

@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

bumping priority as discussed today. we want this change in 1.16.

/milestone v1.16
/priority important-soon
/remove-priority important-longterm

@rosti please remove WIP when see fit.

@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 4, 2019
Whenever kubeadm needs to fetch its configuration from the cluster, it gets
the component configuration of all supported components (currently only kubelet
and kube-proxy). However, kube-proxy is deemed an optional component and its
installation may be skipped (by skipping the addon/kube-proxy phase on init).
When kube-proxy's installation is skipped, its config map is not created and
all kubeadm operations, that fetch the config from the cluster, are bound to
fail with "not found" or "forbidden" (because of missing RBAC rules) errors.

To fix this issue, we have to ignore the 403 and 404 errors, returned on an
attempt to fetch kube-proxy's component config from the cluster.
The `GetFromKubeProxyConfigMap` function now supports returning nil for both
error and object to indicate just such a case.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@rosti rosti force-pushed the rosti:proxyless branch from 2ce7fba to 31b4c78 Sep 5, 2019
@rosti rosti changed the title [WIP] kubeadm: Fetching kube-proxy's config map is now optional kubeadm: Fetching kube-proxy's config map is now optional Sep 5, 2019
@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit c8c1aea into kubernetes:master Sep 5, 2019
23 of 24 checks passed
23 of 24 checks passed
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 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-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
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.