feat: latest skill refactor #17
Conversation
📝 WalkthroughWalkthroughRename package to aws-irsa, refactor CRD/specs to nested role/policy/serviceAccount objects, replace flat templates with a state-driven render pipeline, add observed-resource test mocks and expanded tests, and update CI/workflows, Helm packaging, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Composite as Composite Resource
participant RenderFn as Render Functions (pipeline)
participant Crossplane as Crossplane (Role/Policy/RPA)
participant Observed as Observed Resources
participant Status as Composite Status
Composite->>RenderFn: Provide input spec (raw)
RenderFn->>RenderFn: 000-state-init -> build state.effective/spec
RenderFn->>Crossplane: Emit Role/Policy/RolePolicyAttachment (conditional)
Crossplane-->>Observed: Resources reach Ready/Synced (observed manifests)
Observed->>RenderFn: Feed observed resources back
RenderFn->>RenderFn: Extract observed -> update state.observed
RenderFn->>RenderFn: Assemble state.irsa and compute state.status
RenderFn->>Status: Emit composite status (ready, ARNs, ids)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
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 |
Published Crossplane PackageThe following Crossplane package was published as part of this PR: Package: ghcr.io/hops-ops/aws-irsa:pr-17-ba6a3eff0c7f77c247cc3c9ab2f79db50908ec8e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @apis/irsas/definition.yaml:
- Around line 50-68: Remove the duplicate "type: object" under the "policy"
mapping — the schema already declares "policy: type: object" at the top (line
with the first type) so delete the second occurrence (the trailing "type:
object" after the required list) to avoid duplicate keys; keep the properties
(document, namePrefix, nameOverride, externalName) and the required: - document
block intact under the "policy" mapping.
- Around line 81-94: The schema for the "role" object contains a duplicated
"type: object" entry; remove the trailing duplicate "type: object" (the second
occurrence after the properties block) so the "role" definition only declares
type once, keeping the single "type: object" that opens the "role" object and
leaving properties (namePrefix, nameOverride, externalName) intact.
- Around line 69-80: The schema for providerConfigRef has a duplicated "type:
object" key that will break YAML parsing; remove the extra occurrence so the
object type is declared only once for providerConfigRef (keep the initial "type:
object" above the description and delete the trailing "type: object" after the
properties block), ensuring properties (name, kind) remain nested under
providerConfigRef and the default for kind is preserved.
In @functions/render/007-state-irsa-policy.yaml.gotmpl:
- Around line 1-4: The template currently forces rendering by setting $render to
true; change it to set $state.irsa.policy.render (or the local $render) based on
whether a policy document was actually provided (e.g., test that
$state.irsa.policy.document is non-empty or non-blank) so the flag matches
200-policy.yaml.gotmpl’s conditional rendering; keep the existing $eff :=
$state.spec.effective assignment and replace the hardcoded true with a
conditional expression that evaluates the policy document presence.
In @functions/render/008-state-irsa-attachment.yaml.gotmpl:
- Line 6: The comment "{{- /* Set irsa.attachment slice */ -}}" is inaccurate
because the template creates an object/dict, not a slice; update the comment to
reflect that (e.g., "Set irsa.attachment map/object" or "Set irsa.attachment
dict") so it correctly documents the structure produced by the template.
In @functions/render/100-role.yaml.gotmpl:
- Around line 16-17: The assumeRolePolicy literal block can emit a leading blank
line because the template pipes the raw value into nindent; trim the value
before indenting to remove leading/trailing newlines and whitespace so the
rendered block has no initial empty line (update the pipeline that feeds
assumeRolePolicy, i.e. the expression using $state.irsa.role.assumeRolePolicy
and nindent, to apply a trim function before nindent).
🧹 Nitpick comments (4)
functions/render/999-status.yaml.gotmpl (1)
7-29: Guard against<no value>strings in status output.If any
$s.*field is unset, Go templates often render<no value>into the YAML (e.g.,"{{ $s.accountId }}"). Consider defaulting/omitting optional fields (especially ARNs) to avoid leaking placeholder strings into status.Makefile (1)
28-55: Consider adding atrapto clean uptmpdiron early exit/interrupt.Right now
tmpdiris removed only on the happy-path end of the target.Possible patch
render\:all: - @tmpdir=$$(mktemp -d); \ + @tmpdir=$$(mktemp -d); \ + trap 'rm -rf "$$tmpdir"' EXIT INT TERM; \ pids=""; \ for entry in $(EXAMPLES); do \Also applies to: 58-90
functions/render/010-state-status.yaml.gotmpl (1)
15-28: Status “name” vs “arn Pending” consistency.You always publish
role.name/policy.namefrom$state.irsa.*, but publish"Pending"ARNs when missing. That’s fine, but make sure downstream consumers don’t treat"Pending"as a real ARN value (stringly-typed footgun).tests/test-render/main.k (1)
143-221: Consider adding a label override conflict test.Tests 4-5 validate label merging but don't test what happens when user labels conflict with default labels (e.g., user provides
"hops.ops.com.ai/managed": "false"). Based on the earlier merge order observation, this edge case should be tested to confirm expected behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.github/workflows/on-pr.yaml.github/workflows/on-push-main.yaml.gitops/deploy/Chart.yaml.gitops/deploy/templates/config.yaml.gitops/deploy/values.yamlAGENTS.mdMakefileREADME.mdapis/irsas/composition.yamlapis/irsas/configuration.yamlapis/irsas/definition.yamlexamples/irsas/minimal.yamlexamples/irsas/standard.yamlexamples/test/mocks/observed-resources/standard/steps/1/policy.yamlexamples/test/mocks/observed-resources/standard/steps/1/role.yamlexamples/test/mocks/observed-resources/standard/steps/2/attachment.yamlexamples/test/mocks/observed-resources/standard/steps/2/policy.yamlexamples/test/mocks/observed-resources/standard/steps/2/role.yamlfunctions/render/00-desired-values.yaml.gotmplfunctions/render/000-state-init.yaml.gotmplfunctions/render/001-state-observed-role.yaml.gotmplfunctions/render/002-state-observed-policy.yaml.gotmplfunctions/render/003-state-observed-attachment.yaml.gotmplfunctions/render/005-state-irsa.yaml.gotmplfunctions/render/006-state-irsa-role.yaml.gotmplfunctions/render/007-state-irsa-policy.yaml.gotmplfunctions/render/008-state-irsa-attachment.yaml.gotmplfunctions/render/010-state-status.yaml.gotmplfunctions/render/10-observed-values.yaml.gotmplfunctions/render/100-role.yaml.gotmplfunctions/render/20-role.yaml.gotmplfunctions/render/200-policy.yaml.gotmplfunctions/render/21-policy.yaml.gotmplfunctions/render/22-role-policy-attachment.yaml.gotmplfunctions/render/300-attachment.yaml.gotmplfunctions/render/99-status.yaml.gotmplfunctions/render/999-status.yaml.gotmpltests/e2etest-irsa/main.ktests/test-render/main.kupbound.yaml
💤 Files with no reviewable changes (7)
- AGENTS.md
- functions/render/99-status.yaml.gotmpl
- functions/render/21-policy.yaml.gotmpl
- functions/render/00-desired-values.yaml.gotmpl
- functions/render/22-role-policy-attachment.yaml.gotmpl
- functions/render/10-observed-values.yaml.gotmpl
- functions/render/20-role.yaml.gotmpl
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~9-~9: To elevate your writing, try using a synonym here.
Context: ...crets - Credentials shared across pods, hard to audit - Manual rotation of access ke...
(HARD_TO)
🪛 YAMLlint (1.37.1)
.gitops/deploy/templates/config.yaml
[error] 12-12: syntax error: could not find expected ':'
(syntax)
apis/irsas/definition.yaml
[error] 68-68: duplication of key "type" in mapping
(key-duplicates)
[error] 80-80: duplication of key "type" in mapping
(key-duplicates)
[error] 94-94: duplication of key "type" in mapping
(key-duplicates)
🔇 Additional comments (56)
.gitops/deploy/Chart.yaml (1)
2-5: LGTM! Metadata updates align with the package rename.The chart metadata has been updated to reflect the new package name and purpose. The
appVersionreset tov0.1.0(fromv1.19.0) indicates this refactor is being treated as a fresh release, which is appropriate given the scope of changes in this PR.examples/test/mocks/observed-resources/standard/steps/1/role.yaml (1)
1-16: LGTM! Well-structured test mock for observed Role resource.The mock manifest correctly represents a ready AWS IAM Role resource with appropriate status conditions and atProvider fields. The placeholder AWS account ID (
123456789012) is standard for test fixtures.examples/test/mocks/observed-resources/standard/steps/1/policy.yaml (1)
1-16: LGTM! Well-structured test mock for observed Policy resource.The mock manifest correctly represents a ready AWS IAM Policy resource with appropriate status conditions and atProvider fields. The naming is consistent with the companion Role mock, and the ARN correctly uses the
policypath component.examples/test/mocks/observed-resources/standard/steps/2/role.yaml (1)
1-16: LGTM! Consistent mock for step 2 Role observation.The mock correctly represents the same Role resource observed in step 2. The consistent state with step 1 is appropriate for testing state propagation across composition steps, where the role created in step 1 is observed again when attachments are added in step 2.
apis/irsas/composition.yaml (1)
12-12: Function reference name is correctly updated and consistently defined.The function reference in the pipeline has been correctly renamed from
hops-ops-configuration-aws-irsarendertohops-ops-aws-irsarender. The corresponding Function resource is properly defined in.gitops/deploy/templates/config.yamlwith the matching name, and no old references remain in the codebase.apis/irsas/configuration.yaml (2)
4-8: LGTM! Clean metadata updates.The package rename to
aws-irsais clearer, and the updated description accurately conveys the purpose. Adding the maintainer email improves discoverability.
11-12: The provider path change fromghcr.iotoxpkg.crossplane.iois valid. Thexpkg.crossplane.ioregistry is the official Crossplane community package registry (default since Crossplane v1.20), andprovider-aws-iamversionv2.3.0(satisfying the>=v2.3.0constraint) is available. No action needed.README.md (5)
32-57: LGTM! Clear minimal example.The Stage 1 example effectively demonstrates the minimal configuration needed, and correctly uses the new nested
policy.documentstructure.
68-108: LGTM! Comprehensive Stage 2 example.Good progression showing naming conventions, permissions boundaries, and organizational metadata (labels/tags). The example clearly demonstrates growth patterns.
119-156: LGTM! Excellent enterprise-scale example.The use of
nameOverridefor exact name control is appropriate for cross-account role assumptions. The compliance tagging demonstrates real-world governance requirements.
167-191: LGTM! Well-documented import pattern.The use of
managementPolicieswithoutDeletecorrectly implements the orphan pattern, allowing gradual adoption without disrupting existing resources.
214-237: LGTM! Accurate status and composition documentation.The status fields and composed resources sections provide clear, actionable information for users. The status table format is easy to reference.
examples/irsas/minimal.yaml (1)
10-32: LGTM! Correctly implements nested policy structure.The migration from flat
policyto nestedpolicy.documentaligns with the CRD restructuring objectives. The policy content remains unchanged, just better structured..github/workflows/on-pr.yaml (2)
5-6: LGTM! Sensible branch filter.Restricting the PR workflow to the
mainbranch prevents unnecessary CI runs and focuses automation on the primary integration branch.
35-37: LGTM! Enhanced test coverage for state-based rendering.The expanded examples test the new state-based rendering pipeline with mocked observed resources at different steps. The test data directories exist with appropriate resource fixtures—steps/1 contains initial resources (policy, role) and steps/2 includes the additional attachment, validating state transitions effectively.
functions/render/001-state-observed-role.yaml.gotmpl (1)
1-21: No action needed. The template safely usesmerge $state.observedbecause$state.observedis explicitly initialized as an empty dict in000-state-init.yaml.gotmpl, which runs before this template. The rendering pipeline design ensures this precondition is met.upbound.yaml (1)
4-4: LGTM! Package rename applied consistently.The rename from
configuration-aws-irsatoaws-irsais consistently applied across all metadata fields, including the project name, readme header, repository path, and source path.Also applies to: 17-17, 19-19, 22-23
.gitops/deploy/values.yaml (1)
2-4: Verify the behavioral change to dependency resolution.Changing
skipDependencyResolutionfromtruetofalseenables automatic Crossplane dependency resolution. This will change how dependencies are managed and could affect existing deployments or platform configurations that rely on the previous behavior.Ensure this change aligns with your deployment strategy and won't break existing platform deployments where functions are managed separately.
functions/render/003-state-observed-attachment.yaml.gotmpl (3)
1-6: LGTM! Good defensive coding.The extraction logic safely navigates the nested observed resources structure with appropriate defaults, preventing template errors when fields are missing.
8-14: LGTM! Readiness check follows Kubernetes conventions.The condition-checking logic correctly identifies when an attachment is ready by looking for a condition with
type: "Ready"andstatus: "True", which aligns with standard Kubernetes resource status patterns.
16-20: No action required. The$state.observedfield is properly initialized as an empty dictionary in000-state-init.yaml.gotmpl(line 102) before this template executes. The merge operation on line 17 is safe.Likely an incorrect or invalid review comment.
.github/workflows/on-push-main.yaml (1)
31-33: LGTM! Expanded test coverage for observed resources.The workflow now tests the standard example with different observed-resource states (steps 1 and 2), providing better validation coverage for the state-based rendering pipeline.
functions/render/007-state-irsa-policy.yaml.gotmpl (1)
6-14: The code structure is correct. The template execution order and state initialization are properly sequenced:
002-state-observed-policy.yaml.gotmpl(lines 17–21) initializes$state.observed.policy.readybefore007-state-irsa-policy.yaml.gotmplreferences it (line 12)$state.irsais initialized in000-state-init.yaml.gotmpland properly set by005-state-irsa.yaml.gotmplbefore the merge operation in007-state-irsa-policy.yaml.gotmpl(line 14)All state dependencies are correctly handled.
examples/test/mocks/observed-resources/standard/steps/2/attachment.yaml (1)
1-15: LGTM! Well-formed test fixture.The RolePolicyAttachment manifest is correctly structured for a step 2 observed resource with appropriate Ready and Synced conditions.
examples/test/mocks/observed-resources/standard/steps/2/policy.yaml (1)
1-16: LGTM! Test fixture is correctly structured.The Policy manifest is well-formed with appropriate Ready and Synced conditions and correctly populated atProvider fields.
functions/render/008-state-irsa-attachment.yaml.gotmpl (1)
1-12: LGTM! Template logic is sound.The attachment state construction correctly builds the attachment object and merges it into the IRSA state. The dependencies on
state.irsa(from 005-state-irsa.yaml.gotmpl) andstate.observed.attachment.ready(from 003-state-observed-attachment.yaml.gotmpl) are correctly ordered by file naming convention.functions/render/005-state-irsa.yaml.gotmpl (1)
1-17: LGTM! Clean state construction.The template correctly builds the core IRSA state object from the effective configuration. The comprehensive field mapping provides a solid foundation for downstream rendering templates.
functions/render/002-state-observed-policy.yaml.gotmpl (1)
1-21: LGTM! Robust observed state extraction.The template correctly extracts policy observation with defensive defaults throughout the nested navigation. The readiness detection logic properly scans status conditions for the Ready=True state.
functions/render/200-policy.yaml.gotmpl (1)
16-18: Validatepolicy.documentis always JSON (not YAML) before embedding.
forProvider.policyis expected to be an IAM policy JSON document. If$state.irsa.policy.documentcan be YAML (or contains templating artifacts), the provider call will fail.Makefile (1)
3-17: Makefile updates look consistent with package rename + observed-resources coverage.The
PACKAGE ?= aws-irsaand expandedEXAMPLESentries match the stated goal of validating multiple observed-resource scenarios.functions/render/100-role.yaml.gotmpl (1)
22-24: No action required —providerConfigRef.kindis valid for Upbound AWS Role.The
kindfield inproviderConfigRefis a supported field in the Upbound AWS provider and correctly accepts values like "ProviderConfig" or "ClusterProviderConfig". The implementation is consistent across Role, Policy, and Attachment resources and is validated in the test suite. This field will not fail at apply time.Likely an incorrect or invalid review comment.
functions/render/999-status.yaml.gotmpl (1)
1-29: No changes needed - this is a status-only patch, not a full resource object.The file correctly renders only
apiVersion,kind, andstatuswithoutmetadata.name. In Crossplane compositions, status patches update only the.statussubresource of an existing composite resource. The controller applies these patches knowing the target XR context; metadata is not included in status patches. The structure follows the correct Crossplane composition pattern where managed resources (100-role, 200-policy, 300-attachment) include metadata.name, but the final status patch (999-status) does not.Likely an incorrect or invalid review comment.
functions/render/010-state-status.yaml.gotmpl (1)
4-6: Remove the proposed fix; render flags are always true and optional rendering is not supported.The three resources (role, policy, attachment) always render per files 006-008 which hardcode
$render := true. The render flags are checked in the conditional rendering blocks (100-role.yaml.gotmpl, 200-policy.yaml.gotmpl, 300-attachment.yaml.gotmpl) but are never set to false, so the scenario of intentional non-rendering doesn't exist in this codebase. The proposed patch attempts to respect render flags that cannot actually be false.The readiness computation is correct as written: it requires all three observed resources to be ready before the composite resource is ready. If this behavior needs to change, the actual requirement should be clarified rather than adding conditional logic based on non-existent optional rendering.
functions/render/300-attachment.yaml.gotmpl (2)
1-24: LGTM! RolePolicyAttachment rendering logic is well-structured.The template correctly uses
matchControllerRef: truewithmatchLabelsto bind the attachment to the appropriate Role and Policy resources, and includes properproviderConfigRefwith both name and kind fields.
26-70: LGTM! Usage resources correctly implement deletion protection.The
replayDeletion: trueensures proper cleanup ordering. The conditional rendering based onreadystates prevents Usage resources from being created until the referenced resources exist.functions/render/006-state-irsa-role.yaml.gotmpl (2)
6-21: LGTM! Assume role policy follows AWS IRSA best practices.The policy structure correctly uses
sts:AssumeRoleWithWebIdentitywithStringEqualscondition on the:subclaim to restrict which service accounts can assume the role.
23-31: LGTM! Role state construction is clear and well-organized.The state dictionary correctly captures render flag, naming, assume role policy (as JSON), and ready status from observed state.
.gitops/deploy/templates/config.yaml (1)
1-24: LGTM! Helm template structure is correct.The static analysis YAML error at line 12 is a false positive—this is valid Helm templating where the
---document separator is conditionally rendered. The package renaming fromconfiguration-aws-irsatoaws-irsais consistently applied across both Configuration and Function resources.examples/irsas/standard.yaml (1)
1-47: LGTM! Example demonstrates the new API structure clearly.The manifest correctly showcases the refactored schema with nested
providerConfigRef,role,policy, andserviceAccountobjects. Having bothlabels(for Kubernetes) andtags(for AWS) with similar values is a reasonable pattern for consistent resource categorization across platforms.functions/render/000-state-init.yaml.gotmpl (3)
50-55: Verify label merge precedence.In Sprig's
mergefunction, the first argument takes precedence for conflicting keys. Currently$defaultLabelsis first, meaning if a user provideslabels: {"hops.ops.com.ai/managed": "false"}, the default"true"value would win.If user-provided labels should override defaults, consider swapping the merge order:
Suggested fix
-{{- $labels := merge $defaultLabels ($spec.labels | default dict) }} +{{- $labels := merge ($spec.labels | default dict) $defaultLabels }}Please confirm whether user-provided labels should be able to override default labels, or if the current behavior (defaults always win) is intentional.
57-59: Same merge precedence concern for tags.Line 59 has the same merge order as labels. If user tags should override the default
hops: "true"tag, the order should be reversed.
61-104: LGTM! State structure is well-organized.The separation between
spec.raw(original input) andspec.effective(computed/normalized values) provides good traceability. Initializing emptyobservedandirsadicts allows subsequent template files to populate them incrementally.tests/test-render/main.k (3)
1-59: LGTM! Good test structure with shared policy document.The data-driven approach with
_itemsarray and reusable_policy_documentimproves maintainability. Test 1 correctly validates the default naming convention ofclusterName-metadata.name.
61-141: LGTM! Tests 2 and 3 properly validate naming precedence.Test 2 confirms prefixes are applied, and Test 3 confirms overrides take full precedence, ignoring prefixes entirely.
401-474: LGTM! Test 11 correctly validates Usage resource conditional rendering.The test properly sets up
observedResourceswith Ready/Synced conditions and atProvider ARNs, which triggers the Usage resource rendering logic in the template.tests/e2etest-irsa/main.k (2)
76-99: LGTM! E2E test manifest structure is well-organized.Good practices observed:
- Fake OIDC provider for testing resource creation without a real cluster
- Timestamp-based tags (
test-run = _now) for identifying/cleaning up test resources- Unique cluster name prevents conflicts across parallel test runs
30-31: The hardcoded account ID is intentional and already externalized at the CI/CD level. The CI/CD workflows (on-pr.yaml and on-push-main.yaml) explicitly configureaws-account-id: "034489662075", which matches the value in the test file. The inline comment "update this for your environment" is guidance for local developers, not an oversight. This pattern is consistent with other test files (test-render/main.k) which also use hardcoded values inline. No further externalization is necessary.apis/irsas/definition.yaml (9)
21-21: LGTM! Clear and helpful descriptions.The updated descriptions for
accountIdandclusterNameimprove the API documentation by clarifying their purpose and usage patterns.Also applies to: 24-24
25-30: LGTM! Labels field properly configured.The new labels field is correctly structured with
additionalPropertiesandx-kubernetes-preserve-unknown-fieldsto support flexible label assignment.
46-46: LGTM! Improved field documentation.The updated descriptions clarify the OIDC issuer URL format flexibility and the permissions boundary purpose.
Also applies to: 49-49
96-97: LGTM! Service account properly restructured.The serviceAccount field is correctly defined as an object with the appropriate description and required fields.
109-109: LGTM! Clear tags documentation.The updated description clarifies the tag merging behavior with defaults.
119-121: LGTM! Standard readiness pattern.The addition of the
readyboolean field follows Crossplane best practices for exposing resource readiness status.
125-155: LGTM! Status fields properly structured.The status fields are correctly restructured as objects with detailed properties (ARNs, names, etc.), providing clear resource state information.
156-165: LGTM! Helpful debugging capability.The addition of
status.specwithrawandeffectivefields provides valuable debugging insight into spec processing and transformations.
50-68: No action needed. This is the initial v1alpha1 release with no prior versions in the repository. All examples and documentation already reflect the current schema structure, and there are no existing IRSA resources to migrate. v1alpha1 explicitly permits breaking changes and follows Kubernetes API versioning conventions.
| required: | ||
| - accountId | ||
| - oidc | ||
| - policy | ||
| - serviceAccount | ||
| - managementPolicies | ||
| type: object |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the definition.yaml file and check its structure
find . -name "definition.yaml" -path "*/irsas/*" -type fRepository: hops-ops/aws-irsa
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the full context around lines 111-116
cat -n apis/irsas/definition.yaml | sed -n '70,125p'Repository: hops-ops/aws-irsa
Length of output: 2571
🏁 Script executed:
#!/bin/bash
# Search for validation logic related to clusterName and providerConfigRef
rg -n "clusterName|providerConfigRef" --type yaml -A 3 -B 3 -g '*composition.yaml' -g '*render*'Repository: hops-ops/aws-irsa
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for clusterName definition in the file
rg -n "clusterName" apis/irsas/definition.yaml -B 3 -A 3Repository: hops-ops/aws-irsa
Length of output: 768
🏁 Script executed:
#!/bin/bash
# Look for composition.yaml or other related files in the irsas directory
find . -path "*/irsas/*" -type f -name "*.yaml" -o -name "*.yml"Repository: hops-ops/aws-irsa
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Check if there are any function templates or validation rules
find . -path "*/irsas/*" -type f \( -name "*.gotmpl" -o -name "*.tmpl" -o -name "*template*" \)Repository: hops-ops/aws-irsa
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Read the composition.yaml to see how clusterName and providerConfigRef are used
cat -n apis/irsas/composition.yamlRepository: hops-ops/aws-irsa
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Check the examples to see if clusterName is always provided
cat -n examples/irsas/minimal.yaml examples/irsas/standard.yamlRepository: hops-ops/aws-irsa
Length of output: 2670
🏁 Script executed:
#!/bin/bash
# Look for the render function implementation
find . -path "*hops-ops-aws-irsarender*" -o -name "*render*" -type f | head -20Repository: hops-ops/aws-irsa
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for CEL validation rules or any validation logic in the definition.yaml
cat -n apis/irsas/definition.yaml | head -70Repository: hops-ops/aws-irsa
Length of output: 3396
🏁 Script executed:
#!/bin/bash
# Check if there are validation rules (CEL rules) in the definition.yaml
rg -n "validationRules|rules:" apis/irsas/definition.yaml -B 2 -A 5Repository: hops-ops/aws-irsa
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check full definition.yaml for any validation constraints
cat -n apis/irsas/definition.yaml | tail -100Repository: hops-ops/aws-irsa
Length of output: 4228
🏁 Script executed:
#!/bin/bash
# Search for any render function files or business logic
find . -type f \( -name "*.go" -o -name "*.yaml" -o -name "*.py" \) | xargs rg -l "clusterName.*providerConfig" 2>/dev/null | head -10Repository: hops-ops/aws-irsa
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there are any validation rules elsewhere or if this is handled at runtime
rg -n "clusterName" apis/irsas/configuration.yaml -B 3 -A 3Repository: hops-ops/aws-irsa
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for any Go code or implementation files
find . -type f -name "*.go" | xargs rg -l "clusterName" 2>/dev/nullRepository: hops-ops/aws-irsa
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Check the render function templates that handle the composition logic
cat -n functions/render/000-state-init.yaml.gotmplRepository: hops-ops/aws-irsa
Length of output: 4592
🏁 Script executed:
#!/bin/bash
# Check the IRSA-specific render template
cat -n functions/render/005-state-irsa.yaml.gotmplRepository: hops-ops/aws-irsa
Length of output: 694
Add validation to ensure clusterName or providerConfigRef.name is explicitly provided.
While the render function has a fallback to "default" when both clusterName and providerConfigRef.name are omitted (line 19 of functions/render/000-state-init.yaml.gotmpl), relying on this fallback is fragile. If no ProviderConfig named "default" exists, it will fail at runtime during resource provisioning rather than during validation.
Consider one of these approaches:
- Make
clusterNamerequired (since all examples provide it) - Add a CEL validation rule to ensure at least one of
clusterNameorproviderConfigRef.nameis provided - Document explicitly that the "default" fallback requires a ProviderConfig with that name to exist
| assumeRolePolicy: | | ||
| {{ $state.irsa.role.assumeRolePolicy | nindent 6 }} |
There was a problem hiding this comment.
Avoid emitting a leading blank line in assumeRolePolicy literal block.
Because the action isn’t left-trimmed, you can end up with an initial empty line in the rendered string.
Proposed patch
assumeRolePolicy: |
- {{ $state.irsa.role.assumeRolePolicy | nindent 6 }}
+ {{- $state.irsa.role.assumeRolePolicy | nindent 6 }}🤖 Prompt for AI Agents
In @functions/render/100-role.yaml.gotmpl around lines 16 - 17, The
assumeRolePolicy literal block can emit a leading blank line because the
template pipes the raw value into nindent; trim the value before indenting to
remove leading/trailing newlines and whitespace so the rendered block has no
initial empty line (update the pipeline that feeds assumeRolePolicy, i.e. the
expression using $state.irsa.role.assumeRolePolicy and nindent, to apply a trim
function before nindent).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @README.md:
- Around line 183-187: The example JSON under the YAML key "document" is invalid
because it uses the placeholder "[...]" for the "Statement" value; update the
JSON in the code block so "Statement" is a valid JSON array (either an explicit
example statement object or an empty array) and ensure the surrounding block
remains valid JSON (proper braces, quotes and commas) so users can copy/paste
the policy example without syntax errors.
🧹 Nitpick comments (3)
README.md (1)
241-248: Clarify Usage resource purpose.The Usage resources are listed as "Deletion protection for Role and Policy" but this may be unclear to users unfamiliar with Crossplane Usage resources. Consider adding a brief note or link to Crossplane documentation explaining how Usage resources provide deletion protection.
apis/irsas/definition.yaml (1)
138-144: Consider addingpolicyArnandroleArnto attachment status.The attachment status only exposes the
id. For debugging and cross-referencing, it might be helpful to also surface thepolicyArnandroleArnthat the attachment links together, similar to what AWS returns forRolePolicyAttachment.functions/render/999-status.yaml.gotmpl (1)
7-7: Missing default value for ready field.The
readyboolean is rendered directly without a default value. If$s.readyis nil or undefined, this could produce invalid YAML output (e.g.,ready: <no value>).🛡️ Add default value for ready field
- ready: {{ $s.ready }} + ready: {{ $s.ready | default false }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdapis/irsas/definition.yamlfunctions/render/010-state-status.yaml.gotmplfunctions/render/999-status.yaml.gotmpl
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~9-~9: To elevate your writing, try using a synonym here.
Context: ...crets - Credentials shared across pods, hard to audit - Manual rotation of access ke...
(HARD_TO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: publish / publish
- GitHub Check: e2e / test
🔇 Additional comments (11)
README.md (3)
1-20: LGTM!Clear and concise introduction explaining the IRSA value proposition. The "Without IRSA" vs "With IRSA" comparison effectively communicates the benefits.
32-57: LGTM!Stage 1 example is minimal and aligns well with the CRD definition. The required fields (
accountId,oidc,policy.document,serviceAccount.name,serviceAccount.namespace) are all present.
250-258: LGTM!Development commands are clear and follow standard Makefile conventions.
apis/irsas/definition.yaml (4)
25-30: LGTM!Good use of
x-kubernetes-preserve-unknown-fields: truefor the labels object, allowing flexible label key-value pairs while maintaining type safety withadditionalProperties: type: string.
50-67: LGTM!The policy object structure is well-designed with clear separation of concerns:
document(required) for the IAM policy JSONnamePrefix/nameOverridefor naming flexibilityexternalNamefor importing existing policies
113-145: LGTM!Status schema is well-structured with clear descriptions. The nested objects for
role,policy, andattachmentprovide good observability of the underlying AWS resources.
22-24: Clarify the fallback chain forproviderConfigRef.namein the schema documentation.The descriptions at lines 24 and 74 are incomplete. The render function (line 19 of
functions/render/000-state-init.yaml.gotmpl) implements a three-level fallback:providerConfigRef.name→clusterName→"default". This means if bothclusterNameandproviderConfigRef.nameare omitted, the system defaults to a ProviderConfig named"default", which may not exist in all environments.Update the description of
providerConfigRef.nameto document the full fallback chain and clarify that users should either explicitly setproviderConfigRef.name, provideclusterName, or ensure a ProviderConfig named"default"exists.functions/render/999-status.yaml.gotmpl (2)
2-2: Missing validation for $state.status dependency.This template assumes
$state.statusexists and was properly populated by010-state-status.yaml.gotmpl. If that template fails or is skipped, accessing$state.statuscould cause template errors.Verify that the rendering pipeline guarantees sequential execution and that
010-state-status.yaml.gotmplalways runs before this template.
9-15: Empty string defaults will produce quoted empty values in YAML.The use of
default "" | quoteis functionally correct and prevents nil values, but will render missing fields as""(quoted empty strings) rather than omitting them or using null. This is acceptable for string fields, but be aware of the YAML output format.This defensive pattern ensures the YAML is always valid, even when observed resources haven't been created yet.
functions/render/010-state-status.yaml.gotmpl (2)
7-20: No action required—prior templates guarantee safe property access.The observed-role, observed-policy, and observed-attachment templates each populate their respective objects with required properties (arn, name, id) using defensive coding with
| default dictfallbacks and| default ""value defaults. By the time 010-state-status.yaml.gotmpl executes,$obs.role,$obs.policy, and$obs.attachmentare guaranteed to exist as dicts with populated keys, even when underlying resources are missing. Properties are never nil; they default to empty strings.
4-4: The readiness computation is safe as written. The framework execution order guarantees that$state.observed.role,$state.observed.policy, and$state.observed.attachmentall exist withreadyfields before line 4 executes. Files 001-003 (executed before 010) populate these objects via merge operations, and the same unsafe access pattern appears throughout the codebase (files 006-008) without issue. Defensive checks are unnecessary.Likely an incorrect or invalid review comment.
| document: | | ||
| { | ||
| "Version": "2012-10-17", | ||
| "Statement": [...] | ||
| } |
There was a problem hiding this comment.
Incomplete JSON in example policy document.
The policy document example uses [...] as a placeholder which is not valid JSON. This could confuse users trying to copy the example.
📝 Suggested fix
policy:
externalName: existing-irsa-policy
document: |
{
"Version": "2012-10-17",
- "Statement": [...]
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": ["..."],
+ "Resource": ["..."]
+ }
+ ]
}📝 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.
| document: | | |
| { | |
| "Version": "2012-10-17", | |
| "Statement": [...] | |
| } | |
| document: | | |
| { | |
| "Version": "2012-10-17", | |
| "Statement": [ | |
| { | |
| "Effect": "Allow", | |
| "Action": ["..."], | |
| "Resource": ["..."] | |
| } | |
| ] | |
| } |
🤖 Prompt for AI Agents
In @README.md around lines 183 - 187, The example JSON under the YAML key
"document" is invalid because it uses the placeholder "[...]" for the
"Statement" value; update the JSON in the code block so "Statement" is a valid
JSON array (either an explicit example statement object or an empty array) and
ensure the surrounding block remains valid JSON (proper braces, quotes and
commas) so users can copy/paste the policy example without syntax errors.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.