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

Deprecate default service IP CIDR #81668

Merged

Conversation

@darshanime
Copy link
Contributor

commented Aug 20, 2019

What type of PR is this?
/kind bug
What this PR does / why we need it:
Remove default service CIDR

Which issue(s) this PR fixes:
Fixes #81315

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED:
Deprecate the default service IP CIDR. The previous default was `10.0.0.0/24` which will be removed in 6 months/2 releases. Cluster admins must specify their own desired value, by using `--service-cluster-ip-range` on kube-apiserver.

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


/sig network

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Hi @darshanime. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@darshanime darshanime changed the title Remove default service cidr [WIP] Remove default service cidr Aug 20, 2019
@darshanime

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

/hold

@darshanime darshanime changed the title [WIP] Remove default service cidr [WIP] Remove default service IP CIDR Aug 20, 2019
@k8s-ci-robot k8s-ci-robot requested review from caesarxuchao and liggitt Aug 20, 2019
@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

note that this requires all existing deployments that rely on the default to modify their invocation to replace it with the current default value. that's a change that needs to be pre-announced and rolled out per https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli

pkg/master/services.go Outdated Show resolved Hide resolved
pkg/master/services.go Outdated Show resolved Hide resolved
@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

/remove-sig api-machinery

@darshanime

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

@liggitt if I understand the deprecation policy correctly, we should emit a warning and continue setting the default cidr for 1 release/6 months?

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Emit a warning if the flag is unset and default it to the current default, add the deprecation notice to the release notes until two releases (6 months) have passed

@darshanime darshanime force-pushed the darshanime:remove_default_service_cidr branch from 1d0afcf to 7f24c59 Aug 26, 2019
@@ -114,7 +114,6 @@ func NewServerRunOptions() *ServerRunOptions {
},
ServiceNodePortRange: kubeoptions.DefaultServiceNodePortRange,
}
s.ServiceClusterIPRange = kubeoptions.DefaultServiceIPCIDR

This comment has been minimized.

Copy link
@darshanime

darshanime Aug 26, 2019

Author Contributor

Removing assignment from here so it goes thru ServiceIPRange with a warning.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 29, 2019
@darshanime darshanime force-pushed the darshanime:remove_default_service_cidr branch from d9b80b4 to 169f5ce Aug 29, 2019
@thockin thockin added lgtm and removed do-not-merge/hold labels Aug 30, 2019
@darshanime darshanime force-pushed the darshanime:remove_default_service_cidr branch from 169f5ce to 140cee2 Aug 30, 2019
@k8s-ci-robot k8s-ci-robot added size/S and removed lgtm size/M labels Aug 30, 2019
@darshanime

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@darshanime: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@darshanime

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/ok-to-test

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@darshanime: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

/ok-to-test

Signed-off-by: darshanime <deathbullet@gmail.com>
@darshanime darshanime force-pushed the darshanime:remove_default_service_cidr branch from 140cee2 to aef96c3 Aug 30, 2019
@darshanime

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/retest

@@ -25,12 +25,12 @@ import (
"k8s.io/kubernetes/pkg/registry/core/service/ipallocator"
)

// DefaultServiceIPRange takes a the serviceIPRange flag and returns the defaulted service ip range (if needed),
// ServiceIPRange takes a the serviceIPRange flag and returns the defaulted service ip range (if needed),

This comment has been minimized.

Copy link
@beejeebus

beejeebus Aug 30, 2019

I noticed a couple of typos in the docs, s/takes a the/takes the/, and a double space in 'if needed'.

Beyond the typos, perhaps the docs could be:

// ServiceIPRange checks if the serviceIPRange flag is nil, setting service ip
// range to the default value in kubeoptions.DefaultServiceIPCIDR in that case.
// Returns service ip range, api server service IP, and an error

This comment has been minimized.

Copy link
@darshanime

darshanime Aug 30, 2019

Author Contributor

made the change.
/retest

Signed-off-by: darshanime <deathbullet@gmail.com>
@darshanime

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/test pull-kubernetes-verify
/test pull-kubernetes-bazel-test

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 30, 2019
@k8s-ci-robot k8s-ci-robot merged commit 1d016cc into kubernetes:master Sep 10, 2019
24 checks passed
24 checks passed
cla/linuxfoundation darshanime 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-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.17 milestone Sep 10, 2019
@darshanime darshanime deleted the darshanime:remove_default_service_cidr branch Sep 11, 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.