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

Add a default profile option for profileBindings #1869

Merged
merged 16 commits into from
Jan 23, 2024

Conversation

CoreyCook8
Copy link
Contributor

@CoreyCook8 CoreyCook8 commented Sep 6, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR introduces an "Enforce for all pods" option for ProfileBinding.

Container profile binding will have precedence here, but this will allow developers to create a default seccomp or selinux profile to be defaulted to all pods in a namespace.

If there exists a default profile binding and there has been no match between a container and profile binding for a given pod, the default profile will be applied to the pod level securityContext.

Which issue(s) this PR fixes:

Fixes #1868

Does this PR have test?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds functionality to the profile binding functionality to establish a default seccomp/selinux profile for a given namespace.
Specific image bindings have priority over the default profiles allowing more tailored profiles for specific images while allowing customization of a default profile applied to all pods without having to specify specific images strings.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. 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 Sep 6, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @CoreyCook8. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 6, 2023
@CoreyCook8 CoreyCook8 changed the title Patch 2 Add a default profile option for profileBindings Sep 6, 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 Sep 6, 2023
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! One general suggestion and I assume we need a new e2e test for this.

profileRef:
kind: SeccompProfile
name: profile-complain
image: *
Copy link
Member

Choose a reason for hiding this comment

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

Hm, how about allowing regular expressions here in general? This would end-users allow to select a registry for example.

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 had considered that, the implementation for the container storage is a sync.Map which I think would be difficult to work with a regular expression. I also didn't want to add too much complexity here. So I figured start with this case since it is simple rather than introducing a lot of complexity to the solution.

I can work on the E2E test!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #1869 (394a3ef) into main (b1bb82a) will increase coverage by 0.28%.
Report is 1 commits behind head on main.
The diff coverage is 74.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1869      +/-   ##
==========================================
+ Coverage   45.21%   45.50%   +0.28%     
==========================================
  Files          79       79              
  Lines        7714     7782      +68     
==========================================
+ Hits         3488     3541      +53     
- Misses       4090     4099       +9     
- Partials      136      142       +6     

@saschagrunert
Copy link
Member

/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 Sep 7, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 15, 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 Sep 15, 2023
@saschagrunert
Copy link
Member

/retest

1 similar comment
@CoreyCook8
Copy link
Contributor Author

/retest

@CoreyCook8
Copy link
Contributor Author

/retest

Why do you tell me I have these powers and then not listen to me!! 😭

@CoreyCook8
Copy link
Contributor Author

Is something actually breaking here? Or is it just a flakey test?

@CoreyCook8
Copy link
Contributor Author

Bump on this, is there any work / Updates I need to add to this? Or is it just the test being flakey?

@saschagrunert
Copy link
Member

Bump on this, is there any work / Updates I need to add to this? Or is it just the test being flakey?

The following tests are failing right now:

2023-10-12T07:12:26.1291310Z         --- FAIL: TestSuite/TestSecurityProfilesOperator/namespaced:_Log_Enricher (28.51s)
2023-10-12T07:12:26.1283880Z         --- FAIL: TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding (182.87s)
2023-10-12T07:12:26.1286810Z         --- FAIL: TestSuite/TestSecurityProfilesOperator/cluster-wide:_profile_merging (428.57s)

I assume it's related to this PR since we only test SELinux on Fedora. The profile binding e2e test seems cricital:

2023-10-12T06:46:44.6067240Z I1012 06:46:43.403955   11603 suite_test.go:667]  "msg"="creating ns spo-binding-enabled" 
2023-10-12T06:46:45.1514770Z namespace/spo-binding-enabled unchanged
2023-10-12T06:46:45.1717320Z I1012 06:46:43.993208   11603 suite_test.go:667]  "msg"="switching to ns spo-binding-enabled" 
2023-10-12T06:46:45.5235600Z security-profiles-operatorContext "kubernetes-admin@kubernetes" modified.
2023-10-12T06:46:45.8100230Z namespace/spo-binding-enabled not labeled
2023-10-12T06:46:45.8181920Z I1012 06:46:44.646335   11603 suite_test.go:667]  "msg"="creating policy" 
2023-10-12T06:46:46.3227560Z selinuxprofile.security-profiles-operator.x-k8s.io/testpolicy created
2023-10-12T06:47:09.6218190Z selinuxprofile.security-profiles-operator.x-k8s.io/testpolicy condition met
2023-10-12T06:47:09.6521150Z I1012 06:47:08.480450   11603 suite_test.go:667]  "msg"="Creating test profile binding" 
2023-10-12T06:47:10.1686070Z error: error parsing /tmp/selinuxPolicyBinding-test.yml3306903868: error converting YAML to JSON: yaml: line 10: did not find expected alphabetic or numeric character
2023-10-12T06:47:10.1743010Z     suite_test.go:562: 
2023-10-12T06:47:10.1761600Z         	Error Trace:	/vagrant/test/suite_test.go:562
2023-10-12T06:47:10.1768500Z         	            				/vagrant/test/suite_test.go:586
2023-10-12T06:47:10.1769190Z         	            				/vagrant/test/e2e_test.go:423
2023-10-12T06:47:10.1769810Z         	            				/vagrant/test/e2e_test.go:408
2023-10-12T06:47:10.1770770Z         	            				/vagrant/test/tc_selinux_profilebindings_test.go:171
2023-10-12T06:47:10.1771610Z         	            				/vagrant/test/tc_selinux_profilebindings_test.go:33
2023-10-12T06:47:10.1773180Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:10.1773550Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:10.1775910Z         	Error:      	Expected nil, but got: &errors.errorString{s:"command /usr/bin/kubectl create -f /tmp/selinuxPolicyBinding-test.yml3306903868 did not succeed: error: error parsing /tmp/selinuxPolicyBinding-test.yml3306903868: error converting YAML to JSON: yaml: line 10: did not find expected alphabetic or numeric character\n"}
2023-10-12T06:47:10.1777780Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:10.1778360Z I1012 06:47:09.003741   11603 suite_test.go:667]  "msg"="Creating test pod" 
2023-10-12T06:47:10.9331370Z pod/selinux-profile-test-pod created
2023-10-12T06:47:11.3141180Z NAME                       READY   STATUS     RESTARTS   AGE
2023-10-12T06:47:11.3163110Z selinux-profile-test-pod   0/1     Init:0/1   0          1s
2023-10-12T06:47:13.9173010Z pod/selinux-profile-test-pod condition met
2023-10-12T06:47:13.9221750Z I1012 06:47:12.750740   11603 suite_test.go:667]  "msg"="the workload should not have errored" 
2023-10-12T06:47:14.1663890Z sh: can't create /var/log/test.log: Permission denied
2023-10-12T06:47:14.1805080Z     tc_selinux_profilebindings_test.go:38: 
2023-10-12T06:47:14.1805470Z         	Error Trace:	/vagrant/test/tc_selinux_profilebindings_test.go:38
2023-10-12T06:47:14.1805770Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:14.1806080Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:14.1806820Z         	Error:      	Should be empty, but was sh: can't create /var/log/test.log: Permission denied
2023-10-12T06:47:14.1807400Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:14.1807930Z         	Messages:   	container should not have returned a 'Permissions Denied' error
2023-10-12T06:47:14.3551130Z spo-binding-enabledI1012 06:47:13.182337   11603 suite_test.go:667]  "msg"="Getting selinux profile usage" 
2023-10-12T06:47:14.6715470Z spo-binding-enabledtestpolicy_spo-binding-enabled.processI1012 06:47:13.499099   11603 suite_test.go:667]  "msg"="Testing that pod has securityContext" 
2023-10-12T06:47:14.9274830Z     tc_selinux_profilebindings_test.go:50: 
2023-10-12T06:47:14.9277310Z         	Error Trace:	/vagrant/test/tc_selinux_profilebindings_test.go:50
2023-10-12T06:47:14.9278080Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:14.9288980Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:14.9290520Z         	Error:      	Not equal: 
2023-10-12T06:47:14.9292640Z         	            	expected: "testpolicy_spo-binding-enabled.process"
2023-10-12T06:47:14.9297230Z         	            	actual  : ""
2023-10-12T06:47:14.9297820Z         	            	
2023-10-12T06:47:14.9298050Z         	            	Diff:
2023-10-12T06:47:14.9299540Z         	            	--- Expected
2023-10-12T06:47:14.9299770Z         	            	+++ Actual
2023-10-12T06:47:14.9300070Z         	            	@@ -1 +1 @@
2023-10-12T06:47:14.9300460Z         	            	-testpolicy_spo-binding-enabled.process
2023-10-12T06:47:14.9300710Z         	            	+
2023-10-12T06:47:14.9301190Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:14.9301620Z I1012 06:47:13.754488   11603 suite_test.go:667]  "msg"="Testing that profile binding has pod reference" 
2023-10-12T06:47:15.1100620Z Error from server (NotFound): profilebindings.security-profiles-operator.x-k8s.io "profile-binding-selinux" not found
2023-10-12T06:47:15.1155690Z     suite_test.go:562: 
2023-10-12T06:47:15.1157030Z         	Error Trace:	/vagrant/test/suite_test.go:562
2023-10-12T06:47:15.1158310Z         	            				/vagrant/test/suite_test.go:586
2023-10-12T06:47:15.1158670Z         	            				/vagrant/test/tc_selinux_profilebindings_test.go:53
2023-10-12T06:47:15.1159010Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:15.1160080Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:15.1173930Z         	Error:      	Expected nil, but got: &errors.errorString{s:"command /usr/bin/kubectl get profilebinding profile-binding-selinux --output jsonpath={.status.activeWorkloads[0]} did not succeed: Error from server (NotFound): profilebindings.security-profiles-operator.x-k8s.io \"profile-binding-selinux\" not found\n"}
2023-10-12T06:47:15.1180050Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:15.1180540Z     tc_selinux_profilebindings_test.go:54: 
2023-10-12T06:47:15.1181070Z         	Error Trace:	/vagrant/test/tc_selinux_profilebindings_test.go:54
2023-10-12T06:47:15.1181560Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:15.1181880Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:15.1182150Z         	Error:      	Not equal: 
2023-10-12T06:47:15.1182790Z         	            	expected: "spo-binding-enabled/selinux-profile-test-pod"
2023-10-12T06:47:15.1183210Z         	            	actual  : ""
2023-10-12T06:47:15.1184140Z         	            	
2023-10-12T06:47:15.1184340Z         	            	Diff:
2023-10-12T06:47:15.1184690Z         	            	--- Expected
2023-10-12T06:47:15.1185010Z         	            	+++ Actual
2023-10-12T06:47:15.1185490Z         	            	@@ -1 +1 @@
2023-10-12T06:47:15.1185920Z         	            	-spo-binding-enabled/selinux-profile-test-pod
2023-10-12T06:47:15.1186170Z         	            	+
2023-10-12T06:47:15.1186630Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:15.3760730Z Error from server (NotFound): profilebindings.security-profiles-operator.x-k8s.io "profile-binding-selinux" not found
2023-10-12T06:47:15.3858440Z     suite_test.go:562: 
2023-10-12T06:47:15.3869500Z         	Error Trace:	/vagrant/test/suite_test.go:562
2023-10-12T06:47:15.3876120Z         	            				/vagrant/test/suite_test.go:586
2023-10-12T06:47:15.3879750Z         	            				/vagrant/test/tc_selinux_profilebindings_test.go:55
2023-10-12T06:47:15.3880450Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:15.3883020Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:15.3897710Z         	Error:      	Expected nil, but got: &errors.errorString{s:"command /usr/bin/kubectl get profilebinding profile-binding-selinux --output jsonpath={.metadata.finalizers[0]} did not succeed: Error from server (NotFound): profilebindings.security-profiles-operator.x-k8s.io \"profile-binding-selinux\" not found\n"}
2023-10-12T06:47:15.3899340Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:15.3901130Z     tc_selinux_profilebindings_test.go:56: 
2023-10-12T06:47:15.3901790Z         	Error Trace:	/vagrant/test/tc_selinux_profilebindings_test.go:56
2023-10-12T06:47:15.3905040Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:15.3906590Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:15.3906940Z         	Error:      	Not equal: 
2023-10-12T06:47:15.3908050Z         	            	expected: "active-workload-lock"
2023-10-12T06:47:15.3938620Z         	            	actual  : ""
2023-10-12T06:47:15.3945890Z         	            	
2023-10-12T06:47:15.3956480Z         	            	Diff:
2023-10-12T06:47:15.3959680Z         	            	--- Expected
2023-10-12T06:47:15.3965930Z         	            	+++ Actual
2023-10-12T06:47:15.3970040Z         	            	@@ -1 +1 @@
2023-10-12T06:47:15.3975950Z         	            	-active-workload-lock
2023-10-12T06:47:15.3997570Z         	            	+
2023-10-12T06:47:15.3998840Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:15.4000380Z I1012 06:47:14.213121   11603 suite_test.go:667]  "msg"="Testing that profile has pod reference" 
2023-10-12T06:47:15.5952550Z     tc_selinux_profilebindings_test.go:62: 
2023-10-12T06:47:15.5972350Z         	Error Trace:	/vagrant/test/tc_selinux_profilebindings_test.go:62
2023-10-12T06:47:15.5973270Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:15.5975230Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:15.5977960Z         	Error:      	Not equal: 
2023-10-12T06:47:15.5978740Z         	            	expected: "spo-binding-enabled/selinux-profile-test-pod"
2023-10-12T06:47:15.5979030Z         	            	actual  : ""
2023-10-12T06:47:15.5979240Z         	            	
2023-10-12T06:47:15.5979480Z         	            	Diff:
2023-10-12T06:47:15.5999610Z         	            	--- Expected
2023-10-12T06:47:15.6000570Z         	            	+++ Actual
2023-10-12T06:47:15.6001350Z         	            	@@ -1 +1 @@
2023-10-12T06:47:15.6002270Z         	            	-spo-binding-enabled/selinux-profile-test-pod
2023-10-12T06:47:15.6002990Z         	            	+
2023-10-12T06:47:15.6003940Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:15.8875140Z fedora-deleted    tc_selinux_profilebindings_test.go:66: 
2023-10-12T06:47:15.8887850Z         	Error Trace:	/vagrant/test/tc_selinux_profilebindings_test.go:66
2023-10-12T06:47:15.8888810Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:15.8890510Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:15.8891770Z         	Error:      	"fedora-deleted" does not contain "in-use-by-active-pods"
2023-10-12T06:47:15.8892930Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:16.1991530Z pod "selinux-profile-test-pod" deleted
2023-10-12T06:47:17.6342880Z Error from server (NotFound): profilebindings.security-profiles-operator.x-k8s.io "profile-binding-selinux" not found
2023-10-12T06:47:17.6388210Z     suite_test.go:562: 
2023-10-12T06:47:17.6391860Z         	Error Trace:	/vagrant/test/suite_test.go:562
2023-10-12T06:47:17.6392690Z         	            				/vagrant/test/suite_test.go:586
2023-10-12T06:47:17.6408880Z         	            				/vagrant/test/tc_selinux_profilebindings_test.go:187
2023-10-12T06:47:17.6412110Z         	            				/vagrant/test/tc_selinux_profilebindings_test.go:67
2023-10-12T06:47:17.6417270Z         	            				/vagrant/test/e2e_test.go:157
2023-10-12T06:47:17.6418450Z         	            				/vagrant/vendor/github.com/stretchr/testify/suite/suite.go:112
2023-10-12T06:47:17.6422690Z         	Error:      	Expected nil, but got: &errors.errorString{s:"command /usr/bin/kubectl delete profilebinding profile-binding-selinux did not succeed: Error from server (NotFound): profilebindings.security-profiles-operator.x-k8s.io \"profile-binding-selinux\" not found\n"}
2023-10-12T06:47:17.6433140Z         	Test:       	TestSuite/TestSecurityProfilesOperator/cluster-wide:_Selinux:_Verify_profile_binding
2023-10-12T06:47:17.8475320Z selinuxprofile.security-profiles-operator.x-k8s.io "testpolicy" deleted

May I ask you to check what's wrong here?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2023
@saschagrunert
Copy link
Member

@CoreyCook8 may I ask you to rebase please?

@k8s-ci-robot k8s-ci-robot added do-not-merge/contains-merge-commits and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 19, 2024
@ccojocar
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2024
@k8s-ci-robot k8s-ci-robot merged commit 9086da4 into kubernetes-sigs:main Jan 23, 2024
26 checks passed
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. kind/feature Categorizes issue or PR as related to a new feature. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an "Enforce on all Pods" option for Seccomp Profile Bindings
5 participants