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

Add support for NATOutgoingAddress option #2028

Merged

Conversation

Projects
None yet
6 participants
@maxstr
Copy link
Contributor

commented May 29, 2019

Description

Related to projectcalico/libcalico-go#1089.

As in that PR, this is for addressing projectcalico/calico#2222. 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 the modified felix.

Todos

  • Unit tests (full coverage)
  • Integration tests (delete as appropriate) In plan/Not needed/Done
  • Documentation
  • Backport
  • Release note

Release Note

Support for specifying explicit IP to be used for outgoing NAT

@maxstr maxstr requested a review from projectcalico/core-maintainers as a code owner May 29, 2019

@fasaxc
Copy link
Member

left a comment

Change looks good, and thanks for adding tests!

There's one piece missing though. The new parameter should be added to the data model so that it can be configured through calicoctl too. This libcalico-go commit is an example of adding a new config param with validation.

@@ -225,6 +225,8 @@ type Config struct {

XDPEnabled bool `config:"bool;true"`
GenericXDPEnabled bool `config:"bool;false"`

NATOutgoingAddress net.IP `config:"ipv4;"`

This comment has been minimized.

Copy link
@fasaxc

fasaxc May 29, 2019

Member

Suggest moving this up to line 200 since it naturally groups with NATPortRange

@maxstr

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Added validation to my PR here: projectcalico/libcalico-go#1089. Looks like that PR will need to be merged and the Glidefile updated here in order to build?

@@ -48,18 +48,28 @@ func (r *DefaultRuleRenderer) MakeNatOutgoingRule(protocol string, action iptabl
func (r *DefaultRuleRenderer) NATOutgoingChain(natOutgoingActive bool, ipVersion uint8) *iptables.Chain {
var rules []iptables.Rule
if natOutgoingActive {
var defaultSnatRule iptables.Action = iptables.MasqAction{}
if r.Config.NATOutgoingAddress != nil {

This comment has been minimized.

Copy link
@steven-sheehy

steven-sheehy Jun 5, 2019

Can this also check if the NATOutgoingAddress exists on an interface and fallback to masquerading if not? My use case is for floating/virtual IPs that will only exist on one of the calico nodes. But even without using a VIP it is good error checking to validate the IP exists.

This comment has been minimized.

Copy link
@maxstr

maxstr Jun 5, 2019

Author Contributor

It seems like the correct place/time to check that would be on startup/reconfigure, but not sure if the correct behavior would be to error out or to warn and fallback. Happy to defer to the Calico folks on what it should be and implement it though.

This comment has been minimized.

Copy link
@neiljerram

neiljerram Jun 6, 2019

Member

@steven-sheehy It doesn't feel right to me to add that feature into this PR. I'd rather merge this PR first, then that can be added as a delta if it really needs a Felix code change.

Anyway, I wonder if this can be done (in a Kubernetes context) by combining an initContainer with the downward API? Something like:

  • use the downward API to populate an env var for an initContainer
  • get the initContainer to check that that address really exists
  • if so, also set it as FELIX_NATOUTGOINGADDRESS for the calico/node container.

This comment has been minimized.

Copy link
@steven-sheehy

steven-sheehy Jun 6, 2019

Ok, let's handle it in another PR.

About the initContainer, it can't be done solely at calico startup since this IP can come and go at any time and it needs calico to watch for interface changes and adjust the NAT rules.

@maxstr maxstr referenced this pull request Jun 5, 2019

Merged

Add NATOutgoingAddress documentation #2649

0 of 3 tasks complete

@neiljerram neiljerram referenced this pull request Jun 6, 2019

Closed

Add support for NATOutgoingAddress option #2035

0 of 5 tasks complete
@neiljerram
Copy link
Member

left a comment

@maxstr Thanks, I'm happy with the changes here. Now it just needs a rebase and addition of the glide pin changes. I've made a parallel PR at #2035 that does that, and which passes CI, so basically you just need to make this PR look like that one, by rebasing and adding the "Bump ..." commit.

(I think it will be better to merge this PR, rather than #2035, so you get the credit, and also because #2035 seems to have a problem with the CLAbot.)

@maxstr maxstr force-pushed the bloomberg:add_masquerade_snat_override branch from 5a94fa8 to ee18eea Jun 6, 2019

@maxstr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Rebased and cherry-picked your bump.

@neiljerram
Copy link
Member

left a comment

Thanks @maxstr , I'll merge once the CI goes through.

Data model change in libcalico-go is now done.

@neiljerram neiljerram merged commit a19ab45 into projectcalico:master Jun 6, 2019

2 checks passed

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

@caseydavenport caseydavenport added this to the Calico v3.8.0 milestone Jun 13, 2019

@maxstr maxstr deleted the bloomberg:add_masquerade_snat_override branch Jul 9, 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.