-
Notifications
You must be signed in to change notification settings - Fork 40k
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
WIP: kube-proxy: remove OS conditionals. #40414
Conversation
[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files: We suggest the following people: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels partial. The userspace and iptables modes only apply on Linux, so why are they in the os-agnostic section?
return fmt.Errorf("Proxying is not supported in this build") | ||
} | ||
|
||
func getConntrackMax(config *options.ProxyServerConfig) (int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this part of the "interface" only used in Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
} | ||
|
||
// Tune conntrack, if requested | ||
func (s *ProxyServer) tuneConnTracker() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't this just embedded in the _linux code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're working on having connection tracking on Windows. Even though, I have reviewed partially addressed your concerns in a new commit.
80b03ab
to
79014b4
Compare
@jbhurat I have rebased the PR so make sure you have updated your working copy. |
a12c4cc
to
79651da
Compare
79651da
to
ce1a774
Compare
@pires PR needs rebase |
What this PR does / why we need it: in 1.5 we added Windows support to kube-proxy. This is part of an ongoing effort to improve what has been done. In this PR, we focused on removing OS checks based on previous feedback.
Which issue this PR fixes: fixes #36277
Special notes for your reviewer: @thockin a second step we'd like to discuss is how can we remove the need for the Windows code to not know about
iptables.Interface
type. A third step could be to extract proxy modes as plug-ins, so that we can provide different implementations for Windows as well.Release note: