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

WIP: kube-proxy: remove OS conditionals. #40414

Closed
wants to merge 3 commits into from

Conversation

pires
Copy link
Contributor

@pires pires commented Jan 25, 2017

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:

NONE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jan 25, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @lavalamp
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@pires pires changed the title kube-proxy: remove OS conditionals. WIP: kube-proxy: remove OS conditionals. Jan 25, 2017
@lavalamp lavalamp assigned thockin and unassigned lavalamp Jan 26, 2017
Copy link
Member

@thockin thockin left a 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) {
Copy link
Member

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.

Copy link
Contributor Author

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 {
Copy link
Member

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?

Copy link
Contributor Author

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.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2017
@pires pires force-pushed the refactor_kube_proxy branch from 80b03ab to 79014b4 Compare February 2, 2017 08:19
@pires
Copy link
Contributor Author

pires commented Feb 2, 2017

@jbhurat I have rebased the PR so make sure you have updated your working copy.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2017
@pires pires force-pushed the refactor_kube_proxy branch 2 times, most recently from a12c4cc to 79651da Compare February 2, 2017 09:33
@pires pires force-pushed the refactor_kube_proxy branch from 79651da to ce1a774 Compare February 2, 2017 10:25
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2017
@k8s-github-robot
Copy link

@pires PR needs rebase

@pires pires closed this Feb 27, 2017
@pires pires deleted the refactor_kube_proxy branch February 27, 2017 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor kube-proxy to remove os conditional checks
6 participants