Skip to content

feat: add dynamicLabels to session pod config#113

Draft
ian-flores wants to merge 5 commits intomainfrom
dynamic-labels
Draft

feat: add dynamicLabels to session pod config#113
ian-flores wants to merge 5 commits intomainfrom
dynamic-labels

Conversation

@ian-flores
Copy link
Collaborator

@ian-flores ian-flores commented Mar 4, 2026

Summary

  • Adds DynamicLabelRule struct and dynamicLabels field on PodConfig CRD, enabling per-session pod labels derived from runtime .Job data
  • Supports two modes: direct field mapping (labelKey) and regex pattern extraction (match + labelPrefix)
  • Adds Go template logic in job.tpl to render dynamic labels at session launch time

Closes PTDC-199

Context

A customer needs to track Workbench session costs per Entra ID group using OpenCost. Group names are extracted from session launch arguments and added as pod labels. The customer had a working Go template solution, but the operator reconciles the template ConfigMap, overwriting manual edits. This adds a first-class CRD field.

Usage example

sessionConfig:
  pod:
    dynamicLabels:
      - field: "args"
        match: "_entra_[^ ,]+"
        trimPrefix: "_entra_"
        labelPrefix: "posit.entra.group."
        labelValue: "true"

Produces: posit.entra.group.research_team: "true", posit.entra.group.data_science: "true"

Test plan

  • Unit tests for JSON serialization (direct mapping + pattern extraction)
  • just mgenerate succeeds (deepcopy + CRD manifests)
  • just test passes
  • Deploy adhoc image to test cluster, apply Workbench CR with dynamicLabels, launch session, verify pod labels

Add DynamicLabelRule struct and dynamicLabels field on PodConfig,
enabling per-session pod labels derived from runtime .Job data.
Supports direct field mapping (labelKey) and regex pattern extraction
(match + labelPrefix) for use cases like Entra group cost tracking.
@claude
Copy link

claude bot commented Mar 4, 2026

Claude finished @ian-flores's task —— View job


Review: feat: add dynamicLabels to session pod config

Overall the design is clean — a CRD field with two operating modes (direct mapping vs. regex extraction) that flows through job.tpl. The struct, deepcopy, and CRD manifests are consistent. A few items worth addressing:


1. Mutual exclusivity not enforced — api/product/session_config.go:64-85

labelKey and match/labelPrefix are documented as mutually exclusive, and labelPrefix is described as "required when match is set," but neither constraint is enforced via kubebuilder validation markers or programmatic checks. A user could set both labelKey and match — the template silently prefers labelKey. At minimum, document which takes precedence; ideally add validation.

2. Security: field as an arbitrary key into .Jobapi/templates/2.5.0/job.tpl:83

{{- if hasKey $.Job $rule.field }}

field is user-supplied CRD input used as a key into $.Job via index. If .Job contains internal/sensitive fields, anyone with CRD write access could surface that data as a pod label (visible to get pods RBAC). This is partially mitigated since CRD write is privileged, but consider validating field against an allowlist of known-safe fields (e.g., user, args, name, tags) or documenting the trust model.

3. join " " on non-array fields — api/templates/2.5.0/job.tpl:87

{{- $str := index $.Job $rule.field | join " " }}

join " " will fail at template execution time if the field value is a string rather than an array. If someone uses the match path on a scalar field like user, this causes a runtime template error. Consider guarding with a type check or documenting which fields are arrays vs. strings.

4. Regex / label key validation — api/templates/2.5.0/job.tpl:88-90

  • User-supplied regex is passed directly to regexFindAll. Go's RE2 (used by Sprig) is resistant to catastrophic backtracking, so ReDoS isn't a concern, but a malformed regex will cause a template execution error at session launch time — a bad feedback loop.
  • The resulting label key (prefix + cleaned match) isn't validated against the Kubernetes label format (63-char name limit, valid DNS subdomain prefix). Invalid labels would fail pod creation at runtime with a potentially confusing error. A length/format check in the template or CRD-level validation would improve the user experience.

5. Only added to template 2.5.0 — api/templates/2.5.0/job.tpl:81

Templates 2.3.1 and 2.4.0 don't include the dynamic labels block. If those versions are still in active use, users who configure dynamicLabels will get no labels and no error. Is this intentional? A note about minimum template version in docs or the CRD description would help.

6. Test coverage — api/product/session_config_test.go

The two new tests verify JSON serialization round-trips, which is good. But the real complexity lives in the Go template logic in job.tpl. Consider adding template rendering tests that:

  • Exercise direct mapping with a mock .Job containing a scalar field
  • Exercise pattern extraction with a mock .Job containing an array field
  • Test edge cases: no matches, trimPrefix longer than the match, empty field

This would catch issues like the join " " on non-array fields before they reach a real cluster.

7. No self-review comments

Per repo norms (review guidelines), PR authors are expected to add inline comments on areas of concern or key decisions. There are several template logic decisions here (e.g., lower | replace " " "_" | replace "-" "_" normalization) that would benefit from author context.


The CRD struct, deepcopy, and generated manifests all look correct and consistent. The template logic is the area that carries the most risk — the items above are mostly about improving guardrails and test coverage for runtime behavior.

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Review of dynamicLabels feature — see detailed feedback below.

Changes:
- Add `ValidateDynamicLabelRules()` function that validates regex compilation (ReDoS prevention via Go's RE2 engine) and enforces mutual exclusivity between `labelKey` and `match`/`labelPrefix`
- Call validation from `GenerateSessionConfigTemplate()` to reject invalid rules before template rendering
- Fix template to handle scalar fields in the `match` branch using `kindIs "slice"` type check instead of unconditional `join`
- Add `trunc 63` to label key suffix in template to enforce Kubernetes label value length limits
- Remove lossy `replace "-" "_"` transformation — hyphens are valid in label keys
- Add kubebuilder `MaxLength`/`MinLength` validation markers to `DynamicLabelRule` fields
- Add validation unit tests covering mutual exclusivity, missing fields, invalid regex, and generation-time rejection
{{- $str := (kindIs "slice" $val) | ternary ($val | join " ") ($val | toString) }}
{{- $matches := regexFindAll $rule.match $str -1 }}
{{- range $match := $matches }}
{{ trimPrefix ($rule.trimPrefix | default "") $match | lower | replace " " "_" | trunc 63 | printf "%s%s" $rule.labelPrefix }}: {{ $rule.labelValue | default "true" | quote }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Design choice: The lower | replace " " "_" | replace "-" "_" normalization is intentionally hardcoded rather than configurable. This covers the common case (Kubernetes label compliance) and avoids complexity. If customers need different sanitization, we can add a transform field later.

// to a label (using labelKey) or extracts multiple labels via regex (using match).
// +kubebuilder:object:generate=true
type DynamicLabelRule struct {
// Field is the name of a top-level .Job field to read (e.g., "user", "args").
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Trust model: field accepts any top-level .Job key, which means CRD authors can surface any launcher job field as a pod label. This is acceptable because CRD write access is already a privileged operation (cluster admin or namespace admin). Documented this explicitly in the field comment.

LabelValue string `json:"labelValue,omitempty"`
}

// ValidateDynamicLabelRules validates a slice of DynamicLabelRule, checking for
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Validation strategy: Mutual exclusivity (labelKey vs match) and regex compilation are validated programmatically in ValidateDynamicLabelRules(), called at template generation time. Kubebuilder markers handle length constraints at admission. This catches errors before they reach the Go template engine at session launch.

{{- if $rule.labelKey }}
{{ $rule.labelKey }}: {{ $val | toString | quote }}
{{- else if $rule.match }}
{{- $str := (kindIs "slice" $val) | ternary ($val | join " ") ($val | toString) }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Scalar vs array handling: Uses kindIs "slice" with a ternary to handle both array fields (like args) and scalar fields (like user). Scalars get toString, arrays get join " ". This prevents the runtime template error that would occur if join was called on a string value.

@ian-flores
Copy link
Collaborator Author

Review findings addressed

All 7 items from the review have been addressed across commits 7c9de7c and c70f317:

# Finding Resolution
1 Mutual exclusivity not enforced Added ValidateDynamicLabelRules() with programmatic checks, called at template generation time
2 field as arbitrary .Job key Documented trust model in field comment — relies on CRD write being privileged
3 join " " on scalar fields Template now uses kindIs "slice" with ternary to handle both arrays and scalars
4 Regex/label validation Regex compiled at generation time; kubebuilder MaxLength markers on all string fields; trunc 63 in template
5 Only template 2.5.0 Added doc comment: "Requires template version 2.5.0 or later; ignored by older templates"
6 Test coverage Added TestValidateDynamicLabelRules (7 sub-tests) and TestGenerateSessionConfigTemplate_DynamicLabels_Validation (2 sub-tests)
7 Self-review comments Added inline comments on normalization, trust model, validation strategy, and type handling

Not addressed (future scope):

  • Template rendering tests with mock .Job data (would require test harness for the Helm template engine)
  • Field allowlist (documented trust model instead)

- Merge main and regenerate Helm chart CRDs
- Remove accidentally committed .claude/tsc-cache files
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.

1 participant