Bug 2108858: lib/resourcemerge: change SecurityContext reconcile #804

jottofar · 2022-07-22T17:31:39Z

to handle securityContext changes differently. Since d9f6718, if a securityContext is not explicitly specified in the manifest the resource's securityContext will remain unchanged and it will continue to use the securityContext setting of the currently running resource (if there is one). We're not sure of the exact reason the logic was originally developed in this manner but this change joins a series of similar previous tightenings, including 02bb9ba (lib/resourcemerge/core: Clear env and envFrom if unset in
manifest, 2021-04-20, #549) and ca299b8 (lib/resourcemerge: remove ports which are no longer required, 2020-02-13, #322).

Reconciliation has been changed such that the entire securityContext structure, or any sub field of it, will be cleared if not specified in the manifest. This change affects Deployments, Jobs, and DaemonSets. It affects the securityContext found in both a PodSpec and a Container. Since the functions setInt64Ptr and setBoolPtr have been changed the impact is wide affecting ServiceAccounts, the PodSpec fields ShareProcessNamespace and TerminationGracePeriodSeconds, and the Job fields
ActiveDeadlineSeconds and ManualSelector.

For example, prior to this change assume Deployment machine-api-operator is running on the cluster with the following:

securityContext:
runAsNonRoot: true
runAsUser: 65534

and during an upgrade the Deployment machine-api-operator no longer specifies a securityContext. The resulting upgraded Deployment machine-api-operator will still have the original securityContext:

securityContext:
runAsNonRoot: true
runAsUser: 65534

Similarly, there is no way to remove, or clear, a securityContext field such as runAsUser. You can only modify it.

After this change the above scenario will correctly result in the Deployment machine-api-operator not specifying securityContext upon upgrade completion.

The changes apply to both the SecurityContext [1] within a Container and the PodSecurityContext [2] within a PodSpec.

[1]

cluster-version-operator/vendor/k8s.io/api/core/v1/types.go

Line 2429 in e0c9203

    
           SecurityContext *SecurityContext `json:"securityContext,omitempty" protobuf:"bytes,15,opt,name=securityContext"`

[2]

cluster-version-operator/vendor/k8s.io/api/core/v1/types.go

Line 2429 in e0c9203

    
           SecurityContext *SecurityContext `json:"securityContext,omitempty" protobuf:"bytes,15,opt,name=securityContext"`

lib/resourcemerge/core.go

jottofar · 2022-07-25T22:15:49Z

/retitle Bug 2108858: lib/resourcemerge: change SecurityContext reconcile

openshift-ci · 2022-07-25T22:15:59Z

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

Requesting review from QA contact:
/cc @shellyyang1989

Details

In response to this:

Bug 2108858: lib/resourcemerge: change SecurityContext reconcile

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.

lib/resourcemerge/core.go

lib/resourcemerge/core_test.go

wking · 2022-07-25T22:44:48Z

TestCVO_UpgradeFailedPayloadLoadWithCapsChanges failed, but that seems unlikely to be related.

/test unit

openshift-ci · 2022-07-25T23:27:30Z

@jottofar: This pull request references Bugzilla bug 2108858, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @shellyyang1989

Details

In response to this:

Bug 2108858: lib/resourcemerge: change SecurityContext reconcile

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 · 2022-07-26T14:22:05Z

@jottofar: This pull request references Bugzilla bug 2108858, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @shellyyang1989

Details

In response to this:

Bug 2108858: lib/resourcemerge: change SecurityContext reconcile

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.

jottofar · 2022-07-26T14:57:14Z

/test unit

lib/resourcemerge/core.go

to handle securityContext changes differently. Since d9f6718, if a securityContext is not explicitly specified in the manifest the resource's securityContext will remain unchanged and it will continue to use the securityContext setting of the currently running resource (if there is one). We're not sure of the exact reason the logic was originally developed in this manner but this change joins a series of similar previous tightenings, including openshift@02bb9ba (lib/resourcemerge/core: Clear env and envFrom if unset in manifest, 2021-04-20, openshift#549) and openshift@ca299b8 (lib/resourcemerge: remove ports which are no longer required, 2020-02-13, openshift#322). Reconciliation has been changed such that the entire securityContext structure, or any sub field of it, will be cleared if not specified in the manifest. This change affects Deployments, Jobs, and DaemonSets. It affects the securityContext found in both a PodSpec and a Container. Since the functions setInt64Ptr and setBoolPtr have been changed the impact is wide affecting ServiceAccounts, the PodSpec fields ShareProcessNamespace and TerminationGracePeriodSeconds, and the Job fields ActiveDeadlineSeconds and ManualSelector. For example, prior to this change assume Deployment machine-api-operator is running on the cluster with the following: securityContext: runAsNonRoot: true runAsUser: 65534 and during an upgrade the Deployment machine-api-operator no longer specifies a securityContext. The resulting upgraded Deployment machine-api-operator will still have the original securityContext: securityContext: runAsNonRoot: true runAsUser: 65534 Similarly, there is no way to remove, or clear, a securityContext field such as runAsUser. You can only modify it. After this change the above scenario will correctly result in the Deployment machine-api-operator not specifying securityContext upon upgrade completion. The changes apply to both the SecurityContext within a Container and the PodSecurityContext within a PodSpec.

wking

/lgtm

LalatenduMohanty

/lgtm

openshift-ci · 2022-07-26T18:56:41Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [LalatenduMohanty,jottofar,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-robot · 2022-07-26T20:57:04Z

/retest-required

Remaining retests: 2 against base HEAD e0c9203 and 8 for PR HEAD 619baae in total

openshift-ci · 2022-07-26T22:55:03Z

@jottofar: all tests passed!

Full PR test history. Your PR dashboard.

Details

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-ci · 2022-07-26T22:57:33Z

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

openshift/cluster-version-operator#804

Bugzilla bug 2108858 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2108858: lib/resourcemerge: change SecurityContext reconcile

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 bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2022

openshift-ci bot requested review from LalatenduMohanty and wking July 22, 2022 17:32

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2022

jottofar force-pushed the sec-ctx branch 2 times, most recently from cd65134 to f220066 Compare July 23, 2022 14:05

wking reviewed Jul 25, 2022

View reviewed changes

lib/resourcemerge/core.go Show resolved Hide resolved

jottofar force-pushed the sec-ctx branch 5 times, most recently from c6b101d to 3811c8c Compare July 25, 2022 22:14

openshift-ci bot changed the title ~~WIP: lib/resourcemerge: change SecurityContext reconcile~~ Bug 2108858: lib/resourcemerge: change SecurityContext reconcile Jul 25, 2022

openshift-ci bot requested a review from shellyyang1989 July 25, 2022 22:16

wking reviewed Jul 25, 2022

View reviewed changes

lib/resourcemerge/core.go Show resolved Hide resolved

wking reviewed Jul 25, 2022

View reviewed changes

lib/resourcemerge/core.go Show resolved Hide resolved

wking reviewed Jul 25, 2022

View reviewed changes

lib/resourcemerge/core_test.go Outdated Show resolved Hide resolved

jottofar force-pushed the sec-ctx branch 2 times, most recently from dcbfb82 to c4b0b4a Compare July 26, 2022 14:19

LalatenduMohanty suggested changes Jul 26, 2022

View reviewed changes

lib/resourcemerge/core.go Show resolved Hide resolved

jottofar force-pushed the sec-ctx branch from c4b0b4a to 619baae Compare July 26, 2022 18:13

wking approved these changes Jul 26, 2022

View reviewed changes

openshift-ci bot assigned wking Jul 26, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2022

LalatenduMohanty approved these changes Jul 26, 2022

View reviewed changes

openshift-ci bot assigned LalatenduMohanty Jul 26, 2022

openshift-merge-robot merged commit 32457da into openshift:master Jul 26, 2022

JoelSpeed mentioned this pull request Aug 8, 2022

Bug 2111972: operator NS manifest: Set empty openshift.io/run-level openshift/machine-api-operator#1053

Merged

wking mentioned this pull request Oct 20, 2022

Bug 1939855: controllers/updateservice_controller.go: handle init container changes openshift/cincinnati-operator#150

Merged

This was referenced Jul 10, 2023

OCPBUGS-5011: Backport SecurityContext reconciliation behavior #940

Merged

OCPBUGS-5011: Backport seccompProfile reconciliation behavior #947

Closed

Bug 2108858: lib/resourcemerge: change SecurityContext reconcile #804

Bug 2108858: lib/resourcemerge: change SecurityContext reconcile #804

Uh oh!

Conversation

jottofar commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jottofar commented Jul 25, 2022

Uh oh!

openshift-ci bot commented Jul 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wking commented Jul 25, 2022

Uh oh!

openshift-ci bot commented Jul 25, 2022

Uh oh!

openshift-ci bot commented Jul 26, 2022

Uh oh!

jottofar commented Jul 26, 2022

Uh oh!

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

LalatenduMohanty left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 26, 2022

Uh oh!

openshift-ci-robot commented Jul 26, 2022

Uh oh!

openshift-ci bot commented Jul 26, 2022

Uh oh!

openshift-ci bot commented Jul 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jottofar commented Jul 22, 2022 •

edited

Loading