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

new selectors for NP and GNP #1133

Merged
merged 14 commits into from Oct 1, 2019

Conversation

@rafaelvanoni
Copy link
Member

rafaelvanoni commented Sep 25, 2019

Support top-level namespaceSelector for global network policies
Support top-level serviceAccountSelector for global and namespaced network policies
Copy link
Member

caseydavenport left a comment

@rafaelvanoni this is looking good - did a quick look through and will do a more focused one tomorrow.

I think my main feedback is that we should add a lot more tests for this functionality to make sure we're handling all the different edge cases correctly - it's a bit of code that historically has proven to be very prone to subtle bugs.

@caseydavenport caseydavenport added this to the Calico v3.10.0 milestone Sep 26, 2019
Copy link
Member

caseydavenport left a comment

@rafaelvanoni this looks really good.

I added a few more suggestions for tests I think we should have, and I think I spotted a bug in the existing code re: service account all() selectors.

Other than that just some minor cleanups

// v3 model.KVPair revision
var TestRev string = "1234"

func MustParseCIDR(cidr string) *cnet.IPNet {

This comment has been minimized.

Copy link
@caseydavenport

caseydavenport Sep 28, 2019

Member

Does this need to be public now? I would have thought not since it was just a file move, but could be missing something.

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 30, 2019

Author Member

Fixed

lib/backend/syncersv1/updateprocessors/shared_test.go Outdated Show resolved Hide resolved
lib/backend/syncersv1/updateprocessors/shared_test.go Outdated Show resolved Hide resolved

nsSelector := spec.NamespaceSelector
if nsSelector != "" {
selector = PrefixAndAppendSelector(selector, nsSelector, conversion.NamespaceLabelPrefix)

This comment has been minimized.

Copy link
@caseydavenport

caseydavenport Sep 28, 2019

Member

Hm, looking at this now, I'm wondering if we need an equivalent for the SA selector as well. I bet that is an existing bug in the rule logic as well.

e.g., serviceAccountSelector: "all()" should end up as has(projectcalico.org/serviceaccount).

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Oct 1, 2019

Author Member

Fixed

lib/backend/syncersv1/updateprocessors/selectors.go Outdated Show resolved Hide resolved
@rafaelvanoni rafaelvanoni changed the title WIP: new selectors for NP and GNP new selectors for NP and GNP Oct 1, 2019
Copy link
Member

caseydavenport left a comment

@rafaelvanoni looks really good - one minor comment, but also I think we're still missing test cases for the namespace and serviceaccount selectors using value all().

I agree with you on the testing against real labels - let's do that as a follow on / stretch goal once you've completed the other work on your plate.

@rafaelvanoni rafaelvanoni requested a review from caseydavenport Oct 1, 2019
rafaelvanoni added 2 commits Oct 1, 2019
@caseydavenport caseydavenport merged commit 262b6a9 into projectcalico:master Oct 1, 2019
2 checks passed
2 checks passed
license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details
@rafaelvanoni rafaelvanoni deleted the rafaelvanoni:rafael-libcalico-go-1 branch Oct 1, 2019
@lmm lmm added docs-completed and removed docs-pr-required labels Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.