fix: secondarySubnets are CIDR strings, not sec-sub ids#49
Merged
Conversation
added 2 commits
July 15, 2026 10:55
Per the API spec (confirmed by the VKS team), the create bodies take secondarySubnets as a list of CIDRs (e.g. '10.5.60.0/22'); the SecondarySubnetDto's sec-sub-* uuid is NOT accepted. SubnetItem was extracting the uuid, and every description/guide said "Secondary subnet IDs" — an agent following them would build a body the backend rejects. - SubnetItem.from_api extracts `cidr` (plain strings pass through). - Field descriptions fixed on SubnetItem.secondary_subnets, NodeGroupSpec.secondarySubnets, CreateClusterComboDto.secondarySubnets — all now say CIDR strings with an example, "NOT sec-sub ids". - Cluster guide (CILIUM_NATIVE_ROUTING step) + getting-started routing + create_cluster docstring updated the same way. Live-verified: list_subnets on a VPC with secondary subnets now returns ['10.5.60.0/22', '10.5.71.0/26', '10.5.70.0/26']. Note: the greennode-cli --secondary-subnets help text has the same "IDs" wording bug (tracked separately; the CLI forwards raw strings so its behavior is unaffected).
…'s zones
Field finding: an agent speculated that a subnet outside the cluster's
own subnet list (MULTI-AZ cluster on zones B+C, node group on 1A) might
be rejected at create — false product knowledge invented in the gap our
guidance left. The rule is now explicit at three layers: the nodegroup
guide's subnet step, validate_nodegroup_create's docstring, and
list_subnets' workflow ("ANY ACTIVE subnet of the cluster's VPC is
eligible — it does not need to match the subnets/zones the cluster
itself uses, including MULTI-AZ").
This was referenced Jul 15, 2026
Merged
vks-team
added a commit
that referenced
this pull request
Jul 15, 2026
* docs: generate_kubeconfig asks the user for the expiration The expiration_days param (1-1825, default 30 = CLI parity) existed but nothing told agents to offer it, so they silently minted 30-day credentials. Same failure class as the optional-config groups: the docstring and param description now instruct asking the user (suggest 30), since a credential lifetime is the user's decision. * docs: one never-silent-defaults principle instead of per-docstring ASK notes Review pushback: behavioral "ASK the user" notes were re-accreting into docstrings — the same anti-pattern reverted in #46. Root cause of every such case (expiration_days, optional-config groups, os/public toggles) is one principle, now stated once in SERVER_INSTRUCTIONS: never silently accept a default for a parameter that encodes a user decision — ask. - generate_kubeconfig docstring/param stripped back to contract (1-1825, default 30, "a user decision"). - CLAUDE.md gains a guidance-placement policy table (docstring = contract; guide/prompts = choreography; instructions = session-wide principles; errors = next step) so the drift does not repeat. * fix: get_pod_logs handles pods with no logs yet (QC crash) A pod that has not started its container (Pending — e.g. a cluster with 0 nodes) yields None from the kubernetes client; the tool crashed with "'NoneType' object has no attribute 'splitlines'" instead of reporting an empty log. Guard with (logs or "") — the response is now a normal PodLogsData with log_lines=[] and the agent can say "no logs yet". * feat!: apply_yaml takes YAML content; generate_app_manifest writes no files QC finding: yaml_path pointed at the SERVER's filesystem — with the HTTP/remote deployment the server can never read the user's local file, so apply_yaml was unusable there (agents fell back to manage_k8s_resource with inline bodies). Inherited from EKS MCP, whose distribution model is local-stdio-only; ours also ships remotely. - apply_yaml: yaml_path (removed) -> yaml_content — the manifest itself, multi-document supported; transport-independent. The client reads local files on its side and passes the content. - generate_app_manifest: output_dir removed — a remotely-generated file was stranded inside the server container. The YAML now comes back in the response only; workflow is generate -> review -> apply_yaml (yaml_content). With no filesystem write it is a pure generator, so the (inconsistent) --allow-write gate is gone too — the tool was already annotated READ. BREAKING CHANGE: apply_yaml's yaml_path parameter is replaced by yaml_content; generate_app_manifest no longer accepts output_dir and requires no --allow-write. --------- Co-authored-by: tytv2 <tytv2@vng.com.vn>
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.
What
Per the API spec (VKS team confirmation), create bodies take
secondarySubnetsas CIDR strings ("10.5.60.0/22"), not thesec-sub-*uuids. The MCP server was extracting the uuid fromSecondarySubnetDto {uuid, name, cidr}and every description/guide said "Secondary subnet IDs" — an agent following the chain would build a body the backend rejects.SubnetItem.from_apinow extractscidr(plain strings pass through unchanged).SubnetItem.secondary_subnets,NodeGroupSpec.secondarySubnets,CreateClusterComboDto.secondarySubnets, the cluster guide's CILIUM_NATIVE_ROUTING step, getting-started routing, and thecreate_clusterdocstring — all now say CIDRs with an example and an explicit "NOT sec-sub ids".Verification
Live:
list_subnetsonvipt-multi-azreturns['10.5.60.0/22', '10.5.71.0/26', '10.5.70.0/26'](wassec-sub-…ids). 256 tests green.Note:
greennode-cli --secondary-subnetshelp text has the same "IDs" wording bug — CLI behavior is unaffected (it forwards raw strings), tracked separately.