fix: update VPN and VPC API structures and improve error handling#11
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR standardizes API shapes (VPN customer gateways, VPN users, VPC VPN gateways, network, ipaddress), makes service Create/Update tolerant to metadata/null responses by falling back to GET or post-listing, and aligns CLI flags, request wiring, output, and tests with those schema changes. ChangesAPI Schema Updates and CLI Command Alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/commands/vpn.go (1)
84-103: ⚡ Quick winNew
CustomerGatewayRequestfields are not exposed via CLI.The API model now includes
IKEDH,ESPDH, andESPPFSfields (lines 41-45 inpkg/api/vpn/customergateway.go), butaddCustomerGatewayFlagsdoes not register corresponding--ike-dh,--esp-dh, or--esp-pfsflags. Users cannot set these parameters via the CLI.If this is intentional for a phased rollout, consider adding a TODO comment. Otherwise, wire the flags:
♻️ Proposed fix to add missing DH/PFS flags
func addCustomerGatewayFlags(cmd *cobra.Command, name, gateway, cidr, psk, ikePolicy, espPolicy *string, ikeLifetime, espLifetime, ikeEncryption, ikeHash, ikeVersion, espEncryption, espHash *string, + ikeDH, espDH, espPFS *string, forceEncap, splitConnection, dpd *bool) { cmd.Flags().StringVar(name, "name", "", "Customer gateway name") // ... existing flags ... cmd.Flags().StringVar(espHash, "esp-hash", "", "ESP hash algorithm") + cmd.Flags().StringVar(ikeDH, "ike-dh", "", "IKE Diffie-Hellman group (optional)") + cmd.Flags().StringVar(espDH, "esp-dh", "", "ESP Diffie-Hellman group (optional)") + cmd.Flags().StringVar(espPFS, "esp-pfs", "", "ESP Perfect Forward Secrecy group (optional)") cmd.Flags().BoolVar(forceEncap, "force-encap", false, "Force UDP encapsulation") // ... }Then update the request construction in both create and update commands to include these fields.
🤖 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 `@internal/commands/vpn.go` around lines 84 - 103, addCustomerGatewayFlags is missing CLI flags for the new CustomerGatewayRequest fields IKEDH, ESPDH and ESPPFS, so users cannot set --ike-dh, --esp-dh or --esp-pfs; add StringVar flags for each (e.g. bind variables for ikeDH, espDH, espPFS with flag names "ike-dh", "esp-dh", "esp-pfs" and appropriate help text) in addCustomerGatewayFlags, then update the create and update request construction code that builds CustomerGatewayRequest to populate IKEDH, ESPDH and ESPPFS from those flag variables (ensure you use the exact field names IKEDH, ESPDH, ESPPFS and the same flag variable names you added).
🤖 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 `@pkg/api/ipaddress/ipaddress.go`:
- Line 48: The CreateRequest struct gained an optional Project field but the "ip
allocate" command that builds an ipaddress.CreateRequest is not populating it;
add a new CLI flag (e.g. --project) to the "ip allocate" command, wire the flag
into the command's options parsing, and set CreateRequest.Project when
constructing ipaddress.CreateRequest; also update any related tests to pass the
project value (or add coverage for the flag) so the new field is exercised.
In `@pkg/api/network/network.go`:
- Line 127: The UpdateRequest struct's Description field is now always
serialized (json:"description") which causes CLI code that builds
network.UpdateRequest{ Name: name, Description: description } in
internal/commands/network.go to send an empty description when --description
isn't provided; either revert the struct tag to json:"description,omitempty" on
UpdateRequest.Description or change the CLI/service to only set the Description
field when the user explicitly supplies --description (e.g., populate
Description conditionally before constructing network.UpdateRequest) so existing
descriptions aren't unintentionally cleared.
In `@pkg/api/vpc/vpc.go`:
- Around line 237-244: The current fallback in the create flow calls
s.ListVPNGateways(ctx, vpcSlug) and returns gateways[0], which can return the
wrong gateway if multiple exist; instead capture the pre-create list (IDs) by
calling s.ListVPNGateways before creating, then after the create call call
s.ListVPNGateways again and diff the two lists to locate the newly added gateway
(compare by unique identifier such as gateway.ID or another stable field used in
your model), and return that matched gateway; update the logic around the create
function and the post-create listing code (the block using s.ListVPNGateways and
returning gateways[0]) to use this comparison and handle the case where no new
gateway is found by returning a clear error.
---
Nitpick comments:
In `@internal/commands/vpn.go`:
- Around line 84-103: addCustomerGatewayFlags is missing CLI flags for the new
CustomerGatewayRequest fields IKEDH, ESPDH and ESPPFS, so users cannot set
--ike-dh, --esp-dh or --esp-pfs; add StringVar flags for each (e.g. bind
variables for ikeDH, espDH, espPFS with flag names "ike-dh", "esp-dh", "esp-pfs"
and appropriate help text) in addCustomerGatewayFlags, then update the create
and update request construction code that builds CustomerGatewayRequest to
populate IKEDH, ESPDH and ESPPFS from those flag variables (ensure you use the
exact field names IKEDH, ESPDH, ESPPFS and the same flag variable names you
added).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fc5579a-7079-420a-9011-5bff40716730
📒 Files selected for processing (7)
internal/commands/vpc.gointernal/commands/vpn.gopkg/api/ipaddress/ipaddress.gopkg/api/network/network.gopkg/api/vpc/vpc.gopkg/api/vpn/customergateway.gopkg/api/vpn/user.go
…N customer gateways
- Enhanced API documentation for VPN, DNS, Projects, ISOs, Affinity Groups, Templates, Monitoring, Object Storage, Billing, Profile, SSH Keys, Support, Plans, Discovery, Store, and Auth endpoints. - Added new endpoints for VPN customer gateways and updated existing ones. - Updated command taxonomy to reflect changes in CLI commands and parameters. - Modified network and VPC update commands to allow optional description parameter. - Improved error handling and response validation in VPN customer gateway update logic. - Updated tests to ensure correct behavior for new and modified API endpoints and commands.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/api/vpn/vpn_test.go (1)
113-123: ⚡ Quick winAssert the fallback GET path in
TestCustomerGatewayUpdate.The GET branch currently accepts any path. Add a path assertion so fallback-route regressions are caught (not just “GET happened”).
Suggested test hardening
var gotBody map[string]interface{} var gotPutPath string + var gotGetPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") switch r.Method { case http.MethodPut: gotPutPath = r.URL.Path json.NewDecoder(r.Body).Decode(&gotBody) // Return metadata envelope (no VPN fields) — Update always falls back to Get. json.NewEncoder(w).Encode(apiEnvelope{Status: "ok", Data: vpn.CustomerGateway{Slug: "cgw-1", Name: "updated-cgw"}}) case http.MethodGet: + gotGetPath = r.URL.Path json.NewEncoder(w).Encode(apiEnvelope{Status: "ok", Data: fullCG}) default: http.Error(w, "unexpected method", http.StatusMethodNotAllowed) } })) @@ if gotPutPath != "/vpn-customer-gateways/cgw-1" { t.Errorf("path = %q, want %q", gotPutPath, "/vpn-customer-gateways/cgw-1") } + if gotGetPath != "/vpn-customer-gateways/cgw-1" { + t.Errorf("GET path = %q, want %q", gotGetPath, "/vpn-customer-gateways/cgw-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 `@pkg/api/vpn/vpn_test.go` around lines 113 - 123, In TestCustomerGatewayUpdate's fake handler switch (the r.Method cases), add an assertion in the http.MethodGet branch to verify the incoming fallback GET path is what you expect instead of accepting any path; for example, compute the expected fallback path from the test fixture (e.g., using fullCG.Slug or the same route pattern the client should call) and assert r.URL.Path == expectedPath (use t.Fatalf or t.Errorf) so route regressions are caught; locate this change inside the TestCustomerGatewayUpdate test where gotPutPath/gotBody and the GET branch are defined.pkg/api/vpc/vpc_test.go (1)
190-238: ⚡ Quick winCover the new update contract in this test.
TestVPCUpdatenow passes a non-nilDescription, but it never asserts that"description"is serialized, and it still doesn't exercise the newdata:null→Getfallback inService.Update. The changed update semantics can regress without a test failure.🤖 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 `@pkg/api/vpc/vpc_test.go` around lines 190 - 238, Update TestVPCUpdate to cover the new update contract: make the fake server record request sequence and body, have the first PUT handler respond with {"status":"ok","data":null} to trigger the Service.Update fallback to perform a GET (ensure svc.Update is the method under test and UpdateRequest contains Description), then respond to that GET with the full VPC JSON (the updated variable). Assert that the PUT request body contains both "name" and "description" keys with expected values, and assert that the test observed a subsequent GET to "/vpcs/vpc-upd-1" (i.e., that svc.Update performed the Get fallback when data is null) as well as the returned v.Slug check.
🤖 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 `@pkg/api/vpc/vpc.go`:
- Around line 229-233: Remove the hard failure when the pre-create snapshot call
ListVPNGateways fails inside CreateVPNGateway: treat the call as best-effort by
capturing/logging the error and setting the pre-list variable (before) to nil or
an empty sentinel, then continue to attempt the POST/create. Ensure subsequent
logic in CreateVPNGateway checks whether the pre-list is present before using it
to diff and only requires it when the create response is null and the diff
fallback runs. Reference: ListVPNGateways, CreateVPNGateway, and the before
variable.
In `@pkg/api/vpn/customergateway.go`:
- Around line 105-107: In Create, stop swallowing JSON decode errors for
env.Data: when len(env.Data) > 0 && string(env.Data) != "null" call
json.Unmarshal into partial and check its error; if err != nil return a clear
decode/BadRequest (or wrapped) error indicating metadata JSON could not be
parsed (include err), rather than assigning to _ so failures surface immediately
(update the block around env.Data, partial in Create to validate and return on
unmarshal error).
---
Nitpick comments:
In `@pkg/api/vpc/vpc_test.go`:
- Around line 190-238: Update TestVPCUpdate to cover the new update contract:
make the fake server record request sequence and body, have the first PUT
handler respond with {"status":"ok","data":null} to trigger the Service.Update
fallback to perform a GET (ensure svc.Update is the method under test and
UpdateRequest contains Description), then respond to that GET with the full VPC
JSON (the updated variable). Assert that the PUT request body contains both
"name" and "description" keys with expected values, and assert that the test
observed a subsequent GET to "/vpcs/vpc-upd-1" (i.e., that svc.Update performed
the Get fallback when data is null) as well as the returned v.Slug check.
In `@pkg/api/vpn/vpn_test.go`:
- Around line 113-123: In TestCustomerGatewayUpdate's fake handler switch (the
r.Method cases), add an assertion in the http.MethodGet branch to verify the
incoming fallback GET path is what you expect instead of accepting any path; for
example, compute the expected fallback path from the test fixture (e.g., using
fullCG.Slug or the same route pattern the client should call) and assert
r.URL.Path == expectedPath (use t.Fatalf or t.Errorf) so route regressions are
caught; locate this change inside the TestCustomerGatewayUpdate test where
gotPutPath/gotBody and the GET branch are defined.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05ced36c-2b3c-4953-9ceb-52505b186029
📒 Files selected for processing (13)
CHANGELOG.mdRELEASE_NOTES.mddocs/api-inventory.mddocs/command-taxonomy.mdinternal/commands/ip.gointernal/commands/network.gointernal/commands/vpc.gointernal/commands/vpn.gopkg/api/network/network.gopkg/api/vpc/vpc.gopkg/api/vpc/vpc_test.gopkg/api/vpn/customergateway.gopkg/api/vpn/vpn_test.go
✅ Files skipped from review due to trivial changes (3)
- docs/command-taxonomy.md
- RELEASE_NOTES.md
- CHANGELOG.md
…mer gateway operations
ditahm6
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation