HYPERFLEET-1108 - feat: Label all E2E-created resources with a unique run ID (DRAFT)#131
HYPERFLEET-1108 - feat: Label all E2E-created resources with a unique run ID (DRAFT)#131mliptak0 wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
mliptak0
changed the title
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA globally scoped E2E run ID is introduced across the test infrastructure. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes CWE-807 / env variable trust boundary — CWE-330 / insufficient randomness — CWE-20 / deploy-scripts/lib/adapter.sh:170–179 — The shell-level CWE-78 / shell injection in adapter.sh:216–218 — Supply chain note — Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/helper/adapter.go (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument intentional crypto/rand fallback.
Silent fallback to time-based randomness when
crypto/randfails is intentional degradation. Per path instructions ERR-01 and QUAL-02, add a comment explaining why this is acceptable (diagnostic log suffix collision is low-impact).📝 Suggested comment
n, err := rand.Int(rand.Reader, big.NewInt(int64(len(charset)))) if err != nil { + // Fallback to time-based randomness for diagnostic log suffix; crypto quality not required b[i] = charset[(time.Now().UnixNano()+int64(i))%int64(len(charset))]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helper/adapter.go` around lines 25 - 27, Add a comment above or within the error handling block (after the rand.Int call fails) in the charset randomization logic that explains the intentional fallback to time-based randomness is acceptable because diagnostic log suffix collisions have low impact. Reference that this is intentional degradation per error handling and quality guidelines, making it clear to future maintainers that the silent fallback using time.Now().UnixNano() is not an oversight but a deliberate design choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy-scripts/lib/adapter.sh`:
- Around line 216-218: The hardcoded array indices env[0].name and env[0].value
on lines 216-217 will overwrite existing environment variables instead of
appending to them. Coordinate with the chart maintainers to modify the Helm
chart to accept a dedicated values path (such as e2e.runID) and have the chart
template merge this value into the env array without using fixed indices.
Replace the --set "env[0].name=E2E_RUN_ID" and --set
"env[0].value=${e2e_run_id}" calls with a --set call to your new dedicated
values path so conflicts are avoided.
- Around line 170-179: The E2E_RUN_ID validation block is missing character
validation for Kubernetes label values. After the existing length check (which
validates that e2e_run_id does not exceed 63 characters), add an additional
validation check to ensure the e2e_run_id matches the Kubernetes label value
regex pattern which allows only alphanumeric characters, hyphens, underscores,
and periods, and must start and end with an alphanumeric character. Use a
conditional test to match the variable against the required pattern and log an
error with a clear message describing which characters are allowed if the
validation fails.
In `@testdata/adapter-configs/cl-crash/adapter-task-config.yaml`:
- Line 56: The label key `e2e.hyperfleet.io/run-id` on line 56 of the
adapter-task-config.yaml file is inconsistent with the Helm injection that uses
`e2e.hyperfleet.io/run` in pkg/helper/adapter.go (set via --set
labels.e2e\\.hyperfleet\\.io/run=%s). Change the label key from
`e2e.hyperfleet.io/run-id` to `e2e.hyperfleet.io/run` to unify the label keys
across both Helm and adapter resources, ensuring cleanup and reporting use a
single consistent selector key.
In
`@testdata/adapter-configs/cl-deployment/adapter-task-resource-deployment.yaml`:
- Line 10: The run-id label is currently only applied to the Deployment metadata
but not to the Pod template metadata. Add the same label
`e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}"` to the
`spec.template.metadata.labels` section of the YAML configuration so that Pods
created from this Deployment template inherit the run-id label and become
selectable by E2E run-id selectors.
In `@testdata/adapter-configs/cl-job/adapter-task-resource-job.yaml`:
- Line 11: The label e2e.hyperfleet.io/run-id is currently set only on the Job
object metadata, but Pods created by this Job will not inherit it. To fix this,
add the same label e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}" under the Pod
template specification at spec.template.metadata.labels so that all Pods spawned
by this Job will carry the run-id label for proper tracking and identification.
In `@testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml`:
- Line 85: The e2e.hyperfleet.io/run-id label is currently only applied to the
ManifestWork wrapper resource at line 85 but is missing from the nested manifest
resources (Namespace and ConfigMap) that start at line 126. You need to
propagate the run-id label by adding the same e2e.hyperfleet.io/run-id: "{{
.e2eRunId }}" label to the metadata.labels section of each nested resource
definition so that target-cluster resources can be properly filtered and cleaned
by run-id.
---
Nitpick comments:
In `@pkg/helper/adapter.go`:
- Around line 25-27: Add a comment above or within the error handling block
(after the rand.Int call fails) in the charset randomization logic that explains
the intentional fallback to time-based randomness is acceptable because
diagnostic log suffix collisions have low impact. Reference that this is
intentional degradation per error handling and quality guidelines, making it
clear to future maintainers that the silent fallback using time.Now().UnixNano()
is not an oversight but a deliberate design choice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a1d0dac1-18dc-4467-af82-bee96dd31978
📒 Files selected for processing (23)
cmd/hyperfleet-e2e/test/cmd.godeploy-scripts/deploy-clm.shdeploy-scripts/lib/adapter.shpkg/e2e/suite.gopkg/helper/adapter.gopkg/helper/helper.gopkg/helper/suite.gotestdata/adapter-configs/cl-crash/adapter-task-config.yamltestdata/adapter-configs/cl-deployment/adapter-task-config.yamltestdata/adapter-configs/cl-deployment/adapter-task-resource-deployment.yamltestdata/adapter-configs/cl-invalid-resource/adapter-task-config.yamltestdata/adapter-configs/cl-job/adapter-task-config.yamltestdata/adapter-configs/cl-job/adapter-task-resource-job.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-maestro/adapter-task-config.yamltestdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yamltestdata/adapter-configs/cl-namespace/adapter-task-config.yamltestdata/adapter-configs/cl-precondition-error/adapter-task-config.yamltestdata/adapter-configs/cl-stuck/adapter-task-config.yamltestdata/adapter-configs/np-configmap/adapter-task-config.yamltestdata/adapter-configs/np-configmap/adapter-task-resource-configmap.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| local e2e_run_id="${E2E_RUN_ID:-}" | ||
| if [[ -z "${e2e_run_id}" ]]; then | ||
| log_error "E2E_RUN_ID is not set. Pass --e2e-run-id to deploy-clm.sh or set the E2E_RUN_ID environment variable." | ||
| return 1 | ||
| fi | ||
| if (( ${#e2e_run_id} > 63 )); then | ||
| log_error "E2E_RUN_ID '${e2e_run_id}' is ${#e2e_run_id} characters, exceeds the 63-character Kubernetes label value limit." | ||
| return 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing character validation for E2E_RUN_ID label value.
Lines 170-179 validate length (≤63 chars) but not allowed characters. Kubernetes label values must match (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?. Invalid characters (e.g., spaces, special chars) will cause Helm deployment failure at line 218.
🔒 Proposed validation fix
if (( ${`#e2e_run_id`} > 63 )); then
log_error "E2E_RUN_ID '${e2e_run_id}' is ${`#e2e_run_id`} characters, exceeds the 63-character Kubernetes label value limit."
return 1
fi
+ if [[ ! "${e2e_run_id}" =~ ^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$ ]]; then
+ log_error "E2E_RUN_ID '${e2e_run_id}' contains invalid characters. Must match Kubernetes label value format: ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]"
+ return 1
+ fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy-scripts/lib/adapter.sh` around lines 170 - 179, The E2E_RUN_ID
validation block is missing character validation for Kubernetes label values.
After the existing length check (which validates that e2e_run_id does not exceed
63 characters), add an additional validation check to ensure the e2e_run_id
matches the Kubernetes label value regex pattern which allows only alphanumeric
characters, hyphens, underscores, and periods, and must start and end with an
alphanumeric character. Use a conditional test to match the variable against the
required pattern and log an error with a clear message describing which
characters are allowed if the validation fails.
| --set "env[0].name=E2E_RUN_ID" | ||
| --set "env[0].value=${e2e_run_id}" | ||
| --set "labels.e2e\\.hyperfleet\\.io/run=${e2e_run_id}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Hardcoded env array index risks clobbering existing environment variables.
Lines 216-217 inject env[0].name=E2E_RUN_ID and env[0].value=<run-id>. Same issue as adapter.go: Helm array indexing replaces, doesn't append. If chart defines env or another --set targets env[0], this overwrites existing entries.
Coordinate with chart maintainers to use a dedicated values path (e.g., e2e.runID) that the chart merges into env without index conflicts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy-scripts/lib/adapter.sh` around lines 216 - 218, The hardcoded array
indices env[0].name and env[0].value on lines 216-217 will overwrite existing
environment variables instead of appending to them. Coordinate with the chart
maintainers to modify the Helm chart to accept a dedicated values path (such as
e2e.runID) and have the chart template merge this value into the env array
without using fixed indices. Replace the --set "env[0].name=E2E_RUN_ID" and
--set "env[0].value=${e2e_run_id}" calls with a --set call to your new dedicated
values path so conflicts are avoided.
| labels: | ||
| hyperfleet.io/cluster-id: "{{ .clusterId }}" | ||
| hyperfleet.io/cluster-name: "{{ .clusterName }}" | ||
| e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unify run label key across Helm and adapter resources
Line 56 uses e2e.hyperfleet.io/run-id, but Helm injection uses e2e.hyperfleet.io/run in pkg/helper/adapter.go (--set labels.e2e\\.hyperfleet\\.io/run=%s). This splits selectors across two keys and breaks single-key cleanup/reporting.
Proposed fix
- e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}"
+ e2e.hyperfleet.io/run: "{{ .e2eRunId }}"Or standardize the Helm key to run-id everywhere instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testdata/adapter-configs/cl-crash/adapter-task-config.yaml` at line 56, The
label key `e2e.hyperfleet.io/run-id` on line 56 of the adapter-task-config.yaml
file is inconsistent with the Helm injection that uses `e2e.hyperfleet.io/run`
in pkg/helper/adapter.go (set via --set labels.e2e\\.hyperfleet\\.io/run=%s).
Change the label key from `e2e.hyperfleet.io/run-id` to `e2e.hyperfleet.io/run`
to unify the label keys across both Helm and adapter resources, ensuring cleanup
and reporting use a single consistent selector key.
| hyperfleet.io/cluster-id: "{{ .clusterId }}" | ||
| hyperfleet.io/resource-type: "job" | ||
| app: test-job | ||
| e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add run-id label on Job Pod template
Line 11 labels only the Job object. Pods created by this Job are not guaranteed to carry e2e.hyperfleet.io/run-id unless it is set under spec.template.metadata.labels.
Proposed fix
spec:
backoffLimit: 0
template:
+ metadata:
+ labels:
+ e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}"
spec:
restartPolicy: Never🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testdata/adapter-configs/cl-job/adapter-task-resource-job.yaml` at line 11,
The label e2e.hyperfleet.io/run-id is currently set only on the Job object
metadata, but Pods created by this Job will not inherit it. To fix this, add the
same label e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}" under the Pod template
specification at spec.template.metadata.labels so that all Pods spawned by this
Job will carry the run-id label for proper tracking and identification.
| hyperfleet.io/component: "infrastructure" | ||
| hyperfleet.io/generation: "{{ .generation }}" | ||
| hyperfleet.io/resource-group: "cluster-setup" | ||
| e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Run-id is not propagated to nested ManifestWork payload resources
Line 85 labels the ManifestWork wrapper only. The nested Namespace/ConfigMap manifests (lines 126+ context) are created without e2e.hyperfleet.io/run-id, so target-cluster resources cannot be filtered/cleaned by run-id.
Proposed fix pattern
- apiVersion: v1
kind: Namespace
metadata:
name: "{{ .clusterId | lower }}-{{ .adapter.name }}-namespace"
labels:
app.kubernetes.io/component: adapter-task-config
app.kubernetes.io/instance: "{{ .adapter.name }}"
app.kubernetes.io/name: cl-maestro
app.kubernetes.io/transport: maestro
+ e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}"
- apiVersion: v1
kind: ConfigMap
metadata:
name: "{{ .clusterId | lower }}-{{ .adapter.name }}-configmap"
namespace: "{{ .clusterId | lower }}-{{ .adapter.name }}-namespace"
labels:
app.kubernetes.io/component: adapter-task-config
app.kubernetes.io/instance: "{{ .adapter.name }}"
app.kubernetes.io/name: cl-maestro
app.kubernetes.io/version: 1.0.0
app.kubernetes.io/transport: maestro
+ e2e.hyperfleet.io/run-id: "{{ .e2eRunId }}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml` at
line 85, The e2e.hyperfleet.io/run-id label is currently only applied to the
ManifestWork wrapper resource at line 85 but is missing from the nested manifest
resources (Namespace and ConfigMap) that start at line 126. You need to
propagate the run-id label by adding the same e2e.hyperfleet.io/run-id: "{{
.e2eRunId }}" label to the metadata.labels section of each nested resource
definition so that target-cluster resources can be properly filtered and cleaned
by run-id.
8afab37 to
4b66a75
Compare
mliptak0
force-pushed
the
HYPERFLEET-1108
branch
from
4b66a75 to
7f56e0d
Compare
…apter resource tracking - Add E2E_RUN_ID environment variable support for labeling test resources across cluster and nodepool adapters - Implement GenerateRunID() with automatic UUID v7 fallback for local development and validation for Kubernetes label constraints - Update DeployAdapter() to inject run ID into Helm chart resources and adapter environment - Update adapter shell scripts to validate and propagate E2E_RUN_ID with length and format checks - Add run ID parameter (e2eRunId) to all adapter task configurations to enable resource tracking by test run - Update adapter task resource templates to include e2e.hyperfleet.io/run-id labels - Add --e2e-run-id CLI flag to test command for explicit run ID specification - Fix deployment YAML trailing newline issue Co-authored-by: Cursor <cursoragent@cursor.com>
mliptak0
force-pushed
the
HYPERFLEET-1108
branch
from
7f56e0d to
8dc8a16
Compare
… flag for passing run id
1 participant
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)