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.

Sign up for GitHub

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

Jump to bottom

Update unpack job pod security #2793

Merged
merged 2 commits into from
Jun 13, 2022
Merged

Update unpack job pod security #2793

merged 2 commits into from
Jun 13, 2022
+105 −27

Conversation

perdasilva
Copy link
Collaborator

@perdasilva perdasilva commented Jun 8, 2022 •
edited
Loading

Description of the change:
Updates the security context stanzas for the bundle unpacking job's pod and containers to be more explicit and limited

Motivation for the change:
https://bugzilla.redhat.com/show_bug.cgi?id=2088541

Architectural changes:
I've added a security package with a function to update a pod spec with the standard security settings and refactored the CatalogSource pod creation function to use it as well

Testing remarks:
I've updated the unit tests for the bundle unpacking job creation to pass. This also ensures that the correct security settings are in the job pod spec. I've also run the e2e tests and they passed.

NOTE: This PR is ready for review, though it will stay blocked until #2782 is merged - as this is a continuation of that PR

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2022
@perdasilva perdasilva force-pushed the unpack_job_security branch 3 times, most recently from 353bcdb to aa2706c Compare June 8, 2022 13:15
@perdasilva perdasilva changed the title [WIP] Unpack job security [WIP] Update unpack job pod security Jun 8, 2022
@perdasilva perdasilva force-pushed the unpack_job_security branch from aa2706c to a51282a Compare June 9, 2022 07:49
@perdasilva
Copy link
Collaborator Author

/hold waiting for #2782 to merge before I can rebase this branch and get it merged.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2022
@perdasilva perdasilva changed the title [WIP] Update unpack job pod security Update unpack job pod security Jun 9, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2022
@perdasilva perdasilva force-pushed the unpack_job_security branch 3 times, most recently from 048cb72 to 5987159 Compare June 9, 2022 10:24
@perdasilva
Update unpack job security
f8737d1 
Signed-off-by: perdasilva <[email protected]>
@perdasilva perdasilva force-pushed the unpack_job_security branch from 5987159 to 9b82800 Compare June 9, 2022 14:15
@perdasilva
Copy link
Collaborator Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2022
timflannagan
timflannagan approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

I have a couple of questions/nits/comments but nothing that seemed blocking.

pkg/controller/bundle/bundle_unpacker.go Outdated
@@ -7,6 +7,7 @@ import (
"strings"
"time"

"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/security"
Copy link
Member

Choose a reason for hiding this comment

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

nit: move this to the other OLM package imports that are grouped together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are there some gofmt settings I can use to to automagically sort them in the required way?

Copy link
Member

Choose a reason for hiding this comment

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

I think goimportas actually handles this, which is why I opened #2798 when I was initially reviewing this PR. That linter configuration is already present in rukpak/deppy, and has helped cut down on the back-and-forth's for style-related PR review comments.

pkg/controller/security/security.go Outdated
// See: https://github.com/operator-framework/operator-registry/blob/master/Dockerfile#L27
var runAsUser int64 = 1001

var containerSecurityContext = &corev1.SecurityContext{
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify the "privileged" field here too, or is that the default value and we don't need to be explicit here?

Copy link
Member

Choose a reason for hiding this comment

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

Also: it looks like the container also has a SeccompProfile field. Do we need to propagate that to the container security context as well, or do we inherit that from the top-level podspec security context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

privileged does indeed default to false. But I have made it explicit, just in case. If I'm reading the docs correctly, the container should inherit the seccomp profile from the pod security context. So, I think we should be good. All good call outs!

@openshift-ci
Copy link

openshift-ci bot commented Jun 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva, timflannagan

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:
  • OWNERS [perdasilva,timflannagan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@perdasilva perdasilva force-pushed the unpack_job_security branch from 9b82800 to 5d27231 Compare June 13, 2022 06:28
@perdasilva perdasilva force-pushed the unpack_job_security branch from 5d27231 to 1fc56db Compare June 13, 2022 12:50
@timflannagan
Copy link
Member

Holding in case we need more reviewers to take a look at these changes.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2022
@perdasilva
Copy link
Collaborator Author

/hold cancel - let's put it though. Need to downstream asap ahead of CF.

@perdasilva
Copy link
Collaborator Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2022
@perdasilva perdasilva merged commit eedad28 into operator-framework:master Jun 13, 2022
perdasilva added a commit to exdx/operator-lifecycle-manager that referenced this pull request Jun 14, 2022
@perdasilva
Update unpack job pod security (operator-framework#2793)
414268d 
* Update unpack job security

Signed-off-by: perdasilva <[email protected]>

* Refactor catsrc pod creation to use security package

Signed-off-by: perdasilva <[email protected]>
perdasilva added a commit to exdx/operator-lifecycle-manager that referenced this pull request Jun 14, 2022
@perdasilva
Update unpack job pod security (operator-framework#2793)
ee04368 
* Update unpack job security

Signed-off-by: perdasilva <[email protected]>

* Refactor catsrc pod creation to use security package

Signed-off-by: perdasilva <[email protected]>
openshift-ci bot pushed a commit that referenced this pull request Jun 14, 2022
@perdasilva
e2e test utilities update (#2799)
a538831 
* Update unpack job pod security (#2793)

* Update unpack job security

Signed-off-by: perdasilva <[email protected]>

* Refactor catsrc pod creation to use security package

Signed-off-by: perdasilva <[email protected]>

* Refactor MagicCatalog removing superfluous interface and add factory method to create from file

Signed-off-by: perdasilva <[email protected]>

* Switch TestContext client to be the e2e client and add crd garbage collection

Signed-off-by: perdasilva <[email protected]>

* Add determined e2e client that retries on failure

Signed-off-by: perdasilva <[email protected]>

* Small fixes

Signed-off-by: perdasilva <[email protected]>

* Add olm gomega assertions and matchers

Signed-off-by: perdasilva <[email protected]>
perdasilva added a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Jun 23, 2022
@perdasilva
Revert "Update unpack job pod security (operator-framework#2793)"
775658b 
This reverts commit eedad28.
perdasilva added a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Jun 23, 2022
@perdasilva
Revert "Update unpack job pod security (operator-framework#2793)"
183bbfe 
This reverts commit eedad28.

Signed-off-by: perdasilva <[email protected]>
perdasilva added a commit that referenced this pull request Jun 23, 2022
@perdasilva
Sec revert (#2808)
0b7970c 
* Revert "Unpack job security updates (#2805)"

This reverts commit e568cde.

Signed-off-by: perdasilva <[email protected]>

* Revert "Update unpack job pod security (#2793)"

This reverts commit eedad28.

Signed-off-by: perdasilva <[email protected]>

* Revert "Update CatalogSource Pod security context (#2782)"

This reverts commit 99b51e7.

Signed-off-by: perdasilva <[email protected]>
perdasilva added a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Jul 4, 2022
@perdasilva
Update unpack job pod security (operator-framework#2793)
17a5b6d 
* Update unpack job security

Signed-off-by: perdasilva <[email protected]>

* Refactor catsrc pod creation to use security package

Signed-off-by: perdasilva <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan approved these changes

@gallettilance gallettilance Awaiting requested review from gallettilance

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

Assignees

@timflannagan timflannagan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@perdasilva @timflannagan