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.

Sign up for GitHub

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

Jump to bottom

Add conformance test for .spec.Priority field in ANP #113

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Add conformance test for .spec.Priority field in ANP #113

merged 1 commit into from
Jun 30, 2023

Conversation

tssurya
Copy link
Contributor

@tssurya tssurya commented May 29, 2023 •
edited
Loading

This commit adds tests that check if the
.spec.Priority field is respected and works
as expected.
Note that we use Pods field to express
Subject and Peers so that we test that aspect
unlike the rest of the tests so far which
have always used Namespaces aspect.

Looking good here:

    --- PASS: TestConformance/AdminNetworkPolicyIntegration (25.67s)                                                                                                         
        --- PASS: TestConformance/AdminNetworkPolicyIntegration/Should_Deny_traffic_from_slytherin_to_gryffindor_respecting_ANP (6.25s)                                      
        --- PASS: TestConformance/AdminNetworkPolicyIntegration/Should_Deny_traffic_to_slytherin_from_gryffindor_respecting_ANP (6.29s)                                      
        --- PASS: TestConformance/AdminNetworkPolicyIntegration/Should_support_a_'pass-ingress'_policy_for_ANP_and_respect_the_match_for_network_policy (0.24s)              
        --- PASS: TestConformance/AdminNetworkPolicyIntegration/Should_support_a_'pass-egress'_policy_for_ANP_and_respect_the_match_for_network_policy (0.20s)               
        --- PASS: TestConformance/AdminNetworkPolicyIntegration/Should_support_a_'pass-ingress'_policy_for_ANP_and_respect_the_match_for_baseline_admin_network_policy (6.35s
)                                                                                                                                                                            
        --- PASS: TestConformance/AdminNetworkPolicyIntegration/Should_support_a_'pass-egress'_policy_for_ANP_and_respect_the_match_for_baseline_admin_network_policy (6.28s)
    --- PASS: TestConformance/AdminNetworkPolicyPriorityField (13.25s)                                                                                                       
        --- PASS: TestConformance/AdminNetworkPolicyPriorityField/Should_Deny_traffic_from_slytherin_to_gryffindor_respecting_ANP (6.27s)                                    
        --- PASS: TestConformance/AdminNetworkPolicyPriorityField/Should_Deny_traffic_to_slytherin_from_gryffindor_respecting_ANP (6.38s)                                    
        --- PASS: TestConformance/AdminNetworkPolicyPriorityField/Should_respect_ANP_priority_field;_thus_passing_both_ingress_and_egress_traffic_over_to_BANP (0.53s)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 29, 2023
@netlify
Copy link

netlify bot commented May 29, 2023 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name Link
🔨 Latest commit c43707a
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/649f16a11a6d0c000829fce9
😎 Deploy Preview https://deploy-preview-113--kubernetes-sigs-network-policy-api.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot requested review from astoycos and Dyanngg May 29, 2023 12:56
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2023
@tssurya
Copy link
Contributor Author

tssurya commented May 29, 2023

/test network-policy-api-crd-e2e

@k8s-ci-robot
Copy link
Contributor

@tssurya: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-network-policy-api-crd-e2e
  • /test pull-network-policy-api-verify

Use /test all to run all jobs.

In response to this:

/test network-policy-api-crd-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tssurya
Copy link
Contributor Author

tssurya commented May 29, 2023

/test pull-network-policy-api-crd-e2e

astoycos
astoycos reviewed May 31, 2023
View reviewed changes
conformance/tests/admin-network-policy-core-priority.go
}, clientPod)
framework.ExpectNoError(err, "unable to fetch the server pod")
// draco-malfoy-0 is our client pod in slytherin namespace
// ensure ingress is PASSED to gryffindor from slytherin - the baseline admin network policy ALLOW should take effect
Copy link
Member

Choose a reason for hiding this comment

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

Can we really tell the difference here between PASS and ALLOW?

Copy link
Member

Choose a reason for hiding this comment

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

i.e there's not really a way to verify wether it was

  1. an Explicit ANP Allow ||
  2. an Explicit BANP Allow ||
  3. an full pass through to Allow

Copy link
Member

@astoycos astoycos May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you could eliminate 3. with a large-scale baseline deny

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah measuring pass and allow has been tricky actually.. good point here.
if there is something to catch this underneath like we do in the integration tests:

network-policy-api/conformance/tests/admin-network-policy-core-integration.go

Line 127 in fdf177d

t.Run("Should support a 'pass-egress' policy for ANP and respect the match for network policy", func(t *testing.T) {
then we have a proper check, otherwise the two pretty much behave the exact same way.

@tssurya tssurya mentioned this pull request Jun 12, 2023
Merged
34 tasks
@tssurya tssurya force-pushed the anp-conformance-priority branch from be3456e to 459b858 Compare June 30, 2023 17:46
@tssurya
Add conformance test for .spec.Priority
c43707a 
This commit adds tests that check if the
.spec.Priority field is respected and works
as expected.
Note that we use `Pods` field to express
Subject and Peers so that we test that aspect
unlike the rest of the tests so far which
have always used `Namespaces` aspect.

Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
@tssurya tssurya force-pushed the anp-conformance-priority branch from 459b858 to c43707a Compare June 30, 2023 17:53
@astoycos
Copy link
Member

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, tssurya

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 Jun 30, 2023
@k8s-ci-robot k8s-ci-robot merged commit 42095fb into kubernetes-sigs:master Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astoycos astoycos astoycos left review comments

@Dyanngg Dyanngg Awaiting requested review from Dyanngg

Assignees

@astoycos astoycos

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tssurya @k8s-ci-robot @astoycos