Repository navigation
fcvm-metal-arm: a second public IPv4 address, to switch over to - #666
Conversation
- firecracker-dev.tf: one secondary private IPv4 address on the box's ENI (private_ips_count = 1), a new Elastic IP (firecracker_dev_next) associated with it, local.firecracker_dev_public_ips holding both public addresses, and an output for the new one - fcvm-dns-test.tf, gpu-box.tf, ohio.tf, runner-vpc.tf: admit both addresses wherever the box's address is allowlisted - scripts/test-*.py: the three tests that pin those allowlists name the new address Outbound traffic still leaves through the current address. The switch is a later change: associate the new address with the primary private address and release the old one. Tested: terraform validate (1.10.3); test-fcvm-dns-test.py, test-runner-iam-boundary.py, test-ohio-runner-prep.py, test-dev-credential-boundary.py
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Layer / File(s) | Summary |
|---|---|
Allocate and expose the second public IP firecracker-dev.tf |
The ENI requests a secondary private IPv4 address. Terraform associates a second EIP with it, defines an ordered list of both public IPs, and exposes the second IP when the instance is enabled. |
Update DNS and SSH ingress sources fcvm-dns-test.tf, gpu-box.tf, ohio.tf, runner-vpc.tf, scripts/test-fcvm-dns-test.py, scripts/test-ohio-runner-prep.py, scripts/test-runner-iam-boundary.py |
DNS client CIDRs and SSH ingress rules include the new Firecracker address. The tests check the updated CIDRs and ingress rules. |
Priority:
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Feature
Possibly related PRs
- ejc3/aws#656: Creates the DNS client CIDR list that this change updates to include both Firecracker public IPs.
Merge Risk: 
c27ca
-
No supported merge-blocking risk remains in the reviewed changes.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (5 skipped: 5 … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: adding a second public IPv4 address for the Firecracker development box to support a later switch-over. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (5 skipped: 5 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c27ca7d134
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex security review |
✅ Action performed
|
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
@codex security review |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Adds a second inbound public IPv4 address to the ARM metal host through a secondary private address on its managed ENI. The original public address remains the outbound address; cutover and retirement of that address belong to a later change.
The DNS-test token and GPU/runner SSH allowlists admit both addresses. A managed State Manager association writes the secondary /32 into a netplan drop-in and adds it live without netplan apply or a networking restart. The drop-in survives ordinary reboots. Invalid generated configuration is rolled back before any live address change.
Validation: secondary-address behavior tests (including failure rollback), GPU launch boundaries, dev credential boundaries, DNS token, runner IAM and Ohio preparation tests pass. A native netplan generate --root-dir probe confirmed that the boot unit retains both DHCP and the secondary /32. CI also runs the new regression tests. Terraform formatting passes; live plan and verification follow merge.