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

Update Netpol e2e tests to use framework CreateNamespace #111789

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

antoninbas
Copy link
Contributor

The main purpose of this change is to update the e2e Netpol tests to use
the srandard CreateNamespace function from the Framework. Before this
change, a custom Namespace creation function was used, with the
following consequences:

  • Pod security admission settings had to be enforced locally (not using
    the centralized mechanism)
  • the custom function was brittle, not waiting for default Namespace
    ServiceAccount creation, causing tests to fail in some infrastructures
  • tests were not benefiting from standard framework capabilities:
    Namespace name generation, automatic Namespace deletion, etc.

As part of this change, we also do the following:

  • clearly decouple responsibilities between the Model, which defines the
    K8s objects to be created, and the KubeManager, which has access to
    runtime information (actual Namespace names after their creation by
    the framework, Service IPs, etc.)
  • simplify / clean-up tests and remove as much unneeded logic / funtions
    as possible for easier long-term maintenance
  • remove the useFixedNamespaces compile-time constant switch, which
    aimed at re-using existing K8s resources across test cases. The
    reasons: a) it is currently broken as setting it to true causes most
    tests to panic on the master branch, b) it is not a good idea to have
    some switch like this which changes the behavior of the tests and is
    never exercised in CI, c) it cannot possibly work as different test
    cases have different Model requirements (e.g., the protocols list can
    differ) and hence different K8s resource requirements.

For #108298

Signed-off-by: Antonin Bas abas@vmware.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

For #108298

It only addresses the issue for Netpol tests. Some other e2e tests may still be using a custom Namespace creation function instead of the Framework one.

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.:

The main purpose of this change is to update the e2e Netpol tests to use
the srandard CreateNamespace function from the Framework. Before this
change, a custom Namespace creation function was used, with the
following consequences:

* Pod security admission settings had to be enforced locally (not using
  the centralized mechanism)
* the custom function was brittle, not waiting for default Namespace
  ServiceAccount creation, causing tests to fail in some infrastructures
* tests were not benefiting from standard framework capabilities:
  Namespace name generation, automatic Namespace deletion, etc.

As part of this change, we also do the following:

* clearly decouple responsibilities between the Model, which defines the
  K8s objects to be created, and the KubeManager, which has access to
  runtime information (actual Namespace names after their creation by
  the framework, Service IPs, etc.)
* simplify / clean-up tests and remove as much unneeded logic / funtions
  as possible for easier long-term maintenance
* remove the useFixedNamespaces compile-time constant switch, which
  aimed at re-using existing K8s resources across test cases. The
  reasons: a) it is currently broken as setting it to true causes most
  tests to panic on the master branch, b) it is not a good idea to have
  some switch like this which changes the behavior of the tests and is
  never exercised in CI, c) it cannot possibly work as different test
  cases have different Model requirements (e.g., the protocols list can
  differ) and hence different K8s resource requirements.

For kubernetes#108298

Signed-off-by: Antonin Bas <abas@vmware.com>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 10, 2022
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.25 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.25.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Aug 10 13:31:11 UTC 2022.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 10, 2022
@k8s-ci-robot
Copy link
Contributor

@antoninbas: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @antoninbas. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Aug 10, 2022
@k8s-ci-robot k8s-ci-robot added area/network-policy Issues or PRs related to Network Policy subproject area/test sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 10, 2022
@antoninbas
Copy link
Contributor Author

Ran the test suite manually (with Antrea as CNI provider):

------------------------------
[ReportAfterSuite] PASSED [0.000 seconds]
[ReportAfterSuite] Kubernetes e2e suite report
test/e2e/e2e_test.go:146

  Begin Captured GinkgoWriter Output >>
    [ReportAfterSuite] TOP-LEVEL
      test/e2e/e2e_test.go:146
  << End Captured GinkgoWriter Output
------------------------------

Ran 51 of 7067 Specs in 1173.822 seconds
SUCCESS! -- 51 Passed | 0 Failed | 0 Pending | 7016 Skipped
PASS

@antoninbas
Copy link
Contributor Author

/cc @s-urbaniak @danwinship

@tnqn
Copy link
Member

tnqn commented Aug 11, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2022
@astoycos
Copy link
Contributor

cc @jayunit100 @stlaz

@mattfenwick
Copy link
Contributor

+1 nice one Antonin ! 🚀

@aojea
Copy link
Member

aojea commented Aug 22, 2022

/assign

Let's try to get this early in the cycle, release is tomorrow

@jayunit100
Copy link
Member

I just saw this! Thanks

@jayunit100
Copy link
Member

B) I actually like the use fixed names paces switch for certain test hacking but there's an e2e flag that u can use that persists the names paces instead so... ok makes sense

@jayunit100
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoninbas, jayunit100

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 lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 23, 2022
@jayunit100
Copy link
Member

everyone else ok with this one ?

@knabben
Copy link
Member

knabben commented Aug 23, 2022

/lgtm

@antoninbas
Copy link
Contributor Author

@jayunit100

I actually like the use fixed names paces switch for certain test hacking but there's an e2e flag that u can use that persists the names paces instead so... ok makes sense

I do see the appeal as well. It was actually one of the initial selling points for the updated netpol framework: create a set of Namespaces / Pods / Services once and run a bunch of tests with different NetworkPolicies to validate a CNI implementation. However, the ability to do that was lost over time after upstreaming the framework and making it work with the standard upstream e2e framework. Before removing the flag, I tried to run it locally and confirmed it was broken. I tried to fix it to keep the functionality, but couldn't find a good way to do it. One of the reasons is that different test cases within the test suite have different protocols requirements, and that requires test Pods to be re-created. Even if it's fixed somehow, it will probably break again within a couple of months as folks make additional changes to these files, given that this way of running tests would not be exercised in CI. When doing local development, I find that a common pattern is running an individual test case, in which case the ability to use the same K8s resources is not super useful.

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/network-policy Issues or PRs related to Network Policy subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants