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

Windows support for preserving the destination IP as the VIP when loadbalancing with DSR #74825

Merged
merged 2 commits into from May 31, 2019

Conversation

@ksubrmnn
Copy link
Contributor

commented Mar 1, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR allows the user to specify the annotation "preserve-destination". When this is configured, DNAT is skipped and loadbalanced packets are delivered with VIP/ClusterIP as the destination IP.

This PR also allows for local NodePort access from the host/local pods with the LocalRoutedVIP feature.

Which issue(s) this PR fixes:

Kube Proxy did not support preserving the destination IP.
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Windows support for preserving the destination IP as the VIP when loadbalancing with DSR.
@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

/hold

@k8s-ci-robot k8s-ci-robot added sig/network and removed needs-sig labels Mar 1, 2019

@ksubrmnn ksubrmnn force-pushed the ksubrmnn:preserve_dip branch 2 times, most recently from d42294e to 6540a98 Mar 1, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

/priority important-soon

@ksubrmnn ksubrmnn force-pushed the ksubrmnn:preserve_dip branch 6 times, most recently from 8986c0b to 1c53477 Mar 1, 2019

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

/retest

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Today is code freeze for 1.14. Is 1.15 OK, and what's the OS version needed? I don't think it can make 1.14 since it's not already in the PR queue.

cc @michmike @craiglpeters

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Today is code freeze for 1.14. Is 1.15 OK, and what's the OS version needed? I don't think it can make 1.14 since it's not already in the PR queue.
cc @michmike @craiglpeters

1.15 is fine. This PR still needs to be worked on/validated. This is a 19h1 feature

@michmike

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

/sig windows

@madhanrm

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 22, 2019

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

/assign @brendandburns

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@dims can you PTAL at this vendoring change?

@dims

This comment has been minimized.

Copy link
Member

commented May 23, 2019

/approve

(the vendoring changes)

@madhanrm

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

/approve

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

/assign @brendandburns

@dineshgovindasamy

This comment has been minimized.

Copy link

commented May 28, 2019

/approve

@dineshgovindasamy
Copy link

left a comment

/LGTM

@@ -207,10 +207,30 @@ func (hns hnsV2) getLoadBalancer(endpoints []endpointsInfo, isILB bool, isDSR bo
if len(vip) > 0 {
vips = append(vips, vip)
}

lbPortMappingFlags := hcn.LoadBalancerPortMappingFlagsNone
if flags.isILB {

This comment has been minimized.

Copy link
@dineshgovindasamy

dineshgovindasamy May 28, 2019

Can we document What is ILB, Preserving DIP means?

@cblecker

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I'm good with the vendor changes here.

/assign @liggitt


klog.Infof("Service %q preserve-destination: %v", svcPortName.NamespacedName.String(), service.Annotations["preserve-destination"])

preserveDIP := service.Annotations["preserve-destination"] == "true"

This comment has been minimized.

Copy link
@liggitt

liggitt May 29, 2019

Member

cc @kubernetes/sig-network-api-reviews

is this annotation-driven behavior novel for in-tree components? if not, is preserve-destination used by kube-proxy as well? should this be scoped in a k8s.io/... prefix, or even a windows-specific k8s.io prefix?

This comment has been minimized.

Copy link
@madhanrm

madhanrm May 30, 2019

Contributor

Currently this is specific to windows, but can be extendend to general kube-proxy and can be implemented across.

@liggitt

This comment has been minimized.

Copy link
Member

commented May 29, 2019

vendor bump looks fine.

one question on use of the annotations for driving this behavior. @thockin might have the most context on use of annotations in services like this

/assign @thockin

@fejta-bot

This comment has been minimized.

Copy link

commented May 29, 2019

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.

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

just realized this was in review queue but not tagged
/milestone v1.15

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 30, 2019

@liggitt liggitt removed their assignment May 30, 2019

@PatrickLang PatrickLang moved this from Backlog (v.1.15) to In Progress+Review in SIG-Windows May 31, 2019

@thockin
Copy link
Member

left a comment

Thanks!

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, dineshgovindasamy, ksubrmnn, madhanrm, 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

@fejta-bot

This comment has been minimized.

Copy link

commented May 31, 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 8b7e777 into kubernetes:master May 31, 2019

21 checks passed

cla/linuxfoundation ksubrmnn 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-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

SIG-Windows automation moved this from In Progress+Review to Done (v1.15) May 31, 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.