From ec3a219f07c83a16ac533c1a39af67fbfbcc7e08 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 12 Dec 2022 14:47:36 -0800 Subject: [PATCH 1/4] add validation for matchExpression values --- .../controlplane/translation/parseSelector.go | 34 ++- .../translation/parseSelector_test.go | 139 ++++++++- .../translation/translatePolicy.go | 14 +- .../translation/translatePolicy_test.go | 285 ++++++++++++++++-- 4 files changed, 437 insertions(+), 35 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index ded4ffcc47..d9b119571a 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -3,15 +3,21 @@ package translation import ( "fmt" + "regexp" + "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// validLabelRegex is a match for this requirement: +// must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character +var validLabelRegex = regexp.MustCompile("[A-Za-z0-9][-A-Za-z0-9_.]*") + // flattenNameSpaceSelector will help flatten multiple nameSpace selector match Expressions values // into multiple label selectors helping with the OR condition. -func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSelector { +func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelSelector, error) { /* This function helps to create multiple labelSelectors when given a single multivalue nsSelector Take below example: this nsSelector has 2 values in a matchSelector. @@ -48,11 +54,11 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe // To avoid any additional length checks, just return a slice of labelSelectors // with original nsSelector if nsSelector == nil { - return []metav1.LabelSelector{} + return []metav1.LabelSelector{}, nil } if len(nsSelector.MatchExpressions) == 0 { - return []metav1.LabelSelector{*nsSelector} + return []metav1.LabelSelector{*nsSelector}, nil } // create a baseSelector which needs to be same across all @@ -71,6 +77,12 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe // to create multiple nsSelectors to preserve OR condition across all labels and expressions switch { case (req.Operator == metav1.LabelSelectorOpIn) || (req.Operator == metav1.LabelSelectorOpNotIn): + for _, v := range req.Values { + if !isValidLabelValue(v) { + return nil, ErrInvalidMatchExpressionValues + } + } + if len(req.Values) == 1 { // for length 1, add the matchExpr to baseSelector baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) @@ -89,7 +101,7 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe // If there are no multiValue NS selector match expressions // return the original NsSelector if !multiValuePresent { - return []metav1.LabelSelector{*nsSelector} + return []metav1.LabelSelector{*nsSelector}, nil } // Now use the baseSelector and loop over multiValueMatchExprs to create all @@ -101,7 +113,7 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe flatNsSelectors = zipMatchExprs(flatNsSelectors, req) } - return flatNsSelectors + return flatNsSelectors, nil } // zipMatchExprs helps with zipping a given matchExpr with given baseLabelSelectors @@ -253,6 +265,12 @@ func parsePodSelector(policyKey string, selector *metav1.LabelSelector) ([]label } switch op { case metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn: + for _, v := range req.Values { + if !isValidLabelValue(v) { + return nil, ErrInvalidMatchExpressionValues + } + } + // "(!) + matchKey + : + matchVal" case if len(req.Values) == 1 { setName = util.GetIpSetFromLabelKV(req.Key, req.Values[0]) @@ -284,3 +302,9 @@ func unsupportedOpsInWindows(op metav1.LabelSelectorOperator) bool { return util.IsWindowsDP() && (op == metav1.LabelSelectorOpNotIn || op == metav1.LabelSelectorOpDoesNotExist) } + +// isValidLabelValue ensures the string is empty or satisfies validLabelRegex. +func isValidLabelValue(v string) bool { + return v == "" || + validLabelRegex.ReplaceAllString(v, "") == "" +} diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 6285051e6c..133f9f73ba 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -1,23 +1,27 @@ package translation import ( + "fmt" "reflect" "testing" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestFlattenNameSpaceSelectorCases(t *testing.T) { firstSelector := &metav1.LabelSelector{} - testSelectors := flattenNameSpaceSelector(firstSelector) + testSelectors, err := flattenNameSpaceSelector(firstSelector) + require.Nil(t, err) if len(testSelectors) != 1 { t.Errorf("TestFlattenNameSpaceSelectorCases failed @ 1st selector length check %+v", testSelectors) } var secondSelector *metav1.LabelSelector - testSelectors = flattenNameSpaceSelector(secondSelector) + testSelectors, err = flattenNameSpaceSelector(secondSelector) + require.Nil(t, err) if len(testSelectors) > 0 { t.Errorf("TestFlattenNameSpaceSelectorCases failed @ 1st selector length check %+v", testSelectors) } @@ -61,7 +65,8 @@ func TestFlattenNameSpaceSelector(t *testing.T) { MatchLabels: commonMatchLabel, } - testSelectors := flattenNameSpaceSelector(firstSelector) + testSelectors, err := flattenNameSpaceSelector(firstSelector) + require.Nil(t, err) if len(testSelectors) != 1 { t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors) } @@ -105,7 +110,8 @@ func TestFlattenNameSpaceSelector(t *testing.T) { MatchLabels: commonMatchLabel, } - testSelectors = flattenNameSpaceSelector(secondSelector) + testSelectors, err = flattenNameSpaceSelector(secondSelector) + require.Nil(t, err) if len(testSelectors) != 8 { t.Errorf("TestFlattenNameSpaceSelector failed @ 2nd selector length check %+v", testSelectors) } @@ -399,7 +405,8 @@ func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) { }, } - testSelectors := flattenNameSpaceSelector(firstSelector) + testSelectors, err := flattenNameSpaceSelector(firstSelector) + require.Nil(t, err) if len(testSelectors) != 2 { t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors) } @@ -471,3 +478,125 @@ func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) { t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector deepEqual check.\n Expected: %+v \n Actual: %+v", expectedSelectors, testSelectors) } } + +func TestFlattenNamespaceSelectorError(t *testing.T) { + tests := []struct { + name string + selector *metav1.LabelSelector + wantErr bool + }{ + { + name: "good alphanumeric with hyphen", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "good", + "good-1", + "good2-too", + "good-end-in-hyphen-", + }, + }, + { + Key: "testNotIn", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{ + "good", + "good-1", + "good2-too", + "good-end-in-hyphen-", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "bad in", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "good-1", + "bad$", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "bad not in", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testNotIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad$", + "good-1", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "good and bad", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "good-1", + }, + }, + { + Key: "testNotIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad$", + "good-1", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "bad with space", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + "good-1", + }, + }, + }, + }, + wantErr: true, + }, + } + + for i, tt := range tests { + tt := tt + t.Run(fmt.Sprintf("test %d", i), func(t *testing.T) { + s, err := flattenNameSpaceSelector(tt.selector) + if tt.wantErr { + require.Error(t, err) + require.Nil(t, s) + } else { + require.NoError(t, err) + require.NotNil(t, s) + } + }) + } +} diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index c1dbe00730..5cfcfc7228 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -28,6 +28,8 @@ var ( ErrUnsupportedExceptCIDR = errors.New("unsupported Except CIDR block translation features used on windows") // ErrUnsupportedSCTP is returned when SCTP protocol is used in windows. ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows") + // ErrInvalidMatchExpressionValues ensures proper matchExpression label values since k8s doesn't perform this check. + ErrInvalidMatchExpressionValues = errors.New("matchExpression values must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character") ) type podSelectorResult struct { @@ -407,7 +409,11 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire if peer.PodSelector == nil && peer.NamespaceSelector != nil { // Before translating NamespaceSelector, flattenNameSpaceSelector function call should be called // to handle multiple values in matchExpressions spec. - flattenNSSelector := flattenNameSpaceSelector(peer.NamespaceSelector) + flattenNSSelector, err := flattenNameSpaceSelector(peer.NamespaceSelector) + if err != nil { + return err + } + for i := range flattenNSSelector { nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, &flattenNSSelector[i]) npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...) @@ -444,7 +450,11 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire // Before translating NamespaceSelector, flattenNameSpaceSelector function call should be called // to handle multiple values in matchExpressions spec. - flattenNSSelector := flattenNameSpaceSelector(peer.NamespaceSelector) + flattenNSSelector, err := flattenNameSpaceSelector(peer.NamespaceSelector) + if err != nil { + return err + } + for i := range flattenNSSelector { nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, &flattenNSSelector[i]) npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...) diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index a4fbe25c01..671d08407e 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -563,14 +563,15 @@ func TestPodSelector(t *testing.T) { policyKey := "test-ns/test-policy" policyKeyWithDash := policyKey + "-" tests := []struct { - name string - namespace string - matchType policies.MatchType - labelSelector *metav1.LabelSelector - podSelectorIPSets []*ipsets.TranslatedIPSet - childPodSelectorIPSets []*ipsets.TranslatedIPSet - podSelectorList []policies.SetInfo - skipWindows bool + name string + namespace string + matchType policies.MatchType + labelSelector *metav1.LabelSelector + podSelectorIPSets []*ipsets.TranslatedIPSet + childPodSelectorIPSets []*ipsets.TranslatedIPSet + podSelectorList []policies.SetInfo + skipWindows bool + wantExpressionValuesErr bool }{ { name: "all pods selector in default namespace in ingress", @@ -778,6 +779,47 @@ func TestPodSelector(t *testing.T) { }, skipWindows: true, }, + { + name: "bad in expression", + namespace: defaultNS, + matchType: matchType, + labelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + "v11", + }, + }, + }, + }, + wantExpressionValuesErr: true, + }, + { + name: "bad notIn expression", + namespace: defaultNS, + matchType: matchType, + labelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{ + "bad-char$", + }, + }, + }, + }, + wantExpressionValuesErr: true, + }, } for _, tt := range tests { @@ -795,7 +837,7 @@ func TestPodSelector(t *testing.T) { if psResult == nil { psResult = &podSelectorResult{} } - if tt.skipWindows && util.IsWindowsDP() { + if tt.wantExpressionValuesErr || (tt.skipWindows && util.IsWindowsDP()) { require.Error(t, err) } else { require.NoError(t, err) @@ -815,6 +857,7 @@ func TestNameSpaceSelector(t *testing.T) { labelSelector *metav1.LabelSelector nsSelectorIPSets []*ipsets.TranslatedIPSet nsSelectorList []policies.SetInfo + wantErr bool }{ { name: "namespaceSelector for all namespaces in ingress", @@ -1394,13 +1437,14 @@ func TestIngressPolicy(t *testing.T) { emptyString := intstr.FromString("") // TODO(jungukcho): add test cases with more complex rules tests := []struct { - name string - targetSelector *metav1.LabelSelector - rules []networkingv1.NetworkPolicyIngressRule - npmNetPol *policies.NPMNetworkPolicy - wantErr bool - skipWindows bool - windowsNil bool + name string + targetSelector *metav1.LabelSelector + rules []networkingv1.NetworkPolicyIngressRule + npmNetPol *policies.NPMNetworkPolicy + wantErr bool + skipWindows bool + windowsNil bool + wantTargetSelectorErr bool }{ { name: "only port in ingress rules", @@ -1949,6 +1993,98 @@ func TestIngressPolicy(t *testing.T) { }, }, }, + { + name: "bad target selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + rules: []networkingv1.NetworkPolicyIngressRule{ + {}, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantTargetSelectorErr: true, + }, + { + name: "bad pod selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, + { + name: "bad ns selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, } for _, tt := range tests { @@ -1964,6 +2100,11 @@ func TestIngressPolicy(t *testing.T) { require.Error(t, err) return } + if tt.wantTargetSelectorErr { + require.Error(t, err) + return + } + require.NoError(t, err) npmNetPol.PodSelectorIPSets = psResult.psSets npmNetPol.ChildPodSelectorIPSets = psResult.childPSSets @@ -1987,13 +2128,14 @@ func TestEgressPolicy(t *testing.T) { targetPodMatchType := policies.EitherMatch peerMatchType := policies.DstMatch tests := []struct { - name string - targetSelector *metav1.LabelSelector - rules []networkingv1.NetworkPolicyEgressRule - npmNetPol *policies.NPMNetworkPolicy - wantErr bool - skipWindows bool - windowsNil bool + name string + targetSelector *metav1.LabelSelector + rules []networkingv1.NetworkPolicyEgressRule + npmNetPol *policies.NPMNetworkPolicy + wantErr bool + skipWindows bool + windowsNil bool + wantTargetSelectorErr bool }{ { name: "only port in egress rules", @@ -2542,6 +2684,98 @@ func TestEgressPolicy(t *testing.T) { }, }, }, + { + name: "bad target selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + rules: []networkingv1.NetworkPolicyEgressRule{ + {}, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantTargetSelectorErr: true, + }, + { + name: "bad pod selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, + { + name: "bad ns selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, } for _, tt := range tests { @@ -2557,6 +2791,11 @@ func TestEgressPolicy(t *testing.T) { require.Error(t, err) return } + if tt.wantTargetSelectorErr { + require.Error(t, err) + return + } + require.NoError(t, err) npmNetPol.PodSelectorIPSets = psResult.psSets npmNetPol.ChildPodSelectorIPSets = psResult.childPSSets From 0a664c708f5508d8453090909b577a1881c9b2f6 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 12 Dec 2022 15:25:28 -0800 Subject: [PATCH 2/4] fix lint --- npm/pkg/controlplane/translation/translatePolicy.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 5cfcfc7228..0ede73986f 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -29,7 +29,9 @@ var ( // ErrUnsupportedSCTP is returned when SCTP protocol is used in windows. ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows") // ErrInvalidMatchExpressionValues ensures proper matchExpression label values since k8s doesn't perform this check. - ErrInvalidMatchExpressionValues = errors.New("matchExpression values must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character") + ErrInvalidMatchExpressionValues = errors.New( + "matchExpression label values must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character", + ) ) type podSelectorResult struct { From 572257fa38b8cba57157b18e314873cd4152fe81 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 13 Dec 2022 14:36:25 -0800 Subject: [PATCH 3/4] make regex identical to kubectl validation and include more test cases --- .../controlplane/translation/parseSelector.go | 18 +++++--- .../translation/parseSelector_test.go | 41 ++++++++++++++++++- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index d9b119571a..2ec57f59d7 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -11,9 +11,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// validLabelRegex is a match for this requirement: -// must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character -var validLabelRegex = regexp.MustCompile("[A-Za-z0-9][-A-Za-z0-9_.]*") +// validLabelRegex is defined from the result of kubectl (this includes empty string matches): +// a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with +// an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?' +var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?") // flattenNameSpaceSelector will help flatten multiple nameSpace selector match Expressions values // into multiple label selectors helping with the OR condition. @@ -304,7 +305,14 @@ func unsupportedOpsInWindows(op metav1.LabelSelectorOperator) bool { } // isValidLabelValue ensures the string is empty or satisfies validLabelRegex. +// Given that v != "", ReplaceAllString() would yield "" when v matches this regex exactly once. func isValidLabelValue(v string) bool { - return v == "" || - validLabelRegex.ReplaceAllString(v, "") == "" + matches := validLabelRegex.FindAllStringIndex(v, -1) + // v = "abc-123" would produce [[0 7]], which satisfies the below + // v = "" will produce [[0 0]], which satisfies the below + // v = "$" would produce [[0 0] [1 1]], which would fail the below + // v = "abc$" would produce [[0 3] [4 4]], which would fail the below + fmt.Printf("%+v\n", validLabelRegex.FindAllStringIndex("abc-123", -1)) + fmt.Printf("%+v\n", validLabelRegex.FindAllStringIndex("abc$", -1)) + return len(matches) == 1 && matches[0][0] == 0 && matches[0][1] == len(v) } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 133f9f73ba..0993a60dd8 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -496,7 +496,6 @@ func TestFlattenNamespaceSelectorError(t *testing.T) { "good", "good-1", "good2-too", - "good-end-in-hyphen-", }, }, { @@ -506,7 +505,6 @@ func TestFlattenNamespaceSelectorError(t *testing.T) { "good", "good-1", "good2-too", - "good-end-in-hyphen-", }, }, }, @@ -600,3 +598,42 @@ func TestFlattenNamespaceSelectorError(t *testing.T) { }) } } + +func TestIsValidLabel(t *testing.T) { + require.Fail(t, "hey") + + good := []string{ + "", + "1", + "abc", + "ABC", + "abc1", + "ABC1", + "abc-1", + "ABC-1", + "ABC_1", + "ABC_-a54--f", + } + + for _, g := range good { + require.True(t, isValidLabelValue(g), "string was [%s]", g) + } + + bad := []string{ + "-", + "_", + "$", + " ", + "abc-", + "abc$", + "abc$123", + "bad space", + "end-with-hyphen-", + "end-with-underscore_", + "end-with-space ", + } + + for _, b := range bad { + require.False(t, isValidLabelValue(b), "string was [%s]", b) + } +} From c7869d44fe60d1adc694791bf2a5bb1dded013cd Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Wed, 14 Dec 2022 09:44:29 -0800 Subject: [PATCH 4/4] remove debugging lines --- npm/pkg/controlplane/translation/parseSelector.go | 2 -- npm/pkg/controlplane/translation/parseSelector_test.go | 2 -- 2 files changed, 4 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index 2ec57f59d7..e3fce8056c 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -312,7 +312,5 @@ func isValidLabelValue(v string) bool { // v = "" will produce [[0 0]], which satisfies the below // v = "$" would produce [[0 0] [1 1]], which would fail the below // v = "abc$" would produce [[0 3] [4 4]], which would fail the below - fmt.Printf("%+v\n", validLabelRegex.FindAllStringIndex("abc-123", -1)) - fmt.Printf("%+v\n", validLabelRegex.FindAllStringIndex("abc$", -1)) return len(matches) == 1 && matches[0][0] == 0 && matches[0][1] == len(v) } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 0993a60dd8..e93f99500a 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -600,8 +600,6 @@ func TestFlattenNamespaceSelectorError(t *testing.T) { } func TestIsValidLabel(t *testing.T) { - require.Fail(t, "hey") - good := []string{ "", "1",