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

Bug 2110590: pkg/cvo/updatepayload: Set 'readOnlyRootFilesystem: false' #807

Conversation

wking
Copy link
Member

@wking wking commented Jul 29, 2022

This blocks us from being associated with SecurityContextConstraints that set readOnlyRootFilesystem: true, because from the upstream docs:

The set of SCCs that admission uses to authorize a pod are determined by the user identity and groups that the user belongs to. Additionally, if the pod specifies a service account, the set of allowable SCCs includes any constraints accessible to the service account.

Admission uses the following approach to create the final security context for the pod:

  1. Retrieve all SCCs available for use.
  2. Generate field values for security context settings that were not specified on the request.
  3. Validate the final settings against the available constraints.

If we leave readOnlyRootFilesystem implicit, we may get associated with a SCC that set readOnlyRootFilesystem: true, and the version-* actions will fail like:

$ oc -n openshift-cluster-version get pods
NAME                                        READY   STATUS    RESTARTS   AGE
cluster-version-operator-6b5c8ff5c8-4bmxx   1/1     Running   0          33m
version-4.10.20-smvt9-6vqwc                 0/1     Error     0          10s
$ oc -n openshift-cluster-version logs version-4.10.20-smvt9-6vqwc
oc logs version-4.10.20-smvt9-6vqwc
mv: cannot remove '/manifests/0000_00_cluster-version-operator_00_namespace.yaml': Read-only file system
mv: cannot remove '/manifests/0000_00_cluster-version-operator_01_adminack_configmap.yaml': Read-only file system
...

For a similar change in another repository, see openshift/cluster-openshift-apiserver-operator#437.

Also likely relevant, 4.10 both grew pod-security.kubernetes.io/* annotations and cleared the openshift.io/run-level annotation.

$ git --no-pager log --oneline -3 origin/release-4.10 -- install/0000_00_cluster-version-operator_00_namespace.yaml
539e9449 (origin/pr/623) Fix run-level label to empty string.
f58dd1c5 (origin/pr/686) install: Add description annotations to manifests
6e5e23e3 (origin/pr/668) podsecurity: enforce privileged for openshift-cluster-version namespace

None of those were in 4.9:

$ git --no-pager log --oneline -1 origin/release-4.9 -- install/0000_00_cluster-version-operator_00_namespace.yaml
70097361 (origin/pr/543) Add management workload annotations

And all of them landed in 4.10 via master (so they're in 4.10 before it GAed, and in 4.11 and later too):

$ git --no-pager log --oneline -4 origin/master -- install/0000_00_cluster-version-operator_00_namespace.yaml
539e9449 (origin/pr/623) Fix run-level label to empty string.

@wking wking changed the title pkg/cvo/updatepayload: Set 'readOnlyRootFilesystem: false' Bug 2110590: pkg/cvo/updatepayload: Set 'readOnlyRootFilesystem: false' Jul 29, 2022
@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jul 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 29, 2022

@wking: This pull request references Bugzilla bug 2110590, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @shellyyang1989

In response to this:

Bug 2110590: pkg/cvo/updatepayload: Set 'readOnlyRootFilesystem: false'

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.

Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

/lgtm

@LalatenduMohanty
Copy link
Member

/test gofmt

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2022
@LalatenduMohanty
Copy link
Member

/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 Jul 29, 2022
@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Jul 29, 2022

From https://docs.openshift.com/container-platform/4.10/rest_api/security_apis/securitycontextconstraints-security-openshift-io-v1.html

ReadOnlyRootFilesystem  when set to true will force containers to run with a read only root  file system.  If the container specifically requests to run with a  non-read only root file system the SCC should deny the pod. If set to  false the container may run with a read only root file system if it  wishes but it will not be forced to.

@LalatenduMohanty
Copy link
Member

/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 Jul 29, 2022
@LalatenduMohanty
Copy link
Member

We need this as making CVO read only will make break things for us.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD d6bb4dd and 8 for PR HEAD 63e6dc9 in total

This blocks us from being associated with SecurityContextConstraints
that set 'readOnlyRootFilesystem: true', because from [1]:

> The set of SCCs that admission uses to authorize a pod are
> determined by the user identity and groups that the user belongs
> to.  Additionally, if the pod specifies a service account, the set of
> allowable SCCs includes any constraints accessible to the service
> account.
>
> Admission uses the following approach to create the final security
> context for the pod:
>
> 1. Retrieve all SCCs available for use.
> 2. Generate field values for security context settings that were not
>    specified on the request.
> 3. Validate the final settings against the available constraints.

If we leave readOnlyRootFilesystem implicit, we may get associated
with a SCC that set 'readOnlyRootFilesystem: true', and the version-*
actions will fail like [2]:

  $ oc -n openshift-cluster-version get pods
  NAME                                        READY   STATUS    RESTARTS   AGE
  cluster-version-operator-6b5c8ff5c8-4bmxx   1/1     Running   0          33m
  version-4.10.20-smvt9-6vqwc                 0/1     Error     0          10s
  $ oc -n openshift-cluster-version logs version-4.10.20-smvt9-6vqwc
  oc logs version-4.10.20-smvt9-6vqwc
  mv: cannot remove '/manifests/0000_00_cluster-version-operator_00_namespace.yaml': Read-only file system
  mv: cannot remove '/manifests/0000_00_cluster-version-operator_01_adminack_configmap.yaml': Read-only file system
  ...

For a similar change in another repository, see [3].

Also likely relevant, 4.10 both grew pod-security.kubernetes.io/*
annotations [4] and cleared the openshift.io/run-level annotation [5].

$ git --no-pager log --oneline -3 origin/release-4.10 -- install/0000_00_cluster-version-operator_00_namespace.yaml
539e944 (origin/pr/623) Fix run-level label to empty string.
f58dd1c (origin/pr/686) install: Add description annotations to manifests
6e5e23e (origin/pr/668) podsecurity: enforce privileged for openshift-cluster-version namespace

None of those were in 4.9:

$ git --no-pager log --oneline -1 origin/release-4.9 -- install/0000_00_cluster-version-operator_00_namespace.yaml
7009736 (origin/pr/543) Add management workload annotations

And all of them landed in 4.10 via master (so they're in 4.10 before
it GAed, and in 4.11 and later too):

$ git --no-pager log --oneline -4 origin/master -- install/0000_00_cluster-version-operator_00_namespace.yaml
539e944 (origin/pr/623) Fix run-level label to empty string.

[1]: https://docs.openshift.com/container-platform/4.10/authentication/managing-security-context-constraints.html#admission_configuring-internal-oauth
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=2110590#c0
[3]: openshift/cluster-openshift-apiserver-operator#437
[4]: openshift#668
[5]: openshift#623
@wking wking force-pushed the version-pod-write-to-root-filesystem branch from 63e6dc9 to 0c5e0c3 Compare July 29, 2022 21:02
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2022
@LalatenduMohanty
Copy link
Member

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, wking

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 [LalatenduMohanty,wking]

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 lgtm Indicates that a PR is ready to be merged. label Jul 29, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD d6bb4dd and 8 for PR HEAD 0c5e0c3 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD d6bb4dd and 7 for PR HEAD 0c5e0c3 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD d6bb4dd and 6 for PR HEAD 0c5e0c3 in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 30, 2022

@wking: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit e56d8be into openshift:master Jul 30, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 30, 2022

@wking: All pull requests linked via external trackers have merged:

Bugzilla bug 2110590 has been moved to the MODIFIED state.

In response to this:

Bug 2110590: pkg/cvo/updatepayload: Set 'readOnlyRootFilesystem: false'

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.

@wking wking deleted the version-pod-write-to-root-filesystem branch August 1, 2022 21:23
@wking
Copy link
Member Author

wking commented Aug 2, 2022

VERIFIED in 4.12.

/cherrypick release-4.11

@openshift-cherrypick-robot

@wking: new pull request created: #811

In response to this:

VERIFIED in 4.12.

/cherrypick release-4.11

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. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants