feat(aws): add ---compute-families flag to filter compute selector output - #872
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an optional AWS compute-family allowlist to compute request arguments. AWS instance types use exact family-prefix matching. CLI and Tekton create commands pass the allowlist only when ChangesInstance-family filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TektonTask
participant MaptCLI
participant ComputeRequest
participant AWSInstanceSelector
TektonTask->>MaptCLI: Pass --compute-families during create
MaptCLI->>ComputeRequest: Populate ComputeFamilies
ComputeRequest->>AWSInstanceSelector: Request instance types
AWSInstanceSelector->>AWSInstanceSelector: Filter exact family prefixes
AWSInstanceSelector-->>ComputeRequest: Return filtered types capped by MaxResults
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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: 2
🤖 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 `@pkg/provider/aws/data/compute-request.go`:
- Line 52: Update the compute request filtering flow around filterByFamily so
InstanceFamilies is applied before MaxResults truncates selector results. When
args.InstanceFamilies is provided, avoid the premature cap or expand it
sufficiently while filtering the allowlisted families, then apply the requested
result limit to the filtered set.
In `@tkn/template/infra-aws-ocp-snc.yaml`:
- Around line 268-270: Stop interpolating the externally supplied
instance-families value into the eval-based command construction; build the
command as an argument list and invoke it directly with safe quoting or a Bash
array. Apply the fix to tkn/template/infra-aws-ocp-snc.yaml lines 268-270, then
regenerate or apply the equivalent change to tkn/infra-aws-ocp-snc.yaml lines
268-270; make the same change in tkn/template/infra-aws-rhel.yaml lines 283-285
and tkn/infra-aws-rhel.yaml lines 283-285.
🪄 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: Pro Plus
Run ID: b3182e54-14b7-4474-af4a-489a865a6fdd
📒 Files selected for processing (8)
cmd/mapt/cmd/params/params.gopkg/provider/api/compute-request/compute-request.gopkg/provider/aws/data/compute-request.gopkg/provider/aws/data/compute-request_test.gotkn/infra-aws-ocp-snc.yamltkn/infra-aws-rhel.yamltkn/template/infra-aws-ocp-snc.yamltkn/template/infra-aws-rhel.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tkn/infra-aws-ocp-snc.yaml (1)
274-276: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not interpolate these parameters into an
evalcommand.The assembled command is executed with
evalat Line 325.operator-channelis inserted unquoted, allowing values such aspackage=stable; ...to execute arbitrary shell commands; an apostrophe or command substitution ininstance-familiescan similarly escape its wrapper. Because the task loads AWS credentials, this can expose credentials or alter provisioning.Build the command as a Bash argument array and invoke it without
eval, or strictly validate and shell-escape both parameters before appending them; also reject malformedpackage=channelentries.As per path instructions, focus on major issues impacting security and avoid nitpicks and verbosity.
Also applies to: 294-299
🤖 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 `@tkn/infra-aws-ocp-snc.yaml` around lines 274 - 276, Replace the eval-based command construction with a Bash argument array and invoke it directly, preserving each parameter as a separate argument. Safely pass params.instance-families and operator-channel without interpolation, and validate operator-channel as a well-formed package=channel entry before execution. Ensure malformed or unexpected values are rejected before the AWS provisioning command runs.Source: Path instructions
🤖 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.
Outside diff comments:
In `@tkn/infra-aws-ocp-snc.yaml`:
- Around line 274-276: Replace the eval-based command construction with a Bash
argument array and invoke it directly, preserving each parameter as a separate
argument. Safely pass params.instance-families and operator-channel without
interpolation, and validate operator-channel as a well-formed package=channel
entry before execution. Ensure malformed or unexpected values are rejected
before the AWS provisioning command runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27bc3a1f-0472-444d-8cfe-c8c48cdba136
📒 Files selected for processing (1)
tkn/infra-aws-ocp-snc.yaml
ppitonak
left a comment
ppitonak
left a comment
There was a problem hiding this comment.
Any reason not to update other infra-aws-* Tekton tasks?
|
I tested spot instances and it seems to work fine. |
done! |
| nestedVirtDesc string = "Use cloud instance that has nested virtualization support" | ||
| computeSizes string = "compute-sizes" | ||
| computeSizesDesc string = "Comma seperated list of sizes for the machines to be requested. If set this takes precedence over compute by args" | ||
| instanceFamilies string = "instance-families" |
There was a problem hiding this comment.
Can we name it computeFamilies as this may / should have a matching functionality in azure?
There was a problem hiding this comment.
done.but i have not tested azure with the new code.not sure i have access.
|
Aso rebase and fix conflicts |
…ibility Rename all references from instanceFamilies/InstanceFamilies to computeFamilies/ComputeFamilies throughout codebase to support Azure VM family naming (e.g. StandardD8v3Family). Changes: - pkg/provider/api/compute-request: ComputeFamilies field with AWS/Azure docs - cmd/mapt/cmd/params: --compute-families flag and viper binding - pkg/provider/aws/data: use ComputeFamilies in selector and filter - tkn/template: compute-families param in SNC and RHEL tasks - Regenerated tkn/*.yaml Per @adrianriobo review comment on PR redhat-developer#872. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
amastbau
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/provider/aws/data/compute-request_test.go`:
- Around line 58-79: Update TestFilterByFamily_CapAppliedAfterFilter to exercise
the production filter-and-cap path through getInstanceTypes, using a stubbed
selector or equivalent test setup. Configure computerequest.MaxResults and
assert that matching m6i values beyond the cap remain available, rather than
testing filterByFamily directly.
In `@tkn/infra-aws-ocp-snc.yaml`:
- Around line 268-270: Replace shared eval-based command construction at
tkn/infra-aws-ocp-snc.yaml lines 268-270, tkn/infra-aws-rhel.yaml lines 283-285,
and tkn/template/infra-aws-rhel.yaml lines 283-285 with argument-vector
handling: pass compute-families as a separate data argument, validate each
family token before execution, and preserve omission when the value is empty.
Apply the same change consistently in all three files.
🪄 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: Pro Plus
Run ID: ab517f8b-016f-4e79-a9b3-5e51d1c2f644
📒 Files selected for processing (8)
cmd/mapt/cmd/params/params.gopkg/provider/api/compute-request/compute-request.gopkg/provider/aws/data/compute-request.gopkg/provider/aws/data/compute-request_test.gotkn/infra-aws-ocp-snc.yamltkn/infra-aws-rhel.yamltkn/template/infra-aws-ocp-snc.yamltkn/template/infra-aws-rhel.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tkn/template/infra-aws-ocp-snc.yaml
- pkg/provider/aws/data/compute-request.go
- cmd/mapt/cmd/params/params.go
Replace string concatenation + eval with Bash array (cmd=(...); "${cmd[@]}")
in infra-aws-rhel and infra-aws-ocp-snc tasks (templates and generated).
Eliminates shell injection risk when externally supplied Tekton params
(compute-families, spot-excluded-regions, etc.) contain shell metacharacters.
Addresses CodeRabbit critical finding on PR redhat-developer#872; tracked in issue redhat-developer#874.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@adrianriobo Addressed the
FixReplaced string concatenation + # Before (vulnerable)
cmd="mapt aws rhel $(params.operation) "
cmd+="--compute-families '$(params.compute-families)' "
eval "${cmd}"
# After (safe)
cmd=(mapt aws rhel "$(params.operation)")
cmd+=(--compute-families "$(params.compute-families)")
"${cmd[@]}"Each param value is now a discrete array element — the shell never re-parses it as syntax. Applies to all 4 files: E2E Verification (2026-08-02)Full provision + verify + destroy on live AWS: Instance created: Resolves the CodeRabbit critical finding. The separate |
amastbau
changed the title
|
@amastbau is showing conflicts, fix as LGTM so I can merge |
Post-filters getInstanceTypes() output to only instance types whose family prefix matches the allowlist. Bypassed when ComputeSizes is set. Fixes: redhat-developer#684
Comma-separated allowlist of AWS family prefixes (e.g. m5,m6i,m7i). Post-filters instance selector output. No-op when --compute-sizes is set.
Passes --instance-families to mapt when set. Conditional: only in the else branch (when compute-sizes is empty) since compute-sizes bypasses the selector entirely.
When InstanceFamilies is set, skip MaxResults in the selector so allowlisted families ranked outside the top 20 are not silently dropped. Cap to MaxResults manually after filterByFamily.
Verifies that allowlisted families ranked outside the top MaxResults are not silently dropped when InstanceFamilies is set.
…ibility Rename all references from instanceFamilies/InstanceFamilies to computeFamilies/ComputeFamilies throughout codebase to support Azure VM family naming (e.g. StandardD8v3Family). Changes: - pkg/provider/api/compute-request: ComputeFamilies field with AWS/Azure docs - cmd/mapt/cmd/params: --compute-families flag and viper binding - pkg/provider/aws/data: use ComputeFamilies in selector and filter - tkn/template: compute-families param in SNC and RHEL tasks - Regenerated tkn/*.yaml Per @adrianriobo review comment on PR redhat-developer#872. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace string concatenation + eval with Bash array (cmd=(...); "${cmd[@]}")
in infra-aws-rhel and infra-aws-ocp-snc tasks (templates and generated).
Eliminates shell injection risk when externally supplied Tekton params
(compute-families, spot-excluded-regions, etc.) contain shell metacharacters.
Addresses CodeRabbit critical finding on PR redhat-developer#872; tracked in issue redhat-developer#874.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
7ea2467 to
dd734a9
Compare
Add TestFilters_MaxResultsOmittedWhenFamiliesSet and TestFilters_MaxResultsSetWhenNoFamilies to exercise the production filters() code path directly, verifying that MaxResults is omitted from the selector call when ComputeFamilies is set (cap applied after filterByFamily) and set normally when no family filtering is active. Addresses CodeRabbit minor finding on PR redhat-developer#872. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
dd734a9 to
fcdc056
Compare
Regenerated tkn/infra-aws-ocp-snc.yaml from template to restore operator-channel param definition lost during rebase cleanup. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Changes will be made in follow up
3 participants
Summary
Adds `--compute-families` flag (comma-separated allowlist of family prefixes, e.g. `m5,m6i,m7i` for AWS) that post-filters the compute selector output to only matching families.
Fixes #684
Problem
When using `--cpus` and `--memory`, the instance selector returns up to 20 types including expensive specialized families (d3en, p3, x1e — dense storage, GPU, high-memory) alongside cheap general-purpose ones. Spot picks the cheapest at that moment, which may be a temporarily cheap specialized instance. Example: `d3en.12xlarge` and `m5a.12xlarge` have identical vCPU/Memory but the former is ~3x more expensive.
Changes
Behavior
Cross-cloud naming
Cap fix
When `ComputeFamilies` is set, `MaxResults` is no longer passed to `FilterVerbose`. Without this fix, the selector could return 20 results all outside the allowlist (e.g. GPU/storage types ranked highest), leaving nothing after `filterByFamily` even though matching types exist beyond position 20. The cap is now applied after filtering.
Verification (2026-08-10)
Unit tests — 10/10 pass
Full regression suite — `make test` — 8 packages, 0 failures
E2E — RHEL (mapt aws rhel create)
E2E — SNC (mapt aws openshift-snc create)
mapt aws openshift-snc create --cpus 8 --memory 32 --compute-families m5,m6i \ --pull-secret-file ~/pull-secret.json \ --project-name test-snc-e2e --backed-url file:///tmp/mapt-snc-stateComparison (no filter vs with filter)
Without `--compute-families` (4 vCPU / 16 GiB): 20 types including d3en, g4ad, g4dn, g5, g6, inf2, m4, m5, m5a, m5d, m6a, m6i...
With `--compute-families m5,m6i`: `m5.xlarge`, `m6i.xlarge` only.
🤖 Generated with Claude Code