Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

update default pod/service cidr to be within carrier-nat shared IP block #904

Merged
merged 4 commits into from
Dec 4, 2019

Conversation

knisbet
Copy link
Contributor

@knisbet knisbet commented Nov 21, 2019

This is another issue we've seen surprise a few customers, where the IP range used for the overlay / service subnets conflicts with the IP space of the customers internal network, and the gravity installation prevents the communications from working.

The carrier grade NAT space is not meant for this purpose, but does have precedent where others are using it for the internal range for their kubernetes clusters. This will only really cause issues, if a customer wishes to deploy a gravity cluster in a location where it can be reached by devices in the cgnat space without the NAT. I'm not aware of any customers currently operating this way. It's also plausible that some customer networks will also use this IP space in a similar way. In these cases the installer flag to set the service/pod subnet can still be used.

Reference:
kubernetes/kops#7325
cloudposse/docs#455
https://aws.amazon.com/about-aws/whats-new/2018/10/amazon-eks-now-supports-additional-vpc-cidr-blocks/

Alternatives:
EKS appears to also support using 198.18.0.0/15 which is a rarer block in the reserved IP space. It's a smaller block than 10.64.0.0/10 which makes it a bit harder to pick visually distinct IP addresses for pods and services.

Risks:

  • This is not in line with RFC recommendations
  • It's still possible this will create conflicts down the line
  • Endpoint security software is less likely to have a policy that allows 100.64/16 compared to 10/8.

const defaultServiceSubnet = '10.100.0.0/16';
const defaultPodSubnet = '10.244.0.0/16';
const defaultServiceSubnet = '100.100.0.0/16';
const defaultPodSubnet = '100.96.0.0/16';
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also update the docs as well?

https://github.com/gravitational/gravity/blob/master/docs/6.x/installation.md#L115

Must be a minimum of /16 so Kubernetes is able to allocate /24 to each node. Defaults to 10.244.0.0/16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't planning on backporting this, so until we separate out the 7.x docs I don't think we should change them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess what I was trying to say that it needs to be updated in the docs too. I would create an issue so we do not forget to make this change in docs before 7.0 release.

@@ -89,419 +89,419 @@ storiesOf('GravityInstaller', module)
});
Copy link
Contributor

@a-palchikov a-palchikov Nov 21, 2019

Choose a reason for hiding this comment

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

Where is this change coming from? It seems to include more than just the new CIDR defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I saved it in vs-code it reformatted the doc, similar idea to running gofmt.

@knisbet knisbet merged commit 1443882 into master Dec 4, 2019
@knisbet knisbet deleted the kevin/experimental/default-network-cidrs branch December 4, 2019 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants