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

Bridge off-cluster traffic into services by masquerading. #24429

Merged
merged 1 commit into from
Apr 20, 2016
Merged

Bridge off-cluster traffic into services by masquerading. #24429

merged 1 commit into from
Apr 20, 2016

Conversation

cjcullen
Copy link
Member

Allows service IPs to be accessed from outside of the cluster. This requires that the service range be routed to a node (or several nodes) in the cluster.

Addresses #24224.

@thockin @ArtfulCoder

@thockin
Copy link
Member

thockin commented Apr 18, 2016

I am flagging this for Cherrypick

@thockin thockin added this to the v1.2 milestone Apr 18, 2016
thockin
thockin reviewed Apr 18, 2016
View reviewed changes
cmd/kube-proxy/app/options/options.go
@@ -75,6 +75,7 @@ func (s *ProxyServerConfig) AddFlags(fs *pflag.FlagSet) {
fs.DurationVar(&s.IPTablesSyncPeriod.Duration, "iptables-sync-period", s.IPTablesSyncPeriod.Duration, "How often iptables rules are refreshed (e.g. '5s', '1m', '2h22m'). Must be greater than 0.")
fs.DurationVar(&s.ConfigSyncPeriod, "config-sync-period", s.ConfigSyncPeriod, "How often configuration from the apiserver is refreshed. Must be greater than 0.")
fs.BoolVar(&s.MasqueradeAll, "masquerade-all", s.MasqueradeAll, "If using the pure iptables proxy, SNAT everything")
fs.StringVar(&s.ClusterCIDR, "cluster-cidr", s.ClusterCIDR, "The CIDR range of Pods in the Cluster.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't capitalize Pod and Cluster here :)

say what happens if this is not provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Apr 18, 2016
thockin
thockin reviewed Apr 18, 2016
View reviewed changes
pkg/proxy/iptables/proxier.go
@@ -713,6 +715,9 @@ func (proxier *Proxier) syncProxyRules() {
if proxier.masqueradeAll {
writeLine(natRules, append(args, "-j", string(kubeMarkMasqChain))...)
}
if len(proxier.clusterCIDR) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This will write once per-service, won't it? Can you achieve the same effect by writing once at ~L687

Copy link
Member

Choose a reason for hiding this comment

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

or is that too broad? Hmm, don't do it yet, let me think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought about that too. I copied the functionality of --masquerade-all

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you're right. It's unfortunate, but any simpler solution is either too broad or costs the same number of rules.

@k8s-bot
Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit 20fb8c70da6580e8e3a92242710a1689698ae7c0.

@k8s-bot
Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit 8aaf6466040d01a16e74eeafd3d8dde12e2913f7.

@cjcullen cjcullen force-pushed the bastion branch 2 times, most recently from eb8b18f to 8cbc137 Compare April 19, 2016 00:04
thockin
thockin reviewed Apr 19, 2016
View reviewed changes
cmd/kube-proxy/app/options/options.go
@@ -75,6 +75,7 @@ func (s *ProxyServerConfig) AddFlags(fs *pflag.FlagSet) {
fs.DurationVar(&s.IPTablesSyncPeriod.Duration, "iptables-sync-period", s.IPTablesSyncPeriod.Duration, "How often iptables rules are refreshed (e.g. '5s', '1m', '2h22m'). Must be greater than 0.")
fs.DurationVar(&s.ConfigSyncPeriod, "config-sync-period", s.ConfigSyncPeriod, "How often configuration from the apiserver is refreshed. Must be greater than 0.")
fs.BoolVar(&s.MasqueradeAll, "masquerade-all", s.MasqueradeAll, "If using the pure iptables proxy, SNAT everything")
fs.StringVar(&s.ClusterCIDR, "cluster-cidr", s.ClusterCIDR, "The CIDR range of pods in the cluster. It is used to masquerade traffic coming from outside of the cluster. If not provided, no off-cluster masquerading will be performed.")
Copy link
Member

Choose a reason for hiding this comment

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

s/masquerade/bridge ?

@thockin
Copy link
Member

thockin commented Apr 19, 2016

Only a nit on the flag description.

@thockin thockin added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 19, 2016
@thockin thockin changed the title Masquerade traffic from off-cluster going through kube-proxy. Bridge off-cluster traffic into services by masquerading. Apr 19, 2016
@thockin
Copy link
Member

thockin commented Apr 19, 2016

I re-titled this for release note

@thockin
Copy link
Member

thockin commented Apr 19, 2016

feel free to self-LGTM when nit is done.

@k8s-bot
Copy link

k8s-bot commented Apr 19, 2016

GCE e2e build/test passed for commit eb8b18f5dddabfa0896ca7fce3fdc957f95d38ac.

@k8s-bot
Copy link

k8s-bot commented Apr 19, 2016

GCE e2e build/test passed for commit 55855778134d37a41d5faa16791a8400ad7a3447.

@k8s-bot
Copy link

k8s-bot commented Apr 19, 2016

GCE e2e build/test passed for commit 8cbc137de76534ad5a6be1dfd4c0961bd1c560ed.

@cjcullen cjcullen assigned thockin and unassigned erictune Apr 19, 2016
@cjcullen cjcullen added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 19, 2016

GCE e2e build/test passed for commit 0dd7cf8e881c1ba816e22a5ce1808bc29ccbe1df.

@cjcullen
Copy link
Member Author

@pwittrock Any info on how to dig into the node e2e failure?

@pwittrock
Copy link
Member

@cjcullen The node e2e tests don't post comments (it was decided that adding yet another comment bot was too much spam), so the details link is the best way. If you have jenkins access, you can also find the build there.

build logs

In this case the issue seems to be:

Failure [10.173 seconds]
15:40:09 PrivilegedPod
15:40:09 /var/lib/jenkins/workspace/node-pull-build-e2e-test/go/src/k8s.io/kubernetes/test/e2e_node/privileged_test.go:84
15:40:09 should test privileged pod [It]
15:40:09 /var/lib/jenkins/workspace/node-pull-build-e2e-test/go/src/k8s.io/kubernetes/test/e2e_node/privileged_test.go:83
15:40:09
15:40:09 Error running command "curl -q 'http://172.17.0.2:8080/shell?shellCommand=ip+link+add+dummy1+type+dummy'": error executing remote command: error executing command in container: Error executing in Docker Container: 7
15:40:09 Expected error:
15:40:09 <*errors.errorString | 0xc8204ca400>: {
15:40:09 s: "error executing remote command: error executing command in container: Error executing in Docker Container: 7",
15:40:09 }
15:40:09 error executing remote command: error executing command in container: Error executing in Docker Container: 7
15:40:09 not to have occurred
15:40:09
15:40:09 /var/lib/jenkins/workspace/node-pull-build-e2e-test/go/src/k8s.io/kubernetes/test/e2e_node/privileged_test.go:149

I filed #24508 for this since I didn't see any other issues with the same symptom.

@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 0dd7cf8e881c1ba816e22a5ce1808bc29ccbe1df.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 0dd7cf8e881c1ba816e22a5ce1808bc29ccbe1df.

@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 20, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7dd0776 into kubernetes:master Apr 20, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 7605687.

@zmerlynn
Copy link
Member

@karlkfi - Can you help @cjcullen with the Mesosphere failure? I'm going to cherrypick-approve this for now, but I don't want to bust you guys completely, either.

@zmerlynn zmerlynn added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 20, 2016
@cjcullen cjcullen mentioned this pull request Apr 20, 2016
Merged
@andyzheng0831 andyzheng0831 mentioned this pull request Apr 20, 2016
Merged
zmerlynn added a commit that referenced this pull request Apr 20, 2016
@zmerlynn
Merge pull request #24517 from cjcullen/automated-cherry-pick-of-#244…
01f52a0 
…29-upstream-release-1.2

Automated cherry pick of #24429
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-github-robot pushed a commit that referenced this pull request Apr 20, 2016
Merge pull request #24531 from andyzheng0831/manifest
70bfe6a 
Automatic merge from submit-queue

Trusty: Handle the new var in kube-proxy manifest

This is to capture the kube-proxy manifest change in PR #24429.

@roberthbailey @fabioy @zmerlynn please review this change and mark it as cherry pick candidate. We need to catch up 1.2.3 release.

cc/ @dchen1107 @wonderfly @cjcullen FYI.

I have verified this fix. Without this fix, kube-proxy pod in Trusty nodes cannot be started correctly, i.e., the command line has an unhadled variable. And some other kube-system pods do not work correctly as kube-proxy is not working well. After applying this fix, kube-proxy can be started correctly, and all kube-system pods run successfully.
@cjcullen cjcullen mentioned this pull request Apr 22, 2016
Closed
@karlkfi
Copy link
Contributor

karlkfi commented Apr 27, 2016

@zmerlynn looks like the k8sm CI has been hosed for a while. We'll have to fix master.

@thockin thockin mentioned this pull request Jun 11, 2016
Closed
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@zmerlynn
Merge pull request kubernetes#24517 from cjcullen/automated-cherry-pi…
3a3d67c 
…ck-of-#24429-upstream-release-1.2

Automated cherry pick of kubernetes#24429
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@zmerlynn
Merge pull request kubernetes#24517 from cjcullen/automated-cherry-pi…
aa87525 
…ck-of-#24429-upstream-release-1.2

Automated cherry pick of kubernetes#24429
@micahhausler micahhausler mentioned this pull request Jan 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thockin thockin

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet