Improve Antrea-native Policy CRD schema verification #2125

wenqiq · 2021-04-25T15:41:11Z

Add namespaceSelector/podSelector validations in all CRD schema.

The incorrect input of labelSelector fields that have known schema should be rejected;

In the following example, we want to create ClusterNetworkPolicy resource:

spec.appliedTo.namespaceSelector must be matchLabels or matchExpressions,
actually in the existing version the spelling mistake ‘matchLables’ can not be checked out.
Save the CustomResourceDefinition to resourcedefinition.yaml:

cat acnp_demo.yaml

apiVersion: crd.antrea.io/v1alpha1
kind: ClusterNetworkPolicy
metadata:
  name: blacklist
spec:
    priority: 5
    tier: securityops
    appliedTo:
      - namespaceSelector:
          matchLables:
            env: dummy
    ingress:
      - action: Drop

We created it succeeded without any error.

After fixed the issue, we will get the following error message as expected:

kubectl create -f acnp_demo.yaml
error: error validating "acnp_demo.yaml": error validating data: ValidationError(ClusterNetworkPolicy.spec.appliedTo[0].namespaceSelector): unknown field "matchLables" in io.antrea.crd.v1alpha1.ClusterNetworkPolicy.spec.appliedTo.namespaceSelector; if you choose to ignore these errors, turn validation off with --validate=false

In addition, all of namespaceSelector/podSelector parameters verification in CRD schema will be improved.

Fix #2090

codecov-commenter · 2021-04-25T15:45:22Z

Codecov Report

Merging #2125 (2a50612) into main (d258bbc) will increase coverage by 19.83%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #2125       +/-   ##
===========================================
+ Coverage   41.41%   61.25%   +19.83%     
===========================================
  Files         131      269      +138     
  Lines       16502    20453     +3951     
===========================================
+ Hits         6834    12528     +5694     
+ Misses       9084     6631     -2453     
- Partials      584     1294      +710

Flag	Coverage Δ
kind-e2e-tests	`52.09% <ø> (?)`
unit-tests	`41.40% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/apiserver/handlers/webhook/mutation_labels.go	`24.71% <0.00%> (ø)`
...ntroller/crdmirroring/crdhandler/externalentity.go	`16.27% <0.00%> (ø)`
...lientset/versioned/typed/ops/v1alpha1/traceflow.go	`24.09% <0.00%> (ø)`
...tset/versioned/typed/core/v1alpha2/clustergroup.go	`24.09% <0.00%> (ø)`
pkg/agent/apiserver/handlers/errors.go	`0.00% <0.00%> (ø)`
pkg/ovs/ovsctl/interface.go	`0.00% <0.00%> (ø)`
pkg/agent/interfacestore/interface_cache.go	`81.37% <0.00%> (ø)`
pkg/signals/signals.go	`100.00% <0.00%> (ø)`
...gacyclient/listers/core/v1alpha2/externalentity.go	`6.25% <0.00%> (ø)`
pkg/ipfix/ipfix_process.go	`100.00% <0.00%> (ø)`
... and 225 more

tnqn

Could you remove the ending ";" and issue ID from the commit title?

build/yamls/base/crds.yml

tnqn

Is the PR ready for review? I see commented code

wenqiq · 2021-04-27T11:47:38Z

Is the PR ready for review? I see commented code

yes, wait for code review

tnqn

LGTM overall, maybe @Dyanngg can take a look too.

tnqn · 2021-04-27T11:56:46Z

test/e2e/antreapolicy_test.go

+
+	t.Run("TestGroupInvalidLabelSelectorInResource", func(t *testing.T) {
+		t.Run("Case=InvalidACNPPodSelectorNsSelectorMatchExpressions", func(t *testing.T) {
+			testInvalidACNPPodSelectorNsSelectorMatchExpressions(t)


Could you add it to TestGroupValidateAntreaNativePolicies? it seems not different from cases in that group.
And maybe remove testInvalidCGPPodSelectorNsSelectorMatchExpressions as it uses same verification as policies (normally it's fine to have a separate test for ClusterGroup but we are working on reducing e2e runtime #2014 so let's avoid adding it unless it can cover something new)

Done in a new commit

tnqn

LGTM

tnqn · 2021-04-27T13:31:10Z

/test-all

tnqn · 2021-04-28T03:01:39Z

All e2e tests failed on antrea networkpolicy case, which seems related to this change. @wenqiq could you take a look at the failures?

wenqiq · 2021-04-28T03:10:08Z

All e2e tests failed on antrea networkpolicy case, which seems related to this change. @wenqiq could you take a look at the failures?

well, I will check, It sames the e2e tests fails even with the upstream/main branch as I test yesterday.

Improve Antrea-native Policy CRD schema verification

tnqn · 2021-04-28T03:50:27Z

We run e2e tests on VMs and Kind clusters, all tests related to Antrea policy failed on all platforms, and only they failed. This indicated it's related to the PR.

tnqn · 2021-04-28T16:09:11Z

build/yamls/base/crds.yml

+                                  items:
+                                    type: string
+                          matchLabels:
+                            type: object


I guess you need to set "x-kubernetes-preserve-unknown-fields: true" for matchLabels, otherwise all keys are pruned so the e2e tests failed. Glad to see it's caught by the tests.
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#controlling-pruning

Thanks for advice，obviously，some resource can not created as expected in e2e test cases cause the fails, but I have verified those crds in my local kind cluster; I tried reviewing the e2e test cases to find sth, however I did not find an efficient method to implement the e2e test in my local dev env.

https://github.com/vmware-tanzu/antrea/runs/2463089906?check_suite_focus=true
It sames work, thks so much.

wenqiq · 2021-04-29T05:35:24Z

e2e test back to normal, pls review the latest changes, thanks a lot @tnqn
https://github.com/vmware-tanzu/antrea/actions/runs/794762925

tnqn

Thanks, LGTM

tnqn · 2021-04-29T13:58:58Z

/test-all

tnqn · 2021-04-29T13:59:14Z

/test-windows-e2e

Dyanngg · 2021-04-29T20:15:04Z

.gitignore

@@ -14,3 +14,4 @@ bin

 .idea/
 .vscode/
+vendor


nit: this should be a local ignore?

github/gitignore/Go.gitignore doesn't add vendor/ to its .gitignore file.

# Dependency directories (remove the comment below to include it) # vendor/

gitignore.io/api/go does add vendor/ to its .gitignore.

# Dependency directories (remove the comment below to include it) # vendor/ ### Go Patch ### /vendor/ /Godeps/

@wenqiq Normally it would be better to add this via a separate PR if it is controversial so this PR can proceed faster.

Anyway Antrea doesn't include vendor directory in the repo so at least it's harmless to add it here for the convenience of vendor mode developers. @Dyanngg Do you have strong opinion to remove it from this PR? If not, I'm going to merge it as is.

I don't have a strong opinion for this. Merged.

Dyanngg · 2021-04-29T20:15:41Z

/test-networkpolicy

vmwclabot added the cla-not-required label Apr 25, 2021

tnqn reviewed Apr 26, 2021

View reviewed changes

build/yamls/base/crds.yml Outdated Show resolved Hide resolved

wenqiq force-pushed the fix-issue-2090 branch 3 times, most recently from 22559c6 to 4463d19 Compare April 27, 2021 03:58

wenqiq closed this Apr 27, 2021

wenqiq reopened this Apr 27, 2021

wenqiq force-pushed the fix-issue-2090 branch 3 times, most recently from 4907307 to a59b35b Compare April 27, 2021 08:05

tnqn reviewed Apr 27, 2021

View reviewed changes

wenqiq force-pushed the fix-issue-2090 branch from a59b35b to f54298e Compare April 27, 2021 11:44

tnqn reviewed Apr 27, 2021

View reviewed changes

wenqiq force-pushed the fix-issue-2090 branch from ea9f56e to e5cf4fa Compare April 27, 2021 13:16

tnqn previously approved these changes Apr 27, 2021

View reviewed changes

wenqiq added 4 commits April 28, 2021 11:11

Add namespaceSelector/podSelector validations in all CRD schema

0f6c5f2

Improve Antrea-native Policy CRD schema verification

update the generate manifest YAMLs

9d3c390

add e2e test cases in antrea policy verify invalid labelSelector;

f493da8

formalize e2e test case path of testInvalidACNPPodSelector;

af90d67

wenqiq dismissed tnqn’s stale review via af90d67 April 28, 2021 03:16

wenqiq force-pushed the fix-issue-2090 branch from e5cf4fa to af90d67 Compare April 28, 2021 03:16

wenqiq force-pushed the fix-issue-2090 branch 2 times, most recently from 2a50612 to c6183c3 Compare April 28, 2021 11:59

tnqn reviewed Apr 28, 2021

View reviewed changes

wenqiq force-pushed the fix-issue-2090 branch from c6183c3 to bf71db4 Compare April 29, 2021 01:58

verify e2e test;

6c3d314

wenqiq force-pushed the fix-issue-2090 branch from bf71db4 to 6c3d314 Compare April 29, 2021 03:26

tnqn approved these changes Apr 29, 2021

View reviewed changes

Dyanngg approved these changes Apr 29, 2021

View reviewed changes

Dyanngg merged commit 64e017b into antrea-io:main May 6, 2021

tnqn mentioned this pull request May 7, 2021

Bump ClusterGroup to crd/v1alpha3 and deprecate single ipBlock #2008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Antrea-native Policy CRD schema verification #2125

Improve Antrea-native Policy CRD schema verification #2125

wenqiq commented Apr 25, 2021

codecov-commenter commented Apr 25, 2021 •

edited

Loading

tnqn left a comment

tnqn left a comment

wenqiq commented Apr 27, 2021

tnqn left a comment

tnqn Apr 27, 2021

wenqiq Apr 27, 2021

tnqn left a comment

tnqn commented Apr 27, 2021

tnqn commented Apr 28, 2021

wenqiq commented Apr 28, 2021

tnqn commented Apr 28, 2021

tnqn Apr 28, 2021

wenqiq Apr 29, 2021 •

edited

Loading

wenqiq commented Apr 29, 2021 •

edited

Loading

tnqn left a comment

tnqn commented Apr 29, 2021

tnqn commented Apr 29, 2021

Dyanngg Apr 29, 2021

wenqiq Apr 30, 2021

tnqn May 6, 2021

Dyanngg May 6, 2021

Dyanngg commented Apr 29, 2021

Improve Antrea-native Policy CRD schema verification #2125

Improve Antrea-native Policy CRD schema verification #2125

Conversation

wenqiq commented Apr 25, 2021

codecov-commenter commented Apr 25, 2021 • edited Loading

Codecov Report

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

wenqiq commented Apr 27, 2021

tnqn left a comment

Choose a reason for hiding this comment

tnqn Apr 27, 2021

Choose a reason for hiding this comment

wenqiq Apr 27, 2021

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 27, 2021

tnqn commented Apr 28, 2021

wenqiq commented Apr 28, 2021

tnqn commented Apr 28, 2021

tnqn Apr 28, 2021

Choose a reason for hiding this comment

wenqiq Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

wenqiq commented Apr 29, 2021 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 29, 2021

tnqn commented Apr 29, 2021

Dyanngg Apr 29, 2021

Choose a reason for hiding this comment

wenqiq Apr 30, 2021

Choose a reason for hiding this comment

tnqn May 6, 2021

Choose a reason for hiding this comment

Dyanngg May 6, 2021

Choose a reason for hiding this comment

Dyanngg commented Apr 29, 2021

codecov-commenter commented Apr 25, 2021 •

edited

Loading

wenqiq Apr 29, 2021 •

edited

Loading

wenqiq commented Apr 29, 2021 •

edited

Loading