HYPERFLEET-839 - fix: require explicit NAMESPACE to prevent Pub/Sub c…#86
HYPERFLEET-839 - fix: require explicit NAMESPACE to prevent Pub/Sub c…#86ma-hill wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
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 |
WalkthroughThe PR makes namespace handling user-specific and stricter: deploy-scripts/.env.example now defaults NAMESPACE to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy-scripts/README.md`:
- Around line 180-184: The README example for running
./deploy-scripts/deploy-clm.sh shows a broken multi-line shell command: the line
with "--namespace pr-123" is missing a trailing backslash, causing the next
option lines to be treated as a separate command; update the example so each
continued line ends with a backslash (e.g., add a "\" after "--namespace
pr-123") so the full invocation of deploy-clm.sh with --action, --namespace,
--api-image-repo and --api-image-tag is executed as one multi-line command.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 6699e988-d74b-4d63-87a0-2b50afe0694b
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy-scripts/.env.example`:
- Line 12: The NAMESPACE export uses unguarded $USER expansion which can trigger
an "unbound variable" under set -u; update the export for NAMESPACE (the line
that currently reads export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$USER}") to
use safe parameter expansion such as export
NAMESPACE="${NAMESPACE:-hyperfleet-e2e-${USER:-unknown}}" (or ${USER:-} if you
prefer empty) so that missing USER won’t abort deploy-clm.sh.
In `@deploy-scripts/deploy-clm.sh`:
- Around line 360-366: The script currently only checks NAMESPACE presence; add
DNS-1123 format validation (lowercase alphanumerics and hyphens, no
leading/trailing hyphen, max 63 chars) right after the existing empty check. In
the block around the NAMESPACE check, validate length (<=63) and match the regex
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$; if it fails, call log_error with a clear
message about invalid namespace format and then print_usage and exit 1 (same
flow as the empty-case). Reference: NAMESPACE variable and existing
log_error/print_usage usage in deploy-clm.sh.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 357fd5ee-da38-4294-8f22-f05f7ff8e918
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
✅ Files skipped from review due to trivial changes (1)
- deploy-scripts/README.md
| ./deploy-scripts/deploy-clm.sh --action install \ | ||
| --namespace dev-test \ | ||
| --namespace <unique_namespace> |
There was a problem hiding this comment.
Missing trailing backslash. This breaks the multi-line shell example. The image repo and tag lines would be interpreted as a separate command instead of arguments to deploy-clm.sh.
| # If NAMESPACE is not set, uses a default value of hyperfleet-e2e-$USER | ||
| # Ideally, if you continue to see Pub/Sub conflicts, you can pass in the | ||
| # --namespace flag to the deploy-clm.sh script. | ||
| export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$USER}" |
There was a problem hiding this comment.
$USER can contain uppercase characters on some systems, which would produce an invalid Kubernetes namespace (DNS-1123 requires lowercase). Consider lowercasing it here or adding DNS-1123 validation in the script to catch this early.
There was a problem hiding this comment.
Addressed and added a check for namespace names that must be valid RFC 1123 DNS labels.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deploy-scripts/.env.example (1)
12-12: ⚡ Quick winSanitize and bound the
USERsuffix before composingNAMESPACE.Line 12 lowercases
USER, but it still allows characters/length that fail the DNS-1123 check indeploy-scripts/deploy-clm.sh(Line 368). This can make the default.envvalue fail on first run for valid system usernames likejohn_doeor long IDs.Suggested patch
-export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$(echo ${USER:-default} | tr '[:upper:]' '[:lower:]')}" +export NAMESPACE="${NAMESPACE:-hyperfleet-e2e-$(printf '%s' "${USER:-default}" | tr '[:upper:]' '[:lower:]' | sed -E 's/[^a-z0-9]+/-/g; s/^-+|-+$//g; s/^$/default/; s/^(.{48}).*$/\1/')}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy-scripts/.env.example` at line 12, Sanitize and bound the USER suffix used to build NAMESPACE: when exporting NAMESPACE (the export NAMESPACE= line), lower-case the USER, replace any chars not in [a-z0-9-] with '-', strip leading/trailing '-' and enforce a maximum suffix length so the full name (prefix "hyperfleet-e2e-") stays within the DNS-1123 63-char label limit (i.e., cap the suffix to 63 minus the prefix length); ensure an empty result falls back to "default"; apply this change in the .env.example export NAMESPACE line and verify it satisfies the DNS-1123 check used in deploy-clm.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deploy-scripts/.env.example`:
- Line 12: Sanitize and bound the USER suffix used to build NAMESPACE: when
exporting NAMESPACE (the export NAMESPACE= line), lower-case the USER, replace
any chars not in [a-z0-9-] with '-', strip leading/trailing '-' and enforce a
maximum suffix length so the full name (prefix "hyperfleet-e2e-") stays within
the DNS-1123 63-char label limit (i.e., cap the suffix to 63 minus the prefix
length); ensure an empty result falls back to "default"; apply this change in
the .env.example export NAMESPACE line and verify it satisfies the DNS-1123
check used in deploy-clm.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 85cd9844-f0f0-4082-9f38-9899c6b94688
📒 Files selected for processing (3)
deploy-scripts/.env.exampledeploy-scripts/README.mddeploy-scripts/deploy-clm.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy-scripts/README.md
| 3. **Run the deployment:** | ||
| ```bash | ||
| ./deploy-clm.sh --action install | ||
| ./deploy-clm.sh --action install --namespace hyperfleet-e2e-$USER |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
Two small doc issues with the namespace examples:
-
The README mixes
<unique_namespace>(placeholder) andhyperfleet-e2e-$USER(literal) without a consistent rule. For example, Quick Start install uses the placeholder but Quick Start uninstall uses the literal. Consider standardizing — either use<unique_namespace>everywhere with a note explaining the convention, or usehyperfleet-e2e-$USEReverywhere for copy-pasteable examples. -
Option 2 step 3 (line 87) adds
--namespace hyperfleet-e2e-$USERto the .env-based workflow, but the whole point of Option 2 is that NAMESPACE is configured in.env. The--namespaceCLI flag overrides the.envvalue, making step 2's config irrelevant. The script's own help text shows the correct pattern:
# Install with .env defaults
deploy-clm.sh --action installConsider reverting line 87 to just ./deploy-clm.sh --action install (no --namespace) to match the .env-based intent.
Impact warningsThe following files are NOT in this PR but still reference the old hardcoded
These are outside the PR scope — just flagging for awareness so the migration away from the hardcoded namespace is complete. |
3 participants
Summary
Removes hardcoded default namespace of
hyperfleet-e2ein deploy-clm.sh to enforce that users must set NAMESPACE via .env or --namespace flag. This prevents GCP Pub/Sub topic/subscription collisions when multiple users run tests concurrently.Changes
Test Plan
deploy-clm.sh scripts successfulmake testpassesmake e2epassesmake lintpassesJira Issue
Summary by CodeRabbit