cloudfoundry-attic / warden

Cloud Foundry - the open platform as a service project
Apache License 2.0
283 stars 108 forks source link

Use MASQUERADE instead of SNAT for container NAT #89

Closed sykesm closed 9 years ago

sykesm commented 9 years ago

When warden is running on hosts with multiple networks, the address associated with the default route should not always be used as the source of the traffic. By using MASQUERADE, the packets will pick up the address associated with adapter used to flow the request.

cfdreddbot commented 9 years ago

Hey sykesm!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

cf-gitbot commented 9 years ago

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/94296812.

goonzoid commented 9 years ago

Thanks @sykesm. The team will try to take a look at this before the end of the week.

glyn commented 9 years ago

Will:

--source ${POOL_NETWORK} \
      ! --destination ${POOL_NETWORK} \

use the correct adapter address for traffic between two containers on the same host?

sykesm commented 9 years ago

Traffic between two containers on the same host will not be subjected to NAT; packets arriving at the destination will arrive with the source address of the container.

Without ! --destination #{POOL_NETWORK}, packets will appear to come from the host.

In addition to resolving the multi-home networking issues we have in our deployments, it resolves the issue we have with bosh-lite where container-to-container traffic arrives with the IP of the host instead of the source.

goonzoid commented 9 years ago

Sounds good to me. Happy to have this merged if @glyn is.

glyn commented 9 years ago

Yes.

sykesm commented 9 years ago

Given the conversation above, it sounds like this is likely to be merged. Do you want the garden equivalent code done via PR or is that something you'd like to handle in the garden team?

Thanks.

goonzoid commented 9 years ago

I expect the timing isn't great right now, as we're knee deep in porting most of garden-linux to Go, but @glyn is better placed to comment on that.

If it's not appropriate now, I'll make a story in the backlog to make sure it happens when the Go stuff has landed.

glyn commented 9 years ago

Porting everything to Go is going to take a while, so feel free to send in a PR (assuming this will include suitable tests). We can always refactor it if the code moves significantly before we merge it. I'd rather keep warden and garden in step.

craigfurman commented 9 years ago

Thanks @sykesm!

iamralch commented 9 years ago

@sykesm: We are rejecting this pull request, because there is a failing test.

sykesm commented 9 years ago

@svett as I said in #i91, it's fair to revert but there's no pointer here to what was failing or how to recreate it.

badie commented 9 years ago

Hey @sykesm, It looks like master was broken before the PR. We reapplied the commit and we are going to fix the failing tests on master.

Thanks!