Skip to content

Conversation

@perdasilva
Copy link
Collaborator

@perdasilva perdasilva commented Jul 4, 2022

Description of the change:
This PR updates the namespace and operator deployment resources and the catalog source and bundle unpacker job to enforce the PSA restricted profile.

NOTE These changes will break sqlite-based catalog sources due to a bug in opm fix, in which a copy of the database was made on the root of the filesystem.

Motivation for the change:
Upstream k8s is rolling out the new Pod Security Admission (PSA) mechanism to improve cluster security and substitute the Pod Security Policies feature link.

Architectural changes:
None

Testing remarks:
Manually tested on kind with k8s 1.24

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

* Update ci artifact collection

Signed-off-by: perdasilva <perdasilva@redhat.com>

* Update e2e test images to use FBC

Signed-off-by: perdasilva <perdasilva@redhat.com>

* Update CatalogSource Pod security context

Signed-off-by: perdasilva <perdasilva@redhat.com>
* Update unpack job security

Signed-off-by: perdasilva <perdasilva@redhat.com>

* Refactor catsrc pod creation to use security package

Signed-off-by: perdasilva <perdasilva@redhat.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
@openshift-ci openshift-ci bot requested review from awgreene and njhale July 4, 2022 12:19
@openshift-ci
Copy link

openshift-ci bot commented Jul 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 4, 2022
serviceAccountName: olm-operator-serviceaccount
securityContext:
runAsNonRoot: true
runAsUser: 1001
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 4, 2022

Choose a reason for hiding this comment

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

This one cannot be qualified to access restricted-v2 because of the runAsUser
That will requires access to anyuid SCC. However, if we test in k8s env that can run in a restricted ns and is qualified under the k8s rules to be restricted. See:

⚠️ With some configurations your solution might comply with the restricted Kubernetes definition but will not be accepted under the SCC restricted-v2 in OCP. In K8s you can specify the runAsUser and get the Pod/Container running in a restricted namespace however, for Openshift’s restricted/restricted-v2 SCC you MUST leave the runAsUser field empty, or provide a value that falls within the specific user range for the namespace. Otherwise, you will only be able to run the Pod if it has access to the SCC nonroot-v2 or anyuid.

Therefore, do we really need to define userId?

⚠️ Be aware that this code will not work on OCP since there you would need to define a user in a big range. The range is not fixed as well and depends on the ns. So, the best approach is not needed to define it at all. Following an example of the error that will be faced if we try to apply this config in OCP:

Leave the runAsUser field empty, or provide a value that falls within the specific user range for the namespace. provide a value in the ranges: [1000680000, 1000689999

WORKDIR /build

# install dependencies and go 1.16
# install dependencies and go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for we are not using 1.18 yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We probably should move to 1.18 here - though I think it should be done in another PR

RunAsNonRoot: pointer.Bool(runAsNonRoot),
RunAsUser: pointer.Int64(runAsUser),
SeccompProfile: &corev1.SeccompProfile{
Type: corev1.SeccompProfileTypeRuntimeDefault,
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 4, 2022

Choose a reason for hiding this comment

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

⚠️ It only works on OCP >= 4.11 and k8s >= 1.19

Signed-off-by: perdasilva <perdasilva@redhat.com>

var expectedPodSecurityContext = &corev1.PodSecurityContext{
RunAsNonRoot: &expectedRunAsNonRoot,
RunAsUser: &expectedRunAsUser,
Copy link
Contributor

Choose a reason for hiding this comment

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

}

// Update pod security
security.ApplyPodSpecSecurity(&pod.Spec, security.WithRunAsUser(1001))
Copy link
Contributor

Choose a reason for hiding this comment

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

expectedReadOnlyRootFilesystem := false
expectedAllowPrivilegeEscalation := false
expectedRunAsNonRoot := true
expectedRunAsUser := int64(1001)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to use RunAsUser for the catalog source pods for now. Otherwise, only catalogs based off the latest opm image will work. We may need to handle this like a deprecation or something like that giving people some time to rebuild their catalogs.

Signed-off-by: perdasilva <perdasilva@redhat.com>
@perdasilva
Copy link
Collaborator Author

/hold until we figure out how to migrate. These changes are only valid from k8s >= 1.19. Should we cut a release before merging and only support 1.19 going forward?

@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 Jul 5, 2022
Signed-off-by: perdasilva <perdasilva@redhat.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
@perdasilva perdasilva changed the title PSA Updates [WIP] PSA Updates Jul 7, 2022
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 7, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jul 11, 2022

@perdasilva: PR needs rebase.

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.

@anik120
Copy link
Member

anik120 commented Aug 23, 2022

/close

as this is out of date.

@openshift-ci openshift-ci bot closed this Aug 23, 2022
@openshift-ci
Copy link

openshift-ci bot commented Aug 23, 2022

@anik120: Closed this PR.

In response to this:

/close

as this is out of date.

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.

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants