SREP-3820: Add IAM policy simulation to release validation pipeline#871
SREP-3820: Add IAM policy simulation to release validation pipeline#871ratnam915 wants to merge 4 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ratnam915: This pull request references SREP-3820 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@ratnam915: This pull request references SREP-3820 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ratnam915 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 |
![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
🧹 Nitpick comments (3)
cmd/iampermissions/simulate.go (2)
381-391: Omit redundant type declaration.The explicit
io.Writertype can be inferred from the assignment.♻️ Proposed fix
- var w io.Writer = o.outputWriter + w := o.outputWriter if o.OutputFile != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/iampermissions/simulate.go` around lines 381 - 391, In simulateOptions.writeOutput, remove the redundant explicit type on the w declaration: instead of "var w io.Writer = o.outputWriter" simply initialize w from o.outputWriter (e.g., "w := o.outputWriter" or "var w = o.outputWriter"); keep the rest of the logic that reassigns w when OutputFile is set and ensure defer f.Close() and error handling around os.Create remain unchanged.
354-379:convertPolicyConditionToContextsilently ignores unsupported value types.When
valis neitherstringnor[]interface{}, the value is silently dropped with an emptyValuesslice. Consider logging a warning or returning an error for unexpected types.♻️ Proposed improvement
func convertPolicyConditionToContext(condition cco.IAMPolicyCondition) map[string]policies.ContextKeyDef { contextDefs := make(map[string]policies.ContextKeyDef) for _, keyValues := range condition { for key, val := range keyValues { def := policies.ContextKeyDef{ Type: "string", } switch v := val.(type) { case string: def.Values = []string{v} case []interface{}: for _, item := range v { if s, ok := item.(string); ok { def.Values = append(def.Values, s) } } + default: + fmt.Fprintf(os.Stderr, "Warning: unsupported condition value type for key %s: %T\n", key, val) + continue } contextDefs[key] = def } } return contextDefs }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/iampermissions/simulate.go` around lines 354 - 379, The convertPolicyConditionToContext function currently drops unsupported value types silently; update the switch over val in convertPolicyConditionToContext to handle the default case by emitting a warning that includes the affected key and the actual type (use reflect.TypeOf(val)) and skip adding invalid entries to contextDefs (or leave Values nil), and also log when a []interface{} contains non-string items (include key and offending item type); reference the function name convertPolicyConditionToContext, variables val and key, and the policies.ContextKeyDef/contextDefs map so you add the warning path without changing the function signature.pkg/policies/report.go (1)
12-62: Consider handling the tabwriter Flush error.
tw.Flush()at line 43 returns an error that is currently ignored. While rare in practice, write failures could occur when outputting to files or network streams.♻️ Proposed fix to return error
Change the method signature to return an error:
-func (r *SimulationReport) FormatTable(w io.Writer) { +func (r *SimulationReport) FormatTable(w io.Writer) error { tw := tabwriter.NewWriter(w, 0, 4, 2, ' ', 0) // ... existing code ... - tw.Flush() + if err := tw.Flush(); err != nil { + return err + } // ... rest of output ... + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/policies/report.go` around lines 12 - 62, The tabwriter Flush error is ignored in SimulationReport.FormatTable; change the method signature of SimulationReport.FormatTable to return error, capture the result of tw.Flush() (e.g., err := tw.Flush()), and if non-nil wrap/return it (using fmt.Errorf or fmt package) so write failures propagate; ensure the function returns nil on success and update all callers to handle the returned error accordingly (references: SimulationReport.FormatTable, tw.Flush, io.Writer w).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/iampermissions/simulate.go`:
- Around line 258-268: The temp directory returned by o.downloadFunc in
runCredentialsRequestSimulations is never removed; after obtaining dir you
should schedule its cleanup (e.g., call os.RemoveAll(dir)) to avoid accumulating
temp files—add a defer to remove the directory immediately after download
succeeds (check dir is non-empty) and optionally log or wrap any RemoveAll
error; keep the defer placed before further processing (before calling
policies.ParseCredentialsRequestsInDir) so dir is always cleaned up when the
function returns.
---
Nitpick comments:
In `@cmd/iampermissions/simulate.go`:
- Around line 381-391: In simulateOptions.writeOutput, remove the redundant
explicit type on the w declaration: instead of "var w io.Writer =
o.outputWriter" simply initialize w from o.outputWriter (e.g., "w :=
o.outputWriter" or "var w = o.outputWriter"); keep the rest of the logic that
reassigns w when OutputFile is set and ensure defer f.Close() and error handling
around os.Create remain unchanged.
- Around line 354-379: The convertPolicyConditionToContext function currently
drops unsupported value types silently; update the switch over val in
convertPolicyConditionToContext to handle the default case by emitting a warning
that includes the affected key and the actual type (use reflect.TypeOf(val)) and
skip adding invalid entries to contextDefs (or leave Values nil), and also log
when a []interface{} contains non-string items (include key and offending item
type); reference the function name convertPolicyConditionToContext, variables
val and key, and the policies.ContextKeyDef/contextDefs map so you add the
warning path without changing the function signature.
In `@pkg/policies/report.go`:
- Around line 12-62: The tabwriter Flush error is ignored in
SimulationReport.FormatTable; change the method signature of
SimulationReport.FormatTable to return error, capture the result of tw.Flush()
(e.g., err := tw.Flush()), and if non-nil wrap/return it (using fmt.Errorf or
fmt package) so write failures propagate; ensure the function returns nil on
success and update all callers to handle the returned error accordingly
(references: SimulationReport.FormatTable, tw.Flush, io.Writer w).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb8e9ccf-adbc-4a57-8164-232312e76ad0
📒 Files selected for processing (7)
cmd/iampermissions/cmd.gocmd/iampermissions/simulate.gopkg/policies/report.gopkg/policies/simulate.gopkg/policies/simulate_test.gopkg/policies/testdata/manifests/ebs-csi-driver.yamlpkg/policies/testdata/policies/ROSAAmazonEBSCSIDriverOperatorPolicy.json
| func (o *simulateOptions) runCredentialsRequestSimulations(ctx context.Context, client policies.IAMSimulator, policyDocs map[string]string) ([]*policies.SimulationReport, error) { | ||
| fmt.Fprintf(os.Stderr, "Downloading CredentialsRequests for %s\n", o.ReleaseVersion) | ||
| dir, err := o.downloadFunc(o.ReleaseVersion, o.Cloud) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to download CredentialsRequests: %w", err) | ||
| } | ||
|
|
||
| crs, err := policies.ParseCredentialsRequestsInDir(dir) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse CredentialsRequests: %w", err) | ||
| } |
There was a problem hiding this comment.
Temporary directory is not cleaned up after use.
The downloaded CredentialsRequest files are stored in a temp directory (line 260), but this directory is never removed after processing completes. This will accumulate temporary files over repeated invocations.
🧹 Proposed fix to clean up temp directory
func (o *simulateOptions) runCredentialsRequestSimulations(ctx context.Context, client policies.IAMSimulator, policyDocs map[string]string) ([]*policies.SimulationReport, error) {
fmt.Fprintf(os.Stderr, "Downloading CredentialsRequests for %s\n", o.ReleaseVersion)
dir, err := o.downloadFunc(o.ReleaseVersion, o.Cloud)
if err != nil {
return nil, fmt.Errorf("failed to download CredentialsRequests: %w", err)
}
+ defer os.RemoveAll(dir)
crs, err := policies.ParseCredentialsRequestsInDir(dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (o *simulateOptions) runCredentialsRequestSimulations(ctx context.Context, client policies.IAMSimulator, policyDocs map[string]string) ([]*policies.SimulationReport, error) { | |
| fmt.Fprintf(os.Stderr, "Downloading CredentialsRequests for %s\n", o.ReleaseVersion) | |
| dir, err := o.downloadFunc(o.ReleaseVersion, o.Cloud) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to download CredentialsRequests: %w", err) | |
| } | |
| crs, err := policies.ParseCredentialsRequestsInDir(dir) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse CredentialsRequests: %w", err) | |
| } | |
| func (o *simulateOptions) runCredentialsRequestSimulations(ctx context.Context, client policies.IAMSimulator, policyDocs map[string]string) ([]*policies.SimulationReport, error) { | |
| fmt.Fprintf(os.Stderr, "Downloading CredentialsRequests for %s\n", o.ReleaseVersion) | |
| dir, err := o.downloadFunc(o.ReleaseVersion, o.Cloud) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to download CredentialsRequests: %w", err) | |
| } | |
| defer os.RemoveAll(dir) | |
| crs, err := policies.ParseCredentialsRequestsInDir(dir) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse CredentialsRequests: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/iampermissions/simulate.go` around lines 258 - 268, The temp directory
returned by o.downloadFunc in runCredentialsRequestSimulations is never removed;
after obtaining dir you should schedule its cleanup (e.g., call
os.RemoveAll(dir)) to avoid accumulating temp files—add a defer to remove the
directory immediately after download succeeds (check dir is non-empty) and
optionally log or wrap any RemoveAll error; keep the defer placed before further
processing (before calling policies.ParseCredentialsRequestsInDir) so dir is
always cleaned up when the function returns.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/iampermissions/simulate.go`:
- Around line 303-314: The code in simulate.go incorrectly assigns a single
loaded policy to every CredentialsRequest when len(policyDocs) == 1 and silently
skips unmatched requests when multiple policies exist; update the matching logic
in the loop that sets policyJSON (referencing policyDocs, cr, policyJSON, and
CredentialsRequest) to require an explicit policy-to-CredentialsRequest mapping
in release mode: if only one policy is loaded, treat it as ambiguous and emit an
error or require an explicit match instead of reusing it; when multiple policies
are loaded, fail and report an error for any CredentialsRequest with no exact
key in policyDocs (do not silently continue), ensuring the function returns a
non-zero exit or error so release validation cannot pass with implicit or
missing mappings.
- Around line 58-62: The example usage for the "osdctl iampermissions simulate"
command references the wrong manifest file name; update the Examples block in
the simulate command help text so the manifest file is "ebs-csi-driver.yaml"
instead of "ebs-csi-driver-scenarios.yaml" (i.e., edit the example shown for
osdctl iampermissions simulate in simulate.go to use --manifest-file
./ebs-csi-driver.yaml).
- Around line 102-117: The run method in simulateOptions accepts non-AWS --cloud
values but always creates an AWS IAM client; add an upfront validation in
simulateOptions.run (before creating the IAM client and before any early
returns) that rejects any --cloud value other than "" or "aws" with a clear
error (e.g., return fmt.Errorf("unsupported cloud provider %q: only 'aws' is
supported", o.Cloud)). Update the same validation point that currently allows
gcp/wif to fall through to the skip path so the command fails fast and clearly
instead of later returning "no simulations were run".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d910e29-afd3-49fc-82f0-617b754c94c3
📒 Files selected for processing (5)
cmd/iampermissions/simulate.godocs/README.mddocs/osdctl_iampermissions.mddocs/osdctl_iampermissions_simulate.mdpkg/policies/simulate.go
✅ Files skipped from review due to trivial changes (1)
- docs/osdctl_iampermissions.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/policies/simulate.go
| func (o *simulateOptions) run() error { | ||
| ctx := context.Background() | ||
|
|
||
| // Validate inputs | ||
| if o.PolicyFile == "" && o.PolicyDir == "" { | ||
| return fmt.Errorf("one of --policy-file or --policy-dir is required") | ||
| } | ||
| if o.ManifestFile == "" && o.ManifestDir == "" && o.ReleaseVersion == "" { | ||
| return fmt.Errorf("one of --manifest-file, --manifest-dir, or --release-version is required") | ||
| } | ||
|
|
||
| // Create IAM client | ||
| client, err := o.iamClientFunc(ctx, o.Region) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create IAM client: %w", err) | ||
| } |
There was a problem hiding this comment.
Reject non-AWS --cloud values up front.
simulate always creates an AWS IAM client and only understands AWS provider specs, but the inherited flag still advertises gcp and wif. In release mode those values just fall into the skip path at Line 275 and end with a confusing no simulations were run error instead of a clear validation failure.
Also applies to: 273-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/iampermissions/simulate.go` around lines 102 - 117, The run method in
simulateOptions accepts non-AWS --cloud values but always creates an AWS IAM
client; add an upfront validation in simulateOptions.run (before creating the
IAM client and before any early returns) that rejects any --cloud value other
than "" or "aws" with a clear error (e.g., return fmt.Errorf("unsupported cloud
provider %q: only 'aws' is supported", o.Cloud)). Update the same validation
point that currently allows gcp/wif to fall through to the skip path so the
command fails fast and clearly instead of later returning "no simulations were
run".
|
/test format |
|
@ratnam915: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
This PR introduces the osdctl iampermissions simulate subcommand to validate ROSA-managed IAM policies against OCP component requirements using the AWS SimulateCustomPolicy API.
Motivation (CFE-1131): The EBS CSI driver calls ec2:CreateTags on existing volumes for day-2 tag reconciliation. However, the ROSAAmazonEBSCSIDriverOperatorPolicy only permits CreateTags when the ec2:CreateAction condition is present during CreateVolume or CreateSnapshot. The existing osdctl iampermissions diff command cannot catch this regression because it compares CredentialsRequest manifests rather than condition-key behaviors.
What Changed
New Subcommand: Added osdctl iampermissions simulate.
Policy Validation: Validates IAM policy JSON against test manifest YAML scenarios.
Condition Key Support: Evaluates contexts like ec2:CreateAction, aws:ResourceTag/*, etc.
Versatile Output: Supports table, JSON, and JUnit XML output formats.
CredentialsRequest Integration: Enables validation via the --release-version flag.
New Test Assets: Added an EBS CSI driver test manifest covering 17 scenarios and bundled policy JSON (v6).
Unit Testing: Included 10 unit tests utilizing a mock IAM client, with zero regressions on existing tests.
Example Usage:
osdctl iampermissions simulate
--policy-file ./ROSAAmazonEBSCSIDriverOperatorPolicy.json
--manifest-file ./ebs-csi-driver.yaml
Test Plan
[x] 10/10 unit tests pass (mock IAM client).
[x] 5/5 existing iampermissions tests pass (no regressions).
[x] Integration tested against bundled policy JSON (15/17 pass, 2 FAIL—correctly catching CFE-1131).
[x] Integration tested against live ROSAAmazonEBSCSIDriverOperatorPolicy (v6) from AWS (identical results).
[x] All 3 output formats validated (table, JSON, JUnit XML).
[x] CLI error handling verified (missing flags, invalid JSON, non-zero exit on failures).
References
Ticket: SREP-3820
CFE-1131: EBS CSI driver CreateTags AccessDenied
Release Repo PR: (link)
AWS Docs: ROSAAmazonEBSCSIDriverOperatorPolicy