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

Support for supplying NATOutgoingAddress to Felix #1089

Merged
merged 2 commits into from Jun 5, 2019

Conversation

Projects
None yet
4 participants
@maxstr
Copy link
Contributor

commented May 29, 2019

Description

This is a PR required for addressing projectcalico/calico#2222. Addressing the issue also requires a change in Felix which I will link below. We chose to implement this by allowing users to specify the IP address being used rather than using the BGP address by default because it seemed more flexible. For our use case we are using the Kubernetes downward API to supply this address. I've tested this by setting/unsetting FELIX_NATOUTGOINGADDRESS in a calico node container using our modified felix.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Support for specifying explicit IP to be used for outgoing NAT

@maxstr maxstr force-pushed the bloomberg:add_nat_outgoing branch from e205dd7 to aab15a2 May 29, 2019

@neiljerram
Copy link
Member

left a comment

An initial overall question. Also, am I right that this is not yet complete working code? I'm pretty sure that this change is not sufficient on its own (even within libcalico-go, I mean) because it's missing code to convert NATOutgoingAddress in v3.IPPool to a corresponding field in model.IPPool (in lib/backend/syncersv1/updateprocessors/ippoolprocessor.go).

Show resolved Hide resolved lib/apis/v3/felixconfig.go
@neiljerram

This comment has been minimized.

Copy link
Member

commented May 31, 2019

OK, to follow up on my previous comment... I see now that:

  • You did the Felix change first, using the downward API to set the config in the environment, and that gives you a complete working solution.
  • You then added this PR, following @fasaxc's request based on a general desire we have for the data model to be in sync with Felix configuration options. But you're not relying on the data model for your own solution, hence not picking up that the PR here is incomplete.
  • In addition, there are considerations that if we're going to have data model for this, perhaps there are better options.

Let me think through a bit more what the upshot of all that is. Will write again shortly.

@neiljerram

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@maxstr I'm sorry, my comment above about IPPool and missing conversion code was complete rubbish. Of course the new field is in FelixConfiguration, not IPPool, and there is no per-field conversion code needed for FelixConfiguration.

Also, regarding @caseydavenport 's comment that I pointed to... given that these PRs already Just Work for you, with the downward API, I think we should bank them as they are, and NATOutgoingCIDR or NATOutgoingInterface can be added by further enhancements later if there is sufficient motivation for them.

So, this PR is good to go and I'll now merge it.

@neiljerram neiljerram merged commit 7cda6d7 into projectcalico:master Jun 5, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details
@neiljerram

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@maxstr We'll also need a docs PR to add the new field at https://github.com/projectcalico/calico/blob/master/master/reference/felix/configuration.md and https://github.com/projectcalico/calico/blob/master/master/reference/resources/felixconfig.md. Would you like to do that? I'm happy to do it if you prefer. It's just a matter of credit where credit's due, I think, but we do need it in the next day or two, in time for the upcoming Calico 3.8 release.

@maxstr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Thanks for the merge and the review! Will try and get you a PR by EOD for the doc updates.

@caseydavenport

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Was there ever a docs PR merged for this feature?

@caseydavenport

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Nvm, I see that it is present in the reference material for v3.8.

@maxstr maxstr deleted the bloomberg:add_nat_outgoing branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.