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.

Sign up for GitHub

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

KEP-3503 Initial KEP for windows host-network support #3507

Merged
merged 1 commit into from Oct 6, 2022
Merged

KEP-3503 Initial KEP for windows host-network support #3507

merged 1 commit into from Oct 6, 2022

Conversation

marosset
Copy link
Contributor

Signed-off-by: Mark Rossetti marosset@microsoft.com

  • One-line PR description:
  • Other comments:

@k8s-ci-robot k8s-ci-robot added 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 labels Sep 12, 2022
@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 12, 2022
@marosset marosset added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/network Categorizes an issue or PR as relevant to SIG Network. labels Sep 13, 2022
@marosset marosset force-pushed the windows-host-network-initial-kep branch from 9f8620f to ab5a67a Compare September 14, 2022 18:44
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2022
@marosset marosset mentioned this pull request Sep 15, 2022
Open
4 tasks
aojea
aojea reviewed Sep 17, 2022
View reviewed changes
keps/sig-windows/3503-host-network-support-for-windows-pods/README.md
Comment on lines +177 to +184
Today it is possible to set `hostNetwork=true` for Windows pods but it doesn't change anything
(unless the pod contains `hostProcess` containers). This can be confusing for users.
Copy link
Member

Choose a reason for hiding this comment

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

I remember this discussion 2 years ago in sig-network

https://groups.google.com/g/kubernetes-sig-network/c/bRWRos3H0sM/m/MwNwG0q2AAAJ

and I could check there was a KEP that seems implemented

https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/1981-windows-privileged-container-support#host-network-mode

is this paragraph a reference to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is related to that.
HostProcessContainers run everything in the root namespaceses so we decided to require hostNetwork=true be set for these pods/contianers. This KEP will focus on non-HostProcessContainers support

@marosset
Copy link
Contributor Author

@aojea Can I add you as a reviewer or approver for this KEP for sig-network?

@aojea
Copy link
Member

aojea commented Sep 21, 2022

@aojea Can I add you as a reviewer or approver for this KEP for sig-network?

I have limited knowledge of windows, but I can be reviewer, just prepare yourself for some naive questions 😄

@marosset
Copy link
Contributor Author

@aojea Can I add you as a reviewer or approver for this KEP for sig-network?

I have limited knowledge of windows, but I can be reviewer, just prepare yourself for some naive questions 😄

No problem!

jayunit100
jayunit100 reviewed Sep 21, 2022
View reviewed changes
keps/sig-windows/3503-host-network-support-for-windows-pods/README.md

### User Stories (Optional)

<!--
Copy link
Member

Choose a reason for hiding this comment

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

need user stories, heres a quick take:

#### Story 1

As a user of a legacy application I want to bind many arbitrary ports to a host network namespace on a single node, as opposed to taking all the node ports of a cluster.

#### Story 2 
As a daemonset which runs before CNI providers are installed, for example for security, application bootstrapping, cni bootstrapping, and so on - i want to be able to run a container that isn't fully priveliged (i.e. that isnt a host process) but which is on the host's network

#### Story 3 

As a user creating a windows pod with `hostNetwork` , I want correct behaviour (i.e. I dont want to  silently ignore the  hostNetwork knob.

@jayunit100
Copy link
Member

i think lgtm since we know this works, and its really just a bugfix

@marosset
Copy link
Contributor Author

/assign @saschagrunert

@marosset
Copy link
Contributor Author

/assign @aojea @johnbelamaric

@marosset
Copy link
Contributor Author

/hold
For reviews

mrunalp
mrunalp reviewed Oct 4, 2022
View reviewed changes
keps/sig-windows/3503-host-network-support-for-windows-pods/README.md

In clusters with large amounts of services Windows nodes can experience port exhaustion.
One such situation is where a small amount of pods need to expose many ports it can then be desirable to use use host networking instead of using nodePorts.
Another situation is using `hostNetwork=true` as alternative to relying on 'hostPort' CNI feature for exposing many ports in many pods.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there use cases where privileged pods modify the hosts network settings?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, I see that covered in use cases below.

@mrunalp
Copy link
Contributor

mrunalp commented Oct 4, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 4, 2022
@marosset
Copy link
Contributor Author

marosset commented Oct 4, 2022

/label tide/merge-method-squash
(i'll try to manually squash too)

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 4, 2022
@mrunalp
Copy link
Contributor

mrunalp commented Oct 4, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2022
@aojea
Copy link
Member

aojea commented Oct 5, 2022 •
edited

The KEP brings parity between Linux and Windows containers with hostNetwork: true, my knowledge about windows environment is minimal, but we have a good suite of e2e tests for Linux that will help to verify this is correct

I defer the LGTM to @jayunit100 , he knows these area better than me

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022
@jayunit100
Copy link
Member

/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 5, 2022
@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jayunit100, johnbelamaric, marosset, mrunalp

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 6, 2022
@marosset
Copy link
Contributor Author

marosset commented Oct 6, 2022

I'm going to squash commits now

@marosset
Initial KEP for windows host-network support
e154277 
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
@marosset marosset force-pushed the windows-host-network-initial-kep branch from d8ac55a to e154277 Compare October 6, 2022 16:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2022
@marosset
Copy link
Contributor Author

marosset commented Oct 6, 2022

/hold cancel
@jayunit100 or @jsturtevant can one of you LGTM?

@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 Oct 6, 2022
@jsturtevant
Copy link
Contributor

/lgtm

Excited to see more parity on windows side!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2022
@k8s-ci-robot k8s-ci-robot merged commit b6222e7 into kubernetes:master Oct 6, 2022
SIG Node PR Triage automation moved this from Triage to Done Oct 6, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 6, 2022
ahmedtd pushed a commit to ahmedtd/enhancements that referenced this pull request Feb 2, 2023
@marosset @ahmedtd
Initial KEP for windows host-network support (kubernetes#3507)
86baba8 
Signed-off-by: Mark Rossetti <marosset@microsoft.com>

Signed-off-by: Mark Rossetti <marosset@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp mrunalp left review comments

@jayunit100 jayunit100 jayunit100 left review comments

@thockin thockin thockin left review comments

@aojea aojea aojea left review comments

@johnbelamaric johnbelamaric johnbelamaric left review comments

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

Assignees

@mrunalp mrunalp

@jsturtevant jsturtevant

@jayunit100 jayunit100

@aojea aojea

@johnbelamaric johnbelamaric

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. 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/windows Categorizes an issue or PR as relevant to SIG Windows. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
SIG Node PR Triage
Done
[sig-windows] PR tracking
Status: Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@marosset @aojea @jayunit100 @mrunalp @johnbelamaric @k8s-ci-robot @jsturtevant @thockin @saschagrunert