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

integration: Disable installing Security Profile Operator #2486

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

mauriciovasquezbernal
Copy link
Member

The security profile operator is only used by TestAuditSeccomp to install a simple seccomp profile in the cluster. This is an overkill.

This commit uses a simpler approach by copying the profile by using a DaemonSet to the node and disables installing this by default. The code to deploy/undeploy the SPO isn't removed as it could be useful to test the "advise seccomp" gadget in the future.

Some reasons to avoid deploying the SPO are:

  • It needs quite a lot of resources
  • It takes a lot of time to be deployed
  • Some times it fails to be deployed, and also to be cleanup, leaving it running on the cluster

The security profile operator is only used by TestAuditSeccomp to
install a simple seccomp profile in the cluster. This is an overkill.

This commit uses a simpler approach by copying the profile by using
a DaemonSet to the node and disables installing this by default.
The code to deploy/undeploy the SPO isn't removed as it could be useful
to test the "advise seccomp" gadget in the future.

Some reasons to avoid deploying the SPO are:
- It needs quite a lot of resources
- It takes a lot of time to be deployed
- Some times it fails to be deployed, and also to be cleanup, leaving
  it running on the cluster

Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>
Copy link
Member

@mqasimsarfraz mqasimsarfraz left a comment

Choose a reason for hiding this comment

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

I tested it on minikube and AKS is working fine. I agree that removing SPO as a dependency frees resources and simplify things.

Perhaps we should confirm with @blanquicet as well before removing SPO.

@@ -25,8 +25,6 @@ import (

func TestAuditSeccomp(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we should also have a TestAuditSeccompSPO which allows testing with SPO and is guarded by:

if *doNotDeploySPO {
  t.Skip("Skipping test since SPO is not deployed")
}

Not sure if it makes sense or not.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary as using this manual sp installation approach fully tests the audit-seccomp gadget.

Copy link
Member

@blanquicet blanquicet left a comment

Choose a reason for hiding this comment

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

I didn't test it, but I agree with the suggested changed.

@@ -25,8 +25,6 @@ import (

func TestAuditSeccomp(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary as using this manual sp installation approach fully tests the audit-seccomp gadget.

doNotDeploySPO = flag.Bool("no-deploy-spo", false, "don't deploy the Security Profiles Operator (SPO)")
doNotDeploySPO = flag.Bool("no-deploy-spo", true, "don't deploy the Security Profiles Operator (SPO)")
Copy link
Member

Choose a reason for hiding this comment

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

I agree to keep the code because it will be helpful to test the advise seccomp-profile gadget, see #975.

@mauriciovasquezbernal
Copy link
Member Author

Merging now, I'll monitor this test to see if there are any issues later on.

@mauriciovasquezbernal mauriciovasquezbernal merged commit 24c0689 into main Feb 12, 2024
57 checks passed
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/integration-disable-spo branch February 12, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants