Repository navigation
feat(cluster): --dev-cluster for Ankra Cloud Kubernetes, credential optional for Ankra Cloud (ankra-t9c7w.167.6) - #428
Conversation
…ntial needed for Ankra Cloud (ankra-t9c7w.167.6) ankra cluster managed create --provider ankracloud_k8s --dev-cluster creates an Ankra Cloud dev cluster: one server that is control plane, node, load balancer and gateway, sized by --node-pool-size. It sends one pool named dev with one node and refuses another count, --network-cidr and --autoscaling. --credential-id is now optional with --provider ankracloud_k8s: the platform uses the organisation's built-in Ankra Cloud credential when the request names none, so the CLI omits the field instead of sending an empty id (which the platform rejects). Every other provider still requires it.
Ankra AI reviewVerdict: looks good. The change makes --credential-id optional only for ankracloud_k8s (with omitempty so the empty value is not sent) and adds a well-guarded --dev-cluster flag that rejects incompatible options and is covered by solid tests. One assumption about the request's node pool slice could not be verified from the diff. Note that CI status for the head commit is unknown, so build/test success is not confirmed. Findings
Reviewed commit 0825297. This review is read-only and advisory. |
![ankra-platform[bot]](https://avatars.githubusercontent.com/in/1164800?s=60&v=4){kind=small}
| if cmd.Flags().Changed("autoscaling") { | ||
| return withExitCode(exitUsage, errors.New("--dev-cluster is one server and cannot autoscale: omit --autoscaling")) | ||
| } | ||
| pool := &request.NodePools[0] |
There was a problem hiding this comment.
[medium] Possible index-out-of-range on request.NodePools[0]
applyDevClusterFlags dereferences request.NodePools[0] unconditionally, but the diff does not show where NodePools is populated relative to the applyManagedNetworkOptionFlags call. If the slice can be empty at this point (e.g. pool construction happens after network options are applied), --dev-cluster panics instead of returning a usage error. Confirm the ordering or guard with a length check.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Not reachable: the create command builds the request with exactly one node pool (the NodePools literal at the top of RunE in cmd/cluster_managed.go) before applyManagedNetworkOptionFlags runs, so NodePools[0] always exists here. No guard added, since it would be dead code.
There was a problem hiding this comment.
Moved in 0825297: the create command now calls applyDevClusterFlags itself, right after building the request with its one node pool, so NodePools[0] visibly exists. The network-options helper only sets options.DevCluster.
| // one node pool of exactly one node (named dev unless --node-pool-name says | ||
| // otherwise), no autoscaling and no network range, since the one server is | ||
| // the whole cluster and joins no network Ankra creates. | ||
| func applyDevClusterFlags(cmd *cobra.Command, request *client.CreateManagedClusterRequest) error { |
There was a problem hiding this comment.
[low] --dev-cluster does not reject --private-network-id
The doc comment says a dev cluster 'joins no network Ankra creates', and --network-cidr is rejected, but --private-network-id (adopting an existing network) is silently accepted alongside --dev-cluster. If a dev cluster truly joins no network, that combination should be refused too; otherwise the accepted-but-ignored flag is a silent no-op that misleads the user.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Fixed in 9bceb58: --dev-cluster now refuses --private-network-id with a usage error, and the refusal test covers it.
|
Clusters fleet lane, read-only read of e13a90f (3 files); nothing changed on the branch. Two notes on the Ankra AI review, verified against the code:
Evidence remark: "this build created the first dev cluster in se-sto1" is the create request succeeding through the API. That cluster (sto-dev-check) then failed after 30 min on the IPv6 probe (ankra-t9c7w.167.7, fixed by ankra-cloud#268 at 23:22Z), so the CLI path is proven to the API, not to a running cluster; worth restating once a dev cluster is actually up. Checks on e13a90f: lint-test-build success (two runs), Ankra AI review neutral. ankra-t9c7w.167.6 carries this PR's URL and the body's |
| if cmd.Flags().Changed("autoscaling") { | ||
| return withExitCode(exitUsage, errors.New("--dev-cluster is one server and cannot autoscale: omit --autoscaling")) | ||
| } | ||
| pool := &request.NodePools[0] |
There was a problem hiding this comment.
[medium] applyDevClusterFlags indexes NodePools[0] without a length guard
This dereferences request.NodePools[0] unconditionally; if the node pool list is ever empty when applyManagedNetworkOptionFlags runs (e.g. a future code path that builds options before pools, or pool construction being skipped on some validation path), this panics instead of returning a usage error. The diff does not show where NodePools is populated relative to this call, so I cannot confirm it is always non-empty here. Consider an explicit len check that returns an error, or move the dev-cluster shaping to where the pool is created.
Ankra AI review. Read-only and advisory.
| options.PublicIPv4 = &publicIPv4 | ||
| isSet = true | ||
| } | ||
| if devCluster, _ := cmd.Flags().GetBool("dev-cluster"); devCluster { |
There was a problem hiding this comment.
[low] Dev-cluster validation lives inside the network-options function
applyDevClusterFlags rejects --node-pool-count, --autoscaling, and --private-network-id, none of which are network options, so applyManagedNetworkOptionFlags now does more than its name and comment claim. Not a bug, but moving the dev-cluster validation call up into the create run function (next to the credential check) would keep the network helper honest and make the validation order obvious.
Ankra AI review. Read-only and advisory.
…de the request it shapes (ankra-t9c7w.167.6)
| if cmd.Flags().Changed("autoscaling") { | ||
| return withExitCode(exitUsage, errors.New("--dev-cluster is one server and cannot autoscale: omit --autoscaling")) | ||
| } | ||
| pool := &request.NodePools[0] |
There was a problem hiding this comment.
[low] NodePools[0] indexed without a length check
applyDevClusterFlags indexes request.NodePools[0] directly. The diff does not show how request.NodePools is populated earlier in the command, so I could not verify it is always non-empty at this point; if it can ever be empty (e.g. a future code path or flag combination), this panics. Confirm the slice is guaranteed to contain the initial pool, or guard with a length check.
Ankra AI review. Read-only and advisory.
|
Clusters fleet lane, post-merge read of 0825297 (merged 01:36:10Z 10-05), read-only; nothing changed on the branch. The two commits after the lane's 00:32Z read match the threads: 9bceb58 closes the [low] on the CLI side ( The server half of the |
2 participants
Bead: ankra-t9c7w.167.6
What
ankra cluster managed create --provider ankracloud_k8s --dev-clustercreates an Ankra Cloud dev cluster: one server that is the control plane, the only node, the load balancer and its own gateway, sized by--node-pool-size, with no control-plane fee. The platform side shipped in cluster#3891. The CLI sends one node pool nameddevwith one node, and refuses--node-pool-countother than 1,--network-cidrand--autoscaling.--credential-idis now optional with--provider ankracloud_k8s. The platform uses the organisation's built-in Ankra Cloud credential when the request names none; the CLI now omits the field instead of sending an empty id, which the platform rejects with 422. Every other provider still requires it.Verification
go vetandgo test ./cmd/ ./internal/client/pass, with new tests for the dev cluster request, its refusals, and the optional credential.🤖 Generated with Claude Code