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

Extend CiliumNetworkPolicy validation to allow IPv6 CIDRs #4004

Merged
merged 6 commits into from May 11, 2018

Conversation

@joestringer
Contributor

joestringer commented May 4, 2018

On Kubernetes versions 1.9 and later, we validate the contents of CiliumNetworkPolicy resources when they are applied to the kubernetes cluster. Previously, the regex for validating CIDR policies would only allow IPv4 prefixes, so if a user attempts to install a CIDR policy that allows IPv6 traffic, the validator would reject it with an error like:

validation failure list:
must validate one and only one schema (oneOf)
spec.egress.toCIDR in body should match '^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/([0-9]|[1-2][0-9]|3[0-2])$'

This PR loosens the restrictions on CIDR policies to allow IPv6 addresses to be specified as well.

Note that if you deploy CIDR IPv6 policies, then attempt to downgrade to a version that does not contain this patch, then Cilium will go into a bad state because it is unable to apply your previous policy. The minimum expected versions when using CIDR IPv6 policies are v1.0.2 or v1.1.

Fixes: #4000

@raybejjani

This comment has been minimized.

Contributor

raybejjani commented May 4, 2018

test-me-please

@raybejjani

This seems fine to me, but I added aanm in case there's something I missed

}
// The below is always false, we shouldn't hit this with valid
// CIDR prefixes.
c.Assert(input, Equals, "failed to match CIDR.OneOf[*].Pattern")

This comment has been minimized.

@raybejjani

raybejjani May 4, 2018

Contributor

This assert is fatal to the test, right? From the code structure, it looks like you intended to continue the outer loop, though.

This comment has been minimized.

@joestringer

joestringer May 4, 2018

Contributor

That's fine, these are all positive tests so if this ever triggers, then something is wrong. I manually added some counter-examples in local testing and observed that this causes the test to fail.

A subsequent update could extend this testsuite to add some other negative examples.

This comment has been minimized.

@joestringer

joestringer May 5, 2018

Contributor

I added negative tests.

@@ -77,3 +78,29 @@ func (s *CiliumV2RegisterSuite) TestNeedsUpdateCorruptedVersion(c *C) {
c.Assert(needsUpdate(crd), Equals, true)
}
func (s *CiliumV2RegisterSuite) TestCIDRRegex(c *C) {
testCIDRs := []string{

This comment has been minimized.

@raybejjani

raybejjani May 4, 2018

Contributor

Since the regex is inscrutable, is there any chance we can add some more IPv6 addresses here? in particular, longer prefixes and ones with the middle 0s filled in (I'm pretty sure that is legal and the :: is just a convenience).

This comment has been minimized.

@joestringer

joestringer May 5, 2018

Contributor

Sure, I added a few examples. Let me know if you have any more that you'd like to test.

This comment has been minimized.

@raybejjani

raybejjani May 7, 2018

Contributor

I think that covers it. I just didn't want to miss a more verbose address. thanks!

@raybejjani raybejjani requested a review from aanm May 4, 2018

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from 94edb8c to 676b261 May 5, 2018

@aanm

aanm approved these changes May 7, 2018

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from 676b261 to d3b7398 May 8, 2018

@aanm

This comment has been minimized.

Member

aanm commented May 8, 2018

@joestringer it seems the test has failed, if it is a flake please not forget to create a GH issue

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 8, 2018

Failed due to #4042.

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from d3b7398 to ee31686 May 9, 2018

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 9, 2018

test-me-please

@aanm

This comment has been minimized.

Member

aanm commented May 10, 2018

@joestringer removed the ready-to-merge.

Yesterday we have discussed that this PR will be merged but we need to add a note in the documentation that a downgrade will not be possible for users that create an IPv6 cidr-based policy. Can you add this note in the documentation? Probably in the upgrade section.

cc @raybejjani

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 10, 2018

@aanm @raybejjani any thoughts on how that affects 1.0 backport? Should we avoid backporting for this reason?

@joestringer joestringer added the wip label May 10, 2018

@aanm

This comment has been minimized.

Member

aanm commented May 10, 2018

@joestringer this will be backported therefore the downgrade concerns

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from ee31686 to f109cee May 10, 2018

@joestringer joestringer requested a review from cilium/docs as a code owner May 10, 2018

@joestringer joestringer added pending-review and removed wip labels May 10, 2018

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 10, 2018

@aanm @raybejjani Please look over the last commit and suggest any relevant wording changes. I've also updated this PR description to help users that may find this PR via the docs.

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 10, 2018

test-docs-please

@joestringer joestringer changed the title from Extend CRD validation to allow IPv6 CIDRs to Extend CiliumNetworkPolicy validation to allow IPv6 CIDRs May 10, 2018

@ianvernon

This comment has been minimized.

Contributor

ianvernon commented May 10, 2018

@aanm wanted to confirm before merging that this is good to go, is it?

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 10, 2018

@rlenglet had asked me to ensure that we aren't supporting IPv4-compatible IPv6 addresses, and we aren't. It doesn't really make sense for us to support this, so it's fine.

@rlenglet

The regexp still allows IPv4-mapped IPv6 addresses: https://en.wikipedia.org/wiki/IPv6#IPv4-mapped_IPv6_addresses
Those shouldn't be allowed, as I'm not sure we're handling those properly.

`{1,4})?:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}` +
`))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]` +
`{1,4}){0,2}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d))` +
`{3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:` +

This comment has been minimized.

@rlenglet

rlenglet May 10, 2018

Contributor

The alternative on this line and the next is allowing the dot notation for IPv4-mapped IPv6 addresses.

`{3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:` +
`[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|` +
`1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|` +
`((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d` +

This comment has been minimized.

@rlenglet

rlenglet May 10, 2018

Contributor

This one too.

This comment has been minimized.

@joestringer

joestringer May 10, 2018

Contributor

I can't figure out why the regex isn't matching IPv4-mapped IPv6 addresses, but I can deconstruct the regex and strip out the IPv4 references.

k8s: Support IPv6 addresses in CIDR policy
Signed-off-by: Joe Stringer <joe@covalent.io>

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from f109cee to 050884f May 11, 2018

@rlenglet please re-review. There's multiple commits to show the progression of the regex, though the final one isn't crazy to read.

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 11, 2018

test-me-please

@@ -16,6 +16,7 @@ package v2
import (
. "gopkg.in/check.v1"
"regexp"

This comment has been minimized.

@rlenglet

rlenglet May 11, 2018

Contributor

We usually change the order of imports, and always group the stdlib imports first.

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from 050884f to 4a580d0 May 11, 2018

joestringer added some commits May 4, 2018

k8s: Add CRD IP address validation unit tests
Signed-off-by: Joe Stringer <joe@covalent.io>
docs: Describe downgrade impact of IPv6 CRD validation
Signed-off-by: Joe Stringer <joe@covalent.io>
k8s: CIDR: Expand v6 regex to make it more readable
No functional changes.

Signed-off-by: Joe Stringer <joe@covalent.io>
k8s: CIDR: Disallow IPv4-mapped IPv6 addresses
Signed-off-by: Joe Stringer <joe@covalent.io>
k8s: CIDR: Format IPv6 CIDR regex
The precheck script complains if you don't consistently indent things,
appease it even if it makes the regex harder to read.

Signed-off-by: Joe Stringer <joe@covalent.io>

@joestringer joestringer force-pushed the joestringer:submit/crv-ipv6 branch from 4a580d0 to 2af37b2 May 11, 2018

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 11, 2018

test-me-please

@joestringer

This comment has been minimized.

Contributor

joestringer commented May 11, 2018

test-docs-please

@ianvernon ianvernon merged commit f335a3a into cilium:master May 11, 2018

3 checks passed

Cilium-Doc-Test Build finished.
Details
Cilium-Ginkgo-Tests Build finished.
Details
Hound No violations found. Woof!

@joestringer joestringer deleted the joestringer:submit/crv-ipv6 branch May 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment