[ROB-1896] Irsa cross platform #1912
Conversation
WalkthroughAdds cross-account assume-role support for AWS Managed Prometheus: reads AWS_ASSUME_ROLE env var into AWSPrometheusConfig and passes it to KRR via a new --eks-assume-role flag. Updates docs to mention the env/assume-role options. Bumps versions: holmes chart (0.13.3) and prometrix (0.2.3). Updates KRR image to v1.26.1. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Operator
participant Env as Environment
participant Utils as Prometheus Utils
participant AWSP as AWSPrometheusConfig
participant KRR as KRR Runner
User->>Env: Set AWS_REGION, (optional) AWS_ASSUME_ROLE
User->>Utils: generate_prometheus_config()
Env-->>Utils: AWS_REGION, AWS_ASSUME_ROLE
Utils->>AWSP: Create with region, service, assume_role_arn (if provided)
AWSP-->>Utils: Config object
Utils-->>KRR: Prometheus config (AWSPrometheusConfig)
KRR->>KRR: Build cmd args
alt assume_role_arn present
KRR->>KRR: Add --eks-assume-role <arn>
else
KRR->>KRR: Use existing AMP flags only
end
KRR-->>User: Final command-line arguments
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
playbooks/robusta_playbooks/krr.py (5)
5-5: Import error:from pickle import NONEwill crash on import
pickle.NONEisn’t a public symbol. This raises ImportError before any action runs.-from pickle import NONEAll usages should be plain
None(see Line 451 fix below).
408-428: Fix type hints and defaults for _publish_krr_finding; ensure start_time is datetime
timeoutshould be Optional[int], not str.config_json = Optional[str]sets a wrong default value.- start_time is used as a datetime in the report; make it a datetime and convert at the caller.
-def _publish_krr_finding(event: ExecutionBaseEvent, krr_json: Dict[str, Any],scan_id: str, start_time: str, metadata: Dict[str, Any], timeout: Optional[str] = None, config_json = Optional[str]): +def _publish_krr_finding( + event: ExecutionBaseEvent, + krr_json: Dict[str, Any], + scan_id: str, + start_time: datetime, + metadata: Dict[str, Any], + timeout: Optional[int] = None, + config_json: Optional[str] = None, +): @@ - scan_block = _generate_krr_report_block( - scan_id, start_time, krr_scan, metadata, config_json - ) + scan_block = _generate_krr_report_block(scan_id, start_time, krr_scan, metadata, config_json)Follow-ups in callers are below.
451-452: UseNoneand pass a datetime to match the fixed signatureReplace
NONEand parse the ISO start-time string.- _publish_krr_finding(event=event,krr_json=params.result, scan_id=params.scan_id, start_time=params.start_time, metadata=metadata, timeout=None, config_json=NONE) + _publish_krr_finding( + event=event, + krr_json=params.result, + scan_id=params.scan_id, + start_time=datetime.fromisoformat(params.start_time), + metadata=metadata, + timeout=None, + config_json=None, + )
550-551: Store start-time as ISO 8601 so it round-trips cleanlyThis ensures the process_scan path can parse it reliably.
- krr_annotations = {"scan-id": scan_id, "start-time": start_time} + krr_annotations = {"scan-id": scan_id, "start-time": start_time.isoformat()}
548-589: Guard against UnboundLocalError onlogsin the except pathIf an exception is thrown before
logsis assigned, referencing it in the except block raises a new error.- try: + logs = "" + try: @@ - _emit_failed_scan_event(event, scan_id, start_time, metadata, msg, e, logs) + _emit_failed_scan_event(event, scan_id, start_time, metadata, msg, e, logs)(Initialize
logs = ""before the try.)
🧹 Nitpick comments (3)
docs/configuration/metric-providers-aws.rst (2)
81-85: Doc nit: quote ARN and add trust-policy note for cross-account
- Quote the example ARN to avoid YAML surprises.
- Add a one-liner that the target account role must trust the source IRSA role (sts:AssumeRole).
-# assume_role_arn: arn:aws:iam::<ACCOUNT_ID>:role/<AMP_IAM_ROLE> +# assume_role_arn: "arn:aws:iam::<ACCOUNT_ID>:role/<AMP_IAM_ROLE>"
102-107: Doc nit: keep env var example consistent with quoted ARNGood call showing it under runner env; keep the ARN quoted consistently.
-# value: "arn:aws:iam::<ACCOUNT_ID>:role/<AMP_IAM_ROLE>" +# value: "arn:aws:iam::<ACCOUNT_ID>:role/<AMP_IAM_ROLE>"playbooks/robusta_playbooks/krr.py (1)
242-244: Appending--eks-assume-roleonly when set is correct; minor quoting nitFunctional change looks good. To be extra-safe, quote the ARN even though ARNs don’t contain spaces.
- if prom_config.assume_role_arn: - additional_cmd_line_args += f" --eks-assume-role {prom_config.assume_role_arn}" + if prom_config.assume_role_arn: + additional_cmd_line_args += f" --eks-assume-role {shlex.quote(prom_config.assume_role_arn)}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
helm/robusta/Chart.lockis excluded by!**/*.lockpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
docs/configuration/metric-providers-aws.rst(2 hunks)helm/robusta/Chart.yaml(1 hunks)playbooks/robusta_playbooks/krr.py(2 hunks)pyproject.toml(1 hunks)src/robusta/integrations/prometheus/utils.py(2 hunks)
⏰ 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: run_tests
- GitHub Check: run_tests
🔇 Additional comments (2)
pyproject.toml (1)
62-62: Confirmed assume_role_arn present in AWSPrometheusConfig: in prometrix 0.2.3’sprometrix/models/prometheus_config.py,class AWSPrometheusConfigdefinesassume_role_arn: Optional[str] = None, matching the parameter passed fromutils.py.src/robusta/integrations/prometheus/utils.py (1)
32-33: Could you fetch and paste theAWSPrometheusConfigconstructor signature from prometrix v0.2.3 (the exact parameter list) so we can confirm it usesassume_role_arn?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/configuration/metric-providers-aws.rst (1)
102-107: Runner env var example: ensure list indentationMake sure the commented AWS_ASSUME_ROLE example is correctly indented as a list item under runner.additional_env_vars so users can copy/paste.
- # Optional: Configure cross-account role assumption for AMP - # Set this if your Prometheus workspace is in a different AWS account - # than the one running your Kubernetes service account. - # - name: AWS_ASSUME_ROLE - # value: "arn:aws:iam::<ACCOUNT_ID>:role/<AMP_IAM_ROLE>" + # Optional: Configure cross-account role assumption for AMP + # Set this if your Prometheus workspace is in a different AWS account + # than the one running your Kubernetes service account. + # - name: AWS_ASSUME_ROLE + # value: "arn:aws:iam::<ACCOUNT_ID>:role/<AMP_IAM_ROLE>"Also note: this env is only consumed when AWS_REGION is set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
helm/robusta/Chart.lockis excluded by!**/*.lockpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
docs/configuration/metric-providers-aws.rst(2 hunks)helm/robusta/Chart.yaml(1 hunks)playbooks/robusta_playbooks/krr.py(2 hunks)pyproject.toml(1 hunks)src/robusta/integrations/prometheus/utils.py(2 hunks)
⏰ 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). (1)
- GitHub Check: Build images
🔇 Additional comments (5)
pyproject.toml (1)
62-62: Assume-role support present in prometrix 0.2.3 AWSPrometheusConfig now exposesassume_role_arnandget_custom_prometheus_connectforwards it to the AWS connector, ensuring the new KRR assume-role path is reachable.helm/robusta/Chart.yaml (1)
18-18: Gate merge until Chart repo is updated
Could not verify holmes v0.13.3 in the Robusta charts index (index.yaml unreachable) or find a matching release changelog. Please publish version 0.13.3 to the chart repository and confirm the index entry before merging to avoid CI/helm dependency failures.src/robusta/integrations/prometheus/utils.py (2)
32-32: AWS_ASSUME_ROLE env wiring — LGTMReading AWS_ASSUME_ROLE from env is straightforward and non-breaking.
61-69: Propagate assume_role_arn is safe – AWSPrometheusConfig in prometrix 0.2.3 includes anassume_role_arnparameter, so this code path won’t raise at runtime.playbooks/robusta_playbooks/krr.py (1)
39-39: KRR image bump to v1.26.1 — LGTM
Uh oh!
There was an error while loading. Please reload this page.