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

Add flag to masquerade all in kube-proxy when using iptables proxier #12986

Merged
merged 1 commit into from
Aug 21, 2015
Merged

Add flag to masquerade all in kube-proxy when using iptables proxier #12986

merged 1 commit into from
Aug 21, 2015

Conversation

BenTheElder
Copy link
Member

This closes #12843

@BenTheElder
Copy link
Member Author

/cc @thockin @MikaelCluseau

@k8s-bot
Copy link

k8s-bot commented Aug 20, 2015

GCE e2e build/test passed for commit 879cf1a6484072088c15f678ab34ac357e535838.

@mcluseau
Copy link
Contributor

LGTM, thanks :-)
Maybe some test coverage (e2e?) will be required, though.

@k8s-bot
Copy link

k8s-bot commented Aug 20, 2015

GCE e2e build/test passed for commit dcc0c0bc6b39c39f1731ae8b709c70be384a2827.

@BenTheElder
Copy link
Member Author

Yw! Just doing a few more little things before GSoC ends.

I think in general we need a lot more e2e. It's on @thockin's list of
kube-proxy todos. (discussion here:
#12764 (comment))
On Aug 20, 2015 3:19 PM, "Mikaël Cluseau" notifications@github.com wrote:

LGTM, thanks :-)
Maybe some test coverage (e2e?) will be required, though.

—
Reply to this email directly or view it on GitHub
#12986 (comment)
.

@thockin
Copy link
Member

thockin commented Aug 20, 2015

Yeah e2e is on me before we can enable this new proxier. Time ran out for
GSOC :(

On Thu, Aug 20, 2015 at 12:56 PM, Benjamin Elder notifications@github.com
wrote:

Yw! Just doing a few more little things before GSoC ends.

I think in general we need a lot more e2e. It's on @thockin's list of
kube-proxy todos. (discussion here:
#12764 (comment)
)
On Aug 20, 2015 3:19 PM, "Mikaël Cluseau" notifications@github.com
wrote:

LGTM, thanks :-)
Maybe some test coverage (e2e?) will be required, though.

—
Reply to this email directly or view it on GitHub
<
#12986 (comment)

.

—
Reply to this email directly or view it on GitHub
#12986 (comment)
.

thockin
thockin reviewed Aug 20, 2015
View reviewed changes
cmd/kube-proxy/app/server.go
@@ -61,6 +61,7 @@ type ProxyServer struct {
ForceUserspaceProxy bool
SyncPeriod time.Duration
nodeRef *api.ObjectReference // Reference to this node.
SnatAll bool
Copy link
Member

Choose a reason for hiding this comment

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

don't mix-case acronyms in Go.

MasqueradeAll or SNATAll.

@thockin
Copy link
Member

thockin commented Aug 20, 2015

Naming, then LGTM

@mcluseau
Copy link
Contributor

On 08/21/2015 09:24 AM, Tim Hockin wrote:

Yeah e2e is on me before we can enable this new proxier. Time ran out for
GSOC :(

I'll try to help, but I haven't coded much in/around Kubernetes. I'm
prototyping some things interacting with Kubernetes to get at a
reasonable level.

@thockin
Copy link
Member

thockin commented Aug 20, 2015

Any help is appreciated! e2e is tedious but SO SO important. I have a
sketch of the interesting cases we need to make sure are covered

On Thu, Aug 20, 2015 at 3:31 PM, Mikaël Cluseau notifications@github.com
wrote:

On 08/21/2015 09:24 AM, Tim Hockin wrote:

Yeah e2e is on me before we can enable this new proxier. Time ran out for
GSOC :(

I'll try to help, but I haven't coded much in/around Kubernetes. I'm
prototyping some things interacting with Kubernetes to get at a
reasonable level.

—
Reply to this email directly or view it on GitHub
#12986 (comment)
.

@mcluseau
Copy link
Contributor

Okay, I'll pick one, or you can assign me one to begin with. I'll probably only have week-end time on this, though.

@BenTheElder
Copy link
Member Author

Renamed everything to MasqueradeAll since it's more precise.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2015
@thockin
Copy link
Member

thockin commented Aug 20, 2015

LGTM

@k8s-bot
Copy link

k8s-bot commented Aug 20, 2015

GCE e2e build/test passed for commit cbfaf2d9ce594b5d4c7fa47bd14ea7ac0ed5ec2e.

@BenTheElder
Copy link
Member Author

Will squash in a sec.
On Aug 20, 2015 7:08 PM, "Tim Hockin" notifications@github.com wrote:

LGTM

—
Reply to this email directly or view it on GitHub
#12986 (comment)
.

@BenTheElder
Copy link
Member Author

Gotta fix the known-flags file again. Will fixup.

@BenTheElder
Copy link
Member Author

Should be fixed.

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2015

GCE e2e build/test failed for commit 17afba8a3ce4f743ae42debe7b880b4ffd6a39c1.

@thockin
Copy link
Member

thockin commented Aug 21, 2015

Still lgtm

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2015

GCE e2e build/test failed for commit 1f2076c.

@BenTheElder
Copy link
Member Author

For e2e: Looks like all the tests passed but the build timed out?

@thockin
Copy link
Member

thockin commented Aug 21, 2015

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2015

GCE e2e build/test failed for commit 1f2076c.

@BenTheElder
Copy link
Member Author

huh.

Summarizing 2 Failures:

[Fail] Services [It] should serve multiport endpoints from pods 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/service.go:1126

[Fail] Services [It] should serve a basic endpoint from pods 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/service.go:1126

Ran 84 of 146 Specs in 583.312 seconds
FAIL! -- 82 Passed | 2 Failed | 2 Pending | 60 Skipped

@thockin
Copy link
Member

thockin commented Aug 21, 2015

If you run it by hand - does it pass?

@thockin
Copy link
Member

thockin commented Aug 21, 2015

@k8s-bot test this please

@BenTheElder
Copy link
Member Author

Not sure yet, I didn't expect e2e to be effected, it should still be
running the userspace proxy (?)

On Fri, Aug 21, 2015 at 1:12 AM, Tim Hockin notifications@github.com
wrote:

@k8s-bot https://github.com/k8s-bot test this please

—
Reply to this email directly or view it on GitHub
#12986 (comment)
.

@thockin
Copy link
Member

thockin commented Aug 21, 2015

I kicked the tester again. let's see...

On Thu, Aug 20, 2015 at 10:15 PM, Benjamin Elder notifications@github.com
wrote:

Not sure yet, I didn't expect e2e to be effected, it should still be
running the userspace proxy (?)

On Fri, Aug 21, 2015 at 1:12 AM, Tim Hockin notifications@github.com
wrote:

@k8s-bot https://github.com/k8s-bot test this please

—
Reply to this email directly or view it on GitHub
<
#12986 (comment)

.

—
Reply to this email directly or view it on GitHub
#12986 (comment)
.

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2015

GCE e2e build/test passed for commit 1f2076c.

jszczepkowski added a commit that referenced this pull request Aug 21, 2015
@jszczepkowski
Merge pull request #12986 from BenTheElder/masquerade_all_flag
3df1b9e 
Add flag to masquerade all in kube-proxy when using iptables proxier
@jszczepkowski jszczepkowski merged commit 3df1b9e into kubernetes:master Aug 21, 2015
@wojtek-t wojtek-t mentioned this pull request Aug 24, 2015
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
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kube-proxy: iptables implementation: allow a fallback flag to MASQUERADE pod source IPs
6 participants
@BenTheElder @k8s-bot @mcluseau @thockin @googlebot @jszczepkowski