Skip to content

fix(garm): treat 401 from controller-info as initialised#283

Merged
cbartz merged 6 commits into
mainfrom
fix/garm-controller-info-401-initialized
Jul 13, 2026
Merged

fix(garm): treat 401 from controller-info as initialised#283
cbartz merged 6 commits into
mainfrom
fix/garm-controller-info-401-initialized

Conversation

@cbartz

@cbartz cbartz commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Fixes GarmApiClient.is_initialized() so it treats a 401 from GARM's controller-info endpoint as "initialised" (same as 200), instead of raising and failing the charm hook.

Why we need it

The first-run check probes GET /api/v1/controller-info with a deliberately unauthenticated client. GARM puts that endpoint behind two middlewares: an init-required gate (returns 409 until first-run creates the admin user) and JWT auth (returns 401 when there's no token). The code only understood 200 → initialised and 409 → not-initialised, and raised on anything else.

On an already-initialised GARM (e.g. after a charm upgrade with persisted PostgreSQL), the init gate passes and the tokenless probe hits the auth gate → 401, which was wrongly treated as fatal and failed the config-changed hook. It only appeared to "self-heal" because Juju's retry found the workload config hash already applied and short-circuited restart() before re-reaching the probe. The bug is latent for any config/charm change that rewrites the workload config on an initialised controller.

A 401 unambiguously means the request got past the init gate, so it means initialised — the probe never needs a token to answer that question.

Test plan

  • Extended the existing test_is_initialized parametrize table in charms/garm/tests/unit/test_garm_api.py with a 401 → True case.
  • tox -c tox.toml -e unit → 229 passed; -e lint and -e static (pyright: 0 errors) clean.

Review focus

  • The semantic claim that 401 on the unauthenticated controller-info probe reliably means "initialised" (init gate passed, auth gate rejected). wait_for_ready() is intentionally unchanged — 200/401/409 all confirm the HTTP server is up, and it still retries only on connection-level errors.

Potential breaking changes / new dependencies

None. No new dependencies, APIs, or workflow changes.

Checklist

  • Changes comply with the project's coding standards and guidelines (see CONTRIBUTING.md and STYLE.md)
  • CONTRIBUTING.md has been updated upon changes to the contribution/development process (e.g. changes to the way tests are run) — n/a, no process change
  • Technical author has been assigned to review the PR in case of documentation changes (usually *.md files) — n/a, changelog only
  • I updated docs/changelog.md with user-relevant changes
  • I used AI to assist with preparing this PR
  • I added or updated tests as needed (unit and integration)
  • If integration test modules are used: I updated the workflow configuration — n/a
  • If this PR involves a Grafana dashboard: I added a screenshot of the dashboard — n/a
  • If this PR involves Terraform: terraform fmt passes and tflint reports no errors — n/a
  • If this PR involves Rockcraft: I updated the version — n/a
  • If this PR adds/removes a charm, or changes a charm's base class, conventions, tooling, or repo structure: I updated the relevant AGENTS.md — n/a
  • If this PR changes .copilot-collections.yaml or .github/instructions/: I re-checked whether the AGENTS.md "12-factor divergences" guidance still matches upstream — n/a

cbartz added 2 commits July 13, 2026 09:01
is_initialized() probes GET /controller-info without auth. GARM gates
that endpoint on two middlewares: a 409 until first-run has created the
admin user, then JWT auth. On an already-initialised GARM (e.g. after a
charm upgrade with persisted PostgreSQL), the unauthenticated probe
passes the init gate and hits the auth gate, returning 401, which was
being raised as a fatal GarmApiError and failing the config-changed
hook. 401 now means initialised, same as 200; only 409 means
not-yet-initialised.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the GARM charm’s first-run initialization probe so that an unauthenticated GET /api/v1/controller-info returning 401 Unauthorized is treated as “already initialized” (same as 200), rather than raising and failing hooks (notably config-changed) on previously-initialized controllers.

Changes:

  • Update GarmApiClient.is_initialized() to treat ApiException(status=401) as initialized (True), while keeping 409 as not-initialized (False).
  • Extend unit tests to cover the new 401 → True behavior.
  • Document the user-facing fix in docs/changelog.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
charms/garm/src/garm_api.py Adjusts initialization detection logic to treat 401 from controller-info as initialized.
charms/garm/tests/unit/test_garm_api.py Adds a 401 case to the is_initialized() parametrized test table.
docs/changelog.md Notes the hook-failure fix in the changelog entry for 2026-07-13.
Comments suppressed due to low confidence (1)

charms/garm/src/garm_api.py:115

  • wait_for_ready()'s docstring still says True (200) even though is_initialized() now also returns True on 401. This is now misleading for future maintainers reading the readiness logic.
                raise GarmApiError(
                    f"Unexpected response from GARM controller-info ({exc.status}): {exc.body}"
                ) from exc
            except urllib3.exceptions.HTTPError as exc:
                raise GarmConnectionError(f"GARM connection error: {exc}") from exc

Comment thread docs/changelog.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread charms/garm/src/garm_api.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread docs/changelog.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@cbartz cbartz marked this pull request as ready for review July 13, 2026 10:51
@cbartz cbartz enabled auto-merge (squash) July 13, 2026 11:08
@cbartz cbartz merged commit a94a2f4 into main Jul 13, 2026
61 checks passed
@cbartz cbartz deleted the fix/garm-controller-info-401-initialized branch July 13, 2026 11:08
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.

3 participants