Skip to content

Commit c4b0b4a

Browse files
jottofarjottofar
committed
lib/resourcemerge: change SecurityContext reconcile
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.
1 parent e0c9203 commit c4b0b4a

File tree

3 files changed

+91
-27
lines changed

3 files changed

+91
-27
lines changed

‎lib/resourcemerge/core.go‎

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ func EnsureConfigMap(modified *bool, existing *corev1.ConfigMap, required corev1
1818
mergeMap(modified, &existing.Data, required.Data)
1919
}
2020

21-
// EnsureServiceAccount ensures that the existing mathces the required.
21+
// EnsureServiceAccount ensures that the existing matches the required.
2222
// modified is set to true when existing had to be updated with required.
2323
func EnsureServiceAccount(modified *bool, existing *corev1.ServiceAccount, required corev1.ServiceAccount) {
2424
EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
@@ -466,12 +466,10 @@ func ensureVolumeSourceDefaults(required *corev1.VolumeSource) {
466466
}
467467

468468
func ensureSecurityContextPtr(modified *bool, existing **corev1.SecurityContext, required *corev1.SecurityContext) {
469-
// if we have no required, then we don't care what someone else has set
470-
if required == nil {
469+
if *existing == nil && required == nil {
471470
return
472471
}
473-
474-
if *existing == nil {
472+
if *existing == nil || (required == nil && *existing != nil) {
475473
*modified = true
476474
*existing = required
477475
return
@@ -490,12 +488,10 @@ func ensureSecurityContext(modified *bool, existing *corev1.SecurityContext, req
490488
}
491489

492490
func ensureCapabilitiesPtr(modified *bool, existing **corev1.Capabilities, required *corev1.Capabilities) {
493-
// if we have no required, then we don't care what someone else has set
494-
if required == nil {
491+
if *existing == nil && required == nil {
495492
return
496493
}
497-
498-
if *existing == nil {
494+
if *existing == nil || (required == nil && *existing != nil) {
499495
*modified = true
500496
*existing = required
501497
return
@@ -619,12 +615,10 @@ func ensureAffinity(modified *bool, existing *corev1.Affinity, required corev1.A
619615
}
620616

621617
func ensurePodSecurityContextPtr(modified *bool, existing **corev1.PodSecurityContext, required *corev1.PodSecurityContext) {
622-
// if we have no required, then we don't care what someone else has set
623-
if required == nil {
618+
if *existing == nil && required == nil {
624619
return
625620
}
626-
627-
if *existing == nil {
621+
if *existing == nil || (required == nil && *existing != nil) {
628622
*modified = true
629623
*existing = required
630624
return
@@ -676,12 +670,10 @@ func ensurePodSecurityContext(modified *bool, existing *corev1.PodSecurityContex
676670
}
677671

678672
func ensureSELinuxOptionsPtr(modified *bool, existing **corev1.SELinuxOptions, required *corev1.SELinuxOptions) {
679-
// if we have no required, then we don't care what someone else has set
680-
if required == nil {
673+
if *existing == nil && required == nil {
681674
return
682675
}
683-
684-
if *existing == nil {
676+
if *existing == nil || (required == nil && *existing != nil) {
685677
*modified = true
686678
*existing = required
687679
return
@@ -734,12 +726,10 @@ func setBool(modified *bool, existing *bool, required bool) {
734726
}
735727

736728
func setBoolPtr(modified *bool, existing **bool, required *bool) {
737-
// if we have no required, then we don't care what someone else has set
738-
if required == nil {
729+
if *existing == nil && required == nil {
739730
return
740731
}
741-
742-
if *existing == nil {
732+
if *existing == nil || (required == nil && *existing != nil) {
743733
*modified = true
744734
*existing = required
745735
return
@@ -774,12 +764,10 @@ func setInt64(modified *bool, existing *int64, required int64) {
774764
}
775765

776766
func setInt64Ptr(modified *bool, existing **int64, required *int64) {
777-
// if we have no required, then we don't care what someone else has set
778-
if required == nil {
767+
if *existing == nil && required == nil {
779768
return
780769
}
781-
782-
if *existing == nil {
770+
if *existing == nil || (required == nil && *existing != nil) {
783771
*modified = true
784772
*existing = required
785773
return

‎lib/resourcemerge/core_test.go‎

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,12 @@ func TestEnsurePodSpec(t *testing.T) {
3030
expected: corev1.PodSpec{},
3131
},
3232
{
33-
name: "remove regular containers from existing",
33+
name: "remove regular containers from existing, PodSecurityContext none",
3434
existing: corev1.PodSpec{
3535
Containers: []corev1.Container{
3636
{Name: "test"},
37-
{Name: "to-be-removed"}}},
37+
{Name: "to-be-removed"}},
38+
SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false)}},
3839
input: corev1.PodSpec{
3940
Containers: []corev1.Container{
4041
{Name: "test"}}},
@@ -44,6 +45,71 @@ func TestEnsurePodSpec(t *testing.T) {
4445
Containers: []corev1.Container{
4546
{Name: "test"}}},
4647
},
48+
{
49+
name: "PodSecurityContext empty",
50+
existing: corev1.PodSpec{
51+
SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false),
52+
RunAsUser: int64Ptr(int64(1234)),
53+
RunAsGroup: int64Ptr(int64(1234)),
54+
FSGroup: int64Ptr(int64(1234)),
55+
SELinuxOptions: &corev1.SELinuxOptions{User: "foo"},
56+
}},
57+
input: corev1.PodSpec{
58+
SecurityContext: &corev1.PodSecurityContext{}},
59+
60+
expectedModified: true,
61+
expected: corev1.PodSpec{
62+
SecurityContext: &corev1.PodSecurityContext{}},
63+
},
64+
{
65+
name: "PodSecurityContext changes",
66+
existing: corev1.PodSpec{
67+
SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(true),
68+
RunAsUser: int64Ptr(int64(1234)),
69+
RunAsGroup: int64Ptr(int64(1234))}},
70+
input: corev1.PodSpec{
71+
SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false),
72+
RunAsGroup: int64Ptr(int64(5))}},
73+
74+
expectedModified: true,
75+
expected: corev1.PodSpec{
76+
SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false),
77+
RunAsGroup: int64Ptr(int64(5))}},
78+
},
79+
{
80+
name: "container SecurityContext none",
81+
existing: corev1.PodSpec{
82+
Containers: []corev1.Container{
83+
{SecurityContext: &corev1.SecurityContext{RunAsNonRoot: boolPtr(false),
84+
RunAsUser: int64Ptr(int64(1234)),
85+
Capabilities: &corev1.Capabilities{
86+
Add: []corev1.Capability{"bar"}},
87+
SELinuxOptions: &corev1.SELinuxOptions{User: "foo"},
88+
}}}},
89+
input: corev1.PodSpec{},
90+
91+
expectedModified: true,
92+
expected: corev1.PodSpec{},
93+
},
94+
{
95+
name: "container SecurityContext empty",
96+
existing: corev1.PodSpec{
97+
Containers: []corev1.Container{
98+
{SecurityContext: &corev1.SecurityContext{RunAsNonRoot: boolPtr(false),
99+
RunAsUser: int64Ptr(int64(1234)),
100+
Capabilities: &corev1.Capabilities{
101+
Add: []corev1.Capability{"bar"}},
102+
SELinuxOptions: &corev1.SELinuxOptions{User: "foo"},
103+
}}}},
104+
input: corev1.PodSpec{
105+
Containers: []corev1.Container{
106+
{SecurityContext: &corev1.SecurityContext{}}}},
107+
108+
expectedModified: true,
109+
expected: corev1.PodSpec{
110+
Containers: []corev1.Container{
111+
{SecurityContext: &corev1.SecurityContext{}}}},
112+
},
47113
{
48114
name: "remove regular and init containers from existing",
49115
existing: corev1.PodSpec{
@@ -1559,3 +1625,11 @@ func defaultPodSpec(in *corev1.PodSpec, from corev1.PodSpec) {
15591625
modified := pointer.BoolPtr(false)
15601626
ensurePodSpec(modified, in, from)
15611627
}
1628+
1629+
func boolPtr(b bool) *bool {
1630+
return &b
1631+
}
1632+
1633+
func int64Ptr(i int64) *int64 {
1634+
return &i
1635+
}

‎lib/resourcemerge/meta_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import (
1414

1515
func init() {
1616
klog.InitFlags(flag.CommandLine)
17+
_ = flag.CommandLine.Lookup("v").Value.Set("2")
18+
_ = flag.CommandLine.Lookup("alsologtostderr").Value.Set("true")
1719
}
1820

1921
func TestMergeOwnerRefs(t *testing.T) {

0 commit comments

Comments
 (0)