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

ROX-23967: Tenant network policies tests #1835

Merged
merged 4 commits into from
Jun 18, 2024
Merged

ROX-23967: Tenant network policies tests #1835

merged 4 commits into from
Jun 18, 2024

Conversation

vladbologa
Copy link
Contributor

@vladbologa vladbologa commented May 27, 2024

Description

This PR introduces tests for the tenant network policies.

The fake-service service and fake-client deployment can then be used to mock ACS components, by applying the appropriate labels and setting the ports where they listen or connect to. They can easily be installed using Helm.

They can then be used to write tests, such as the following:

  • create a fake Central in namespace A, and a fake Scanner in namespace B
  • show that Scanner can connect to Central
  • apply the network policies, in turn, in namespace A and then namespace B
  • shows that in both cases, the communication is interrupted - proving that the network policies prevent lateral communication between tenants in this case

They will run on OpenShift CI, as part of the e2e test suite.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
  • Add secret to app-interface Vault or Secrets Manager if necessary
  • RDS changes were e2e tested manually
  • Check AWS limits are reasonable for changes provisioning new resources
  • (If applicable) Changes to the dp-terraform Helm values have been reflected in the addon on integration environment

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

Copy link
Contributor

openshift-ci bot commented May 27, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@vladbologa
Copy link
Contributor Author

/test all

@vladbologa vladbologa requested a review from ebensh June 3, 2024 13:23
Copy link
Collaborator

@ebensh ebensh left a comment

Choose a reason for hiding this comment

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

High level comments -
Cool stuff :)
Might need to support multiple ports on test deployment, since e.g. Central has ~4 (2 monitoring, 1 UI, 1 data)
Nifty nginx usage and readiness check

@vladbologa
Copy link
Contributor Author

High level comments - Cool stuff :) Might need to support multiple ports on test deployment, since e.g. Central has ~4 (2 monitoring, 1 UI, 1 data) Nifty nginx usage and readiness check

I was thinking that we'd just redeploy with a different port. Do you think we need testing multiple ports simultaneously?

@ebensh
Copy link
Collaborator

ebensh commented Jun 5, 2024

High level comments - Cool stuff :) Might need to support multiple ports on test deployment, since e.g. Central has ~4 (2 monitoring, 1 UI, 1 data) Nifty nginx usage and readiness check

I was thinking that we'd just redeploy with a different port. Do you think we need testing multiple ports simultaneously?

I thought the test would set up a fake central, fake scanner, etc., create the netpols, and then do the connectivity checks in a bunch, instead of starting up/stopping deployments to change ports. If it's easier that way that's fine too :)

@vladbologa
Copy link
Contributor Author

High level comments - Cool stuff :) Might need to support multiple ports on test deployment, since e.g. Central has ~4 (2 monitoring, 1 UI, 1 data) Nifty nginx usage and readiness check

I was thinking that we'd just redeploy with a different port. Do you think we need testing multiple ports simultaneously?

I thought the test would set up a fake central, fake scanner, etc., create the netpols, and then do the connectivity checks in a bunch, instead of starting up/stopping deployments to change ports. If it's easier that way that's fine too :)

I think it's easier, and I'd also just test one policy at a time.

@vladbologa vladbologa changed the title [WIP] ROX-23967: Tenant network policies tests ROX-23967: Tenant network policies tests Jun 6, 2024
@vladbologa vladbologa marked this pull request as ready for review June 6, 2024 14:28
@vladbologa vladbologa requested a review from ebensh June 14, 2024 17:05
default_type application/octet-stream;

server {
listen 8443;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be parameterized on {{ .Values.service.port }}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to do that, good catch. I'll probably want to add some tests on different ports as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ebensh
Copy link
Collaborator

ebensh commented Jun 18, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 18, 2024
Copy link
Contributor

openshift-ci bot commented Jun 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ebensh, vladbologa

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

@vladbologa
Copy link
Contributor Author

/test e2e

1 similar comment
@vladbologa
Copy link
Contributor Author

/test e2e

@vladbologa vladbologa merged commit 943c934 into main Jun 18, 2024
6 checks passed
@vladbologa vladbologa deleted the vb/netpol-tests branch June 18, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants