From 52dd93924602375a92ef2f37c24b863c87d549b4 Mon Sep 17 00:00:00 2001 From: GraysonWu Date: Thu, 24 Feb 2022 00:32:38 -0800 Subject: [PATCH] Validate labels in CRDs (#3331) Fixes #3285. This PR added some additional validations to ANP, ACNP and CG in validating webhook. Antrea will validate labels used in ANP, ACNP and CG under PodSelector, NamespaceSelector and ExternalEntitySelector. Signed-off-by: wgrayson --- pkg/controller/networkpolicy/validate.go | 35 +++++ pkg/controller/networkpolicy/validate_test.go | 129 ++++++++++++++++++ 2 files changed, 164 insertions(+) diff --git a/pkg/controller/networkpolicy/validate.go b/pkg/controller/networkpolicy/validate.go index bdab883d2a9..2188bf7d8a8 100644 --- a/pkg/controller/networkpolicy/validate.go +++ b/pkg/controller/networkpolicy/validate.go @@ -27,6 +27,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apiserver/pkg/authentication/serviceaccount" "k8s.io/klog/v2" @@ -472,6 +473,9 @@ func (v *antreaPolicyValidator) validateAppliedTo(ingress, egress []crdv1alpha1. if eachAppliedTo.ServiceAccount != nil && appliedToFieldsNum > 1 { return "serviceAccount cannot be set with other peers in appliedTo", false } + if reason, allowed := checkSelectorsLabels(eachAppliedTo.PodSelector, eachAppliedTo.NamespaceSelector, eachAppliedTo.ExternalEntitySelector); !allowed { + return reason, allowed + } } return "", true } @@ -511,6 +515,9 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1alpha1.Rule if peer.ServiceAccount != nil && peerFieldsNum > 1 { return "serviceAccount cannot be set with other peers in rules", false } + if reason, allowed := checkSelectorsLabels(peer.PodSelector, peer.NamespaceSelector, peer.ExternalEntitySelector); !allowed { + return reason, allowed + } } return "", true } @@ -549,6 +556,31 @@ func numFieldsSetInPeer(peer crdv1alpha1.NetworkPolicyPeer) int { return num } +// checkSelectorsLabels validates labels used in all selectors passed in. +func checkSelectorsLabels(selectors ...*metav1.LabelSelector) (string, bool) { + validateLabels := func(labels map[string]string) (string, bool) { + for k, v := range labels { + err := validation.IsQualifiedName(k) + if err != nil { + return fmt.Sprintf("Invalid label key: %s: %s", k, strings.Join(err, "; ")), false + } + err = validation.IsValidLabelValue(v) + if err != nil { + return fmt.Sprintf("Invalid label value: %s: %s", v, strings.Join(err, "; ")), false + } + } + return "", true + } + for _, selector := range selectors { + if selector != nil { + if reason, allowed := validateLabels(selector.MatchLabels); !allowed { + return reason, allowed + } + } + } + return "", true +} + // validateTierForPolicy validates whether a referenced Tier exists. func (v *antreaPolicyValidator) validateTierForPolicy(tier string) (string, bool) { // "tier" must exist before referencing @@ -706,6 +738,9 @@ func validateAntreaGroupSpec(s crdv1alpha2.GroupSpec) (string, bool) { } selector, serviceRef, ipBlock, ipBlocks, childGroups := 0, 0, 0, 0, 0 if s.NamespaceSelector != nil || s.ExternalEntitySelector != nil || s.PodSelector != nil { + if reason, allowed := checkSelectorsLabels(s.PodSelector, s.NamespaceSelector, s.ExternalEntitySelector); !allowed { + return reason, allowed + } selector = 1 } if s.IPBlock != nil { diff --git a/pkg/controller/networkpolicy/validate_test.go b/pkg/controller/networkpolicy/validate_test.go index d5f65466f55..8297815b075 100644 --- a/pkg/controller/networkpolicy/validate_test.go +++ b/pkg/controller/networkpolicy/validate_test.go @@ -23,6 +23,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1" + crdv1alpha2 "antrea.io/antrea/pkg/apis/crd/v1alpha2" ) func TestValidateAntreaPolicy(t *testing.T) { @@ -940,6 +941,84 @@ func TestValidateAntreaPolicy(t *testing.T) { }, expectedReason: "", }, + { + name: "acnp-invalid-label-key-applied-to", + policy: &crdv1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "acnp-invalid-label-key-applied-to", + }, + Spec: crdv1alpha1.ClusterNetworkPolicySpec{ + AppliedTo: []crdv1alpha1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo=": "bar"}, + }, + }, + }, + }, + }, + expectedReason: "Invalid label key: foo=: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')", + }, + { + name: "acnp-invalid-label-value-applied-to", + policy: &crdv1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "acnp-invalid-label-value-applied-to", + }, + Spec: crdv1alpha1.ClusterNetworkPolicySpec{ + Ingress: []crdv1alpha1.Rule{ + { + Action: &allowAction, + From: []crdv1alpha1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar"}, + }, + }, + }, + AppliedTo: []crdv1alpha1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo1": "bar="}, + }, + }, + }, + }, + }, + }, + }, + expectedReason: "Invalid label value: bar=: 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])?')", + }, + { + name: "acnp-invalid-label-key-rule", + policy: &crdv1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "acnp-invalid-label-key-rule", + }, + Spec: crdv1alpha1.ClusterNetworkPolicySpec{ + AppliedTo: []crdv1alpha1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar"}, + }, + }, + }, + Ingress: []crdv1alpha1.Rule{ + { + Action: &allowAction, + From: []crdv1alpha1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo=": "bar1"}, + }, + }, + }, + }, + }, + }, + }, + expectedReason: "Invalid label key: foo=: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -955,3 +1034,53 @@ func TestValidateAntreaPolicy(t *testing.T) { }) } } + +func TestValidateClusterAntreaGroup(t *testing.T) { + tests := []struct { + name string + group *crdv1alpha2.ClusterGroup + expectedReason string + }{ + { + name: "cg-invalid-label-key", + group: &crdv1alpha2.ClusterGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cg-invalid-label-key", + }, + Spec: crdv1alpha2.GroupSpec{ + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo=": "bar"}, + }, + }, + }, + expectedReason: "Invalid label key: foo=: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')", + }, + { + name: "cg-invalid-label-value", + group: &crdv1alpha2.ClusterGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cg-invalid-label-value", + }, + Spec: crdv1alpha2.GroupSpec{ + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar="}, + }, + }, + }, + expectedReason: "Invalid label value: bar=: 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])?')", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, c := newController() + v := NewNetworkPolicyValidator(c.NetworkPolicyController) + actualReason, allowed := v.validateAntreaGroup(tt.group, nil, admv1.Create, authenticationv1.UserInfo{}) + assert.Equal(t, tt.expectedReason, actualReason) + if tt.expectedReason == "" { + assert.True(t, allowed) + } else { + assert.False(t, allowed) + } + }) + } +}