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

Roll back problematic pod labeling feature #1088

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Aug 15, 2022

What type of PR is this?

/kind cleanup
/kind feature

What this PR does / why we need it:

Some time ago I introduced a feature for the SPO to label denials for
pods. While this seemed like a good idea at the time, it introduces too
much privileges for te SPOd (update and patch all pods).

While there hasn't been any security incidents reported for the SPOd,
the drawbacks of giving such privileges are more critical than the
features the labeling would have enabled. So, let's get rid of this
feature and revisit the problem another time.

This fully removes the flag as there is no documented usage of the feature.

Which issue(s) this PR fixes:

None

Does this PR have test?

The test was removed.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Pod denials labeling feature was removed.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 15, 2022
@JAORMX JAORMX force-pushed the rollback-label-pods branch 2 times, most recently from 300b5b8 to ea9e50a Compare August 15, 2022 10:46
@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2022

Codecov Report

Merging #1088 (3f9e128) into main (ad89007) will increase coverage by 0.51%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1088      +/-   ##
==========================================
+ Coverage   50.13%   50.65%   +0.51%     
==========================================
  Files          42       42              
  Lines        4789     4740      -49     
==========================================
  Hits         2401     2401              
+ Misses       2309     2262      -47     
+ Partials       79       77       -2     

@JAORMX JAORMX force-pushed the rollback-label-pods branch from ea9e50a to 43d6357 Compare August 15, 2022 10:52
@JAORMX JAORMX requested a review from jhrozek August 15, 2022 11:56
Some time ago I introduced a feature for the SPO to label denials for
pods. While this seemed like a good idea at the time, it introduces too
much privileges for te SPOd (update and patch all pods).

While there hasn't been any security incidents reported for the SPOd,
the drawbacks of giving such privileges are more critical than the
features the labeling would have enabled. So, let's get rid of this
feature and revisit the problem another time.

Signed-off-by: Juan Antonio Osorio <juan.osoriorobles@eu.equinix.com>
@JAORMX JAORMX force-pushed the rollback-label-pods branch from 43d6357 to 3f9e128 Compare August 16, 2022 06:57
@jhrozek
Copy link
Contributor

jhrozek commented Aug 16, 2022

/test pull-security-profiles-operator-test-e2e

@JAORMX
Copy link
Contributor Author

JAORMX commented Aug 16, 2022

/retest

@jhrozek
Copy link
Contributor

jhrozek commented Aug 16, 2022

So I'm fine removing this functionality (AFAIK nothing and nobody uses it now), but I'd like to discuss alternatives, because I think it's something users will ask for.

The use-case I care about the most is:

  • a developer records an initial policy in CI
  • an SRE deploys the app and the policy. Both acknowledge that the initial policy version is not 100% correct and want to find out AVCs/seccomp denials
  • so far I was thinking about adding a "permissive" flag to be added to the profiles which would change the defaultAction to SCMP_ACT_LOG for seccomp profiles and inherit from typepermissive process for SELinux policies.
  • this would produce logs, then the question is how to gather them and there we'd probably do what your approach did, but maybe label the profiles instead of the workloads? I think this would be more palatable from the RBAC perspective. Maybe we could issue events, too, not sure what would be best for SREs in the end.

@JAORMX
Copy link
Contributor Author

JAORMX commented Aug 16, 2022

@jhrozek I was actually thinking about doing events instead. But struggled to introduce them without adding a lot of extra complexity to the code-base. We might need to do some refactoring.

@jhrozek
Copy link
Contributor

jhrozek commented Aug 17, 2022

@jhrozek I was actually thinking about doing events instead. But struggled to introduce them without adding a lot of extra complexity to the code-base. We might need to do some refactoring.

Let's file an issue, then

Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, jhrozek

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

@JAORMX
Copy link
Contributor Author

JAORMX commented Aug 17, 2022

/retest

@k8s-ci-robot k8s-ci-robot merged commit 863d457 into kubernetes-sigs:main Aug 17, 2022
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. 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.

4 participants