securityContext default override not as expected

[rwong2888]

Pre-requisites

• I have double-checked my configuration
• I can confirm the issues exists when I tested with :latest
• I'd like to contribute the fix myself (see contributing guide)

What happened/what you expected to happen?

#11127

I have the below controller configmap on v3.4.7.

apiVersion: v1
kind: ConfigMap
metadata:
  name: workflow-controller-configmap
data:
  # https://blog.argoproj.io/practical-argo-workflows-hardening-dd8429acc1ce
  mainContainer: |
    securityContext:
      runAsNonRoot: true
      runAsUser: 8737
      runAsGroup: 8737
      privileged: false
      readOnlyRootFilesystem: true
      allowPrivilegeEscalation: false
      capabilities:
        drop:
          - ALL
  executor: |
    securityContext:
      runAsNonRoot: true
      runAsUser: 8737
      runAsGroup: 8737
      privileged: false
      allowPrivilegeEscalation: false
      capabilities:
        drop:
          - ALL
  workflowDefaults: |
    spec:
      securityContext:
        runAsNonRoot: true
        runAsUser: 8737
        runAsGroup: 8737

I am trying to remove securityContext.capabilities.drop from the main container in a containerset.

I've tried setting capabilities to null or {} on my main container.

I've tried setting drop to null or [] on my main container.

e.g.

            securityContext:
              runAsUser: 1000
              runAsGroup: 1000
              capabilities: null

            securityContext:
              runAsUser: 1000
              runAsGroup: 1000
              capabilities: {}

But when I check the yaml of my pod, the container's securityContext capabilities is still drop all.

Referencing @alexec's blog post

• https://blog.argoproj.io/practical-argo-workflows-hardening-dd8429acc1ce

And helm's deleting a default key

• https://helm.sh/docs/chart_template_guide/values_files/#deleting-a-default-key

@terrytangyuan

Seems like a bug. The logic here needs to be revisited: https://github.com/argoproj/argo-workflows/blob/master/workflow/controller/workflowpod.go#L607-L629

Version

v3.4.7

Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

n/a

Logs from the workflow controller

n/a

Logs from in your workflow's wait container

n/a

[rwong2888]

I also tried @crenshaw-dev 's suggestion of podSpecPatch which did not work

      containerSet:
        podSpecPatch: '{"containers":[{"name":"main", "securityContext":{"capabilities":{}}}]}'
        containers:

      containerSet:
        podSpecPatch: '{"containers":[{"name":"main", "securityContext":{"capabilities":null}}]}'
        containers:

Referencing

• https://github.com/argoproj/argo-workflows/blob/master/examples/pod-spec-patch.yaml#L13

[sarabala1979]

@rohankmr414 it looks like a JSON merge issue. Please take a look at operator.go and workflowPod.go. These two places Json will merge to construct the securitycontext.

[mslapek]

The merge logic is in createWorkflowPod from workflowpod.go:

argo-workflows/workflow/controller/workflowpod.go

Lines 111 to 129 in 0f1b436

	if ctrDefaults := woc.controller.Config.MainContainer; ctrDefaults != nil {
	// essentially merge the defaults, then the template, into the container
	a, err := json.Marshal(ctrDefaults)
	if err != nil {
	return nil, err
	}
	b, err := json.Marshal(c)
	if err != nil {
	return nil, err
	}
	if err := json.Unmarshal(a, &c); err != nil {
	return nil, err
	}
	if err = json.Unmarshal(b, &c); err != nil {
	return nil, err
	}
	}
	mainCtrs[i] = c

However, I'm afraid we cannot have the expected result with mainCtrs []apiv1.Container as an input – because we cannot differ between two kinds of null:

1. Set by user explicitly in JSON – so we want to delete the key.
2. Omitted by user in JSON – so we want to use a default value.

To solve the issue, we would need to take untyped JSON as an input mainCtrs []map[string]any - so json.Unmarshal would decode the intent properly.

Question Should we make big-change of mainCtrs to this new type? (looks like a big churn ☹️)

[terrytangyuan]

@jswxstw Are you interested in working on this?

[terrytangyuan]

That sounds confusing though. Let's try reusing the current fields if possible

[terrytangyuan]

@jswxstw Please do not delete comments unless necessary since others will lose the context.

[jswxstw]

That sounds confusing though. Let's try reusing the current fields if possible

ok, podSpecPatch seems like it can be used to solve this problem, I will try to check out why it did not work.

[jswxstw]

'{"containers":[{"name":"main", "securityContext":{"capabilities":{}}}]}'

The correct usage of podSpecPatch:

spec:
  entrypoint: whalesay
  templates:
  - name: whalesay
    podSpecPatch: '{"containers":[{"name":"main", "securityContext":{"capabilities":null}}]}'
    containerSet:

However, there is still a bug here: @terrytangyuan

argo-workflows/workflow/controller/workflowpod.go

Lines 350 to 372 in 29cf90e

	// Apply the patch string from template
	if woc.hasPodSpecPatch(tmpl) {
	tmpl.PodSpecPatch, err = util.PodSpecPatchMerge(woc.wf, tmpl)
	if err != nil {
	return nil, errors.Wrap(err, "", "Failed to merge the workflow PodSpecPatch with the template PodSpecPatch due to invalid format")
	}
	// Final substitution for workflow level PodSpecPatch
	localParams := make(map[string]string)
	if tmpl.IsPodType() {
	localParams[common.LocalVarPodName] = pod.Name
	}
	tmpl, err := common.ProcessArgs(tmpl, &wfv1.Arguments{}, woc.globalParams, localParams, false, woc.wf.Namespace, woc.controller.configMapInformer.GetIndexer())
	if err != nil {
	return nil, errors.Wrap(err, "", "Failed to substitute the PodSpecPatch variables")
	}
	patchedPodSpec, err := util.ApplyPodSpecPatch(pod.Spec, tmpl.PodSpecPatch)
	if err != nil {
	return nil, errors.Wrap(err, "", "Error applying PodSpecPatch")
	}
	pod.Spec = *patchedPodSpec
	}

before PodSpecPatchMerge: '{"containers":[{"name":"main", "securityContext":{"capabilities":null}}]}'
after PodSpecPatchMerge: '{"containers":[{"name":"main", "securityContext":{"capabilities":{}}}]}'

[terrytangyuan]

Thanks. Could you take a look at my comment?

[tczhao]

I think I made a mistake in #12476
According to

• https://argo-workflows.readthedocs.io/en/latest/default-workflow-specs/
• https://argo-workflows.readthedocs.io/en/latest/template-defaults/#configuring-templatedefaults-in-controller-level

The priority should be:
workflowDefault > template podSpecPatch > templateDefault.
instead of current
template podSpecPatch > templateDefault > workflowDefault.

---

For this issue, instead of how podspecpatch is applied, the problem area should be at

argo-workflows/workflow/controller/workflowpod.go

Lines 111 to 128 in 66e64c8

	if ctrDefaults := woc.controller.Config.MainContainer; ctrDefaults != nil {
	// essentially merge the defaults, then the template, into the container
	a, err := json.Marshal(ctrDefaults)
	if err != nil {
	return nil, err
	}
	b, err := json.Marshal(c)
	if err != nil {
	return nil, err
	}
	mergedContainerByte, err := strategicpatch.StrategicMergePatch(a, b, apiv1.Container{})
	if err != nil {
	return nil, err
	}
	c = apiv1.Container{}
	if err := json.Unmarshal(mergedContainerByte, &c); err != nil {
	return nil, err

[jswxstw]

The priority should be:
workflowDefault > template podSpecPatch > templateDefault.
instead of current
template podSpecPatch > templateDefault > workflowDefault.

I think current priority is correct. workflowDefault is usually a common config, when I submit a specific workflow with different requirements, I will set a new config to override it.

For this issue, instead of how podspecpatch is applied, the problem area should be at

You are correct, but it needs significant changes as #11130 (comment) analized.
#12476 is a simpler workaround and this issue can be fixed as #11130 (comment).

[tczhao]

I think current priority is correct. workflowDefault is usually a common config, when I submit a specific workflow with different requirements, I will set a new config to override it.

Right, I misread

[tooptoop4]

maybe related to #13697