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] Pod without labels will fail mutation #2517

Closed
brandtkeller opened this issue May 17, 2024 · 6 comments · Fixed by #2518
Closed

[bug] Pod without labels will fail mutation #2517

brandtkeller opened this issue May 17, 2024 · 6 comments · Fixed by #2518

Comments

@brandtkeller
Copy link
Contributor

Environment

Device and OS: AMD64 linux Server (ubuntu)
App version: v0.33.2
Kubernetes distro being used: Kind

Steps to reproduce

  1. Deploy zarf init package
  2. Create a pod that contains no labels

Expected result

Pod should be created and mutated by the zarf agent

Actual Result

internal server error

Internal error occurred: replace operation does not apply: doc is missing path: /metadata/labels/zarf-agent: missing value

Additional Context

Likely an edge case that does not occur often due to many clients adding default labels.

@lucasrod16
Copy link
Contributor

I would like to better understand the use case that runs into this problem. Pods deployed by Zarf will always have a label on them, so I assume this problem only occurs when resources are deployed with kubectl into a Zarf cluster without the zarf.dev/agent: ignore label?

A pod with no labels that was not deployed by Zarf, even if mutated properly by the agent, would still fail to deploy due to other requirements that Zarf handles automatically:

  • the image would have to manually be pushed to the Zarf registry before deployment
  • imagePullSecret must exist in the same namespace and must be referenced in the pod spec to authenticate to pull the image from the Zarf registry

For these reasons, I would recommend adding the zarf.dev/agent: ignore label to the resources that are not deployed and managed by Zarf.

@brandtkeller
Copy link
Contributor Author

This is very much handling for edge cases. The thread that spawned review revolved around the local-path-provisioner chart and how it uses a helper pod in a configmap as an "operator-like" execution for how it performs PVC creation.

Easily worked-around by ensuring you change the configuration to include some arbitrary label. That does require forking and there may be a possibility that other operators orchestrate pods and do not set any labels by default? TBD - as I haven't seen this issue often but have seen it a handful of times over the last 2 years.

I'd argue low effort/low impact overall without more evidence that this is likely to continue to appear more often.

lucasrod16 pushed a commit that referenced this issue May 17, 2024
## Description

identify when a pod has no labels (and thus has no `/metadata/labels`
for jsonpatch replace operations. use the `add` jsonpath operation to
add the labels path with the map[string]string value of `{"zarf-agent":
"patched"}`

## Related Issue

Fixes #2517 

## Testing
Deploy a pod without labels to test:
```
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  name: test
  namespace: test
spec:
  containers:
  - image: nginx
    name: test
    resources: {}
  dnsPolicy: ClusterFirst
  restartPolicy: Always
status: {}
```

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/.github/CONTRIBUTING.md#developer-workflow)
followed
@salaxander salaxander removed this from Zarf (old) Jun 25, 2024
AustinAbro321 pushed a commit that referenced this issue Jul 23, 2024
## Description

identify when a pod has no labels (and thus has no `/metadata/labels`
for jsonpatch replace operations. use the `add` jsonpath operation to
add the labels path with the map[string]string value of `{"zarf-agent":
"patched"}`

## Related Issue

Fixes #2517

## Testing
Deploy a pod without labels to test:
```
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  name: test
  namespace: test
spec:
  containers:
  - image: nginx
    name: test
    resources: {}
  dnsPolicy: ClusterFirst
  restartPolicy: Always
status: {}
```

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/.github/CONTRIBUTING.md#developer-workflow)
followed

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@joelcomp1
Copy link

@brandtkeller do you have any more details on that work around? I am running into this right now on local path provisioner and updated the configmap to have a label but that isn't doing the trick. Is there anything else you had to do?

@brandtkeller
Copy link
Contributor Author

@joelcomp1 provided you set some arbitrary label in the configmap this error should not appear. The attached PR - which was merged in May should prevent this from occurring - but you may have identified another edge-case for consideration.

If you still are having issues and on the latest zarf version - we might want to have the team re-open this issue.

Can you copy your configmap content here and I'll work to reproduce?

@joelcomp1
Copy link

Ah OK, it also is probably my old version of zarf I was running 0.32.6 in a full airgap environment so not easy to update at that point. I seemed to be able to resolve by doing a zarf destroy then a zarf init

@AustinAbro321
Copy link
Contributor

@joelcomp1 You actually don't need to run zarf destroy, even zarf has already been installed on the cluster, re-running zarf init with a newer version will deploy the new agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants