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

fix: support for ephemeral container mutation #3560

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

brandtkeller
Copy link
Contributor

@brandtkeller brandtkeller commented Mar 7, 2025

Description

See the related issue - Ephemeral Containers are not explicitly supported by the zarf agent for mutation. This adds support by ensuring the pods/ephemeralcontainers subresource is being included in the MutatingWebhookConfiguration.

largely this should always be an operation that is being done on a pod that has already been patched - therefore we need to enable an exception for a pre-patched pod to still allow mutation of the ephemeral containers even if previously patched.

Changes

A pod cannot be created or updated with ephemeral containers defined declaratively. The original code for ephemeral containers should not be reachable - therefor moved it out of the primary pod mutation logic and into the ephemeral containers mutation logic.

Related Issue

Fixes #2153

Checklist before merging

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
@brandtkeller brandtkeller self-assigned this Mar 7, 2025
Copy link

netlify bot commented Mar 7, 2025

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 10355b2
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/67cf43a5b1bd060008ee9d05

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 73.91304% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/agent/hooks/pods.go 73.91% 9 Missing and 3 partials ⚠️
Files with missing lines Coverage Δ
src/internal/agent/hooks/pods.go 74.61% <73.91%> (-2.06%) ⬇️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
@brandtkeller
Copy link
Contributor Author

Current side-effect - image annotations get updated as a not-entirely-idempotent workflow. Working to address this.

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
@AustinAbro321
Copy link
Contributor

@brandtkeller To give some more context, the reason the agent is not fully idemopotent is because of the crchash we append to images, see https://docs.zarf.dev/ref/init-package/#image-mutation-to-unique-hashed-tags. If the agent was to mutate these resources twice, the crchash would no longer be correct. Open to other ways of checking besides a patched label, but I know that confused me when I first saw it

@brandtkeller
Copy link
Contributor Author

Thanks for the context @AustinAbro321. Agree on multiple mutation side-effects.

I do believe that the transform has an early return in the case of the crchash scenario that would prevent re-compute? Does this sound correct or is there something I am not considering?

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
@brandtkeller brandtkeller marked this pull request as ready for review March 10, 2025 20:24
@brandtkeller brandtkeller requested review from a team as code owners March 10, 2025 20:24
@AustinAbro321
Copy link
Contributor

Great point, I thought I remembered running into a gotcha here, maybe it was just the annotations

Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add to the E2E tests to ensure the kube API server is sending us the information we expect, I.E. an empty sub resource on non ephemeral container requests and responding to the changes in the webhook.yaml properly.

Other than that looks good

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

Successfully merging this pull request may close these issues.

EphemeralContainers via kubectl debug bypass Zarf webhook
2 participants