Ec2 connection/non static ports#7
Conversation
📝 WalkthroughWalkthroughEC2 deployment now uses configured SSH private keys without UserData key injection, supports configurable workflow API ports across Docker and runtime paths, updates EC2 defaults and documentation, and adds validation tests for key presence and permissions. ChangesEC2 deployment flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as cmd_build
participant Generator as generate_workflow_docker
participant Specs as write_agent_specs
participant Runtime as EC2 runtime
participant AWS as EC2 run_instances
participant Host as EC2 host
CLI->>Generator: pass api_port
Generator->>Generator: generate Dockerfile EXPOSE api_port
Specs->>Runtime: provide persisted api_port
Runtime->>Runtime: validate ssh_private_key_path
Runtime->>AWS: provision without UserData key injection
Runtime->>Host: transfer image over restricted SSH
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This PR changes two things:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ventis/stub_generator.py (1)
461-472: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the workflow container
EXPOSEport at 8080
api_portis the host-side mapping (<api_port>:8080); the container listens on 8080. Exposingapi_porthere makes the image metadata anddocker run -Padvertise the wrong port.Suggested fix
-EXPOSE {api_port} +EXPOSE 8080🤖 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 `@ventis/stub_generator.py` around lines 461 - 472, Update the Dockerfile template generated by the stub generator so the workflow container exposes port 8080 instead of interpolating api_port. Keep api_port reserved for the host-side mapping and preserve the existing other EXPOSE declaration.Source: MCP tools
ventis/controller/cloud_provider_logic/EC2/_runtime.py (1)
146-182: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove SSH-key validation inside the
try, and decoupleterminate_instance()from_aws_clients().
Inventis/controller/cloud_provider_logic/EC2/_runtime.py:146-182,284-296, a bootstrap-time_aws_clients()failure skips cleanup, and the fallback termination path can fail for the same reason. Add a regression test for this failure path.🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 146 - 182, The bootstrap flow in bootstrap_instance must catch failures from _aws_clients() so cleanup still runs; move SSH-key/client validation into its try block. Update terminate_instance to obtain or receive the required AWS client independently of _aws_clients(), ensuring fallback termination succeeds when client initialization fails, and add a regression test covering this failure path.
🧹 Nitpick comments (3)
ventis/controller/cloud_provider_logic/EC2/README.md (1)
13-14: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInstall the controller in a virtual environment.
The documentation says requirements are installed “in env” but then uses
--break-system-packages, which explicitly allows pip to modify an externally managed Python installation. Document creating and activating a venv, then runpython -m pip install -e .without that flag. (pip.pypa.io)[details]
Suggested flow:python3 -m venv .venv source .venv/bin/activate python -m pip install -e .[details]
🤖 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 `@ventis/controller/cloud_provider_logic/EC2/README.md` around lines 13 - 14, Update the installation instructions in the README to explicitly create and activate a .venv using python3 -m venv and source .venv/bin/activate, then install the controller with python -m pip install -e . without --break-system-packages.Source: MCP tools
ventis/controller/cloud_provider_logic/EC2/_runtime.py (1)
53-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant SSH-key re-validation.
_ssh_key_path(cfg)is called in_aws_clients()(Line 66) but its return value is discarded, then recomputed again in_bootstrap_instance(Line 224). Consider having_aws_clients()return the validated path so callers don't re-stat/re-validate the key file on every bootstrap.Also applies to: 224-224
🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 53 - 67, Update _aws_clients() to retain and return the validated SSH key path from _ssh_key_path(cfg), then adjust _bootstrap_instance to consume that returned path instead of calling _ssh_key_path(cfg) again. Preserve the existing configuration and client return values while avoiding repeated key-file validation.ventis/controller/global_controller.py (1)
508-537: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReuse the shared SSH key validator
ventis/controller/global_controller.py:496-537
This path re-resolvesec2.ssh_private_key_pathwithout_ssh_key_path(), so startup SSH calls can still fall back to a genericsshfailure instead of the same missing/readability/perms check used by EC2 bootstrap. Call the shared helper here, or centralize the resolution in one place.🤖 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 `@ventis/controller/global_controller.py` around lines 508 - 537, The remote execution path in the command-running method bypasses the shared SSH key validation. Replace its direct ec2.ssh_private_key_path expansion with the existing _ssh_key_path() helper, preserving the current SSH command construction while ensuring startup calls use the same missing, readability, and permission checks as EC2 bootstrap.
🤖 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 `@README.md`:
- Line 78: Update the README deployment guidance link so its anchor text
explicitly identifies the Red Hat SSH key-based access guide instead of using
the generic “here,” while preserving the existing destination URL and
surrounding instructions.
In `@ventis/controller/cloud_provider_logic/EC2/README.md`:
- Line 6: Update the SSH setup documentation in the README to use the correct
authorized-keys filename, changing `~/.ssh/authorizedkeys` to
`~/.ssh/authorized_keys` so the public key is placed where SSH expects it.
- Around line 23-24: Update the EC2 IAM action list in the README to use the
valid plural names ec2:RunInstances and ec2:TerminateInstances, while preserving
ec2:DescribeInstances and ec2:CreateTags.
---
Outside diff comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 146-182: The bootstrap flow in bootstrap_instance must catch
failures from _aws_clients() so cleanup still runs; move SSH-key/client
validation into its try block. Update terminate_instance to obtain or receive
the required AWS client independently of _aws_clients(), ensuring fallback
termination succeeds when client initialization fails, and add a regression test
covering this failure path.
In `@ventis/stub_generator.py`:
- Around line 461-472: Update the Dockerfile template generated by the stub
generator so the workflow container exposes port 8080 instead of interpolating
api_port. Keep api_port reserved for the host-side mapping and preserve the
existing other EXPOSE declaration.
---
Nitpick comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 53-67: Update _aws_clients() to retain and return the validated
SSH key path from _ssh_key_path(cfg), then adjust _bootstrap_instance to consume
that returned path instead of calling _ssh_key_path(cfg) again. Preserve the
existing configuration and client return values while avoiding repeated key-file
validation.
In `@ventis/controller/cloud_provider_logic/EC2/README.md`:
- Around line 13-14: Update the installation instructions in the README to
explicitly create and activate a .venv using python3 -m venv and source
.venv/bin/activate, then install the controller with python -m pip install -e .
without --break-system-packages.
In `@ventis/controller/global_controller.py`:
- Around line 508-537: The remote execution path in the command-running method
bypasses the shared SSH key validation. Replace its direct
ec2.ssh_private_key_path expansion with the existing _ssh_key_path() helper,
preserving the current SSH command construction while ensuring startup calls use
the same missing, readability, and permission checks as EC2 bootstrap.
🪄 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: c0fe69c0-0b4c-4d8e-8810-1908b9b72758
📒 Files selected for processing (11)
.gitignoreREADME.mdtests/test_runtime_ec2.pyventis/cli.pyventis/controller/cloud_provider_logic/EC2/README.mdventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/cloud_provider_logic/Local/_runtime.pyventis/controller/global_controller.pyventis/controller/utils/agent_specs.pyventis/stub_generator.pyventis/templates/config/global_controller.yaml
| ``` | ||
| #### Step 2.1 (Only if performing distributed deployment): | ||
| If you are deploying agents and tools to multiple hosts, you need to make sure that the hosts are reachable from the machine where you are running the deploy command. To enable this please ensure that you have passwordless ssh access to the hosts. A guide to enable passwordless ssh access can be found [here](https://www.redhat.com/en/blog/passwordless-ssh). | ||
| If you are deploying agents and tools to multiple hosts, make sure the hosts are reachable from the machine where you are running the deploy command and that SSH key-based access is already configured. A guide to set that up can be found [here](https://www.redhat.com/en/blog/passwordless-ssh). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use descriptive link text.
Replace “here” with text identifying the resource, such as “Red Hat’s SSH key-based access guide,” so the link remains understandable in screen-reader and link-list views.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 78-78: Link text should be descriptive
(MD059, descriptive-link-text)
🤖 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 `@README.md` at line 78, Update the README deployment guidance link so its
anchor text explicitly identifies the Red Hat SSH key-based access guide instead
of using the generic “here,” while preserving the existing destination URL and
surrounding instructions.
Source: Linters/SAST tools
| For local controller | ||
| - AMI with the following installed: | ||
| - Docker | ||
| - Public key in authorizedkeys in ~/.ssh |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the correct SSH authorized-keys filename.
Line 6 says authorizedkeys; the documented filename is ~/.ssh/authorized_keys. Following this literally leaves the public key unused and prevents SSH setup from working.
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: Ensure spelling is correct
Context: ...installed: - Docker - Public key in authorizedkeys in ~/.ssh For global controller - Inst...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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 `@ventis/controller/cloud_provider_logic/EC2/README.md` at line 6, Update the
SSH setup documentation in the README to use the correct authorized-keys
filename, changing `~/.ssh/authorizedkeys` to `~/.ssh/authorized_keys` so the
public key is placed where SSH expects it.
Source: Linters/SAST tools
| - IAM Role to attach to global controller instance allowing | ||
| - ec2:RunInstance,TerminateInstance,DescribeInstances,CreateTags |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== README excerpt ==\n'
sed -n '18,30p' ventis/controller/cloud_provider_logic/EC2/README.md
printf '\n== Search for related EC2 IAM actions in repo ==\n'
rg -n "RunInstance|RunInstances|TerminateInstance|TerminateInstances|DescribeInstances|CreateTags" ventis/controller/cloud_provider_logic/EC2 -S || trueRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 821
🌐 Web query:
AWS EC2 IAM policy actions RunInstances TerminateInstances documentation
💡 Result:
In AWS IAM policies, ec2:RunInstances and ec2:TerminateInstances are specific actions used to control the ability to launch and terminate Amazon EC2 instances, respectively [1]. Both actions support resource-level permissions, allowing you to restrict access to specific EC2 instances by specifying their Amazon Resource Name (ARN) in the Resource element of your IAM policy [1][2]. Key details for using these actions in policies: 1. Service Prefix and Format: These actions use the ec2 service prefix [3]. In a JSON policy document, they are specified as ec2:RunInstances and ec2:TerminateInstances [1][4]. 2. Resource-Level Permissions: You can grant access to all instances using a wildcard ("*") or restrict access to specific instances or groups of instances by providing their specific ARNs [1]. For example, a resource ARN for an instance is typically structured as arn:aws:ec2:region:account-id:instance/instance-id [1]. 3. Security Best Practices: Because RunInstances can be used to create resources with broad permissions, it is common to use conditions, such as ec2:ResourceTag or aws:RequestTag, to ensure that only authorized tags are applied during instance creation or to restrict actions to instances that have specific tags [5]. For the most comprehensive and up-to-date list of all EC2 actions, resource types, and condition keys, refer to the official AWS Service Authorization Reference documentation for Amazon EC2 [3][2].
Citations:
- 1: https://aws.amazon.com/blogs/security/demystifying-ec2-resource-level-permissions/
- 2: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference%5Fpolicies%5Factions-resources-contextkeys.html
- 3: https://docs.aws.amazon.com/service-authorization/latest/reference/list_ec2.html
- 4: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_action.html
- 5: https://repost.aws/knowledge-center/iam-policy-permission-ec2-tags-vpc
Correct the EC2 IAM action names. Use ec2:RunInstances and ec2:TerminateInstances; the singular forms are invalid and will break copied policies with AccessDenied on launch or cleanup.
🤖 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 `@ventis/controller/cloud_provider_logic/EC2/README.md` around lines 23 - 24,
Update the EC2 IAM action list in the README to use the valid plural names
ec2:RunInstances and ec2:TerminateInstances, while preserving
ec2:DescribeInstances and ec2:CreateTags.
Source: MCP tools
2 participants
Summary by CodeRabbit
New Features
t3.microinstances.Documentation