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

OnlyLocal nodeports #33587

Merged
merged 2 commits into from
Oct 1, 2016
Merged

OnlyLocal nodeports #33587

merged 2 commits into from
Oct 1, 2016

Conversation

bprashanth
Copy link
Contributor

@bprashanth bprashanth commented Sep 27, 2016 •
edited by k8s-oncall
Loading

90% unittests.
Code changes:

NodePorts still don't get firewalls: #33586

This change is Reviewable

@bprashanth bprashanth added this to the v1.5 milestone Sep 27, 2016
@bprashanth
Copy link
Contributor Author

I guess I should write an e2e as well

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 27, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 554e649d10cc19be6159a7b398eae22cd7e1d6c0. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

freehan
freehan reviewed Sep 27, 2016
View reviewed changes
pkg/proxy/iptables/proxier.go
@@ -1173,6 +1177,16 @@ func (proxier *Proxier) syncProxyRules() {
localEndpointChains = append(localEndpointChains, endpointChains[i])
}
}
// First rule in the chain redirects all pod -> external vip traffic to the
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand this rule. If you jump to KUBE-SVC chain from KUBE-XLB chain. Then it may reach any backend pods right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see the bug now. You want to allow pods on the node to access the LB.

@freehan
Copy link
Contributor

freehan commented Sep 27, 2016

LGTM

cherry pick it for 1.4? Or since it is alpha, do not care.

@freehan freehan added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Sep 27, 2016
@k8s-github-robot k8s-github-robot added release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Sep 27, 2016
@bprashanth
Copy link
Contributor Author

@k8s-bot test this github issue: #33611

@kdima
Copy link

kdima commented Sep 28, 2016

@bprashanth you mentioned that this might partially fix #33081. Which part does this fix?

@bprashanth
Copy link
Contributor Author

bprashanth commented Sep 28, 2016 •
edited
Loading

@kdima the blackholing part. You just get dnatted to your endpoints, not out to the public lb and back down to your endpoints.

@bprashanth
Copy link
Contributor Author

@k8s-bot test this github issue: #33617

@kdima
Copy link

kdima commented Sep 29, 2016 •
edited
Loading

@bprashanth I have just tried out this patch on our cluster and it still does not seem to work. Turns out I made a mistake in the setup. Everything seems to work! Thanks for fixing this

@bprashanth bprashanth 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 Sep 30, 2016
@bprashanth
Copy link
Contributor Author

@kdima thanks!

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2016
@bprashanth
Copy link
Contributor Author

I didn't actually change anything, just rebased, so I'm re-applying lgtm

@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 06cbb36. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@bprashanth
Copy link
Contributor Author

I would say the failure is from this pr at this point but I've seen identical failures across the board.

@bprashanth
Copy link
Contributor Author

@k8s-bot test this github issue: #33617

@bprashanth bprashanth removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 30, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 06cbb36. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@bprashanth
Copy link
Contributor Author

@k8s-bot unit test this github issue: #33865

@k8s-github-robot k8s-github-robot merged commit 5f216ca into kubernetes:master Oct 1, 2016
@j3ffml j3ffml mentioned this pull request Oct 5, 2016
Closed
@j3ffml
Copy link
Contributor

j3ffml commented Oct 5, 2016

This change is prime suspect for drastic increase in flakiness of the [Feature:ExternalTrafficLocalOnly] e2e test.

@bprashanth bprashanth mentioned this pull request Oct 7, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 10, 2016
Merge pull request #34360 from bprashanth/np_fix
9e2f868 
Automatic merge from submit-queue

Remove onlyLocal NodePort e2e till pr #33957

We were basically testing this bug: #30809
We fixed the bug: #33587, but forgot to remove the "test". 
This pr adds a test for the new feature: #33957 (ensure that nodePort with onlyLocal works only on nodes with endpoints and fails otherwise)

fixes #34124
@bprashanth bprashanth mentioned this pull request Oct 30, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freehan freehan freehan left review comments

Assignees

@freehan freehan

@thockin thockin

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

Networking: external LB in 1.4.0-beta.8 blackholes external ip traffic
9 participants
@bprashanth @k8s-ci-robot @freehan @kdima @j3ffml @mandarjog @googlebot @thockin @k8s-github-robot