Fix cost over usage resource file path and Jenkins exit handling.#1008
Conversation
Write the user resource JSON to /tmp before S3 upload and fail the weekly Jenkins job when podman runs return a non-zero exit code. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR modifies cost_over_usage.py to fix Cost field aggregation, guard instance file creation, and conditionally attach email files, and updates run_upload_es.py to run podman commands in a loop that exits on failure. ChangesCost usage report and alerting fixes
Podman command execution hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant aws_user_usage
participant FileSystem
participant send_email_postfix
aws_user_usage->>aws_user_usage: check cost_usage exceeded
aws_user_usage->>aws_user_usage: evaluate ignore_user_mails
alt Instances present
aws_user_usage->>FileSystem: write /tmp/<to>_resource.json
aws_user_usage->>aws_user_usage: set file_name
end
aws_user_usage->>send_email_postfix: send_email_postfix(**email_kwargs)
Related issues: None specified Related PRs: None specified Suggested labels: bug, enhancement Suggested reviewers: None specified 🐰 A hop, a fix, a JSON file's fate, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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
🧹 Nitpick comments (3)
cloud_governance/policy/aws/cost_over_usage.py (2)
96-96: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueLoop-invariant
literal_evalcall.
self.__ignore_mailsis decoded on every iteration of thefor user_usage in user_dataloop even though it doesn't depend on the loop variable. Hoist it outside the loop.🤖 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 `@cloud_governance/policy/aws/cost_over_usage.py` at line 96, The `literal_eval` on `self.__ignore_mails` inside the `for user_usage in user_data` loop is loop-invariant and should be moved out of the loop. Update the logic in `CostOverUsage` so the decoded `ignore_user_mails` value is computed once before iterating over `user_data`, then reused within the loop; use the existing `self._elastic_upload.literal_eval` and `user_usage` flow to place the hoisted assignment correctly.
103-108: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHardcoded
/tmppath with no cleanup; considertempfilemodule.Static analysis flags this as insecure temp-file usage (S108/CWE-377). The file is written under a fixed
/tmpdirectory keyed byto(user-controlled/ES-derived value) and never cleaned up after the email is sent, risking accumulation and potential collisions/predictable paths across runs.♻️ Suggested fix using tempfile
- file_name = None + file_name = None if user_usage.get('Instances'): used_instances = self.get_user_used_instances(user_used_list=user_usage.get('Instances')) - file_name = os.path.join('/tmp', f'{to}_resource.json') + file_name = os.path.join(tempfile.gettempdir(), f'{to}_resource.json') with open(file_name, 'w') as file: json.dump(used_instances, file, indent=4)Also consider deleting
file_nameaftersend_email_postfixcompletes to avoid stale files accumulating in the temp directory.🤖 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 `@cloud_governance/policy/aws/cost_over_usage.py` around lines 103 - 108, The temporary resource file creation in the cost_over_usage flow uses a hardcoded /tmp path and leaves stale files behind. Update the logic around get_user_used_instances and the send_email_postfix path to create the JSON file via tempfile (using a secure, unique location/name instead of f"{to}_resource.json"), and ensure it is removed after the email is sent. Keep the fix localized to the block that writes file_name and passes it to send_email_postfix.Source: Linters/SAST tools
jenkins/clouds/aws/weekly/cost_over_usage/run_upload_es.py (1)
26-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExit-on-failure loop is correct; consider subprocess over os.system.
Loop logic correctly matches the PR goal of failing when podman returns non-zero. Static analysis flags
os.system(command)with interpolated env vars as command-injection risk (Ruff S605, OpenGrep, ast-grep). Since inputs here come from Jenkins environment variables rather than untrusted external input, actual exploitability is low, but migrating tosubprocess.run(shlex.split(command))or a list-of-args form would remove the flagged pattern and give you captured stdout/stderr for better failure diagnostics.♻️ Suggested refactor
-for command in commands: - if os.system(command) != 0: - sys.exit(1) +for command in commands: + result = subprocess.run(command, shell=True) + if result.returncode != 0: + sys.exit(1)🤖 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 `@jenkins/clouds/aws/weekly/cost_over_usage/run_upload_es.py` around lines 26 - 34, The loop in run_upload_es.py currently uses os.system(command), which triggers command-injection/static-analysis warnings even though the values come from Jenkins env vars. Replace the os.system call with subprocess-based execution in the command loop, using a safe argument form (or shlex.split if you keep the command string) so the podman invocation is preserved while avoiding the flagged pattern and allowing stdout/stderr capture. Keep the existing failure-on-nonzero behavior in the loop and update the command construction around commands/command accordingly.Source: Linters/SAST tools
🤖 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 `@cloud_governance/policy/aws/cost_over_usage.py`:
- Around line 76-79: The aggregation in aggregate_user_sum is using the builtin
sum for the Instances field, which crashes when the column contains list values.
Update the groupby/agg logic in aggregate_user_sum so Instances uses a list-safe
reducer (for example, collecting or combining lists) while keeping the Cost sum
unchanged, and make sure the conditional branch that checks for Instances still
routes through the same aggregation path.
---
Nitpick comments:
In `@cloud_governance/policy/aws/cost_over_usage.py`:
- Line 96: The `literal_eval` on `self.__ignore_mails` inside the `for
user_usage in user_data` loop is loop-invariant and should be moved out of the
loop. Update the logic in `CostOverUsage` so the decoded `ignore_user_mails`
value is computed once before iterating over `user_data`, then reused within the
loop; use the existing `self._elastic_upload.literal_eval` and `user_usage` flow
to place the hoisted assignment correctly.
- Around line 103-108: The temporary resource file creation in the
cost_over_usage flow uses a hardcoded /tmp path and leaves stale files behind.
Update the logic around get_user_used_instances and the send_email_postfix path
to create the JSON file via tempfile (using a secure, unique location/name
instead of f"{to}_resource.json"), and ensure it is removed after the email is
sent. Keep the fix localized to the block that writes file_name and passes it to
send_email_postfix.
In `@jenkins/clouds/aws/weekly/cost_over_usage/run_upload_es.py`:
- Around line 26-34: The loop in run_upload_es.py currently uses
os.system(command), which triggers command-injection/static-analysis warnings
even though the values come from Jenkins env vars. Replace the os.system call
with subprocess-based execution in the command loop, using a safe argument form
(or shlex.split if you keep the command string) so the podman invocation is
preserved while avoiding the flagged pattern and allowing stdout/stderr capture.
Keep the existing failure-on-nonzero behavior in the loop and update the command
construction around commands/command accordingly.
🪄 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: 174804e7-dade-462a-b66e-3aa4baed9835
📒 Files selected for processing (2)
cloud_governance/policy/aws/cost_over_usage.pyjenkins/clouds/aws/weekly/cost_over_usage/run_upload_es.py
github-project-automation
Bot
moved this from In progress
to Done
in Cloud-Governance project
2 participants
Type of change
Note: Fill x in []
Description
Write the user resource JSON to /tmp before S3 upload and fail the weekly Jenkins job when podman runs return a non-zero exit code.
For security reasons, all pull requests need to be approved first before running any automated CI
Assisted-by: Cursor