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

Jump to bottom

dual stack services #91824

Merged
merged 22 commits into from Oct 26, 2020
Merged

dual stack services #91824

merged 22 commits into from Oct 26, 2020

Conversation

khenidak
Copy link
Contributor

@khenidak khenidak commented Jun 5, 2020 •
edited by thockin

implements kubernetes/enhancements#1679 in its eventual form.

status of work

  • api structure change
  • api work
    • helpers, utils etc..
    • defaulting
    • defaulting for existing services (headful + headless + headless without selectors)
    • validation
    • conversion
  • tie dualstack services to EndPointSlice feature - startup flag validation
    • api server
    • controller manager
  • EndpointSlice
    • ensure that EndpointSlice handles multi family services
  • service ip alloc
    • default kuberentes service
    • force kuberenetes default service into dual stack
    • user services
    • strategy: clear IPFamilies, IPFamilyPolicy, ClusterIPs when converting ExternalName type.
    • api-server repair loop
  • pod/pod net
    • env vars handle dual stack services
    • remove kubenet v4,v6 enforcement
  • kube proxy
    • flag validation, tie dual stack to EndPointSlice
    • reserve NodePort for both ipv6/ipv4 by default
    • modify metaproxier to work only with EndPointSlice
    • modify metaproxier to expect EndPointSlice with both families endpoints
    • force ipvs proxier to lookup ClusterIP by family (and correctly select ranges and related fields accordingly)
    • force iptables to lookup ClusterIP by family (and correctly select ranges and related fields accordingly)
    • FIX ipvs mishandling of families.
    • validate that topology keys does not conflict with dualstack
  • kubectl
    • modify kubectl describe to work with multi ClusterIPs
  • e2e
    • fix existing e2e tests that will break due to api change
    • add new e2e tests for upgrade and downgrade scenarios
Add dual-stack Services (alpha).  This is a BREAKING CHANGE to an alpha API.
It changes the dual-stack API wrt Service from a single ipFamily field to 3
fields: ipFamilyPolicy (SingleStack, PreferDualStack, RequireDualStack),
ipFamilies (a list of families assigned), and clusterIPs (inclusive of
clusterIP).  Most users do not need to set anything at all, defaulting will
handle it for them.  Services are single-stack unless the user asks for
dual-stack.  This is all gated by the "IPv6DualStack" feature gate.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 5, 2020
@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 5, 2020
@aramase
Copy link
Member

aramase commented Jun 5, 2020

/cc

@k8s-ci-robot k8s-ci-robot requested a review from aramase June 5, 2020 16:10
@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

aojea
aojea reviewed Jun 5, 2020
View reviewed changes
pkg/apis/core/types.go Outdated Show resolved Hide resolved
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
pkg/apis/core/types.go Outdated Show resolved Hide resolved
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
pkg/apis/core/types.go Outdated Show resolved Hide resolved
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
pkg/apis/core/types.go Outdated Show resolved Hide resolved
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
pkg/apis/core/types.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 11, 2020
@khenidak khenidak force-pushed the dualstack-phase-3 branch 2 times, most recently from 6f17970 to 376014e Compare June 11, 2020 22:53
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 26, 2020
@thockin
Copy link
Member

thockin commented Oct 26, 2020 •
edited

joker_here_we_go

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khenidak, thockin

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2020
@aojea
Copy link
Member

aojea commented Oct 26, 2020

Fantastic job 🥇

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 26, 2020 •
edited

@khenidak: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-iptables-azure-dualstack 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-iptables-azure-dualstack
pull-kubernetes-e2e-azure-dualstack 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-azure-dualstack
pull-kubernetes-e2e-kind-dual-canary 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-kind-dual-canary
pull-kubernetes-e2e-kind-ipvs-dual-canary 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-kind-ipvs-dual-canary
pull-kubernetes-e2e-gci-gce-ipvs 14b3451 link /test pull-kubernetes-e2e-gci-gce-ipvs

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@aojea
Copy link
Member

aojea commented Oct 26, 2020

/retest
I saw this failure before, I don' t think is related

Kubernetes e2e suite: [sig-network] Services should be able to change the type from ClusterIP to ExternalName [Conformance] expand_more

@k8s-ci-robot k8s-ci-robot merged commit 6675eba into kubernetes:master Oct 26, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Oct 26, 2020
@dcbw
Copy link
Member

dcbw commented Oct 26, 2020

@khenidak fantastic. Now part 2 of X is complete!

@khenidak
Copy link
Contributor Author

@dcbw true. We are in a much better shape than before. Second time a charm ;-)

@liggitt
Copy link
Member

liggitt commented Oct 27, 2020

congrats!

I noticed the API testdata changed since the alpha ipFamily field was dropped. Would it make sense to leave a tombstone in place in the v1 API type to leave a hint to future civilizations not to reuse the ipFamily field or 15 protobuf tag? e.g.

diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go
index bd67349d957..1e21f17328c 100644
--- a/staging/src/k8s.io/api/core/v1/types.go
+++ b/staging/src/k8s.io/api/core/v1/types.go
@@ -4142,6 +4142,9 @@ type ServiceSpec struct {
 	// +optional
 	SessionAffinityConfig *SessionAffinityConfig `json:"sessionAffinityConfig,omitempty" protobuf:"bytes,14,opt,name=sessionAffinityConfig"`
 
+	// IPFamily is tombstoned to show why 15 is a reserved protobuf tag.
+	// IPFamily *IPFamily `json:"ipFamily,omitempty" protobuf:"bytes,15,opt,name=ipFamily,Configcasttype=IPFamily"`
+
 	// IPFamilies identifies all the IPFamilies assigned for this Service. If a value
 	// was not provided for IPFamilies it will be defaulted based on the cluster
 	// configuration and the value of service.spec.ipFamilyPolicy. A maximum of two

@aojea aojea mentioned this pull request Oct 27, 2020
Merged
@khenidak khenidak mentioned this pull request Oct 27, 2020
Merged
@khenidak
Copy link
Contributor Author

@liggitt #95924

@github-actions github-actions bot mentioned this pull request Nov 3, 2020
Open
mrmassis pushed a commit to mrmassis/kubernetes that referenced this pull request Nov 12, 2020
@khenidak @mrmassis
dual stack services (kubernetes#91824)
ae5e50d 
* api: structure change

* api: defaulting, conversion, and validation

* [FIX] validation: auto remove second ip/family when service changes to SingleStack

* [FIX] api: defaulting, conversion, and validation

* api-server: clusterIPs alloc, printers, storage and strategy

* [FIX] clusterIPs default on read

* alloc: auto remove second ip/family when service changes to SingleStack

* api-server: repair loop handling for clusterIPs

* api-server: force kubernetes default service into single stack

* api-server: tie dualstack feature flag with endpoint feature flag

* controller-manager: feature flag, endpoint, and endpointSlice controllers handling multi family service

* [FIX] controller-manager: feature flag, endpoint, and endpointSlicecontrollers handling multi family service

* kube-proxy: feature-flag, utils, proxier, and meta proxier

* [FIX] kubeproxy: call both proxier at the same time

* kubenet: remove forced pod IP sorting

* kubectl: modify describe to include ClusterIPs, IPFamilies, and IPFamilyPolicy

* e2e: fix tests that depends on IPFamily field AND add dual stack tests

* e2e: fix expected error message for ClusterIP immutability

* add integration tests for dualstack

the third phase of dual stack is a very complex change in the API,
basically it introduces Dual Stack services. Main changes are:

- It pluralizes the Service IPFamily field to IPFamilies,
and removes the singular field.
- It introduces a new field IPFamilyPolicyType that can take
3 values to express the "dual-stack(mad)ness" of the cluster:
SingleStack, PreferDualStack and RequireDualStack
- It pluralizes ClusterIP to ClusterIPs.

The goal is to add coverage to the services API operations,
taking into account the 6 different modes a cluster can have:

- single stack: IP4 or IPv6 (as of today)
- dual stack: IPv4 only, IPv6 only, IPv4 - IPv6, IPv6 - IPv4

* [FIX] add integration tests for dualstack

* generated data

* generated files

Co-authored-by: Antonio Ojea <aojea@redhat.com>
@wilsonehusin wilsonehusin mentioned this pull request Dec 3, 2020
Merged
@Shashankft9 Shashankft9 mentioned this pull request Dec 10, 2020
Closed
hidetatz pushed a commit to hidetatz/kubernetes that referenced this pull request Jan 12, 2021
@khenidak
dual stack services (kubernetes#91824)
7d26aa0 
* api: structure change

* api: defaulting, conversion, and validation

* [FIX] validation: auto remove second ip/family when service changes to SingleStack

* [FIX] api: defaulting, conversion, and validation

* api-server: clusterIPs alloc, printers, storage and strategy

* [FIX] clusterIPs default on read

* alloc: auto remove second ip/family when service changes to SingleStack

* api-server: repair loop handling for clusterIPs

* api-server: force kubernetes default service into single stack

* api-server: tie dualstack feature flag with endpoint feature flag

* controller-manager: feature flag, endpoint, and endpointSlice controllers handling multi family service

* [FIX] controller-manager: feature flag, endpoint, and endpointSlicecontrollers handling multi family service

* kube-proxy: feature-flag, utils, proxier, and meta proxier

* [FIX] kubeproxy: call both proxier at the same time

* kubenet: remove forced pod IP sorting

* kubectl: modify describe to include ClusterIPs, IPFamilies, and IPFamilyPolicy

* e2e: fix tests that depends on IPFamily field AND add dual stack tests

* e2e: fix expected error message for ClusterIP immutability

* add integration tests for dualstack

the third phase of dual stack is a very complex change in the API,
basically it introduces Dual Stack services. Main changes are:

- It pluralizes the Service IPFamily field to IPFamilies,
and removes the singular field.
- It introduces a new field IPFamilyPolicyType that can take
3 values to express the "dual-stack(mad)ness" of the cluster:
SingleStack, PreferDualStack and RequireDualStack
- It pluralizes ClusterIP to ClusterIPs.

The goal is to add coverage to the services API operations,
taking into account the 6 different modes a cluster can have:

- single stack: IP4 or IPv6 (as of today)
- dual stack: IPv4 only, IPv6 only, IPv4 - IPv6, IPv6 - IPv4

* [FIX] add integration tests for dualstack

* generated data

* generated files

Co-authored-by: Antonio Ojea <aojea@redhat.com>
@xmudrii xmudrii mentioned this pull request Jan 26, 2021
Closed
This was referenced Mar 18, 2021
Merged
Closed
@xliuxu xliuxu mentioned this pull request May 25, 2021
Merged
This was referenced Sep 29, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@anfernee anfernee anfernee left review comments

@robscott robscott robscott left review comments

@jayunit100 jayunit100 jayunit100 left review comments

@bowei bowei bowei left review comments

@liggitt liggitt liggitt left review comments

@thockin thockin thockin requested changes

@aojea aojea aojea left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@sftim sftim sftim left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@lavalamp lavalamp Awaiting requested review from lavalamp

@aramase aramase Awaiting requested review from aramase

@dcbw dcbw Awaiting requested review from dcbw

Assignees

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/ipvs area/kubectl area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet