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

Exposing values to enable users to manage the SecurityProfilesOperatorDaemon config #1376

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

rahulroshan-kachchap
Copy link
Contributor

@rahulroshan-kachchap rahulroshan-kachchap commented Dec 13, 2022

What type of PR is this?

kind feature

What this PR does / why we need it:

Expanding helm chart to expose values to enable users to manage the SecurityProfilesOperatorDaemon config, so that users can toggle what's deployed more easily via three new values: enableSelinux, enableLogEnricher and enableAppArmor.
Added config.yaml file in the templates directory of helm chart which takes configurable input from users via values.yaml.

Which issue(s) this PR fixes:

Fixes #1320

Does this PR have test?

NA

Special notes for your reviewer:

Tested the above changes with only enableAppArmor: true and enableSelinux, enableLogEnricher as false. Verified in the spod logs after deployment:
I1213 04:41:53.620891 2231997 apparmorprofile.go:277] apparmor-spod "msg"="detecting apparmor support..."
I1213 04:41:53.642182 2231997 apparmorprofile.go:284] apparmor-spod "msg"="apparmor enabled: OK"
I1213 04:41:53.642412 2231997 apparmorprofile.go:287] apparmor-spod "msg"="apparmor enforceable: OK"

Does this PR introduce a user-facing change?

Exposed `enableSelinux`, `enableLogEnricher` and `enableAppArmor` values in the helm chart values.yaml to make it configurable by the user during the deployment.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Dec 13, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @rahulroshan-kachchap!

It looks like this is your first PR to kubernetes-sigs/security-profiles-operator 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/security-profiles-operator has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 13, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @rahulroshan-kachchap. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 13, 2022
@saschagrunert
Copy link
Member

/ok-to-test

Thank you for the contribution @rahulroshan-kachchap! I guess this one deserves a release note, doesn't it?

@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 Dec 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

Merging #1376 (064654d) into main (bf24f5c) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1376   +/-   ##
=======================================
  Coverage   44.13%   44.13%           
=======================================
  Files          50       50           
  Lines        5651     5651           
=======================================
  Hits         2494     2494           
  Misses       3037     3037           
  Partials      120      120           

@rahulroshan-kachchap
Copy link
Contributor Author

/ok-to-test

Thank you for the contribution @rahulroshan-kachchap! I guess this one deserves a release note, doesn't it?

Updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rahulroshan-kachchap / name: Rahul Roshan Kachchap (0f02581, 365378e)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 13, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, rahulroshan-kachchap

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 Dec 13, 2022
@rahulroshan-kachchap
Copy link
Contributor Author

/easycla

@jhrozek
Copy link
Contributor

jhrozek commented Dec 13, 2022

would it be possible to squash the patches to one to have a leaner commit history?

@jhrozek
Copy link
Contributor

jhrozek commented Dec 13, 2022

btw looks like you also need to run make deployments and commit the result

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 13, 2022
@rahulroshan-kachchap
Copy link
Contributor Author

would it be possible to squash the patches to one to have a leaner commit history?
Done

@rahulroshan-kachchap
Copy link
Contributor Author

btw looks like you also need to run make deployments and commit the result

@jhrozek Being this as my first PR , can you please elaborate on this? Where do i need to run make deployments? Do i need to post the result somewhere?

@jhrozek
Copy link
Contributor

jhrozek commented Dec 13, 2022

btw looks like you also need to run make deployments and commit the result

@jhrozek Being this as my first PR , can you please elaborate on this? Where do i need to run make deployments? Do i need to post the result somewhere?

Sorry, I don't know helm very much so I'm not sure I can help with the contents of the PR. But the gist of it is that during PR verifications, one of the checks that are run is the same thing as if you run make deployments from the command line in the SPO code tree. This command should not modify the resulting files in any way.

Looking at your PR, it looks like you were modifying deploy/helm/templates/static-resources.yaml which is the file the make deployments target writes to with $(BUILD_DIR)/kustomize build --reorder=none deploy/overlays/helm -o deploy/helm/templates/static-resources.yaml. Shouldn't your PR rather modify files under deploy/overlays/helm so that the result is generated the way you want?

@pjbgf
Copy link
Member

pjbgf commented Dec 15, 2022

Looks good and happy with everything being disabled by default. Tested and works as expected.

However, we still need the commits to be squashed so the changes are a single commit.

/lgtm /hold

@rahulroshan-kachchap
Copy link
Contributor Author

Looks good and happy with everything being disabled by default. Tested and works as expected.

However, we still need the commits to be squashed so the changes are a single commit.

/lgtm /hold

Squashed commits into one

@rahulroshan-kachchap
Copy link
Contributor Author

Hi @pjbgf , what is the expected date for approving this PR?

@JAORMX
Copy link
Contributor

JAORMX commented Dec 22, 2022

Running the CI tests now, waiting for them to finish before tagging this PR as approved.

@rahulroshan-kachchap
Copy link
Contributor Author

@JAORMX are we good here to close the PR as all the required tests are passing

spec:
enableSelinux: {{ .Values.enableSelinux }}
enableLogEnricher: {{ .Values.enableLogEnricher }}
enableAppArmor: {{ .Values.enableAppArmor }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expose also other flags in here such as: enableBpfRecoder, verbosity, enableProfiling. The same all switched off by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ccojocar Will add enableBpfRecorder & enableProfiling.
For verbosity crd says:
verbosity:
description: Verbosity specifies the logging verbosity of the daemon.
type: integer
Since its type is integer, i assume the default needs to be set as 0. Please suggest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ccojocar kindly suggest

Copy link
Member

@pjbgf pjbgf Feb 20, 2023

Choose a reason for hiding this comment

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

+1 considering the title of the PR I think that would make sense.
But also happy for that to be added as a follow-up PR.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rahulroshan-kachchap / name: Rahul Roshan Kachchap (3ca5892)

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed do-not-merge/contains-merge-commits cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 25, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 25, 2023
@pjbgf
Copy link
Member

pjbgf commented Feb 20, 2023

@rahulroshan-kachchap would you mind rebasing this PR again please?

@pjbgf
Copy link
Member

pjbgf commented Feb 21, 2023

@rahulroshan-kachchap thank you for following-up and rebasing. 🙇

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit 7a94361 into kubernetes-sigs:main Feb 21, 2023
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. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling of AppArmor feature by default in Security Profile Operator
8 participants