Skip to content

fix: validate container env var names in PodCliqueSet webhook#470

Merged
shmuel-runai merged 1 commit into
ai-dynamo:mainfrom
shmuel-runai:RUN-240/env-var-validation
Mar 8, 2026
Merged

fix: validate container env var names in PodCliqueSet webhook#470
shmuel-runai merged 1 commit into
ai-dynamo:mainfrom
shmuel-runai:RUN-240/env-var-validation

Conversation

@shmuel-runai

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

Extend the PodCliqueSet admission webhook to catch invalid and duplicate environment variable names at admission time rather than at pod runtime.

Also corrects sliceMustHaveUniqueElements to emit ErrorTypeDuplicate instead of ErrorTypeInvalid.

Tested:

  • UT

Fixes #240

@copy-pr-bot

copy-pr-bot Bot commented Mar 4, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@shmuel-runai shmuel-runai force-pushed the RUN-240/env-var-validation branch from 85b39b7 to 2123b8e Compare March 4, 2026 13:12
Comment thread operator/internal/webhook/admission/pcs/validation/podcliqueset.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Extends the PodCliqueSet admission webhook validation to reject invalid and duplicate container environment variable names at admission time, and adjusts the shared uniqueness validator to report duplicates using Kubernetes’ ErrorTypeDuplicate.

Changes:

  • Add env var name + duplicate checks for spec.containers[].env and spec.initContainers[].env in PCS validation.
  • Update sliceMustHaveUniqueElements to return field.ErrorTypeDuplicate (via field.Duplicate) and update call sites.
  • Add/adjust unit tests for env var validation and duplicate error typing.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
operator/internal/webhook/admission/pcs/validation/util.go Changes uniqueness helper to emit Duplicate errors.
operator/internal/webhook/admission/pcs/validation/util_test.go Updates tests for new duplicate error type (but currently under-asserts).
operator/internal/webhook/admission/pcs/validation/podcliqueset.go Adds container env var validation (invalid names + duplicates).
operator/internal/webhook/admission/pcs/validation/podcliqueset_test.go Adds env var validation test coverage for containers/initContainers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread operator/internal/webhook/admission/pcs/validation/util_test.go
@shmuel-runai shmuel-runai force-pushed the RUN-240/env-var-validation branch from 2123b8e to 5fcdd28 Compare March 5, 2026 10:53
Extend the PodCliqueSet admission webhook to catch invalid and duplicate
environment variable names at admission time rather than at pod runtime.

Also corrects sliceMustHaveUniqueElements to emit ErrorTypeDuplicate
instead of ErrorTypeInvalid.

Tested:
* UT

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@shmuel-runai shmuel-runai force-pushed the RUN-240/env-var-validation branch from 5fcdd28 to 6c7ff75 Compare March 5, 2026 10:59

@shayasoolin shayasoolin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These validations can be implemented on the CRD itself as CEL validations. I think we should consider switching to this validation type, it spares the need for implementing all this logic.

@shmuel-runai shmuel-runai merged commit 2e5439d into ai-dynamo:main Mar 8, 2026
28 of 31 checks passed
Ronkahn21 pushed a commit to Ronkahn21/grove that referenced this pull request Mar 10, 2026
…amo#470)

Extend the PodCliqueSet admission webhook to catch invalid and duplicate
environment variable names at admission time rather than at pod runtime.

Also corrects sliceMustHaveUniqueElements to emit ErrorTypeDuplicate
instead of ErrorTypeInvalid.

Tested:
* UT
enoodle pushed a commit to enoodle/grove that referenced this pull request Mar 24, 2026
…amo#470)

Extend the PodCliqueSet admission webhook to catch invalid and duplicate
environment variable names at admission time rather than at pod runtime.

Also corrects sliceMustHaveUniqueElements to emit ErrorTypeDuplicate
instead of ErrorTypeInvalid.

Tested:
* UT
Signed-off-by: Erez Freiberger <enoodle@gmail.com>
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.

Add environment variable validation to PodCliqueSet webhook

4 participants