Skip to content

Commit 5e60662

Browse files
ozzywalshozzywalsh
authored
Fix invalid fields copied to NGINX init container (#4627)
* Create nginx instrumentation container from scratch instead of cloning Build container spec from scratch (as in other instrumentations). Prevents passing through config that is disallowed in `initContainers` such as `resizePolicy` * Amend unit test ensuring incomptatible fields are not copied to init container * Add changelog entry * Create apachehttpd instrumentation container from scratch instead of cloning * Update changelog to mention apache changes
1 parent c24ed8d commit 5e60662

5 files changed

Lines changed: 69 additions & 40 deletions

File tree

‎.chloggen/fix_nginx-init-container-from-scratch.yaml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
2+
change_type: bug_fix
3+
4+
# The name of the component, or a single word describing the area of concern
5+
component: auto-instrumentation
6+
7+
# A brief description of the change.
8+
note: "Fix NGINX and Apache instrumentation init container creation to avoid copying init-container-incompatible fields."
9+
10+
# One or more tracking issues related to the change
11+
issues: [3729]
12+
13+
# (Optional) One or more lines of additional information to render under the primary note.
14+
subtext: |
15+
The NGINX and Apache instrumentation init containers are now created from scratch instead of
16+
cloning the main container, preventing probes, lifecycle hooks, and resize policies from being
17+
applied to init containers.

‎internal/instrumentation/apachehttpd.go‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,25 +68,23 @@ func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod
6868

6969
apacheConfDir := getApacheConfDir(apacheSpec.ConfigPath)
7070

71-
cloneContainer := container.DeepCopy()
72-
cloneContainer.Name = apacheAgentCloneContainerName
73-
cloneContainer.Command = []string{"/bin/sh", "-c"}
74-
cloneContainer.Args = []string{"cp -r " + apacheConfDir + "/* " + apacheAgentConfDirFull}
75-
cloneContainer.VolumeMounts = append(cloneContainer.VolumeMounts, corev1.VolumeMount{
76-
Name: apacheAgentConfigVolume,
77-
MountPath: apacheAgentConfDirFull,
78-
})
79-
// remove resource requirements since those are then reserved for the lifetime of a pod
80-
// and we definitely do not need them for the init container for cp command
81-
cloneContainer.Resources = apacheSpec.Resources
82-
// remove livenessProbe, readinessProbe, and startupProbe, since not supported on init containers
83-
cloneContainer.LivenessProbe = nil
84-
cloneContainer.ReadinessProbe = nil
85-
cloneContainer.StartupProbe = nil
86-
// remove lifecycle, since not supported on init containers
87-
cloneContainer.Lifecycle = nil
71+
cloneContainer := corev1.Container{
72+
Name: apacheAgentCloneContainerName,
73+
Image: container.Image,
74+
Command: []string{"/bin/sh", "-c"},
75+
Args: []string{"cp -r " + apacheConfDir + "/* " + apacheAgentConfDirFull},
76+
Env: container.Env,
77+
EnvFrom: container.EnvFrom,
78+
VolumeMounts: append(container.VolumeMounts, corev1.VolumeMount{
79+
Name: apacheAgentConfigVolume,
80+
MountPath: apacheAgentConfDirFull,
81+
}),
82+
Resources: apacheSpec.Resources,
83+
SecurityContext: container.SecurityContext,
84+
ImagePullPolicy: container.ImagePullPolicy,
85+
}
8886

89-
pod.Spec.InitContainers = append(pod.Spec.InitContainers, *cloneContainer)
87+
pod.Spec.InitContainers = append(pod.Spec.InitContainers, cloneContainer)
9088

9189
// drop volume mount with volume-provided Apache config from original container
9290
// since it could over-write configuration provided by the injection

‎internal/instrumentation/apachehttpd_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func TestInjectApacheHttpdagent(t *testing.T) {
206206
},
207207
// === Test Removal of probes and lifecycle =============================
208208
{
209-
name: "Probes removed on clone init container",
209+
name: "Init-container-incompatible fields not copied",
210210
ApacheHttpd: v1alpha1.ApacheHttpd{Image: "foo/bar:1"},
211211
pod: corev1.Pod{
212212
Spec: corev1.PodSpec{
@@ -216,6 +216,10 @@ func TestInjectApacheHttpdagent(t *testing.T) {
216216
StartupProbe: &corev1.Probe{},
217217
LivenessProbe: &corev1.Probe{},
218218
Lifecycle: &corev1.Lifecycle{},
219+
ResizePolicy: []corev1.ContainerResizePolicy{
220+
{ResourceName: corev1.ResourceCPU, RestartPolicy: corev1.NotRequired},
221+
{ResourceName: corev1.ResourceMemory, RestartPolicy: corev1.NotRequired},
222+
},
219223
},
220224
},
221225
},
@@ -298,6 +302,10 @@ func TestInjectApacheHttpdagent(t *testing.T) {
298302
StartupProbe: &corev1.Probe{},
299303
LivenessProbe: &corev1.Probe{},
300304
Lifecycle: &corev1.Lifecycle{},
305+
ResizePolicy: []corev1.ContainerResizePolicy{
306+
{ResourceName: corev1.ResourceCPU, RestartPolicy: corev1.NotRequired},
307+
{ResourceName: corev1.ResourceMemory, RestartPolicy: corev1.NotRequired},
308+
},
301309
},
302310
},
303311
},

‎internal/instrumentation/nginx.go‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -85,25 +85,23 @@ export %[4]s="$( { nginx -v ; } 2>&1 )" && echo ${%[4]s##*/} > %[3]s/version.txt
8585
nginxVersionEnvVar,
8686
)
8787

88-
cloneContainer := container.DeepCopy()
89-
cloneContainer.Name = nginxAgentCloneContainerName
90-
cloneContainer.Command = []string{"/bin/sh", "-c"}
91-
cloneContainer.Args = []string{nginxAgentCommands}
92-
cloneContainer.VolumeMounts = append(cloneContainer.VolumeMounts, corev1.VolumeMount{
93-
Name: nginxAgentConfigVolume,
94-
MountPath: nginxAgentConfDirFull,
95-
})
96-
// remove resource requirements since those are then reserved for the lifetime of a pod
97-
// and we definitely do not need them for the init container for cp command
98-
cloneContainer.Resources = nginxSpec.Resources
99-
// remove livenessProbe, readinessProbe, and startupProbe, since not supported on init containers
100-
cloneContainer.LivenessProbe = nil
101-
cloneContainer.ReadinessProbe = nil
102-
cloneContainer.StartupProbe = nil
103-
// remove lifecycle, since not supported on init containers
104-
cloneContainer.Lifecycle = nil
105-
106-
pod.Spec.InitContainers = append(pod.Spec.InitContainers, *cloneContainer)
88+
cloneContainer := corev1.Container{
89+
Name: nginxAgentCloneContainerName,
90+
Image: container.Image,
91+
Command: []string{"/bin/sh", "-c"},
92+
Args: []string{nginxAgentCommands},
93+
Env: container.Env,
94+
EnvFrom: container.EnvFrom,
95+
VolumeMounts: append(container.VolumeMounts, corev1.VolumeMount{
96+
Name: nginxAgentConfigVolume,
97+
MountPath: nginxAgentConfDirFull,
98+
}),
99+
Resources: nginxSpec.Resources,
100+
SecurityContext: container.SecurityContext,
101+
ImagePullPolicy: container.ImagePullPolicy,
102+
}
103+
104+
pod.Spec.InitContainers = append(pod.Spec.InitContainers, cloneContainer)
107105

108106
// drop volume mount with volume-provided Nginx config from original container
109107
// since it could over-write configuration provided by the injection

‎internal/instrumentation/nginx_test.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,9 +240,9 @@ func TestInjectNginxSDK(t *testing.T) {
240240
},
241241
},
242242
}},
243-
// === Test Removal of probes and lifecycle =============================
243+
// === Test init-container-incompatible fields not copied =============================
244244
{
245-
name: "Probes removed on clone init container",
245+
name: "Init-container-incompatible fields not copied",
246246
Nginx: v1alpha1.Nginx{
247247
Image: "foo/bar:1",
248248
},
@@ -257,6 +257,10 @@ func TestInjectNginxSDK(t *testing.T) {
257257
StartupProbe: &corev1.Probe{},
258258
LivenessProbe: &corev1.Probe{},
259259
Lifecycle: &corev1.Lifecycle{},
260+
ResizePolicy: []corev1.ContainerResizePolicy{
261+
{ResourceName: corev1.ResourceCPU, RestartPolicy: corev1.NotRequired},
262+
{ResourceName: corev1.ResourceMemory, RestartPolicy: corev1.NotRequired},
263+
},
260264
},
261265
},
262266
},
@@ -342,6 +346,10 @@ func TestInjectNginxSDK(t *testing.T) {
342346
StartupProbe: &corev1.Probe{},
343347
LivenessProbe: &corev1.Probe{},
344348
Lifecycle: &corev1.Lifecycle{},
349+
ResizePolicy: []corev1.ContainerResizePolicy{
350+
{ResourceName: corev1.ResourceCPU, RestartPolicy: corev1.NotRequired},
351+
{ResourceName: corev1.ResourceMemory, RestartPolicy: corev1.NotRequired},
352+
},
345353
Env: []corev1.EnvVar{
346354
{
347355
Name: "LD_LIBRARY_PATH",

0 commit comments

Comments
 (0)