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

Added Windows KEP clarification on ICMP protocol support. #824

Merged
merged 3 commits into from Feb 27, 2019

Conversation

daschott
Copy link
Contributor

Some common network utilities (e.g. ping, traceroute) used for troubleshooting and diagnosis are based on ICMP messaging. This commit is adding a clarification on what network flows can be expected to work are on Windows containers using the ICMP protocol.

@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Feb 12, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @daschott. 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.

@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/pm needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 12, 2019
@daschott
Copy link
Contributor Author

@neolit123 @PatrickLang @michmike PTAL

@PatrickLang
Copy link
Contributor

/ok-to-test
I'm pinging @bgrant0607 on this to see if he wants to review now or later

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 12, 2019
@PatrickLang
Copy link
Contributor

/LGTM
/hold
(will remove hold once I get a response on whether or not @bgrant0607 wants to approve before merge)

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 12, 2019
@PatrickLang PatrickLang added this to In Review in SIG-Windows Feb 12, 2019
@@ -153,6 +154,10 @@ Note that some features are plain unsupported while some will not work without u
- Not all features of shared namespaces are supported. This is clarified in the API section below
- The existing node problem detector is Linux-only and requires privileged containers. In general, we don't expect these to be used on Windows because there's no privileged support
- Overlay networking support in Windows Server 1803 is not fully functional using the `win-overlay` CNI plugin. Specifically service IPs do not work on Windows nodes. This is currently specific to `win-overlay`; other CNI plugins (OVS, AzureCNI) work. Since Windows Server 1803 is not supported for GA, this is mostly not applicable. We left it here since it impacts beta
- Outbound communication using the ICMP protocol via the `win-overlay`, `win-bridge`, and `Azure-CNI` plugin. Specifically, the Windows data plane ([VFP](https://www.microsoft.com/en-us/research/project/azure-virtual-filtering-platform/)) doesn't support ICMP packet transpositions. This means:
- ICMP packets directed to destinations within the same network (e.g. pod to pod communication via ping) will work as expected and without any limitations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. I don't think we document that ICMP should work anywhere. I searched kubernetes.io docs and didn't find it.

Do we have tests for ICMP?

@thockin @caseydavenport

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we ever specified it. pod-to-pod ping is really useful for debugging only, but things like ICMP reject are pretty important between nodes or for services with no endpoints.

Ping to the outside world is something I use for debug. I honestly have no idea if breaking this is going to be a big deal. I assume the same breakage holds for ICMP reject or other control signals? That's a pretty weird thing to break, but I am not sure it is a critical gap.

Can we bring this up at sig-net next week, or can you start an email thread on sig-net?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, you can replace ping <destination> with curl <destination> to be able to debug connectivity to the outside world. Yes, I can bring it up.

Copy link
Contributor

@michmike michmike Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daschott can you please add this to the docs as well that the alternative is curl/powershell to test connectivity. Once we hear back from the discussion with sig-network we can remove the hold.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought CNI provides IP. ICMP is IP, right?

@bgrant0607
Copy link
Member

cc @kubernetes/sig-network-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Feb 13, 2019
@bgrant0607
Copy link
Member

@daschott @PatrickLang I suggest asking what is expected to work wrt ICMP on the SIG Network mailing list.

@PatrickLang
Copy link
Contributor

@daschott - is there any outstanding feedback on how this is worded? I think we should merge this so it's accurate with what's implemented.

If SIG-Network has outstanding discussions on how ICMP is or should be implemented in the future I think those should be tracked as separate issues, document and adds tests to get ICMP into conformance for a later release.

@PatrickLang
Copy link
Contributor

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Feb 26, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019
@@ -153,6 +154,11 @@ Note that some features are plain unsupported while some will not work without u
- Not all features of shared namespaces are supported. This is clarified in the API section below
- The existing node problem detector is Linux-only and requires privileged containers. In general, we don't expect these to be used on Windows because there's no privileged support
- Overlay networking support in Windows Server 1803 is not fully functional using the `win-overlay` CNI plugin. Specifically service IPs do not work on Windows nodes. This is currently specific to `win-overlay`; other CNI plugins (OVS, AzureCNI) work. Since Windows Server 1803 is not supported for GA, this is mostly not applicable. We left it here since it impacts beta
- Outbound communication using the ICMP protocol via the `win-overlay`, `win-bridge`, and `Azure-CNI` plugin. Specifically, the Windows data plane ([VFP](https://www.microsoft.com/en-us/research/project/azure-virtual-filtering-platform/)) doesn't support ICMP packet transpositions. This means:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does transpositions mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set of operations that perform modifications to network packets such as for example NAT'ing destination or source IPs.

@daschott
Copy link
Contributor Author

@bgrant0607 @michmike @PatrickLang this was discussed during SIG-network meeting on 02/21.
While everyone agreed this behavior is odd, there was consensus that it would be unreasonable to make this a blocking issue for Windows to go stable and the overall goal of the KEP, as there is no spec documenting why ICMP support would be required, and no major limitations to any of the existing Kubernetes constructs pointed out. However, we should definitely document this behavior to avoid possible confusions for end-customers, and furthermore, should enough customer demand this feature for debugging purposes, Microsoft should overcome this platform limitation in Windows.

URL here for the meeting: https://www.youtube.com/watch?v=f1NGvMU5GiE

@michmike
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019
@michmike
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daschott, michmike, PatrickLang

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 merged commit d22e1c0 into kubernetes:master Feb 27, 2019
SIG-Windows automation moved this from In Review to Done (v.1.14) Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants