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 1806438: Remove explicit securityContext and add granular securitycontextconstraints "use" permissions in machine-api-controllers clusterRole #502

Merged
merged 1 commit into from Mar 10, 2020

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Mar 2, 2020

Without the runlabel #496, we’ll run as a high user by default, no need to say run me as non root.

Otherwise when removing the runlevel completely for the openshift-machine-api namespace openshift/cluster-autoscaler-operator#133 the kube controller manager complains with Error creating: pods "machine-api-operator-75c887884f-" is forbidden: unable to validate against any security context constraint: [spec.containers[0].securityContext.securityContext.runAsUser: Invalid value: 65534: must be in the ranges: [1000340000, 1000349999] spec.containers[1].securityContext.securityContext.runAsUser: Invalid value: 65534: must be in the ranges: [1000340000, 1000349999]] https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-autoscaler-operator/133/pull-ci-openshift-cluster-autoscaler-operator-master-e2e-aws/496/artifacts/e2e-aws/pods/openshift-kube-controller-manager_kube-controller-manager-ip-10-0-133-251.us-east-2.compute.internal_kube-controller-manager.log"

Only after this PR makes it to payload then https://github.com/openshift/cluster-autoscaler-operator/pull/133/files should go green as no pods within the openshift-machine-api namespace will set an invalid user.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Mar 2, 2020
@openshift-ci-robot
Copy link
Contributor

@enxebre: This pull request references Bugzilla bug 1806438, which is valid. 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.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

BUG 1806438: Remove explicit securityContext

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.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 2, 2020
@openshift-ci-robot
Copy link
Contributor

@enxebre: This pull request references Bugzilla bug 1806438, which is valid.

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

In response to this:

BUG 1806438: Remove explicit securityContext

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.

2 similar comments
@openshift-ci-robot
Copy link
Contributor

@enxebre: This pull request references Bugzilla bug 1806438, which is valid.

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

In response to this:

BUG 1806438: Remove explicit securityContext

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.

@openshift-ci-robot
Copy link
Contributor

@enxebre: This pull request references Bugzilla bug 1806438, which is valid.

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

In response to this:

BUG 1806438: Remove explicit securityContext

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.

@enxebre
Copy link
Member Author

enxebre commented Mar 2, 2020

/retest

@enxebre
Copy link
Member Author

enxebre commented Mar 2, 2020

/cherrypick release-4.4
/cherrypick release-4.3

@openshift-cherrypick-robot

@enxebre: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.4
/cherrypick release-4.3

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.

@openshift-ci-robot
Copy link
Contributor

@enxebre: This pull request references Bugzilla bug 1806438, which is valid.

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

In response to this:

BUG 1806438: Remove explicit securityContext

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.

1 similar comment
@openshift-ci-robot
Copy link
Contributor

@enxebre: This pull request references Bugzilla bug 1806438, which is valid.

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

In response to this:

BUG 1806438: Remove explicit securityContext

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
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
@markmc
Copy link
Contributor

markmc commented Mar 2, 2020

@enxebre As per comment in openshift/cluster-autoscaler-operator#133 the baremetal pod deployment uses hostNetwork, hostPort, and privileged

/cc @sadasu @stbenjam @hardys

@enxebre
Copy link
Member Author

enxebre commented Mar 2, 2020

@enxebre As per comment in openshift/cluster-autoscaler-operator#133 the baremetal pod deployment uses hostNetwork, hostPort, and privileged

@markmc good catch, I believe adding the specifics SCC to the clusterRole used by the machine-api-controllers should be enough as per openshift/cluster-ingress-operator@1409f54
i.e

- apiGroups:
  - security.openshift.io
  resources:
  - securitycontextconstraints
  verbs:
  - use
  resourceNames:
  - hostnetwork
  - privileged

cc @Miciah @smarterclayton can you confirm?

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2020
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 2, 2020
@enxebre enxebre changed the title BUG 1806438: Remove explicit securityContext BUG 1806438: Remove explicit securityContext and add granular securitycontextconstraints "use" permissions in machine-api-controllers clusterRole Mar 2, 2020
@smarterclayton
Copy link
Contributor

Yes, although ONLY the component that needs those should have a reference to the SCC via a unique service account.

@enxebre
Copy link
Member Author

enxebre commented Mar 2, 2020

Yes, although ONLY the component that needs those should have a reference to the SCC via a unique service account.

Agreed, currently our granularity let us give this to the pod which run the multiple machine controllers. In near future we might consider breaking this pod down into multiple components.

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2020
…raints use permissions in machine-api-controllers clusterRole

Without the runlabel openshift#496, we’ll run as a high user by default, no need to say run me as non root. Otherwise when removing the runlevel completely for the openshift-machine-api namespace openshift/cluster-autoscaler-operator#133 the kube controller manager complains with 'Error creating: pods "machine-api-operator-75c887884f-" is forbidden: unable to validate against any security context constraint: [spec.containers[0].securityContext.securityContext.runAsUser: Invalid value: 65534: must be in the ranges: [1000340000, 1000349999] spec.containers[1].securityContext.securityContext.runAsUser: Invalid value: 65534: must be in the ranges: [1000340000, 1000349999]]' https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-autoscaler-operator/133/pull-ci-openshift-cluster-autoscaler-operator-master-e2e-aws/496/artifacts/e2e-aws/pods/openshift-kube-controller-manager_kube-controller-manager-ip-10-0-133-251.us-east-2.compute.internal_kube-controller-manager.log"
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 3, 2020
@markmc
Copy link
Contributor

markmc commented Mar 3, 2020

/lgtm

Would be good to get feedback from @sadasu's testing too

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020
@sadasu
Copy link
Contributor

sadasu commented Mar 3, 2020

/approve
I was able to patch in the latest fix for this PR with the fix for openshift/cluster-autoscaler-operator#133 by e2e baremetal (virtual) deployment. Happy to report that the metal3 pod came up fine and so did all the workers.

@enxebre
Copy link
Member Author

enxebre commented Mar 4, 2020

/retest

1 similar comment
@enxebre
Copy link
Member Author

enxebre commented Mar 5, 2020

/retest

@enxebre
Copy link
Member Author

enxebre commented Mar 5, 2020

/test e2e-gcp

@JoelSpeed
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, sadasu

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2020
@JoelSpeed
Copy link
Contributor

/retest

verbs:
- use
resourceNames:
- privileged
Copy link
Member

Choose a reason for hiding this comment

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

why does the operator require privileged SCC to run?

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@stlaz
Copy link
Member

stlaz commented Mar 6, 2020

/hold

[spec.containers[0].securityContext.securityContext.runAsUser: Invalid value: 65534: must be in the ranges: [1000340000, 1000349999] spec.containers[1].securityContext.securityContext.runAsUser: Invalid value: 65534: must be in the ranges: [1000340000, 1000349999]]

if this was the only problem then just the removal of runAsUser should do

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2020
@enxebre
Copy link
Member Author

enxebre commented Mar 9, 2020

/retest

@enxebre
Copy link
Member Author

enxebre commented Mar 10, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c63fa2b into openshift:master Mar 10, 2020
@openshift-cherrypick-robot

@enxebre: new pull request created: #511

In response to this:

/cherrypick release-4.4
/cherrypick release-4.3

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/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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants