Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netpol: Add CRUD tests for NetworkPolicy API #95155

Merged
merged 1 commit into from Oct 21, 2020

Conversation

cmluciano
Copy link

@cmluciano cmluciano commented Sep 29, 2020

What type of PR is this?
/kind feature
/area conformance
/area test

What this PR does / why we need it:

  • Adds CRUD API tests for the V1 NetworkPolicy suitable for promotion to conformance.
  • CRUD operations are the extent of conformance testing that we can add
    for NetworkPolicy tests since we require a 3rd party plugin like CNI for enforcement.

Which issue(s) this PR fixes:
xref: #94776

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. area/conformance Issues or PRs related to kubernetes conformance tests area/test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 29, 2020
@cmluciano
Copy link
Author

/sig network

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 29, 2020
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Sep 29, 2020
@jayunit100
Copy link
Member

@cmluciano any thoughts on how we might coordinate on #91592 (implements the KEP in flight to overhaul) ?

@jayunit100
Copy link
Member

oh wait,

i was worried these were new policies validation tests, but it seems like theyre just doing API stuff...

maybe this works as a standalone test, since it doesnt actually test the policies ...

Copy link
Author

@cmluciano cmluciano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feedback comments

test/e2e/network/network_policy.go Show resolved Hide resolved
@cmluciano
Copy link
Author

@jayunit100 Yes these are minimal tests that will eventually be promoted to conformance testing. These tests don't require the plugin and should not cause conflict in a non-fresh cluster.

@aojea
Copy link
Member

aojea commented Oct 1, 2020

/lgtm

it runs in the CI and pass

�[90m------------------------------�[0m
[BeforeEach] [sig-network] NetworkPolicy API
  test/e2e/framework/framework.go:174
�[1mSTEP�[0m: Creating a kubernetes client
Sep 29 17:54:48.153: INFO: >>> kubeConfig: /root/.kube/kind-test-config
�[1mSTEP�[0m: Building a namespace api object, basename networkpolicies
�[1mSTEP�[0m: Waiting for a default service account to be provisioned in namespace
[It] should support creating NetworkPolicy API operations
  test/e2e/network/network_policy.go:2285
�[1mSTEP�[0m: getting /apis
�[1mSTEP�[0m: getting /apis/networking.k8s.io
�[1mSTEP�[0m: getting /apis/networking.k8s.iov1
�[1mSTEP�[0m: creating
�[1mSTEP�[0m: getting
�[1mSTEP�[0m: listing
�[1mSTEP�[0m: watching
Sep 29 17:54:48.392: INFO: starting watch
�[1mSTEP�[0m: cluster-wide listing
�[1mSTEP�[0m: cluster-wide watching
Sep 29 17:54:48.427: INFO: starting watch
�[1mSTEP�[0m: patching
�[1mSTEP�[0m: updating
Sep 29 17:54:48.471: INFO: waiting for watch events with expected annotations
Sep 29 17:54:48.471: INFO: saw patched and updated annotations
�[1mSTEP�[0m: deleting
�[1mSTEP�[0m: deleting a collection
[AfterEach] [sig-network] NetworkPolicy API
  test/e2e/framework/framework.go:175
Sep 29 17:54:48.607: INFO: Waiting up to 3m0s for all (but 0) nodes to be ready
�[1mSTEP�[0m: Destroying namespace "networkpolicies-8160" for this suite.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020
@cmluciano
Copy link
Author

@danwinship Does this look good to you?

@danwinship
Copy link
Contributor

I guess? I don't know a lot about conformance requirements. Are there other APIs where we do something like this?

@andrewsykim
Copy link
Member

I don't think any of the tests here are being added to conformance yet, though I do think they should be promoted in the future once the tests have baked for a bit (similar to the Ingress API tests). @cmluciano was the intention to add these to conformance in this PR?

@danwinship
Copy link
Contributor

He said "suitable for promotion to conformance" so I assume the intention wasn't to make them conformance in this PR. But I was just saying I have no idea if these are actually "suitable for promotion to conformance"...

@andrewsykim
Copy link
Member

Thanks for clarifying :)

I would think these tests are suitable given the Ingress API tests are in conformance today. @johnbelamaric @dims do you foresee any issues promoting these to conformance at some point in the future?

@cmluciano
Copy link
Author

Yes the plan was to promote these to conformance after they are successful for a few weeks. I believe the tests are suitable for promotion since they follow the same conventions and format as the ones I added for Ingress API conformance with #91830

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2020
CRUD operations are the extent of conformance testing that we can add
for NetworkPolicy tests since we require a 3rd party plugin like CNI
for enforcement.

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, cmluciano

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2020
@cmluciano
Copy link
Author

/retest

@cmluciano
Copy link
Author

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 21, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit ffb233f into kubernetes:master Oct 21, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance Issues or PRs related to kubernetes conformance tests area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants