Add global() selector #1229

lmm · 2020-04-27T20:50:47Z

Description

This PR adds support for the global() keyword

Todos

Tests
Documentation
Release note

Release Note

Network policy now has the global() namespace selector which selects host endpoints or global network sets

1) Implement a Token Global for handling `global'

Add support for rules and selector for global keyword and add unit test case

caseydavenport

I think we need some validation that global() isn't specified in regular selectors, since IIUC it won't function properly in that case, right?

e.g.,

spec.Selector will never select non-namespaced things, so it should be imposible to put global() in that field.

caseydavenport · 2020-05-13T23:05:38Z

lib/apis/v3/policy.go

@@ -137,6 +137,9 @@ type EntityRule struct {
 	// For NetworkPolicy, an empty NamespaceSelector implies that the Selector is limited to selecting
 	// only workload endpoints in the same namespace as the NetworkPolicy.
 	//
+	// For NetworkPolicy, `global` NamespaceSelector implies that the Selector is limited to selecting


global -> "global()"

caseydavenport

LGTM, but we need validation to make sure that this selector is not used except in the NamespaceSelector field for NetworkPolicy, and/or main selector in GNPs, since IIUC it's not valid for use in the other selector fields.

bcreane · 2020-05-16T19:43:25Z

@lmm please check with @krishgobinath about adding validation - he's planning to add this in the next week or so.

nelljerram · 2020-06-09T10:19:48Z

@lmm Is there already a doc PR for this? I'd quite like to review that too, preferably before or at the same time as this one. (Or a design doc?)

Specifically, as well as what @caseydavenport says, I'm wondering if global() must be the entire content of any selector field where it is used, or if it is allowed (and makes sense) to combine with other expressions, e.g. global() && color == 'red'.

nelljerram

Am I right that this is mostly copied from our Enterprise code? Please can you list or identify the changes that are additional to that? (Because I think there's less value in reviewing Enterprise-copied code.)

If there isn't already a design or doc that says this, please can you also summarise, for me, where global() is allowed, and why?

lib/apis/v3/policy.go

lmm · 2020-06-10T05:59:26Z

Am I right that this is mostly copied from our Enterprise code? Please can you list or identify the changes that are additional to that? (Because I think there's less value in reviewing Enterprise-copied code.)

Thanks @neiljerram . The first 2 commits are cherry-picked from enterprise code. The last commit is newly added. The docs PR (also cherry-picked) is here: projectcalico/calico#3643

If there isn't already a design or doc that says this, please can you also summarise, for me, where global() is allowed, and why?

global() is allowed as a value for an EntityRule's namespaceSelector.

nelljerram

Just one piece of validation+test missing, I think, and a possible text markup.

nelljerram · 2020-06-10T15:04:31Z

lib/validator/v3/validator.go

+			reflect.ValueOf(spec.Selector),
+			"NetworkPolicySpec.Selector",
+			"",
+			reason(fmt.Sprintf("%s can only be used in a GlobalNetworkPolicy EntityRule", globalSelector)),


Add "namespaceSelector" ? (And similarly in all other occurrences below.)

nelljerram · 2020-06-10T15:06:01Z

lib/validator/v3/validator_test.go

+					},
+				},
+			}, true,
+		),


Need to add test and validation for disallowing global() in EntityRule selector field.

Added in 4c46c87. In bdb4189 I added some tests for EntityRule

global() and projectcalico.org/name == '<value>' cannot be used with other selectors.

lmm · 2020-06-11T05:43:03Z

@neiljerram this is ready for another look. One comment re:

I'm wondering if global() must be the entire content of any selector field where it is used, or if it is allowed (and makes sense) to combine with other expressions, e.g. global() && color == 'red'.

I had a chat with @doublek about this and the intent of the all(), global(), and projectcalico.org/name namespaceSelectors is that they should be used on their own and not in combination. I've added validation for this for global() and projectcalico.org/name.

But I left all() untouched as we already do some canonicalization where all() && some-other-selector results in the selector parser to reduce that to all(), and maybe we want to preserve that behaviour?

nelljerram

Approved because I don't want to block this. I'm not sure about the projectcalico.org/name addition, but happy to leave that to you.

nelljerram · 2020-06-11T09:41:04Z

lib/validator/v3/validator.go

@@ -66,6 +67,13 @@ var (
 	// Hostname  have to be valid ipv4, ipv6 or strings up to 64 characters.
 	prometheusHostRegexp = regexp.MustCompile(`^[a-zA-Z0-9:._+-]{1,64}$`)

+	// global() cannot be used with other selectors.
+	andOr               = `(&&|\|\|)`
+	globalSelectorRegex = regexp.MustCompile(fmt.Sprintf(`%v global\(\)|global\(\) %v`, andOr, andOr))


nit: should the RE have * after the space, to allow for any amount of whitespace?

Hmm, I see you already have a test for "global()||all()", so perhaps in the RE already stands for any amount of whitespace? (Otherwise, how is that test passing?)

We parse the selector string first and use the String() from that result for the validation matching. The parsing will trim out all of the whitespace.

nelljerram · 2020-06-11T11:42:41Z

lib/validator/v3/validator.go

+	globalSelectorRegex = regexp.MustCompile(fmt.Sprintf(`%v global\(\)|global\(\) %v`, andOr, andOr))
+
+	// projectcalico.org/name cannot be used with other selectors.
+	namespaceSelectorRegex = regexp.MustCompile(fmt.Sprintf(`%v projectcalico.org/name == ".*"|projectcalico.org/name == ".*" %v`, andOr, andOr))


This looks plausible to me, but:

is it right to add this validation into the current PR? is there a particular motivation for that?

what about, for example, projectcalico/name in {"red", "blue", ...} - which I think should be allowed on its own, but probably not combined with anything else.

Unless there's a strong reason to include, I think it might be better to split the projectcalico.org/name validation out into another PR, so we can discuss its motivation and details more there.

I think you're right - I'll remove the projectcalico.org/name bits

Comments covered by follow-up commits

#1253) * Update generated crds for #1229 * Re-add manual change to crds

…1229

Add global() namespace for calico network policy

9a7d893

1) Implement a Token Global for handling `global'

marvin-tigera added docs-pr-required release-note-required labels Apr 27, 2020

lmm force-pushed the lmm-port branch 2 times, most recently from 07be0bd to 5b6faa8 Compare April 27, 2020 21:00

Add support for global() selector in calico network policy

f1f9255

Add support for rules and selector for global keyword and add unit test case

lmm force-pushed the lmm-port branch from 5b6faa8 to f1f9255 Compare April 27, 2020 21:02

caseydavenport requested changes Apr 27, 2020

View reviewed changes

caseydavenport added this to the Calico v3.15.0 milestone Apr 27, 2020

caseydavenport reviewed May 13, 2020

View reviewed changes

caseydavenport previously requested changes May 13, 2020

View reviewed changes

Add validation for global() selector

179ddea

lmm force-pushed the lmm-port branch from e06549c to 179ddea Compare June 3, 2020 23:06

nelljerram self-requested a review June 9, 2020 10:17

nelljerram reviewed Jun 9, 2020

View reviewed changes

lib/apis/v3/policy.go Show resolved Hide resolved

lmm mentioned this pull request Jun 9, 2020

Document the global() selector projectcalico/calico#3643

Merged

3 tasks

nelljerram requested changes Jun 10, 2020

View reviewed changes

lmm added 3 commits June 10, 2020 10:06

markups: Add another test

4c46c87

Add validation to EntityRule for special selectors

bdb4189

global() and projectcalico.org/name == '<value>' cannot be used with other selectors.

markups: use better error msg for invalid use of global()

2d112c7

nelljerram approved these changes Jun 11, 2020

View reviewed changes

markups: remove projectcalico.org/name validation

404ace8

lmm merged commit 36db81b into projectcalico:master Jun 11, 2020

lmm deleted the lmm-port branch June 11, 2020 15:55

lmm removed the docs-pr-required label Jun 11, 2020

lmm added the docs-completed label Jun 11, 2020

lmm added a commit to lmm/libcalico-go that referenced this pull request Jun 11, 2020

Update generated crds for projectcalico#1229

317e5af

lmm added a commit that referenced this pull request Jun 11, 2020

Update generated crds for https://github.com/projectcalico/libcalico-… (

0bcaac1

#1253) * Update generated crds for #1229 * Re-add manual change to crds

andreas-p pushed a commit to andreas-p/calico-accountant that referenced this pull request Jun 7, 2021

add global() Selector: cherry picked from projectcalico/libcalico-go#…

4ef7c26

…1229

andreas-p pushed a commit to andreas-p/calico-accountant that referenced this pull request Jun 7, 2021

add global() Selector: cherry picked from projectcalico/libcalico-go#…

ba8876e

…1229

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global() selector #1229

Add global() selector #1229

lmm commented Apr 27, 2020 •

edited

Loading

caseydavenport left a comment

caseydavenport May 13, 2020 •

edited

Loading

caseydavenport left a comment •

edited

Loading

bcreane commented May 16, 2020

nelljerram commented Jun 9, 2020

nelljerram left a comment

lmm commented Jun 10, 2020

nelljerram left a comment

nelljerram Jun 10, 2020

lmm Jun 10, 2020

nelljerram Jun 10, 2020

lmm Jun 10, 2020

lmm commented Jun 11, 2020

nelljerram left a comment

nelljerram Jun 11, 2020

nelljerram Jun 11, 2020

lmm Jun 11, 2020

nelljerram Jun 11, 2020

lmm Jun 11, 2020

Add global() selector #1229

Add global() selector #1229

Conversation

lmm commented Apr 27, 2020 • edited Loading

Description

Todos

Release Note

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport May 13, 2020 • edited Loading

Choose a reason for hiding this comment

caseydavenport left a comment • edited Loading

Choose a reason for hiding this comment

bcreane commented May 16, 2020

nelljerram commented Jun 9, 2020

nelljerram left a comment

Choose a reason for hiding this comment

lmm commented Jun 10, 2020

nelljerram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmm commented Jun 11, 2020

nelljerram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmm commented Apr 27, 2020 •

edited

Loading

caseydavenport May 13, 2020 •

edited

Loading

caseydavenport left a comment •

edited

Loading