Fix server boot failure with telemetry enabled (lazy-load app constants in AuthenticatorGauge) - #3027
Open
Coalkid wants to merge 1 commit into
Open
Fix server boot failure with telemetry enabled (lazy-load app constants in AuthenticatorGauge)#3027Coalkid wants to merge 1 commit into
Coalkid wants to merge 1 commit into
Conversation
…nstants in AuthenticatorGauge
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
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.
Desired Outcome
The telemetry feature can be enabled as documented in TELEMETRY.md
(
CONJUR_TELEMETRY_ENABLED=true) without crashing the server on boot — as itworked up to and including 1.22.3. Currently on 1.24.0 the server fails during
Rails initialization with
NameError: uninitialized constant Monitoring::Metrics::AuthenticatorGauge::DBand crash-loops. Full analysis and reproduction in #3026.
Implemented Changes
Monitoring::Metrics::AuthenticatorGaugeresolved two autoloaded applicationconstants (
DB::Repository::AuthenticatorConfigRepositoryandAuthentication::ImplementedAuthenticators) in default constructor arguments.The gauge is instantiated eagerly in
config/initializers/prometheus.rb,before constants under
app/db/app/domainare autoloadable, so the firstconstant raised
NameError(and the second would raise next — a maskedinstance of the same problem). Regression introduced in 8382ff3, shipped in
1.24.0 alongside the Rails 7 upgrade.
This PR defaults both kwargs to
niland resolves the constants inupdate()via
||=.updateonly runs through the initializer's existinglazy_initpath (on the first request), when autoloading is ready — consistent with the
lazy-setup pattern already used in that initializer. Dependency injection via
kwargs is unaffected.
How to review/verify (no dev environment needed — stock image with the
patched file bind-mounted):
Verified results: with telemetry enabled the server boots,
/metricsreturns200 and
conjur_server_authenticatorreports enabled/installed/configuredauthenticators; with telemetry disabled behaviour is identical to stock
(no
/metricsroute).Unpatched 1.24.0 crashes | 1.22.3 works — see #3026.
Connected Issue/Story
Resolves #3026
CyberArk internal issue ID: N/A (community report)
Definition of Done
Changelog
CHANGELOG update
Note: the entry was added as a new version section per the note at the top of
CHANGELOG.md — happy to move it under
Unreleasedif you prefer.Test coverage
changes, or
Note: the existing request specs in
spec/lib/monitoring/metrics/authenticator_metrics_spec.rbalready exercisethe full path through
update()(including the new lazy resolution) andinstantiate the gauge without kwargs. The bug itself only manifests during
Rails initialization, which the spec environment does not reproduce (the app
is already booted when specs run) — this is also why CI did not catch the
regression.
Documentation
READMEs) were updated in this PRNote: TELEMETRY.md already documents the intended behaviour; this PR restores it.
Behavior
Note: bug fix restoring the documented behaviour of an existing feature;
the default (telemetry disabled) path is untouched.
Security
Note: the change only defers constant resolution; both lazily-resolved classes
are read-only, no authn/authz logic is touched, and the existing
/metricsexposure model is unchanged. The
||=lazy assignment matches the existingunsynchronized
lazy_initpattern inPrometheusCollector#call.