Skip to content

Commit

Permalink
Validate labels in CRDs (antrea-io#3331)
Browse files Browse the repository at this point in the history 
Fixes antrea-io#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 <[email protected]>
  • Loading branch information
@GraysonWu
GraysonWu authored and zbangqi committed Mar 1, 2022
1 parent 76185b7 commit cf51ad5
Show file tree
Hide file tree
Showing 2 changed files with 164 additions and 0 deletions.
35 changes: 35 additions & 0 deletions pkg/controller/networkpolicy/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
129 changes: 129 additions & 0 deletions pkg/controller/networkpolicy/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
}
})
}
}

0 comments on commit cf51ad5

Please sign in to comment.