Add support for namespace label selectors in K8s policies by brecode · Pull Request #566 · contiv/vpp

brecode · 2018-02-02T01:13:07Z

This PR makes a few changes in the policy plugin, adding support for namespace label selectors, both for Ingress and Egress Rules. Also fixes bugs related to empty podSelectors and empty namespaceLabelSelectors. Fixes should be also part of #560 and #565

jmedved · 2018-02-02T01:42:05Z

plugins/policy/cache/cache_impl.go

+	namespaceLabelSelector *policymodel.Policy_LabelSelector) (pods []podmodel.ID) {
+	// An empty namespace selector matches all namespaces.
+	if len(namespaceLabelSelector.MatchExpression) == 0 && len(namespaceLabelSelector.MatchLabel) == 0 {
+		allPods := pc.configuredPods.ListAll()


check that namespaceLabelSelector is not nil first; if it is (possible if no label selectors are present in the policy), the check for MatchExpression will panic.

coveralls · 2018-02-02T03:53:50Z

Coverage remained the same at 76.822% when pulling 8ad1e7e on brecode:master into 1977041 on contiv:master.

…go & plugins/policy/processor/processor.go

brecode · 2018-02-02T04:09:36Z

Commit 8ad1e7e
Fix formatting errors on plugins/policy/processor/matches_calculator.go & plugins/policy/processor/processor.go go files, to correct "make check_format" error from travis by removing unnecessary whitespaces. Solution is to run gofmt on the files producing the error.

tiewei

left some comments

tiewei · 2018-02-02T05:01:11Z

plugins/policy/cache/cache_impl.go

+	namespaceLabelSelector *policymodel.Policy_LabelSelector) (pods []podmodel.ID) {
+	// An empty namespace selector matches all namespaces.
+	if len(namespaceLabelSelector.MatchExpression) == 0 && len(namespaceLabelSelector.MatchLabel) == 0 {
+		allPods := pc.configuredPods.ListAll()


tiewei · 2018-02-02T05:06:12Z

plugins/policy/cache/cache_impl.go

+	matchLabels := namespaceLabelSelector.MatchLabel
+
+	found, namespaceSelectorPods := pc.getPodsByLabelSelector(matchLabels)
+	if !found {


I think this is not required, if not found it namespaceSelectorPods is empty slice anyway

tiewei · 2018-02-02T05:07:36Z

plugins/policy/cache/match_label.go


 // GetPodsByNSLabelSelector returns the pods that match a collection of Label Selectors in the same namespace
 func (pc *PolicyCache) getPodsByNSLabelSelector(namespace string, labels []*policymodel.Policy_Label) (bool, []string) {
+	newPodSet := []string{}


this change doesn't look necessary, but it's fine

tiewei · 2018-02-02T05:11:51Z

plugins/policy/cache/match_label.go

-		tmp := utils.Intersect(prevPodSet, newPodSet)
+		tmp := utils.Intersect(prevNamespaceSet, newNamespaceSet)
 		if len(tmp) == 0 {
 			return false, nil


Here i would return empty string slice instead

tiewei · 2018-02-02T05:14:56Z

plugins/policy/cache/match_label.go

+		if len(pods) == 0 {
+			return false, []string{}
+		}
+		return true, pods


I noticed this file having a lot return Bool, Slice usage, is it really necessary ? an empty slice should be enough for later range operations, the only usage difference when return false is the 2nd return value is a clear empty slice, while at that time the pods is already ensured to be a empty slice, so whats the point of doing it here ?

tiewei · 2018-02-02T05:44:20Z

plugins/policy/cache/match_label.go

-		prevPodSet = newPodSet
-		newPodSet = tmp
+		prevNamespaceSet = newNamespaceSet
+		newNamespaceSet = tmp


this for loop is trying to get the intersect of all labels selector's result, it's a little hard to understand

current = pre = setA for i := 1; i < len(labels); i++ { pre = current setB = query() current = utils.Intersect(pre, setB) } for _, ns : = range current pods = append(pods, xxx) }

In this way you don't have to treat the 1 label case separately

tiewei · 2018-02-02T05:46:39Z

plugins/policy/processor/rules_match_label.go

+		// Get all namespaces that match namespace label selector
+		namespaces := pp.Cache.LookupNamespacesByLabelSelector(label)
+		if len(namespaces) == 0 {
+			return isMatch


not return false ?

tiewei · 2018-02-02T05:47:09Z

plugins/policy/processor/rules_match_label.go

+		// Check if matched namespaces include pod's namespace
+		for _, namespace := range namespaces {
+			if namespace == podNamespace {
+				namespaceExists = true


i would add a break here

This PR addresses comment suggestions of contiv#566. - Refactor code for match expression and match label files to return only []string - Simplified getPodsByNSLabelSelector & getPodsByLabelSelector in match_label.go to be easily readable - Added missing comments for function description and more robust comments in code - Fixed two bugs in processor.go (missing "!" and worng function call)

- Changed Interesct function to calculate more than two slices. Added an additional check for empty slices. - Added an additional check for empty labels in getPodsByNSLabelSelector and getMatchExpressionPods on file match_label.on. If empty labels empty slice is returned - Simplified cache_implementation code by removing if cases when no labels provided. The if cases are now handled in getPodsByNSLabelSelector and getMatchExpressionPods.

- Changed Interesct function to calculate more than two slices. Added an additional check for empty slices. - Added an additional check for empty labels in getPodsByNSLabelSelector and getMatchExpressionPods on file match_label.on. If empty labels empty slice is returned - Simplified cache_implementation code by removing if cases when no labels provided. The if cases are now handled in getPodsByNSLabelSelector and getMatchExpressionPods. - Removed unecessary function call on duplicates to slices on match_expression.go The duplicate removal is handled by Intersect function

Policy Cache & Policy Processor fixes for #566

brecode added 2 commits February 1, 2018 17:03

Fixed Empty Selector corner cases

f59da95

Merge remote-tracking branch 'upstream/master'

1d331e5

brecode requested review from jmedved and milanlenco February 2, 2018 01:13

jmedved requested a review from tiewei February 2, 2018 01:29

jmedved changed the title ~~plugins/policy: added support for namespace label selectors~~ Add support for namespace label selectors in K8s policies Feb 2, 2018

jmedved reviewed Feb 2, 2018

View reviewed changes

Fix formatting errors on plugins/policy/processor/matches_calculator.…

8ad1e7e

…go & plugins/policy/processor/processor.go

brecode force-pushed the master branch from b5542d5 to 8ad1e7e Compare February 2, 2018 04:02

jmedved approved these changes Feb 2, 2018

View reviewed changes

tiewei reviewed Feb 2, 2018

View reviewed changes

jmedved merged commit 324d2cb into contiv:master Feb 2, 2018

brecode mentioned this pull request Feb 3, 2018

Policy Cache & Policy Processor fixes for #566 #570

Merged

milanlenco pushed a commit that referenced this pull request Feb 5, 2018

Merge pull request #570 from brecode/master

f104b8b

Policy Cache & Policy Processor fixes for #566

Conversation

brecode commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brecode commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiewei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 2, 2018 •

edited

Loading

brecode commented Feb 2, 2018 •

edited

Loading