feat(mesh): core command surface for Kong Mesh control planes - #2128
justindavies wants to merge 15 commits into
Conversation
|
Trusted E2E is required for this fork PR.
After maintainer review, run trusted E2E from the base repository: gh workflow run e2e.yaml --repo Kong/kongctl --ref main -f trusted_pr_number=2128 -f trusted_head_sha=76761c57891b86606344472d333398427d4f0791This trusted E2E result will apply only to the exact reviewed SHA above. Re-run trusted E2E if the contributor pushes another commit. |
|
|
rspurgeon
left a comment
rspurgeon
left a comment
There was a problem hiding this comment.
Review: changes requested
Reviewed commit 144018bdc2958bb2edcf0681577594de155e1e8f.
The discovery-driven resource design is useful, and reuse of existing output infrastructure is appropriate. However, target selection, command semantics, configuration integration, and feature-level test coverage need revisions before merge. The inline comments provide the specific locations and requested changes.
Correctness and UX
- A configured control-plane ID overrides an explicit control-plane name. Because the resolver serves writes and deletes, this can select the wrong target.
- The export
--profileoption shadows the global configuration/authentication profile. - Resource listings and inspection collections do not paginate completely.
createcan replace an existing resource without explicit replacement intent.- Advertised self-managed support unconditionally requires Konnect authentication.
- The explicit
get konnect meshcommand path is missing despite the promised direct/explicit convention.
Configuration and HTTP construction
The common ID/name/URL, mesh, and all-meshes options use config.Hook; PAT/base URL use existing configuration paths. Local mock checks confirmed mesh selection honors flag > environment > configuration file > default, and the all-meshes environment setting works. The target-selection defect concerns precedence between alternative selectors, not a general failure to load configuration.
Token-specific options, inspection type, and export selection are read directly from Cobra. Define and document configuration paths for persistent defaults, explicitly identify invocation-only inputs, and validate resolved required values where configuration is supported.
Reuse the existing configuration-aware HTTP construction structures instead of constructing default clients: shared timeout/transport resolution, httpclient.ClientConfig, and logging/authentication composition as appropriate. Reuse or extract a shared builder rather than introducing another construction policy. Keep input-download credentials separate from control-plane credentials.
Testing required
Add complete command-path tests, configuration precedence tests, HTTP integration coverage, and dedicated Mesh E2E scenarios. Include pagination, hosted/self-managed authentication, configured timeout/transport settings, existing-resource replacement behavior, and partial write failures. E2E coverage should include discovery/read, create/get/delete, export/reapply, and supported inspection/token flows, using isolated resources and cleanup.
Modernization, dependencies, and scope
- Follow the repository's Go 1.26 modernization standards, including
slices.SortStableFunc/slices.Sort, and rungo fix ./...plus the required formatting and quality gates after revisions. - Simplify the unnecessary singleton slices and repeated sorting in tag rendering.
- No new dependency/version conflicts were found:
go.modandgo.sumare unchanged. - Consider separating export/migration and inspection into follow-up PRs. The currently empty description does not establish why the full 4,688-line surface belongs in foundational support.
- Use the required
type(category): titlePR title and provide scope, rationale, and validation details.
Validation performed
- Passed: CGO-disabled build, full unit suite, integration suite, generic smoke/version E2E scenario, and local CLI/mock checks.
- Race tests were not run because they require CGO; the repository's CGO-disabled requirement prevents the race-enabled Makefile target from running as written.
- Local lint reported five findings in unchanged files, using golangci-lint 2.13.2 rather than the repository-specified 2.10.1. None pointed to the Mesh additions.
- No dedicated Mesh E2E scenarios exist in this PR; live Mesh behavior remains unverified.
- At review time, GitHub CI tests passed, while trusted E2E and CLA remained pending.
The review left the repository unchanged.
| // live there, so it is resolved here and the identifier written back to | ||
| // configuration — leaving the composition itself in one place. | ||
| func resolveBaseURL(helper cmd.Helper, cfg config.Hook) (string, error) { | ||
| baseURL, err := meshcommon.ResolveControlPlaneAPIURL(cfg) |
There was a problem hiding this comment.
High: honor explicit control-plane selection over configured defaults.
A configured konnect.mesh.control-plane.id makes this resolver return before considering --control-plane-name. I reproduced --control-plane-name new-name sending requests to configured old-id. This resolver also serves writes and deletes, so this can operate on a different control plane than the operator selected.
Make explicit selection override configured alternative selectors, or reject conflicting selections with an actionable error. Include command-level regression tests for ID/name/URL combinations across flags, environment variables, and configuration.
| "Files, directories, URLs, or - for stdin, holding the mesh resources to apply. Repeatable.") | ||
| } | ||
| if verb == verbs.Dump { | ||
| baseCmd.Flags().String(ProfileFlagName, ProfileFederation, |
There was a problem hiding this comment.
High: preserve the global configuration/authentication --profile flag.
This local flag shadows kongctl's global profile option. kongctl dump mesh --profile prod fails with invalid profile "prod" because it interprets the configuration profile as an export selection.
Rename this option, for example to --export-profile, and retain the global profile behavior. Test selecting a non-default configuration profile together with each export selection.
| return &cmd.ConfigurationError{Err: err} | ||
| } | ||
|
|
||
| body, err := fetch(helper, path) |
There was a problem hiding this comment.
Medium: paginate resource listings completely.
This performs only one collection request. In a local mock check, a response with one item, total: 2, and a next link caused a successful exit without fetching the remaining item. Text output also hides the pagination metadata, making the result appear complete.
Aggregate all pages consistently with existing get operations, while retaining single-resource reads. Inspection collections have the same single-fetch pattern and need equivalent handling. Cover multi-page responses and termination conditions with HTTP integration tests.
| mesh = "" | ||
| } | ||
|
|
||
| status, err := sendForStatus(helper, http.MethodPut, descriptor.ItemPath(mesh, resource.Name), resource.Body) |
There was a problem hiding this comment.
Medium: make replacement intent explicit instead of silently updating under create.
Every document is sent with PUT, so an existing resource can be replaced and reported as updated without the operator explicitly requesting replacement. The comment explaining why declarative apply was avoided does not resolve the surprising behavior of create.
Establish explicit replacement semantics, such as an opt-in overwrite operation or a clearly named update operation, and document them in user-facing help. Test existing-resource handling and partial failures, including an E2E create/get/delete lifecycle.
| return nil, 0, err | ||
| } | ||
|
|
||
| tokenSource, err := konnectcommon.GetAccessTokenSource(cfg, logger) |
There was a problem hiding this comment.
Medium: separate self-managed authentication from Konnect authentication.
The command advertises --control-plane-url for self-managed control planes, but this path unconditionally resolves Konnect credentials. I verified that an unauthenticated local control plane receives no request: the CLI fails first with resolve Konnect access token.
Support the appropriate self-managed authentication modes, keeping credentials scoped to their intended destination, or narrow the advertised capability until that support exists. Add authentication-path coverage for both hosted and self-managed targets.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cmd.AddCommand(meshCmd) |
There was a problem hiding this comment.
Medium: register the explicit Konnect command paths as well.
kongctl get mesh is registered here, but kongctl get konnect mesh fails with unknown command "mesh". The new constructors' comments promise both direct and explicit forms, matching the existing product convention.
Register the supported Mesh operations consistently in both trees and test the actual root command paths, not only the Mesh constructors or helpers.
|
|
||
| result, err := apiutil.RequestWithTokenSource( | ||
| ctx, | ||
| httpclient.NewLoggingHTTPClient(logger), |
There was a problem hiding this comment.
Reuse the existing configuration-aware HTTP client construction structures.
NewLoggingHTTPClient(logger) constructs a client with a fixed default timeout and default transport options. Mesh requests therefore bypass the configured HTTP behavior used by existing Konnect operations. The same pattern appears in control-plane listing and resource input downloads.
Reuse the existing timeout/transport resolvers (ResolveHTTPTimeout, ResolveHTTPTransportOptions), httpclient.ClientConfig, and logging wrapper; reuse or extract a shared builder where necessary rather than maintaining another construction policy. Preserve credential separation for input downloads. Add tests proving configured timeout and transport settings reach the client.
| return err | ||
| } | ||
|
|
||
| name, err := cmdObj.Flags().GetString(tokenNameFlagName) |
There was a problem hiding this comment.
Complete and document the configuration contract for Mesh options.
The common control-plane/mesh flags are correctly bound through config.Hook, but token-specific options are read directly from Cobra here. Export selection and inspection type have the same pattern, so they have no corresponding configuration-file/environment-variable support.
For options intended to support persistent defaults, define documented configuration paths, use the shared flag-binding pattern, and read effective values through configuration. Explicitly document invocation-only inputs. Where configuration can satisfy a required value, validate the resolved value rather than relying only on MarkFlagRequired. Add file/environment/flag precedence tests.
| selected = append(selected, descriptor) | ||
| } | ||
|
|
||
| sort.SliceStable(selected, func(i, j int) bool { |
There was a problem hiding this comment.
Follow the repository's Go 1.26 modernization standards.
Use slices.SortStableFunc here instead of sort.SliceStable, and slices.Sort for remaining necessary string sorts. Run go fix ./... followed by the repository's formatting and quality gates when making the revisions.
Also simplify displayTags in printers.go: each value is a singleton slice, the keys are already sorted, and the subsequent value/output sorting adds unnecessary work and complexity.
| } | ||
| } | ||
|
|
||
| func TestRequestPathScoping(t *testing.T) { |
There was a problem hiding this comment.
Add complete command, HTTP integration, and Mesh E2E scenario coverage.
These helper tests are useful, but the added suite does not exercise complete Mesh command wiring or HTTP flows. The reproduced profile collision, missing explicit product path, selector precedence bug, and first-page-only listing all escape the current tests.
Please add command tests for routing/flag validation, configuration precedence tests, and HTTP integration tests for pagination, authentication, configured clients, and write failures. Add isolated Mesh E2E scenarios covering discovery/read, create/get/delete, export/reapply, and supported inspection/token flows, with cleanup, and run them through the trusted E2E workflow. The existing generic smoke/version scenario does not validate this feature.
rspurgeon
changed the title
Introduce a mesh command container reached as `kongctl get mesh ...`, serving Kong Mesh control planes through a thin REST client. The resource surface is driven by the control plane's own /_resources endpoint rather than a resource type table compiled into kongctl, so policies and resource types added by newer Kong Mesh releases, including enterprise types, are usable without a kongctl release. Scope, aliases, read-only status and policy classification all come from the control plane, so no per-type code is needed. The control plane connection resolves from two sources so that Konnect hosted and self managed control planes present the same commands: an explicit control plane URL, or a Konnect control plane identifier composed onto the profile's Konnect base URL. Only the hosted path is wired up here; the seam is what keeps self managed support additive. Adds `get mesh resource-types` to list what a control plane serves. No new dependencies. Hosted control planes are reached over the Konnect base URL kongctl already resolves, authenticated with the existing PAT, following the raw HTTP pattern established by the regions command. Requires Kong Mesh 2.13 or later, the first release reporting shortName in the discovery response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 02c2da1)
Adds `get mesh <type> [name]` for every resource type a control plane
advertises, and retargets the hosted client at the Kong Mesh 3 API line.
A single Konnect mesh control plane is fronted by two prefixes that reach
two different control planes: /v1/mesh/control-planes/{id}/api serves the
2.14 resource set, while /v3/mesh/control-planes/{id} serves Kong Mesh 3.
The /api segment exists only on the v1 line. Verified against a live
control plane, where the v3 line reports 38 resource types against the v1
line's 59 — the 21 absent types being exactly those Kong Mesh 3 removes.
kongctl supports Kong Mesh 3 only, so it now composes the v3 prefix.
The read is driven entirely by /_resources: the type is resolved from the
descriptor by path, type name or short name, the URL is composed from its
path and scope, and the columns come from the three printers kumactl
carries on v3. No resource type is named in code, so a type added by a
newer Kong Mesh release works without a kongctl release — confirmed with
Workload and MeshIdentity, which are new in 3, and with the four
enterprise types, which need no enterprise code.
Notable details:
- There is no Mesh printer. Mesh.mtls was removed from the API in Kong
Mesh 3, so the NAME/mTLS/AGE columns kumactl printed on 2.x cannot be
populated, and Mesh falls through to the global printer. A test
asserts no printer emits an mTLS column.
- TAGS is the resource's labels merged with its gateway tags, labels
winning, which is what the control plane itself displays. It is not
the inbound tags an older Kuma showed.
- Optional /_resources fields keep their fallbacks. The v3 schema marks
all eight required, but a live v3 control plane leaves shortName empty
on 6 of 38 descriptors and the display names empty on 3, so
validating instead of falling back would reject a valid control plane.
Empty shortName marks a type that is deliberately not KRI addressable.
- Errors surface the control plane's own AIP-193 detail rather than a
status code, shared with Konnect's envelope.
- JSON and YAML pass the payload through unchanged, envelope included,
so kumactl-era scripts keep parsing it.
- Resource types cannot be cobra subcommands without a network call at
startup, so the container accepts arbitrary args. A near miss of a
real subcommand is still reported as a mistyped subcommand rather
than sent to the control plane.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 5ae477e)
Adds `create mesh -f` and `delete mesh <type> <name>`, registering the mesh container under the create and delete verbs. kongctl mesh was read only until now. Both are driven by /_resources like the read is, so no resource type is named in code. Verified against a live Kong Mesh 3 control plane creating and deleting MeshTrafficPermission, MeshTimeout and MeshRetry, addressing types by path and by short name. create accepts a file, a directory, stdin, or a URL, and reads a multi document YAML stream document by document the way kumactl apply does. JSON input needs no separate path, being valid YAML. Each document is addressed by its own `type` and `name`; `mesh` falls back to --mesh when a document omits it, and is ignored for global scoped types. The verb is create rather than apply because Kuma creates or replaces with a PUT, while kongctl's apply carries plan-and-diff semantics this does not have. The response status distinguishes a created resource from a replaced one, so re-applying reports "updated". Every document is reported, successes and failures together, so a partial apply stays legible instead of being masked by the first error; a failure sets the exit status once at the end. Writes to a read-only type are refused before sending, naming the type rather than relaying a 405 — the control plane enforces this too. Validation feedback comes from the control plane's AIP-193 invalid_parameters array, so a rejected document reports the offending field: spec.rules[0].allow[0].spiffeID (): must be a valid Spiffe ID: path cannot have a trailing slash The shared client now takes any method and an optional body, and returns the status alongside it. A URL source is fetched without the control plane credential, since the URL is not the control plane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 9a592d6)
Adds `create mesh dataplane-token` and `create mesh zone-token`, so a
dataplane or a zone control plane can be bootstrapped without kumactl.
Flags mirror kumactl generate, including --valid-for being required and
--tag splitting comma separated values into the multi value form the
control plane expects. The token is written to stdout with no trailing
newline, as kumactl does, so redirecting it produces a file holding
exactly the credential.
Verified against a live Kong Mesh 3 control plane. A dataplane token came
back bound to its name, mesh, tags and workload; a zone token came back
carrying its zone and scope.
`create mesh user-token` is deliberately absent. Probing the hosted
control plane, POST /tokens/user answers 404: the user token plugin is not
registered when the API server authenticates through Konnect, which it
does when hosted. User tokens are therefore a self-managed capability and
belong with the rest of phase 3, rather than being a command that cannot
work against any control plane reachable today.
--scope defaults to "cp" on the zone token, which matters more than it
looks. Omitting the scope makes the control plane answer 500 rather than
falling back to the distribution's full scope:
500 {"zone":"zone1","validFor":"60s"}
200 {"zone":"zone1","scope":["cp"],"validFor":"60s"}
kumactl defaults --scope to zone.FullScope, which Kong Mesh populates with
its control plane scope, so kumactl never meets that failure. Sending the
scope by default matches it and routes around the fault rather than
waiting on a fix. Kuma's own 3.0 upgrade note states the endpoint no
longer requires a scope, which holds for Kuma, whose full scope is empty,
but not for Kong Mesh. A test pins the default so it is not tidied away.
A zero or negative --valid-for is refused before sending, since the
control plane would accept it and mint a token that never expires.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 9b3acff)
Adds `get mesh control-planes` and makes --control-plane-name work on every mesh command. Until now a control plane could only be addressed by UUID, and there was no way to discover one from kongctl at all. A name is resolved to an identifier by listing the control planes, which needs a Konnect call and so cannot live in the configuration-only resolver in mesh/common. The client resolves it and writes the identifier back to configuration, leaving URL composition in one place. The no-control-plane case is now a sentinel error, so a caller able to reach Konnect can tell "nothing was selected" from "a name was given" and try the name before surfacing anything. Konnect does not constrain control plane names to be unique, so an ambiguous name is reported with the matching identifiers rather than resolved arbitrarily — picking one would send writes to a control plane the operator did not choose. The listing labels the version column API LINE rather than VERSION. That field is the Konnect API line, v0 or v3, not the version the control plane runs: a control plane labelled v3 is reached on the v3 prefix, while the v1 prefix on the same identifier reaches a 2.14 control plane. Calling it VERSION invites exactly the confusion that made an earlier session conclude no v3 control plane existed. The long help says where the real version comes from. Identifiers abbreviate in text output as they do elsewhere in kongctl, which is unhelpful for a command whose purpose is to hand over an identifier. Rather than special-case the shared output layer, the help names --text-id-format full and -o json, and points out that --control-plane-name usually removes the need to copy one. Verified against the live control plane: the listing, resolution by name, an unknown name, and the unchanged error when nothing is selected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 7e32cc1)
Two golangci-lint failures on the pull request. gopkg.in/yaml.v3 is on the gomodguard blocked list. The blocklist recommends sigs.k8s.io/yaml, which has no streaming decoder and would mean splitting multi document input by hand. go.yaml.in/yaml/v4 is not blocked, is already a direct dependency used in five other places here, and offers the same NewDecoder and Decode with the same io.EOF behaviour, so the change is an import swap. The repository does exempt yaml.v3 in internal/declarative/tags with //nolint:gomodguard_v2, but those exemptions cite its custom tag support, which is not why this code needed it. Moving to the unblocked library is better than widening the exemption. buildRows never used its descriptor parameter: the row carries every column, and which of them are printed is decided by headersFor and cellsFor. Dropped rather than blanked, since nothing needs it. Verified with golangci-lint v2.13.1, the version CI pins, under GOOS=linux: no issues in the mesh packages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 927ed6b)
A successful mesh write is answered with {"warnings":[...]} carrying
deprecation notices for the resource just written. applyResource went
through sendForStatus, which keeps the status code and discards the body,
so those notices were lost: applying a MeshRateLimit whose
onRateLimit.status is below 400 reported only "created" while the control
plane had asked for the field to be changed. Four types emit them today —
MeshService, MeshExternalService, MeshMultiZoneService and MeshRateLimit.
Adds sendForWrite alongside sendForStatus, returning the status and the
parsed warnings, and leaves the delete path on sendForStatus since a
delete response carries none. Warnings print as each document is applied,
so the notice sits with the write that caused it, and go to stderr in
kongctl's existing "warning: ..." form: the summary table on stdout stays
byte-identical and pipes cleanly. They are also carried on the result row,
so -o json does not lose what stderr reported.
Parsing is deliberately forgiving. A body that is empty, is not JSON, or
has no warnings yields none rather than an error, because failing a write
the control plane accepted would be worse than dropping a notice.
Verified against a Konnect control plane: the deprecating policy prints the
warning on stderr with the table on stdout and the warning in JSON output,
and a policy with a valid status prints no stderr at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit be52c508cd520fca1d149c70195ad4073eddafd2)
…rofile Both from review on Kong#2128. Selection accepted three alternatives but resolved them in a fixed order that never consulted the name, so a configured konnect.mesh.control-plane.id answered an explicit --control-plane-name. Since the same resolver serves writes and deletes, that could act on a control plane the operator did not choose. A selector given on the command line now decides the target, and two at once is reported rather than resolved by precedence: --control-plane-id and --control-plane-name select different control planes; provide only one An explicit ID is likewise no longer shadowed by a configured URL, via a new ControlPlaneAPIURLForID that addresses one control plane without re-entering the precedence. Configuration-only selection keeps the documented URL, ID, name order. The export selection was registered as --profile, which shadowed kongctl's global configuration profile: `dump mesh --profile tech` failed as an invalid export selection instead of switching profile. Renamed to --export-profile and given a configuration path (konnect.mesh.export-profile), read through configuration so a persistent default is honoured with the flag winning. Verified against a Konnect control plane: an explicit name that does not exist now fails instead of silently using the configured ID, an existing name resolves, conflicting selectors are rejected, `dump mesh --profile tech` now selects the profile, and `--export-profile all` exports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 2c96f3bbf4559ea308ef844a7b1a011f51aa6f31)
Continuing the review on Kong#2128. Collections were fetched once, so a mesh larger than one page silently reported a subset; the export path already had a correct listAll, so it is now shared from client.go rather than a second policy being written. `get` and both inspect collections use it, and the paging loop is extracted as paginate() taking the page fetch as a function, so its termination rules are testable without HTTP: a short first page is followed, several pages concatenate in order, an empty collection fetches once, and an empty page ends a total that overreports so a control plane cannot make it spin. Structured output rebuilds the envelope from everything collected, without a `next` link that would suggest there is more to read. Verified live: 24 dataplanes in the table, 24 in the JSON, total 24, no next. The constructors promised a direct and an explicit form but only the direct one was registered, so `get konnect mesh` failed with "unknown command". Registered for get, create and dump. Delete is deliberately absent: `delete konnect` is replaced by the declarative delete command and takes its own arguments, so a mesh subcommand there is read as one of them rather than dispatching. That is asserted rather than assumed — the command-path test checks the mesh command's own help appears, which caught the swallowed argument that a check for "unknown command" alone missed. Modernization: sort.SliceStable becomes slices.SortStableFunc, the remaining sort.Strings calls are gone, and displayTags no longer sorts singleton slices or re-sorts an already ordered list — its map is now map[string]string. `go fix ./...` reports no further changes. The repeated "Dataplane" literal is a named constant, which also clears the two pre-existing goconst findings, so the whole repository now lints clean. renderInspect takes the payload rather than raw bytes, removing a marshal-then-unmarshal round trip on the policy inspection path. Also corrected the dump examples, which still told operators to pass --profile for the export selection after it was renamed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 5e8c76db6cb32e4da46175939688dca392f2608b)
From review on Kong#2128. Mesh requests were made with NewLoggingHTTPClient(logger), which installs a default timeout and default transport, so a configured HTTP setting reached every other Konnect operation but not these. The same inline construction appeared in control plane listing and in resource input downloads. All three now go through one newHTTPClient that resolves the settings with the helpers the rest of the Konnect code uses, ResolveHTTPTimeout and ResolveHTTPTransportOptions, composed into httpclient.ClientConfig. The resolution is split into meshClientConfig so it can be asserted: the wrapped client keeps its settings private, so a test of the client itself could only check that one was returned. Credential separation is unchanged. Input downloads still go through apiutil.Request with no token source; what they gain is the configured timeout and transport, which is the part that should not differ by destination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 0aa7b18d7df0b9e3182bf37287f75f03c8facd5d)
Last of the mechanical items from review on Kong#2128. The control plane and mesh flags were bound through config.Hook, but the token options, the inspection type and the export selection were read straight from Cobra, so none of them could be set in a profile or by environment variable. Options that express a policy an operator applies repeatedly now have documented configuration paths, are bound through the shared table in BindFlags, and are read back through configuration so the flag wins over an environment variable, which wins over the file: konnect.mesh.token.valid-for konnect.mesh.token.scope konnect.mesh.inspect.type konnect.mesh.export-profile (added with the flag rename) --valid-for no longer uses MarkFlagRequired, which would have rejected a configured lifetime. The resolved value is validated instead, and the refusal names both ways to supply it: a positive token lifetime is required, for example 24h; set --valid-for or konnect.mesh.token.valid-for The inspection type is likewise validated after resolution, so a value from a profile is checked rather than reaching the request unchecked. The options that identify one invocation's subject -- which dataplane, workload, proxy type, tags or zone a token is for -- deliberately have no configuration path, because a persistent default there would issue a token for something other than what the operator named. Their help says so, and BindFlags records why they are absent. --zone keeps MarkFlagRequired since configuration cannot satisfy it. Precedence is covered by tests for the file, the flag, and the flag winning, alongside the resolution cases for the lifetime including absent and unparseable values. The two gosec G101 hits on the new paths are false positives on the word "token" and are suppressed with a reason; the repository lints clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit e08dcfbf330b333a1cd2ea56e480de0a0967cc9f)
From review on Kong#2128, which asked for explicit replacement semantics because `create` could replace an existing resource and report "updated". The behaviour is right and the verb was wrong. kumactl has no create verb at all: it has apply, whose implementation is a function named upsert that gets the resource, creates it when absent and updates it when present, and whose help reads "Create or modify Kuma resources". The created/updated reporting here comes from that same distinction. kongctl's own apply already means create or update as well -- "create/update only", as against sync, which also deletes -- so the semantics agree and only the mechanism differs, direct upsert rather than plan and diff. `apply mesh` is therefore the primary form, with `create mesh` kept for the name this first shipped under and its help pointing at apply. Neither is reachable as `apply konnect mesh`: that path is replaced by the declarative apply command and takes its own arguments, so mesh is read as one of them, the same reason `delete konnect mesh` cannot be registered. Both are noted where the registration happens. Refusing to replace was the other option the review offered, and it would have broken the export/reapply scenario the same review asks to be tested, since reapplying an export necessarily updates what is already there. Testing that scenario turned up a real gap: `dump mesh` did not strip `kuma.io/origin`, which kumactl removes from its federation profiles because the export exists to seed a global control plane and a resource still marked as zone-origin would be imported as one. Now stripped for the same two profiles, and kept for `all` and `no-dataplanes` as kumactl does. Labels inside spec, such as a HostnameGenerator's selector matchLabels, are configuration and are left alone. What that does not do is make an export reapplyable to the control plane it came from. The origin label is immutable, so a resource that reached global by syncing up from a zone is refused with "cannot be changed from zone to global" however the stream is written. The E2E scenario has to apply to a different control plane, which is worth settling before it is written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 19f2b522922ccc4545f393bc66ce1c277df2b4db)
From review on Kong#2128: --control-plane-url was advertised for self managed control planes, but the request path resolved Konnect credentials unconditionally, so an unauthenticated local control plane never received a request. The command failed first on the missing Konnect token. A self managed control plane authenticates its own callers, so Konnect credentials are neither required nor sent when one is addressed. The URL is what distinguishes the two, since a Konnect control plane is reached by ID or name through Konnect's base URL. Follows what kumactl supports for the same targets: --control-plane-token bearer token, sent as Authorization --ca-cert-file CA that verifies the control plane --client-cert-file client certificate, with --client-key-file --tls-skip-verify opt out of verification and no credential at all, which is the common local case: Kuma authenticates an API caller as admin over loopback. All five have configuration paths and go through the shared binding, so they can be set per profile. TLS settings reach the transport by extending httpclient.TransportOptions with a TLSClientConfig rather than building a second client here, keeping the configured timeout and transport behaviour that the rest of the mesh requests use. It is applied only when a self managed URL is set; Konnect is reached over its own certificates. Verified against a real control plane on both ports. Reads succeed with no credential and with a profile holding no Konnect configuration at all. Over https, verification fails without options, --tls-skip-verify succeeds, and --ca-cert-file gets past trust to fail on hostname instead, which is the port-forward rather than the flag. A missing CA file, a file with no certificate, and a certificate without its key are each reported before any request. Writes are refused by that control plane because its API server is read only, which it reports for all 37 types. The Konnect path is unchanged and still requires its credential. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit b4b8fbadbc191a7215147ee806db97f87058c7a7)
Follows the split: the column header constants arrived with the inspection work, while the printers here already used them. Defined alongside the printers instead, with the remaining literals replaced so a heading cannot drift between two tables showing the same field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pect Left behind by the split: the flag name and configuration path stayed in the shared options while the command that reads them moved to its own branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
144018b to
76761c5
Compare
justindavies
changed the title
rspurgeon
left a comment
rspurgeon
left a comment
There was a problem hiding this comment.
Follow-up review: substantial progress, changes still requested
Reviewed updated commit 76761c57891b86606344472d333398427d4f0791.
Moving export and inspect out is appropriate. Follow-up features depending on this foundation is expected, but the remaining correctness issues and missing feature-level coverage should be addressed before merging. The inline comments provide reproductions and specific requested changes.
Progress against the previous review
| Area | Assessment |
|---|---|
| Export/inspect scope and export profile collision | Removed from this PR |
| Resource pagination | Fixed; a local mock confirmed both pages are returned |
| HTTP client construction | Now uses existing configuration resolvers and client structures; destination-specific TLS isolation needs correction |
| Self-managed authentication | Unauthenticated, bearer-token, and TLS support added; target/authentication selection is still inconsistent |
| Token configuration | Lifetime/scope now support configuration; direct-path environment lifetime confirmed |
| Explicit control-plane selection | Original name-over-configured-ID case fixed; hosted selection over a configured self-managed URL still uses the wrong authentication mode |
| Explicit Konnect commands | Registered, but Mesh flags are not bound on that path |
| Create replacement semantics | Unresolved: adding apply retains the same upsert behavior under create |
| Modernization/simplification | The reported sorting and tag-rendering issues are addressed |
| Test coverage | Improved helper/help tests, but still no Mesh HTTP-flow or E2E scenarios |
| PR metadata | Title fixed; description remains empty |
Remaining findings
- High: remote input downloads inherit control-plane TLS identity and trust settings. A local HTTPS input server received the configured control-plane client certificate, and the control-plane skip-verification setting accepted its self-signed certificate. Keep destination-specific TLS settings separate while reusing general client construction.
- High: explicit hosted selection still uses self-managed authentication. A saved self-managed URL causes authentication/TLS classification to disagree with the newly resolved hosted URL. The local reproduction sent the saved self-managed token to the hosted control-plane endpoint instead of the Konnect credential.
- Medium: explicit Konnect commands ignore Mesh flags. The new registration passes a pre-run handler that does not bind Mesh configuration. The explicit URL command fails with no control plane selected; existing configuration can instead mask the ignored flags.
- Medium: create still replaces existing resources. The identical PUT implementation remains under both apply and create. Establish explicit replacement semantics or remove the redundant resource-create alias.
Coverage and cleanup
Please add complete command/HTTP tests for the above cases and dedicated Mesh E2E scenarios covering discovery/read, apply/get/delete, and token operations, using isolated resources and cleanup.
The added help tests cannot detect missing runtime bindings. Selector helper tests do not cover the combined target/authentication decision, and the configuration precedence test does not actually exercise environment variables. Those gaps explain why these regressions survive the passing suite.
Remove the stale dump mesh example from apply help, reconcile contradictory apply/create comments, and fill in the PR description with scope, rationale, and validation. No dependency changes or conflicts were found.
Validation performed
- Passed locally: CGO-disabled build, full unit suite, integration suite, lint with zero findings, generic smoke/version E2E, and targeted CLI/mock checks.
- Race tests were not run with CGO disabled.
- No live Mesh E2E scenario coverage exists in this PR.
- At evaluation time, GitHub CI tests passed; trusted E2E and CLA remained pending.
The rewritten branch was evaluated in a separate checkout, preserving the original checkout. No source changes were made during the review.
|
|
||
| // The configured timeout and transport apply here too; what stays separate | ||
| // is the credential, which apiutil.Request does not attach. | ||
| client, err := newHTTPClient(cfg, logger) |
There was a problem hiding this comment.
High: keep control-plane TLS credentials and trust settings out of remote input downloads.
This client builder installs the self-managed control plane's client certificate, private CA, and tls-skip-verify setting. Omitting the bearer token from apiutil.Request does not isolate TLS authentication.
I reproduced this with a local HTTPS input server: apply mesh -f https://<input-server>/input.yaml presented the configured control-plane client certificate to the input server. Its self-signed server certificate was also accepted through the control-plane skip-verification setting.
Reuse common timeout/transport construction, but separate destination-specific TLS identity and trust policy. A remote input download must not inherit the control plane's client certificate or skip-verification option. Add an HTTP/TLS integration test asserting that separation.
| // credentials are neither required nor sent. Resolving them regardless | ||
| // meant an unauthenticated local control plane never received a request: | ||
| // the command failed first on the missing Konnect token. | ||
| selfManaged := isSelfManaged(cfg) |
There was a problem hiding this comment.
High: derive authentication and TLS settings from the resolved target.
The URL resolver now honors an explicit hosted ID/name over a saved self-managed URL, but isSelfManaged(cfg) still independently checks whether that URL exists in configuration.
I reproduced a profile with a saved self-managed URL/token and a Konnect PAT, then ran get mesh meshes --control-plane-id new-id. Requests reached /v3/mesh/control-planes/new-id/... with the self-managed token instead of the Konnect credential. The same classification also controls TLS settings.
Resolve the target once, including whether it is hosted or self-managed, and consistently derive its URL, credentials, and TLS policy from that result. Cover explicit hosted selectors overriding a configured self-managed URL with complete request-level tests.
| return nil | ||
| } | ||
|
|
||
| meshCmd, err := mesh.NewMeshCmd(verb, addParentFlags, parentPreRun) |
There was a problem hiding this comment.
Medium: bind Mesh flags on the explicit Konnect command path.
This passes the general Konnect pre-run handler, which does not call meshcommon.BindFlags. Registering the command makes help work, but the actual Mesh flags do not reach configuration.
Reproduced:
get konnect mesh meshes --control-plane-url <local-server>
fails with no Kong Mesh control plane selected, while the direct get mesh form works. With existing configuration, ignored flags can instead leave the command using configured defaults.
Compose the Konnect and Mesh bindings for this path, including token subcommands. Test actual requests through both direct and explicit command trees, verifying that selectors and command-specific settings take effect.
| // which kumactl implements as an upsert and reports as created or updated. | ||
| // `create` is kept because it was the name this shipped under first. | ||
| func appliesResources(verb verbs.VerbValue) bool { | ||
| return verb == verbs.Apply || verb == verbs.Create |
There was a problem hiding this comment.
Medium: adding apply does not resolve replacement under create.
apply mesh is a better name for upsert behavior, but create mesh -f still reaches the identical write implementation. I reproduced a PUT to an existing resource followed by a successful exit and "result": "updated".
Remove the redundant resource-create alias or make replacement intent explicit. The comment that this is retained because it "shipped under" create needs clarification for an unmerged feature; compatibility should not preserve surprising behavior without an established requirement. Token issuance can remain under create.
Also reconcile the older comment in runCreateResources that says the operation uses create rather than apply.
| path := strings.Join(args[:len(args)-1], " ") | ||
|
|
||
| t.Run(path, func(t *testing.T) { | ||
| result := executeRootForTest(t, args...) |
There was a problem hiding this comment.
Add request-level command tests and dedicated Mesh E2E scenarios.
The additional tests are useful, but these command-path tests execute only --help, which bypasses the bindings and requests that currently fail on the explicit Konnect path. Selector tests exercise flag detection rather than the combined URL/authentication decision. TestMeshOptionPrecedence also claims environment coverage but contains no environment-variable case.
Add complete command/HTTP tests for direct and explicit paths, selector/authentication combinations, file/environment/flag precedence, pagination, TLS credential isolation, and write behavior. Add isolated Mesh discovery/read, apply/get/delete, and token E2E scenarios with cleanup, and run them through trusted E2E. There are still no Mesh scenarios in this PR; passing smoke/version does not validate the feature.
| %[1]s apply mesh -f ./policies --control-plane-id <id> | ||
|
|
||
| # Apply from stdin, which is how an export is reapplied | ||
| %[1]s dump mesh --control-plane-id <id> | %[1]s apply mesh -f - --control-plane-id <id>`, meta.CLIName) |
There was a problem hiding this comment.
Update the example after splitting export out of this PR.
This example invokes dump mesh, which is no longer available in this change. Use an available stdin example, and introduce export/reapply examples with the follow-up export feature. Please also reconcile stale apply/create comments so the help and implementation describe one consistent command contract.
3 participants
kongctl + Kong Mesh: user story and walkthrough
Every command below was run against a live Konnect-hosted global control plane
with two federated zone CPs (
zone1,zone2) on Kubernetes. Output is copiedfrom those runs, not composed. Test resources were created and deleted as part
of the walkthrough; both clusters and the global CP are back to their starting
state.
Who this is for
A platform engineer whose Kong Mesh global control plane is hosted in Konnect.
The global CP is Kong's infrastructure — they have no
kubectlaccess to it andno shell on it. A CLI is the only way they can read or write global state.
That framing matters for reviewing this PR: for a Konnect user, these commands
aren't a convenience over
kubectl. They are the only option.The walkthrough
1. Find the control plane
After
kongctl login:2. Ask the control plane what it serves
kongctl carries no built-in list of Kong Mesh resource types. It asks the
control plane, and renders what comes back:
This is the main design decision in the PR. When a Kong Mesh release adds a
policy type, it appears here with no kongctl change.
SCOPE,KINDandWRITABLEare all the control plane's answers —WRITABLE: noondataplanesis reported by the CP, not inferred by kongctl.
3. Write once, land in every zone
The policy was stored at global with
kuma.io/origin: global, and both zoneclusters received it through KDS:
One apply reached both zones. The alternative is applying the same manifest
against every zone cluster by hand.
Re-applying the same file reports
updatedrather than failing, matching theupsert semantics of
kumactl applyandkubectl apply:4. Read it back
5. Export
Profiles:
all,federation,federation-with-policies,no-dataplanes.The federation profiles are for seeding a different global control plane.
6. Tokens
The zone token's payload decodes to
{"Zone":"zone1","Scope":["cp"]}— thecpscope is what the KDS auth validator requires of a zone CP connecting toglobal (
pkg/kds/auth/tokens/zone_validator_adapter.go).7. Delete
Deletion propagates to the zones over KDS. Propagation is not instant — the
zone clusters still showed the resources immediately after the delete returned,
and were clear within a minute. Worth knowing when scripting against this.
Environment
zone1,zone2, both Kubernetes, both federated and connectedkong/kuma-cp:0.0.0-preview.v075f9c95ckumahq/kuma@origin/masterandKong/kong-mesh@origin/master