Skip to content

fix(cluster): include registry_password in Create/UpdateClusterRequest#95

Merged
omattsson merged 1 commit into
mainfrom
fix/cluster-registry-password
May 27, 2026
Merged

fix(cluster): include registry_password in Create/UpdateClusterRequest#95
omattsson merged 1 commit into
mainfrom
fix/cluster-registry-password

Conversation

@omattsson

@omattsson omattsson commented May 27, 2026

Copy link
Copy Markdown
Owner

Summary

CreateClusterRequest / UpdateClusterRequest in cli/pkg/types/types.go had registry_url, registry_username, and image_pull_secret_name but not registry_password. The backend has accepted it since the registry_password column was added in migration 0042 — stackctl just silently dropped it from the JSON body.

Any caller that needed to set ACR / private-registry pull credentials had to drop down to curl because the password fell out. The kvk-k8s-dev bootstrap (scripts/bootstrap-local.sh) and ACR-token refresh (scripts/refresh-acr-token.sh) both hit this and can't fully migrate off direct API calls until this lands.

Change

  • Add RegistryPassword (string + *string in the update variant) to both request types, beside the existing url/username/pull-secret triple.
  • Two new round-trip tests in client_test.go:
    • TestCreateCluster_RegistryCredentialsRoundTrip — asserts all 4 registry fields appear on POST /api/v1/clusters with snake_case keys.
    • TestUpdateCluster_RegistryCredentialsRoundTrip — same for PUT /api/v1/clusters/:id, plus a negative assertion that untouched pointer fields stay out of the body (verifies omitempty on pointers).

No CLI flag added — credentials should not be passed on the command line. Callers use cluster create --from-file / cluster update --from-file, matching the rest of the admin-payload pattern.

Test plan

  • go vet ./... clean
  • go test ./... full suite green
  • Both new round-trip tests pass

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added support for specifying registry password during cluster creation
    • Registry password can now be configured and updated for existing clusters
  • Tests

    • Added test cases verifying registry credentials are correctly serialized during cluster operations
    • Tests ensure unrelated fields are not sent when not provided

Review Change Stack

The two wire types omitted RegistryPassword even though the backend has
accepted it since the registry_password column was added in migration
0042. Any stackctl caller that needed to write ACR / private registry
credentials had to drop down to curl because the password would silently
fall out of the JSON body — the kvk-k8s-dev bootstrap and ACR-token
refresh scripts both hit this.

Added the field to both request types (alongside the existing url/username/
image_pull_secret_name) and locked the wire contract with two tests:

- TestCreateCluster_RegistryCredentialsRoundTrip asserts all four
  registry fields make it onto POST /api/v1/clusters as their snake_case
  JSON keys.
- TestUpdateCluster_RegistryCredentialsRoundTrip asserts the same for
  PUT /api/v1/clusters/:id, and additionally verifies untouched pointer
  fields stay out of the body (omitempty is preserved).

No CLI flag added on purpose — credentials should not be passed on the
command line. Callers use `cluster create --from-file` /
`cluster update --from-file`, which is the same pattern the rest of the
admin payloads use.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6bac0e88-52a7-461c-a67f-b3065b2d44e7

📥 Commits

Reviewing files that changed from the base of the PR and between a13f77d and 571504a.

📒 Files selected for processing (2)
  • cli/pkg/client/client_test.go
  • cli/pkg/types/types.go

📝 Walkthrough

Walkthrough

This PR adds support for registry password credentials in cluster creation and update operations. The RegistryPassword field is introduced to both CreateClusterRequest and UpdateClusterRequest types, enabling clients to specify or update registry authentication. Tests verify that these credentials serialize correctly into HTTP request bodies.

Changes

Registry Password Support

Layer / File(s) Summary
Registry password type definitions
cli/pkg/types/types.go
CreateClusterRequest gains a RegistryPassword string field; UpdateClusterRequest gains an optional RegistryPassword pointer field. Both use JSON/YAML tags for registry_password with omitempty.
Registry credentials serialization tests
cli/pkg/client/client_test.go
TestCreateCluster_RegistryCredentialsRoundTrip and TestUpdateCluster_RegistryCredentialsRoundTrip stand up HTTP test servers to verify registry credentials (URL, username, password, image pull secret) serialize correctly, and that unrelated omitted fields are excluded from the request JSON.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding registry_password field to Create/UpdateClusterRequest types.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cluster-registry-password

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

@omattsson omattsson merged commit 1e2e941 into main May 27, 2026
7 checks passed
@omattsson omattsson deleted the fix/cluster-registry-password branch May 27, 2026 13:21
omattsson added a commit that referenced this pull request May 28, 2026
…olicies, cluster CRUD (#100)

* test(live): expand suite — apikey CRUD, user lifecycle, template versions, cleanup policies, cluster CRUD

Adds five new endpoint-group live tests covering the highest-blast-radius
surfaces still missing from cli/test/live/. All tests are wire-shape
focused (no real workloads created), follow the existing helpers/cleanup
conventions in this package, and run cleanly under the CI api-only flow
introduced in #99.

New files:
  apikey_live_test.go
      - Create → list → revoke cycle against the calling user (whoami).
      - Locks the raw_key contract: sk_-prefixed, returned once, never
        in list. Was implicitly relied on by the CI bootstrap but
        never asserted.

  user_live_test.go
      - Register a throwaway user, list (admin-only path), disable,
        enable, reset-password, delete. Never operates on admin —
        locking out the caller would break the rest of the suite.

  template_versions_live_test.go
      - Publishes the same template twice (description-only change in
        between) to materialise two version snapshots, then exercises
        list → get → diff with shape assertions on left/right/chart_diffs.

  cleanup_policy_live_test.go
      - Full admin CRUD plus a dry-run execution. The condition
        "idle_days:9999" deliberately matches nothing so the run never
        mutates a real instance.

  cluster_lifecycle_live_test.go
      - Stub-cluster create → get → update → delete. IsDefault stays
        false so the test never disrupts requireCluster() for other
        tests. Exercises registry_* + image_pull_secret_name fields
        (the registry_password drift in PR #95 is the canonical
        example of why this surface needs a live test).

Bonus findings surfaced during local validation against rancher-desktop
(left as commented follow-ups, not blockers for this PR):

  - stackctl's UpdateTemplateRequest.Name is `omitempty` but the
    backend rejects PUT with "name is required" when omitted. Either
    drop the omitempty or relax the backend.
  - stackctl's CreateTemplateRequest has no `version` field, so the
    template-level Version is unsettable through the CLI — the version
    snapshot's `version` round-trips empty as a result.
  - Backend rejects kubeconfig_data unless KUBECONFIG_ENCRYPTION_KEY
    is configured (the CI compose env doesn't set it); kubeconfig_path
    works without that prerequisite.

Verified locally against rancher-desktop k8s-stack-manager: full live
suite passes (21 passed, 2 skipped by design, 0 failed).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(live): address review — guard raw_key slice + find-first user lookups

Two correctness fixes from CodeRabbit on PR #100:

  - apikey_live_test.go: promote the raw_key length check from
    assert.Truef to require.Truef. A failing length check followed
    by created.RawKey[:3] would panic instead of failing cleanly.

  - user_live_test.go: after DisableUser/EnableUser, look up the
    created user in the list response first (find-first pattern,
    matched by the other live tests) and require.NotNil before
    asserting on the flag. The previous range-and-skip would silently
    pass if the user was missing from the response.

The companion table-driven + t.Parallel() refactor suggestion for
cleanup_policy_live_test.go is deliberately skipped — same reason as
on PR #99: every other *_live_test.go file in this package uses
ad-hoc subtests and runs serially against a shared backend by design.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Olof Mattsson <olof.mattsson@klaravik.se>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
omattsson added a commit that referenced this pull request May 28, 2026
…er (#101)

* test(contract): assert pkg/types request shapes against backend swagger

Closes #97.

Adds a unit-test-only contract layer that validates every stackctl
request struct against the backend's published OpenAPI (Swagger 2.0)
schema. Catches the class of bug shipped four times this week (#95,
#98, k8s-sm#264, the BulkOperationResult shape) where unit /
integration / e2e tests all stub the backend and decode requests
into stackctl's OWN types — so a json-tag drift between stackctl and
the backend decodes cleanly in tests but 400s the moment a real
backend reads the bytes.

What ships:

  cli/test/contract/testdata/swagger.json
      Vendored copy of omattsson/k8s-stack-manager:backend/docs/swagger.json.
      Pinned (not auto-fetched) so backend-side wire changes show up
      as explicit diffs in this repo.

  cli/test/contract/refresh-swagger.sh
      One-shot refresh: `./refresh-swagger.sh [ref]`. Validates the
      payload is JSON, sha256-compares before overwriting, and tells
      you which test to re-run. Pinned to a ref so partial backend
      rollouts don't break stackctl CI.

  cli/test/contract/contract_test.go
      Embeds swagger.json via go:embed, walks 9 request structs via
      reflection, and asserts bidirectionally:

        1. Every Go json tag exists as a property in swagger.
           Catches stale or typoed Go-side fields.
        2. Every swagger "required" field has a Go json tag.
           Catches missing-required-field bugs.
        3. Field types align (string/boolean/integer/number/array/object).

      Covers: CreateClusterRequest, UpdateClusterRequest,
      BulkInstancesRequest, BulkTemplatesRequest, RegisterRequest,
      LoginRequest, CreateAPIKeyRequest, ResetPasswordRequest,
      CreateCleanupPolicyRequest.

      Plus TestSwaggerVendorIntegrity — sanity floor on the vendored
      copy so a truncated swagger.json doesn't manifest as a pile of
      confusing per-case failures.

Failure-mode validated with a synthetic drift (matches the
pre-fix/bulk-wire-contract bug exactly):

  Error: Go struct field Ids (json:"ids") has no matching property
         in swagger schema — typo, or backend doesn't accept this field
  Error: swagger schema requires field "instance_ids" but the Go
         struct has no field with that json tag — request will fail
         validation

V1 deliberately skips:
  - Response types (issue calls out polymorphism for GET endpoints;
    a separate iteration).
  - CreateTemplateRequest / UpdateTemplateRequest — backend's swag
    annotation uses an unexported struct (createTemplateRequest) so
    no schema is published. Worth a tiny upstream PR to make the
    type exported + annotated.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(contract): refresh-swagger.sh — list python3 in deps, validate via stdin

Both address CodeRabbit on PR #101: the script invokes python3 for
JSON validation but the requirements comment listed only curl and
shasum, and the validation embedded the temp filename in the inline
Python string (mktemp output is normally safe, but stdin redirection
is cleaner regardless and removes any quoting concern).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Olof Mattsson <olof.mattsson@klaravik.se>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant