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

Added NAT_Outgoing parameter to Calico Node #861

Merged

Conversation

VincentS
Copy link
Contributor

@VincentS VincentS commented Jun 20, 2017

This patch adds an NAT_Outgoing Parameter for IPv4 / IPv6 to Calico Node to be configurable on startup. This PR was moved from calicoctl Repository projectcalico/calicoctl#1640

Todos

  • Tests
  • Documentation

Release note

Ability to enable / disable outgoing NAT on the default IP Pool using an environment variable.

@tmjd tmjd self-assigned this Jun 20, 2017
@VincentS VincentS force-pushed the Calico_NAT_Outgoing_Flag branch from 22c49f3 to bde9a5a Compare June 20, 2017 15:00
@VincentS VincentS changed the title [WIP] Added NAT_Outgoing parameter to Calico Node Added NAT_Outgoing parameter to Calico Node Jun 20, 2017
@VincentS VincentS force-pushed the Calico_NAT_Outgoing_Flag branch from bde9a5a to efc352d Compare June 20, 2017 15:34
@@ -138,7 +138,11 @@ def test_default_pools(self, success_expected, param, value, exp_num_pools, ipip
"Didn't find ipip mode in pool %s" % pool

# Check NAT setting
assert pool['spec']['nat-outgoing'] is True, "Didn't find nat enabled in pool %s" % pool
if param == "CALICO_IPV4POOL_CIDR":
Copy link
Member

Choose a reason for hiding this comment

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

From the test error, it looks like you need to check if nat-outgoing is present in pool['spec'].
I think you could just switch the whole block here to something like:

if "nat-outgiong" in pool['spec']:
  assert pool['spec']['nat-outgoing'] is nat_outgoing, \
    "Wrong NAT default in pool %s, expected nat-outgoing to be %s" % (pool, nat_outgoing)
else:
  assert nat_outgoing is False, \
    "Wrong NAT default in pool %s, expecting nat-outgoing to be disabled" % pool

I don't think it is necessary to indicate v4 or v6.

@tmjd
Copy link
Member

tmjd commented Jun 20, 2017

LGTM
@VincentS could you rebase onto master and squash your commits into one please.
@projectcalico/calico-maintainers PTAL

@VincentS VincentS force-pushed the Calico_NAT_Outgoing_Flag branch from 4b22aea to 77c3308 Compare June 21, 2017 06:11
@VincentS
Copy link
Contributor Author

VincentS commented Jun 21, 2017

Rebased and squashed the commits
Documentation update can be found at #775

@nelljerram
Copy link
Member

@tmjd I took a very quick look, but think you need @robbrockbank 's eyes here.

@robbrockbank
Copy link
Contributor

robbrockbank commented Jun 21, 2017

@VincentS LGTM.

I think it's just missing a minor docs update. Would you mind adding an entry to the following doc?:

https://github.com/projectcalico/calico/blob/master/master/reference/node/configuration.md

If you prefer we can merge this as is and we'll add it later - let me know.

@tmjd
Copy link
Member

tmjd commented Jun 21, 2017

@robbrockbank The PR #775 exists for the documentation. That PR existed before calico_node was moved into this repo. Do you think that needs to be merged with this PR? I'm of the opinion that it doesn't matter.

@robbrockbank
Copy link
Contributor

@tmjd TBH I didn't spot that PR, I'm happy to merge this and update the docs later.

@robbrockbank robbrockbank merged commit 4f65624 into projectcalico:master Jun 21, 2017
@caseydavenport caseydavenport added this to the Calico v2.4.0 milestone Jul 13, 2017
@caseydavenport caseydavenport added the release-note-required Change has user-facing impact (no matter how small) label Jul 13, 2017
@caseydavenport caseydavenport mentioned this pull request Jul 31, 2017
3 tasks
caseydavenport pushed a commit that referenced this pull request Dec 14, 2021
…-master

[master] Semaphore Auto Pin Update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants