Issues/fix vpc network acl#14
Open
ditahkk wants to merge 4 commits into
Open
Conversation
…ed replacements - Added support for creating, updating, and deleting ACL rules with detailed request structures. - Introduced methods for listing rules and resolving ACL names to IDs. - Updated ReplaceNetworkACL to use ACL IDs instead of slugs for better accuracy. - Enhanced ACL data structures to align with the live API response formats. fix(apierrors): improve resource not found error handling - Added regex matching for specific error messages indicating invalid resources. - Updated IsResourceNotFound to handle new error cases and improve clarity. feat(network): extend network functionality with VPC support - Added VPC-related fields to Network struct and adjusted JSON unmarshalling for backward compatibility. - Introduced GetDetail method to fetch detailed network information including CIDR and state. - Updated CreateRequest to include VPC and billing cycle parameters. feat(vpc): enhance VPC management with detailed state retrieval - Implemented detailed VPC retrieval including metadata such as CIDR and network domain. - Added error handling for not found cases to distinguish between transport errors and missing resources.
…s and usage instructions
📝 WalkthroughWalkthroughAdds v0.0.16 networking features: network detail and plan listing, VPC subnet (tier) creation with post-create ACL attachment, full ACL-list and per-rule CRUD with name→ID resolution, API model updates, CLI command expansions, tests, smoke-suite updates, and release/docs changes. Changesv0.0.16 VPC and ACL Workflow Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/smoke/cases.sh (1)
286-288:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
lc_ipstill uses the removed category-based gate.
fx_networknow provisions throughdet_network_plan, butlc_ipstill skips unlessdet_network_categoryis set. That will drop IP lifecycle coverage in environments where plan-based network creation works and category discovery does not. Gate this on the plan-based fixture path instead.🤖 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 `@tests/smoke/cases.sh` around lines 286 - 288, The lc_ip lifecycle test currently gates on det_network_category so it is skipped when category discovery is unavailable; update the gate to check the plan-based fixture path (det_network_plan) instead so IP tests run when networks are provisioned via fx_network/det_network_plan. Concretely, in the lc_ip block replace the det_network_category check with a check that det_network_plan is non-empty (or the existing fx_network/FX_IP fixture result), and ensure fx_ip is still invoked to set FX_IP before calling _lc_result and the subsequent "ip in list" run_case invocation.pkg/api/network/network.go (1)
275-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Update()falls back through a lookup path that can drop VPC subnets.When the PUT response comes back as
data:null, this method callss.Get(), andGet()still resolves viaList(). In this fileList()is the isolated-network path, so updating a VPC subnet can succeed remotely and still come back asnetwork "<slug>" not foundlocally. Re-fetch the resource through/networks/{slug}for the fallback instead of going back throughList().🤖 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/network/network.go` around lines 275 - 283, The Update() fallback currently calls s.Get(ctx, slug), which routes through List() and can miss VPC subnets; change the fallback to re-fetch the single resource via the dedicated network-by-slug endpoint rather than via List(). Replace the final return s.Get(ctx, slug) with a call that performs an explicit GET /networks/{slug} (implement or reuse a method like fetchNetworkBySlug or GetNetworkBySlug) so the code unmarshals env.Data into Network and, on null, calls that direct-by-slug fetch to return the accurate Network including VPC subnets.
🧹 Nitpick comments (1)
internal/commands/acl.go (1)
244-247: 💤 Low valuePort display for non-TCP/UDP protocols shows empty string.
When the protocol is
icmp,all, orprotocol_number, bothStartPortandEndPortare typically empty strings. Theportsvariable will be empty, which displays as a blank cell. Consider displaying-orN/Afor clarity.💡 Suggested improvement
ports := "" if r.StartPort != "" { ports = r.StartPort + "-" + r.EndPort + } else { + ports = "-" }🤖 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/acl.go` around lines 244 - 247, The ports string logic leaves a blank when StartPort/EndPort are empty (e.g., for ICMP/all/protocol_number). Update the code that builds ports (using variables/fields r.StartPort, r.EndPort, r.Protocol and the local ports variable) to provide a fallback like "-" or "N/A" when both StartPort and EndPort are empty or when r.Protocol is "icmp", "all", or "protocol_number"; otherwise keep the existing StartPort+"-"+EndPort formatting. Ensure you only change the ports assignment logic so display cells are never empty.
🤖 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 `@docs/configuration.md`:
- Around line 105-112: Replace the insecure inline token insertion that uses
printf with a method that does not expose the token on the command line or shell
history; update the instructions around creating ~/.secrets/zcp.zsh and setting
ZCP_BEARER_TOKEN to tell users to either (a) open and edit ~/.secrets/zcp.zsh
with their editor and add the export line, or (b) use an interactive stdin
prompt to capture the token and write it to ~/.secrets/zcp.zsh so the token
never appears in shell history, ensuring the file remains chmod 600 and that
~/.zshrc sources ~/.secrets/zcp.zsh.
In `@internal/commands/auth.go`:
- Around line 68-70: The validation message printed in the auth flow incorrectly
shows 'overrides profile ""' when an env-only token is used; update the
fmt.Fprintf call inside the os.Getenv("ZCP_BEARER_TOKEN") branch (the block near
the check for ZCP_BEARER_TOKEN) to conditionally include "overrides profile %q"
only when profile.Name is non-empty and otherwise print a message that indicates
an environment-provided token is being validated (reference ResolveProfile and
the existing fmt.Fprintf usage to locate the code).
In `@pkg/api/acl/acl.go`:
- Around line 169-179: The Resolve function currently returns a.ID on name match
even if a.ID is empty; update Resolve(ctx, vpcSlug, nameOrID string) to only
consider a match successful when a.ID is non-empty (i.e., require a.ID != ""
before returning), and in addition accept the legacy slug field by treating
a.Slug (or similar legacy identifier) as an alias when present (check a.Slug ==
nameOrID and ensure a.ID is non-empty before returning a.ID); keep using
List(ctx, vpcSlug) to enumerate ACLs and return a clear error if no valid id is
found.
In `@README.md`:
- Around line 308-309: The docs show the "zcp network create" example without
the required --type flag; update all "network create" examples and the command
description to include the required --type (e.g., --type isolated or the L2
value your CLI expects) and mark it as required in the prose so the command
surface is accurate; specifically, modify the example line "zcp network create
--name my-net --network-plan inet-yow --billing-cycle hourly --cloud-provider
zcp --region yow-1 --project my-project" to include the required "--type
<value>" and add a short note in the command taxonomy/description stating that
--type is mandatory for the isolated/L2 network create flow.
In `@RELEASE_NOTES.md`:
- Around line 16-20: The fenced code blocks containing the zcp CLI examples
(e.g., the block starting with "zcp network create --name web-tier ..." and the
other example around lines later in the file) need a language tag to satisfy
markdownlint MD040; edit RELEASE_NOTES.md and change the opening triple-backtick
markers to include "bash" (```bash) for both fenced examples so they are
recognized as Bash code blocks.
---
Outside diff comments:
In `@pkg/api/network/network.go`:
- Around line 275-283: The Update() fallback currently calls s.Get(ctx, slug),
which routes through List() and can miss VPC subnets; change the fallback to
re-fetch the single resource via the dedicated network-by-slug endpoint rather
than via List(). Replace the final return s.Get(ctx, slug) with a call that
performs an explicit GET /networks/{slug} (implement or reuse a method like
fetchNetworkBySlug or GetNetworkBySlug) so the code unmarshals env.Data into
Network and, on null, calls that direct-by-slug fetch to return the accurate
Network including VPC subnets.
In `@tests/smoke/cases.sh`:
- Around line 286-288: The lc_ip lifecycle test currently gates on
det_network_category so it is skipped when category discovery is unavailable;
update the gate to check the plan-based fixture path (det_network_plan) instead
so IP tests run when networks are provisioned via fx_network/det_network_plan.
Concretely, in the lc_ip block replace the det_network_category check with a
check that det_network_plan is non-empty (or the existing fx_network/FX_IP
fixture result), and ensure fx_ip is still invoked to set FX_IP before calling
_lc_result and the subsequent "ip in list" run_case invocation.
---
Nitpick comments:
In `@internal/commands/acl.go`:
- Around line 244-247: The ports string logic leaves a blank when
StartPort/EndPort are empty (e.g., for ICMP/all/protocol_number). Update the
code that builds ports (using variables/fields r.StartPort, r.EndPort,
r.Protocol and the local ports variable) to provide a fallback like "-" or "N/A"
when both StartPort and EndPort are empty or when r.Protocol is "icmp", "all",
or "protocol_number"; otherwise keep the existing StartPort+"-"+EndPort
formatting. Ensure you only change the ports assignment logic so display cells
are never empty.
🪄 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: b4ca184e-8baf-487f-9973-52e22e267a34
📒 Files selected for processing (27)
CHANGELOG.mdREADME.mdRELEASE_NOTES.mddocs/api-inventory.mddocs/command-taxonomy.mddocs/configuration.mddocs/roadmap.mdinternal/commands/acl.gointernal/commands/auth.gointernal/commands/commands_test.gointernal/commands/helpers.gointernal/commands/network.gointernal/commands/plan.gointernal/commands/profile.gointernal/commands/vpc.gopkg/api/acl/acl.gopkg/api/acl/acl_test.gopkg/api/apierrors/errors.gopkg/api/apierrors/errors_test.gopkg/api/network/network.gopkg/api/network/network_test.gopkg/api/plan/plan.gopkg/api/vpc/vpc.gopkg/api/vpc/vpc_test.gotests/smoke/README.mdtests/smoke/cases.shtests/smoke/lib.sh
…ACL and network operations
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/smoke/cases.sh (1)
337-353:⚠️ Potential issue | 🟡 MinorFilter
lc_acl’s VPC selection to the test region.
tests/smoke/cases.shselects the first VPC viazcp vpc list -o json ... | head -1with nodet_region(or usable-state) filtering; since other smoke resources are region-scoped, this can pick a cross-region/otherwise-incompatible VPC and make ACL tests flaky. Select a VPC scoped todet_regionand limited to usable states (e.g., Running/Stopped), or reuse an existing region-scoped VPC fixture if present.🤖 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 `@tests/smoke/cases.sh` around lines 337 - 353, lc_acl currently picks the first VPC unscoped, causing cross-region flakiness; update the VPC selection in the lc_acl function to restrict results to the test region and usable states by passing/adding the det_region filter and usable-state constraints to the zcp vpc list invocation (e.g., use --region "$det_region" and/or a usable-state filter) and then pipe to jq to only select entries where .region == env DET_REGION (or .region == "$det_region") and .state is one of the usable values (e.g., "Running","Stopped") before taking head -1; ensure the variable name vpc and subsequent calls (zcp acl create/update/delete-rule) remain unchanged so the rest of lc_acl works with the filtered VPC.
🧹 Nitpick comments (1)
tests/smoke/cases.sh (1)
318-326: 💤 Low valueConsider the async creation workaround.
Line 324 introduces a 5-second sleep before the List fallback to account for asynchronous egress rule creation in CloudStack. While this pattern is common in integration tests, fixed sleep durations are inherently brittle—too short and the test becomes flaky; too long and the suite slows down unnecessarily.
♻️ More robust retry pattern
Replace the fixed sleep with a retry loop:
s="$(jq -r '[.[]?|select(.field=="ID")|.value|select(.!="")][0] // empty' <<<"$out" 2>/dev/null)" -# rule creation is async — give CloudStack a moment before the list fallback -[[ -z "$s" ]] && { sleep 5; s="$(zcp egress list --network "$net" -o json 2>/dev/null | jq -r '.[0].id // empty')"; } +if [[ -z "$s" ]]; then + # rule creation is async — retry with exponential backoff + for i in 1 2 4; do + sleep "$i" + s="$(zcp egress list --network "$net" -o json 2>/dev/null | jq -r '.[0].id // empty')" + [[ -n "$s" ]] && break + done +fi🤖 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 `@tests/smoke/cases.sh` around lines 318 - 326, The current lc_egress function uses a fixed sleep (sleep 5) before falling back to zcp egress list, which is brittle; replace that single sleep+one-list attempt with a short retry loop that repeatedly calls zcp egress list and parses the id into s (using the same jq expression currently used: '.[0].id // empty') until a non-empty s or a max timeout/attempt count is reached (e.g., 10 attempts with 1s sleep or exponential backoff), breaking early when s is found and preserving the existing _lc_result "egress rule" "$s" && defer egress "$s" "$net" behavior; implement the loop inside lc_egress around the fallback logic so you no longer rely on the single sleep.
🤖 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/egress/egress.go`:
- Around line 128-138: In Create, don’t swallow errors from s.List and make the
fallback match deterministic: if resp.Data.ID is empty and you call s.List(ctx,
req.NetworkSlug), propagate and return the list error (don’t return &resp.Data,
nil on lerr); when scanning rules (rules[i]), include CIDR in the equality check
(e.g., match Protocol, StartPort, EndPort and, when req.CIDR is non-empty, match
CIDR as well) so you don’t return the wrong rule, and if no matching rule is
found return an explicit error instead of returning &resp.Data with empty ID.
---
Outside diff comments:
In `@tests/smoke/cases.sh`:
- Around line 337-353: lc_acl currently picks the first VPC unscoped, causing
cross-region flakiness; update the VPC selection in the lc_acl function to
restrict results to the test region and usable states by passing/adding the
det_region filter and usable-state constraints to the zcp vpc list invocation
(e.g., use --region "$det_region" and/or a usable-state filter) and then pipe to
jq to only select entries where .region == env DET_REGION (or .region ==
"$det_region") and .state is one of the usable values (e.g.,
"Running","Stopped") before taking head -1; ensure the variable name vpc and
subsequent calls (zcp acl create/update/delete-rule) remain unchanged so the
rest of lc_acl works with the filtered VPC.
---
Nitpick comments:
In `@tests/smoke/cases.sh`:
- Around line 318-326: The current lc_egress function uses a fixed sleep (sleep
5) before falling back to zcp egress list, which is brittle; replace that single
sleep+one-list attempt with a short retry loop that repeatedly calls zcp egress
list and parses the id into s (using the same jq expression currently used:
'.[0].id // empty') until a non-empty s or a max timeout/attempt count is
reached (e.g., 10 attempts with 1s sleep or exponential backoff), breaking early
when s is found and preserving the existing _lc_result "egress rule" "$s" &&
defer egress "$s" "$net" behavior; implement the loop inside lc_egress around
the fallback logic so you no longer rely on the single sleep.
🪄 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: dc0ccbce-0338-4597-bddc-056d26d79100
📒 Files selected for processing (11)
CHANGELOG.mdRELEASE_NOTES.mddocs/configuration.mdinternal/commands/acl.gointernal/commands/auth.gointernal/commands/network.gopkg/api/acl/acl.gopkg/api/acl/acl_test.gopkg/api/egress/egress.gotests/smoke/cases.shtests/smoke/lib.sh
✅ Files skipped from review due to trivial changes (3)
- CHANGELOG.md
- docs/configuration.md
- RELEASE_NOTES.md
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/commands/auth.go
- pkg/api/acl/acl_test.go
- internal/commands/acl.go
- internal/commands/network.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/egress/egress_test.go (1)
160-181: 📐 Maintainability & Code Quality | ⚡ Quick winAdd a regression test for the POST-success + LIST-failure fallback branch.
Current additions cover fallback match and no-match, but not the
Listfailure path that previously regressed. A dedicated test here would lock in the new error-propagation behavior.📌 Proposed test addition
import ( "context" "encoding/json" "fmt" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ func TestCreateFallbackNoMatchErrors(t *testing.T) { @@ } + +func TestCreateFallbackListFailureErrors(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + fmt.Fprint(w, `{"status":"Success","data":null}`) + return + } + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"status":"Error","message":"backend unavailable"}`) + })) + defer srv.Close() + + svc := egress.NewService(newClient(srv.URL)) + _, err := svc.Create(context.Background(), egress.CreateRequest{ + NetworkSlug: "my-net", Protocol: "tcp", StartPort: "80", EndPort: "80", CIDR: "0.0.0.0/0", + }) + if err == nil { + t.Fatal("Create() = nil error, want list failure error") + } + if !strings.Contains(err.Error(), "fetching it back failed") { + t.Fatalf("Create() error = %v, want wrapped list failure context", err) + } +}🤖 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/egress/egress_test.go` around lines 160 - 181, Add a new unit test that covers the POST-success + LIST-failure fallback branch: create a TestCreateFallbackListErrors function modeled after TestCreateFallbackNoMatchErrors where the httptest server returns {"status":"Success","data":null} for the POST but returns an HTTP error or non-success JSON for the subsequent LIST request; instantiate the service with egress.NewService(newClient(srv.URL)) and call svc.Create(...), then assert that the call returns a non-nil error (i.e., the List error is propagated) rather than a silent success; reference TestCreateFallbackNoMatchErrors, egress.NewService, newClient, and svc.Create when adding the test.
🤖 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.
Nitpick comments:
In `@pkg/api/egress/egress_test.go`:
- Around line 160-181: Add a new unit test that covers the POST-success +
LIST-failure fallback branch: create a TestCreateFallbackListErrors function
modeled after TestCreateFallbackNoMatchErrors where the httptest server returns
{"status":"Success","data":null} for the POST but returns an HTTP error or
non-success JSON for the subsequent LIST request; instantiate the service with
egress.NewService(newClient(srv.URL)) and call svc.Create(...), then assert that
the call returns a non-nil error (i.e., the List error is propagated) rather
than a silent success; reference TestCreateFallbackNoMatchErrors,
egress.NewService, newClient, and svc.Create when adding the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7ab28c2-c996-4751-bf73-cd187d6ed7ce
📒 Files selected for processing (2)
pkg/api/egress/egress.gopkg/api/egress/egress_test.go
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
1 participant
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
network getfor detailed network info andplan network(plan tables now include SLUG)Bug Fixes
Documentation