fix: handle metrics-endpoint binding and APIError on k8s - #133
Merged
gfouillet merged 5 commits intoAug 27, 2026
Conversation
Fix three issues that cause the metrics-endpoint relation hooks to fail on k8s controllers (JUJU-10266): 1. controlsocket.APIError was never imported — the charm caught controlsocket.APIError but APIError lives in unixsocket. Any socket error crashed the hook with AttributeError. 2. _ensure_metrics_user only handled HTTP 409; the Juju controller returns HTTP 500 (password destroyed) when the user already exists. Now catches both 409 and 500, removes the stale user, and recreates it cleanly. 3. On k8s, network-get can return a service FQDN instead of an IP for bind_address early in the unit lifecycle. The prometheus_scrape library calls ipaddress.ip_network() on this value, raising ValueError. The charm now catches this, logs a warning, and defers the event so it retries when the binding resolves to an IP. Juju-side fix: https://warthogs.atlassian.net/browse/JUJU-10266
gfouillet
force-pushed
the
gh130-metrics-endpoint-k8s-bind-fqdn
branch
from
August 26, 2026 15:14
b1012aa to
780fbc3
Compare
Add two tests covering the untested re-raise branches introduced in PR juju#133: - test_metrics_ensure_user_reraises_non_recoverable_error: verifies that an APIError with a code not in (409, 500) is re-raised without attempting the remove/recreate recovery. - test_metrics_remove_user_reraises_non_404_error: verifies that a non-404 APIError from remove_metrics_user propagates during the 409-recovery flow.
charm.py now catches controlsocket.APIError (via the re-export added in the previous commit) rather than importing unixsocket directly. This keeps charm.py depending on controlsocket as its public interface and avoids a redundant direct dependency on the underlying socket module. Tests updated to import APIError from controlsocket for consistency.
Strengthen test_metrics_endpoint_update_scrape_job_spec_binding_unresolved by asserting update_scrape_job_spec was actually called after the ValueError, rather than only verifying no exception escaped.
Add four tests covering previously untested scenarios: - test_metrics_ensure_user_retry_failure_propagates: verifies that when both add_metrics_user calls fail with 500, the second error propagates and remove_metrics_user is called exactly once (Mn1). - test_metrics_endpoint_redefers_until_resolved: verifies that the charm keeps deferring while the binding is unresolved and succeeds once it resolves (Mn4). - test_metrics_endpoint_password_stable_across_deferred_retry: verifies that the metrics password does not churn across a deferred retry even when generate_password would return a different value (Mn5). - test_controlsocket_reexports_api_error: verifies that controlsocket.APIError is unixsocket.APIError (I3).
gfouillet
marked this pull request as ready for review
August 27, 2026 09:40
SimonRichardson
approved these changes
Aug 27, 2026
jujubot
added a commit
to juju/juju
that referenced
this pull request
Aug 27, 2026
…lure #23143 ## Description When `AddUser` returns `UserAlreadyExists`, the `auth.Password` passed to it has already been hashed and destroyed by `auth.HashPassword` (which calls `p.Destroy()` via `defer`). Reusing that same password instance for the subsequent `GetUserByAuth` call results in `ErrPasswordDestroyed` and an HTTP 500 response, breaking the idempotency of `POST /metrics-users`. `addMetricsUser` now accepts the raw password string and constructs separate `auth.Password` instances for `AddUser` and `GetUserByAuth`. Also adds `--no-prompt` to `remove-application` calls in the prometheus controller charm test suite to avoid interactive prompts in CI. ## Root cause In `internal/worker/controlsocket/worker.go`, `addMetricsUser` took a single `auth.Password`: ```go _, _, err = w.accessService.AddUser(ctx, service.AddUserArg{ Password: &password, // AddUser hashes and destroys this ... }) if internalerrors.Is(err, usererrors.UserAlreadyExists) { user, err := w.accessService.GetUserByAuth(ctx, validatedName, password) // password is destroyed here -> ErrPasswordDestroyed -> HTTP 500 } ``` The existing unit tests did not catch this because `accessService` was mocked with GoMock and did not execute the real `HashPassword` which zeroes the bytes. ## Fix ```go addPassword := auth.NewPassword(rawPassword) _, _, err = w.accessService.AddUser(ctx, service.AddUserArg{ Password: &addPassword, ... }) if internalerrors.Is(err, usererrors.UserAlreadyExists) { authPassword := auth.NewPassword(rawPassword) // fresh instance user, err := w.accessService.GetUserByAuth(ctx, validatedName, authPassword) } ``` ## Related controller charm PR The juju-controller charm-side fix is in **[juju/juju-controller PR #133](juju/juju-controller#133. ## Testing - `go test ./internal/worker/controlsocket/...` — 24 tests pass (including new `TestMetricsUsersAddAlreadyExistsPasswordNotDestroyed` which simulates `AddUser` destroying the password via `DoAndReturn`). ## QA steps **End-to-end validation with companion PR:** 1. Bootstrap a controller on microk8s with the agent built from this PR. 2. On the deployed controller, copy `src/controlsocket.py` and `src/charm.py` from [juju/juju-controller PR #133](juju/juju-controller#133). 3. Run the reproduce steps from [juju/juju-controller #130](juju/juju-controller#130): ```bash juju offer controller.controller:metrics-endpoint juju deploy prometheus-k8s --channel 1/stable --trust juju relate prometheus-k8s admin/controller.controller ``` 4. **Verify:** The controller SAAS stays `active` after ~5 s (not `error`), and `check_prometheus_targets` passes without retry timeout. ## JIRA [JUJU-10266](https://warthogs.atlassian.net/browse/JUJU-10266) [JUJU-10266]: https://warthogs.atlassian.net/browse/JUJU-10266?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Merged
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
2 participants
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.
Description
Fixes #130, Fixes #112. Related to #93.
JIRA: JUJU-10266
Fixes three issues affecting the
metrics-endpointrelation hooks on k8s and HA controllers:controlsocket.APIErrorwas never imported (Fixes metrics-endpoint-relation-created on every unit fail in cluster mode #112):The charm caught
controlsocket.APIError, butAPIErrorlives inunixsocket.py. Any socket error (e.g., user already exists, not found) crashed the hook withAttributeError: module 'controlsocket' has no attribute 'APIError'._ensure_metrics_userrecovery (Fixes metrics-endpoint-relation-created on every unit fail in cluster mode #112):The charm only handled HTTP 409; the Juju controller returns HTTP 500 (
password destroyed) whenAddUserconsumes and zeroes the password buffer before checkingUserAlreadyExists. The charm now catches both 409 and 500, removes the stale user, and recreates it cleanly.K8s service FQDN bind_address handling (Fixes metrics-endpoint-relation-created hook fails with ValueError on k8s when bind_address returns service FQDN #130, Related to Wrong host address for metrics scrape job #93):
On k8s,
network-getcan return a service FQDN instead of an IP forbind_addressearly in the unit lifecycle. Theprometheus_scrapelibrary callsipaddress.ip_network()on this value, raisingValueError. The charm now catches this, logs a warning, and defers the event so it retries when the binding resolves to a valid IP.Fly-by fixes
controlsocket.APIErrorre-export: Addedfrom unixsocket import APIErrortocontrolsocket.pyso the existingexcept controlsocket.APIErrorincharm.pyactually works. The charm now usescontrolsocket.APIErrorconsistently rather than importingunixsocketdirectly, keepingcontrolsocketas the public interface._ensure_metrics_usernon-(409,500),_remove_metrics_usernon-404), recovery retry failure propagation, repeated deferral until binding resolves, credential stability across deferred retry, andcontrolsocket.APIErrorre-export identity.Related Juju PR
The Juju-side fix for the
password destroyedroot cause is in juju/juju PR #23143.QA steps
End-to-end validation with companion PR:
src/controlsocket.pyandsrc/charm.pyfrom this PR.activeafter ~5 s (noterror), andcheck_prometheus_targetspasses without retry timeout.Testing
./run_tests— 43 unit tests pass (including error recovery for 409, 500, and 404, negative-path re-raise, retry failure propagation, repeated deferral, credential stability, and re-export identity).flake8— clean.microk8sintegration leg is the canonical signal.