fix(rotation): use configurable default policy for certs with NULL rotation_policy_id

[evan-datadog]

Summary

Root cause (IR-57191 / CLOUDR-2089): in_rotation_window used a bare RotationPolicy.days reference that produced a cartesian join against all rotation_policies rows. For certs with rotation_policy_id = NULL, this silently matched the maximum days value across all policy rows (70) rather than raising or using a configured default.

Fixes:

Scope note: the is_attached_to_endpoint fail-closed guard (review Issue #1) is split into a separate PR: #361 — this PR is scoped to the rotation-window fix only.

• Replaces the cross-join with a correlated subquery + COALESCE in the SQL expression path, so each cert resolves its own policy's days, with NULL falling back to LEMUR_DEFAULT_ROTATION_INTERVAL
• Adds a None guard to the Python instance path for the same fallback
• Adds _default_rotation_days() helper: reads LEMUR_DEFAULT_ROTATION_INTERVAL (default 60), validates it is a positive int, and logs a warning + falls back to 60 if invalid
• Sets LEMUR_DEFAULT_ROTATION_INTERVAL = 60 in default.conf.py and tests/conf.py — single source of truth for both the DB seed and the NULL-policy fallback
• Unified the 30/60 default (review Issue Create ci-dd.yml #2): manage.py seeds the "default" RotationPolicy row via _default_rotation_days() instead of a hardcoded 30 fallback, so the seeded policy and the runtime NULL-policy fallback can never diverge.
• Syncs existing "default" policies (follow-up to Issue Create ci-dd.yml #2): lemur init now calls sync_default_rotation_policy() (new in policies/service.py) — creates the policy if missing, otherwise updates an existing row's days in place to the configured default. Deployments seeded by the historical migration (30d) or with a custom value (e.g. sandbox 35d) are brought in sync with the 60d NULL-policy fallback on the next lemur init.
• Data migration for existing deployments (follow-up): lemur init is not run by conductor deploys (containers run lemur start / celery worker / celery beat only), so the sync alone wouldn't reach existing DBs. Added migration c3d4e5f6a7b8 that updates the named "default" RotationPolicy to 60 days, applied via the documented lemur db upgrade step (downgrade reverts to 30). Chained from the master head 44d67c1988a2 (the a1b2c3d4e5f6 break-glass migration is on a separate branch, not master) so it applies cleanly to existing deployments.

⚠️ Behavioral change (broader than NULL-policy certs): The old cross-join matched if any policy row satisfied the condition (effectively max(days) = 70 for all certs). After this fix, certs with an explicit shorter policy no longer match the 70-day window — they use only their own policy's days. Expect a step-down in reissue/expiry-check volume after merge; this is correct behavior, not a regression.

Changes

File	Change
lemur/certificates/models.py	in_rotation_window hybrid property: correlated subquery + COALESCE (SQL path); None-policy guard (instance path); new _default_rotation_days() helper reading LEMUR_DEFAULT_ROTATION_INTERVAL (validated, default 60); .as_scalar() for SQLAlchemy 1.3.24 compat
lemur/policies/service.py	New sync_default_rotation_policy(): creates the "default" policy if missing, or updates an existing row's days to _default_rotation_days() — closes the seed-vs-fallback divergence for existing deployments
lemur/manage.py	InitializeApp calls sync_default_rotation_policy() (was a hardcoded 30 fallback + skip-if-exists) — existing "default" policies are synced to the configured default on lemur init
lemur/migrations/versions/c3d4e5f6a7b8_*.py	Data migration: update the named "default" RotationPolicy to 60 days (the LEMUR_DEFAULT_ROTATION_INTERVAL default) — reaches existing deployments via lemur db upgrade, since lemur init isn't run on deploy; chained from master head 44d67c1988a2; downgrade reverts to 30
lemur/default.conf.py	Add LEMUR_DEFAULT_ROTATION_INTERVAL = 60; correct comment (lemur init, not create_config)
lemur/tests/conf.py	Add LEMUR_DEFAULT_ROTATION_INTERVAL = 60
lemur/tests/test_certificates.py	Unit tests: config override, missing-key fallback (no KeyError), instance-level in/out of window with rotation_policy=None; SQL-level test with wide-policy fixture to prove no cross-join; fixed contradictory docstring
.gitignore	Ignore .worktrees/
local/src/lemur.conf.py	Commented-out local override example for LEMUR_DEFAULT_ROTATION_INTERVAL (60)

Sandbox validation (validate-70d-rota, 2026-08-11)

Deployed this branch (v130078016-de08cd9f) to the Lemur sandbox and validated against real + synthetic certs:

• _default_rotation_days() = 60 post-deploy
• NULL-policy cert expiring 30d → IN rotation window, 60d → IN (boundary <=), 90d → OUT — all PASS
• Explicit 30d-policy cert @60d → OUT (proves the cross-join is gone); explicit 90d-policy cert @60d → IN (own policy respected) — PASS
• get_rotation_candidates() end-to-end + instance/SQL path parity — PASS
• Real API-created NULL-policy cert entered the window after its expiry was flipped to 30d — PASS
• Breakage probes: Issue Latest Upstream changes  #1 confirmed (non-AWS plugins fail-open in the pre-fix code — now fail-closed) and Issue Create ci-dd.yml #2 confirmed (seeded "default" policy 35d vs 60d fallback — now unified) — both addressed in this revision
• Migration applied in sandbox: lemur db upgrade ran 44d67c1988a2 -> c3d4e5f6a7b8, updating the "default" policy from 35 → 60 days; custom policies untouched (saml=65); alembic version advanced to c3d4e5f6a7b8
• Sandbox image restored to mutable-latest-prod, all test certs cleaned up

Test plan

• Unit: _default_rotation_days() uses LEMUR_DEFAULT_ROTATION_INTERVAL value when set
• Unit: _default_rotation_days() returns 60 (no KeyError) when key absent
• Unit: instance in_rotation_window returns True for NULL-policy cert expiring in 30 days
• Unit: instance in_rotation_window returns falsy for NULL-policy cert expiring in 90 days
• Unit (SQL): class-level in_rotation_window includes a NULL-policy cert inside the default window, excludes one outside, and excludes an explicit-short-policy cert that the old cross-join would have pulled in (wide-policy fixture)
• Unit: sync_default_rotation_policy() creates the "default" policy at 60 when missing; updates an existing 30d row to 60; no-ops when already 60
• Sandbox: full rotation-window boundary validation (30/60/90d + explicit policies + rotation candidates) on the deployed branch
• Sandbox: migration c3d4e5f6a7b8 applied via lemur db upgrade — "default" policy 35 → 60, custom policies untouched, alembic version advanced
• EXPLAIN on Certificate.in_rotation_window shows correlated subquery, no cross-join (manual verification)

Fixes CLOUDR-2089

Workspace: local

🤖 Generated with Claude Code

[bencebeky]

Code Review Findings

Findings from a dual-engine review (Claude pr-review-toolkit agents + Codex), covering master...HEAD.

1. KeyError on LEMUR_DEFAULT_ROTATION_POLICY_DAYS — the key is set nowhere

_default_rotation_days() (lemur/certificates/models.py:79-80) uses bracket access with no fallback:

def _default_rotation_days():
    return current_app.config["LEMUR_DEFAULT_ROTATION_POLICY_DAYS"]

The only place that would define this key, local/src/lemur.conf.py, ships the assignment commented out. It's absent from lemur/default.conf.py and lemur/tests/conf.py too. Every other config read in this file uses .get(key, default) — this is the sole bracket-access read. As committed, any certificate with no rotation policy raises KeyError, and on the class-level SQL path this breaks the entire query (not just NULL-policy rows) at both call sites (get_all_pending_reissue() and get_certs_for_expiring_deployed_cert_check() in service.py), since _default_rotation_days() is evaluated once at query-construction time.

Suggested fix: current_app.config.get("LEMUR_DEFAULT_ROTATION_POLICY_DAYS", 60), and uncomment/set the line in local/src/lemur.conf.py.

2. .scalar_subquery() does not exist in the pinned SQLAlchemy 1.3.24

lemur/certificates/models.py:406:

policy_days = (
    select([RotationPolicy.days])
    .where(RotationPolicy.id == cls.rotation_policy_id)
    .correlate(cls)
    .scalar_subquery()
)

requirements.txt/requirements-dev.txt/requirements-tests.txt all pin sqlalchemy==1.3.24. scalar_subquery() was introduced in SQLAlchemy 1.4; the 1.3 spelling is .as_scalar(). Because hybrid expressions evaluate on class-attribute access, every touch of Certificate.in_rotation_window at the class level raises AttributeError — breaking both the reissue task and the expiring-deployed-cert check.

3. No test coverage for the new fallback path

No test in lemur/tests/ exercises in_rotation_window (instance or class-level) for a certificate with rotation_policy=None, and none sets LEMUR_DEFAULT_ROTATION_POLICY_DAYS. lemur/tests/factories.py:69 always attaches a RotationPolicy via SubFactory, so the NULL-policy branch is never exercised. This is exactly why issues #1 and #2 above weren't caught by make test.

Minimum suggested coverage:

• Unit test for _default_rotation_days() respecting a config override
• Instance-level test with rotation_policy=None asserting in_rotation_window doesn't raise
• An integration-style test against the test DB asserting the class-level SQL expression correctly includes a NULL-rotation_policy_id cert inside the default window and excludes one outside it

4. LEMUR_DEFAULT_ROTATION_POLICY_DAYS vs. LEMUR_DEFAULT_ROTATION_INTERVAL — confusing overlap

lemur/manage.py:295 already reads LEMUR_DEFAULT_ROTATION_INTERVAL (default 30 days) to seed a RotationPolicy row named "default". This PR introduces a second, differently-named key, LEMUR_DEFAULT_ROTATION_POLICY_DAYS (default 60), for a different purpose (the fallback when a cert has no policy row at all). An operator could reasonably conflate the two. Consider renaming the new key to something unambiguous (e.g. LEMUR_ROTATION_DAYS_WHEN_NO_POLICY) or cross-referencing both in comments.

6. Divergent failure modes between the Python and SQL paths if the config value is None

If LEMUR_DEFAULT_ROTATION_POLICY_DAYS is present but set to None, the Python-side path raises inside timedelta(days=None), while the SQL-side COALESCE embeds literal(None), causing extract(...) <= NULL to evaluate to NULL (falsy in CASE) — silently reporting zero NULL-policy certificates as due for rotation, DB-wide, with no error. This violates the hybrid_property's implicit equivalence contract between its two branches, and the silent variant is operationally more dangerous (certs quietly stop rotating, no alert fires). Consider validating the configured value's type/range inside _default_rotation_days().

7. Behavioral change is broader than just NULL-policy certificates — worth calling out explicitly

The old unqualified RotationPolicy.days reference cross-joined every certificate against every policy row, so the filter matched if any policy row satisfied the condition (effectively using max(days), e.g. 70, for all certificates regardless of their own policy). After this fix, certificates with an explicit shorter policy also stop matching early — this isn't limited to NULL-policy certs. Expect a step-change in reissue volume and in any monitor built on get_certs_for_expiring_deployed_cert_check(). Not a bug, but worth flagging for reviewers/on-call so the volume change isn't mistaken for a regression after merge.

[bencebeky]

Hey, I got Claude to review this PR and cherry-picked some of the findings that I think are worth addressing. Thanks!

[evan-datadog]

Hey thanks so much fixing them now

[Copilot]

Pull request overview

Fixes certificate rotation-window evaluation for certificates without an assigned rotation policy.

Changes:

• Uses correlated SQL policy lookup with configurable fallback.
• Adds NULL-policy tests and configuration documentation.
• Ignores local worktree directories.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
.gitignore	Ignores .worktrees/.
local/src/lemur.conf.py	Adds local configuration example.
lemur/certificates/models.py	Corrects Python and SQL rotation-window logic.
lemur/default.conf.py	Defines the default rotation interval.
lemur/manage.py	Documents configuration reuse.
lemur/tests/conf.py	Sets the test rotation interval.
lemur/tests/test_certificates.py	Adds NULL-policy regression tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bencebeky]

I ran https://github.com/DataDog/claude-marketplace/tree/main/dual-agents-review on this PR and got the output below. Issue 1 seems legit, and straightforward to address. Issue 2 is worth addressing too. I'm not against shortening the revocation window from 60 days to 30 days, but I'm worried that lumping that together with the current change might make it more difficult to revert the shortening should an issue arise, so I recommend keeping the rotation time uniformly at 60 days for now.

Many of the other findings are worth addressing not because they are significant but because they are easy to fix. As for finding 16, the change to .gitignore is probably incidental, and might make sense to revert.

Dual Code Review Report

Branch: evan/CLOUDR-2089/default-rotation-policy-v2 -> master
Reviewed by: Claude (pr-review-toolkit: code-reviewer, code-simplifier, comment-analyzer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer) + Codex (codex review --base master)
Date: 2026-08-11

---

Summary

Both engines converged strongly on one critical regression: the new hasattr guard in is_attached_to_endpoint() turns a hard failure (uncaught AttributeError, effectively fail-closed) into a silent False for any source plugin — GCP, Azure, DigiCert — that doesn't implement get_endpoint_certificate_names(). That lets the revoke endpoint skip its "still deployed" safety check for non-AWS certificates. Four independent reviewers (Codex, code-reviewer, silent-failure-hunter, type-design-analyzer) flagged this without prompting each other — high-confidence finding.

The second theme, surfaced by nearly every Claude reviewer, is a 30 vs. 60 day default divergence: manage.py's InitializeApp still falls back to 30 days when seeding the default RotationPolicy row, while _default_rotation_days() in models.py (and default.conf.py) use 60. Since real deployments load config from k8s-resources rather than default.conf.py, this isn't hypothetical — it's a live inconsistency between the seeded default policy and the runtime fallback.

Beyond those two, the core SQL fix (correlated subquery + COALESCE replacing an implicit cross-join) was independently validated as correct and well-tested by three reviewers. Remaining findings are mostly test-coverage gaps, comment accuracy issues, and simplification opportunities.

Critical Issues

Issue #1: is_attached_to_endpoint() fails open for non-AWS source plugins, allowing revocation of live certificates

Description: The new guard at certificates/service.py:1052-1057 returns False ("not attached") whenever endpoint.source.plugin lacks get_endpoint_certificate_names(). Only lemur_aws/plugin.py implements this method — Azure and GCP source plugins do not, and there's no base-class contract in plugins/bases/source.py. The sole caller is the revoke endpoint (certificates/views.py:1666), which uses the return value to gate a 403 "cannot revoke, still deployed" response. Before this change, the missing-method case raised an uncaught AttributeError outside the view's try/except, producing a 500 and blocking revocation (accidentally fail-closed). Now it silently returns False, letting service.revoke() proceed even though the certificate may still be serving traffic on a GCP/Azure endpoint. The failure is only logged at warning level (not error), so it won't reach Sentry either.
Location: lemur/certificates/service.py:1052-1057 (caller: lemur/certificates/views.py:1666)
Source: Both Codex & Claude (code-reviewer, silent-failure-hunter, type-design-analyzer)

Suggestions

Issue #2: Default rotation window diverges between seed path (30 days) and fallback path (60 days)

Description: manage.py:296 (InitializeApp) still reads current_app.config.get("LEMUR_DEFAULT_ROTATION_INTERVAL", 30) to seed the "default" RotationPolicy row, while _default_rotation_days() in certificates/models.py:80,90 falls back to 60 (matching default.conf.py). Because default.conf.py is only loaded when no other config source is present, and real deployments configure via k8s-resources (which doesn't set this key per the diff's own commented-out local/src/lemur.conf.py line), lemur init can seed a 30-day default policy while NULL-policy certs elsewhere are evaluated against a 60-day window — the same concept, two silently different real values. Recommended fix: define one constant and have manage.py call _default_rotation_days() instead of independently re-reading and re-defaulting the config key.
Location: lemur/manage.py:295-296, lemur/certificates/models.py:79-90
Source: Both Codex & Claude (code-reviewer, silent-failure-hunter, code-simplifier, comment-analyzer, pr-test-analyzer, type-design-analyzer)

Issue #3: default.conf.py comment references the wrong CLI command

Description: The comment says the value is "used when seeding the default RotationPolicy row (lemur create_config)". Seeding actually happens in InitializeApp (the lemur init command); create_config just renders a config template and doesn't touch RotationPolicy at all.
Location: lemur/default.conf.py:20-22
Source: Both Codex & Claude (code-reviewer, comment-analyzer)

Issue #4: Stale commented-out sample in local/src/lemur.conf.py advertises the wrong default

Description: The added line # LEMUR_DEFAULT_ROTATION_INTERVAL = int(os.environ.get("LEMUR_DEFAULT_ROTATION_INTERVAL", 30)) is inert and uses 30, contradicting the 60 used everywhere else in this PR (default.conf.py, tests/conf.py, models.py). Either delete it or update the example to 60.
Location: local/src/lemur.conf.py:289
Source: Claude (code-reviewer, code-simplifier, comment-analyzer, silent-failure-hunter)

Issue #5: Invalid-value branch of _default_rotation_days() has no test coverage

Description: The try/except (TypeError, ValueError) plus days > 0 guard added in an earlier commit (8ebe47fd) to reject malformed config (non-numeric, zero, negative) is untested. The two existing tests only cover a valid override and a missing key. A regression that broke the validation (e.g., removing the except or the positivity check) would pass CI silently.
Location: lemur/tests/test_certificates.py:2009-2033
Source: Claude (pr-test-analyzer, silent-failure-hunter, code-simplifier)

Issue #6: Python and SQL implementations of in_rotation_window disagree when RotationPolicy.days is NULL

Description: RotationPolicy.days is a nullable column with no validation. If a policy row exists with days IS NULL (a state the schema permits), the instance-level in_rotation_window property raises TypeError from timedelta(days=None), while the SQL expression's COALESCE silently treats the NULL days as "no policy" and substitutes the default. Same conceptual state, two different outcomes depending on which code path is used.
Location: lemur/certificates/models.py:396 (instance) vs. :412-422 (SQL expression)
Source: Claude (type-design-analyzer, silent-failure-hunter)

Issue #7: Misconfigured LEMUR_DEFAULT_ROTATION_INTERVAL only logged at warning, never alerts

Description: When the config value is invalid, _default_rotation_days() logs at warning level and silently falls back. This codebase uses sentry_sdk.capture_exception() for comparable config-correctness issues elsewhere (common/schema.py, common/health.py, common/celery.py), but nothing analogous fires here — Sentry's default integration only surfaces ERROR+ records. A typo'd Vault/k8s-resources value would silently change the rotation window for every unmanaged certificate with no alert.
Location: lemur/certificates/models.py:79-90
Source: Claude (silent-failure-hunter)

Issue #8: Self-contradictory docstring in test_in_rotation_window_class_level_null_policy

Description: The docstring states "The explicit-short-policy assertion below would spuriously pass with that bug," but given the test's actual seeded data, the old cross-join bug would cause that assertion to fail, not spuriously pass. This directly contradicts a correct inline comment three lines below it, and could mislead a future maintainer diagnosing a regression about which outcome indicates the bug is back.
Location: lemur/tests/test_certificates.py:2073-2075
Source: Claude (comment-analyzer)

Issue #9: Missing boundary-condition test at the exact rotation-window cutoff

Description: Both in_rotation_window implementations use <= for the cutoff comparison. All new tests use days comfortably away from the boundary (30/90 vs. a 60-day default); an accidental <= → < change would slip through undetected.
Location: lemur/certificates/models.py:400,420; lemur/tests/test_certificates.py
Source: Claude (pr-test-analyzer)

Issue #10: Regression test never asserts the positive-inclusion case for an explicit policy

Description: test_in_rotation_window_class_level_null_policy only proves a cert with an explicit 30-day policy is excluded from a 60-day query window; it never proves a cert with an explicit policy expiring inside its own window is included. A regression in the COALESCE logic that accidentally overrides a non-null policy_days wouldn't be caught.
Location: lemur/tests/test_certificates.py (test_in_rotation_window_class_level_null_policy)
Source: Claude (pr-test-analyzer)

Issue #11: is_attached_to_endpoint() docstring not updated for new early-return semantics

Description: The :return: line still says "True if certificate is attached... False otherwise," without noting that False is also returned when the plugin can't verify attachment at all (a genuinely unknown state, not a confirmed absence). A caller could reasonably treat False as "definitely not attached."
Location: lemur/certificates/service.py:1043-1050
Source: Claude (comment-analyzer)

Issue #12: New warning log doesn't match sibling pattern and lacks context

Description: The remove_from_destination() warning nearby formats missing-method situations as "... {plugin_name} plugin does not implement 'clean()'". The new warning uses an em dash (atypical for this codebase) and omits the parenthesis suffix on the method name that CLAUDE.md requires for referenced function/method names. It also doesn't include certificate_name/endpoint_name, making the log line untraceable to the originating call.
Location: lemur/certificates/service.py:1052-1057
Source: Claude (code-simplifier, silent-failure-hunter)

Issue #13: Various simplification opportunities in new code and tests

Description: Several smaller cleanups from code-simplifier: literal() around _default_rotation_days()'s return is redundant since coalesce() already coerces it; the try/if/except shape in _default_rotation_days() could flatten to fewer exit points; two near-identical instance-level tests and the three is_attached_to_endpoint tests are parametrize candidates; CertificateFactory() calls followed by cert.rotation_policy = None leave orphaned policy rows that don't match the test's own cross-join reasoning; a stray plugin.plugin_name assignment in one test has no effect since production code reads endpoint.source.plugin_name; function-local import arrow/from unittest.mock import patch duplicate existing module-level imports.
Location: lemur/certificates/models.py, lemur/tests/test_certificates.py (see agent detail for exact lines)
Source: Claude (code-simplifier)

Issue #14: RotationPolicy model enforces no invariants on days

Description: RotationPolicy.days is nullable=True with no CHECK constraint and no validation on the model itself, pushing all correctness burden onto call sites (which, per Issue #6, don't agree with each other). A @validates("days") hook or a nullable=False + CHECK (days > 0) migration would make illegal states unrepresentable rather than handled ad hoc.
Location: lemur/policies/models.py:15-24
Source: Claude (type-design-analyzer)

Issue #15: get_endpoint_certificate_names has no declared contract on the base source-plugin class

Description: The method is implemented only by lemur_aws/plugin.py with no declaration (even as a NotImplementedError stub) on plugins/bases/source.py. This is the structural root of Issue #1 — any caller has to remember to hasattr-guard it, and nothing documents that the method is optional.
Location: lemur/plugins/bases/source.py
Source: Claude (type-design-analyzer)

Issue #16: .gitignore addition doesn't match neighboring conventions

Description: The new .worktrees/ entry lacks the leading / that neighboring root-anchored entries (/venv.bak, /.vscode) use, and isn't grouped with a comment. Also worth considering whether a personal worktree directory belongs in a global gitignore rather than the repo's.
Location: .gitignore
Source: Claude (code-simplifier)

Positive Highlights

Issue #17: Correlated subquery + COALESCE correctly fixes the cross-join bug

Description: The class-level in_rotation_window SQL expression previously joined against rotation_policies implicitly, causing certs to spuriously match against any policy row's window. The rewrite to a correlated scalar subquery with COALESCE for the NULL-policy fallback is verified correct for the pinned SQLAlchemy 1.3.24 (select([...]), .as_scalar(), .correlate(cls) are all valid 1.3 spellings), and is exercised through a real DB query rather than mocked internals.
Location: lemur/certificates/models.py:403-422
Source: Both Codex & Claude (code-reviewer, silent-failure-hunter, pr-test-analyzer)

Issue #18: Regression test is genuinely adversarial

Description: test_in_rotation_window_class_level_null_policy deliberately seeds a wide 90-day policy alongside the 60-day default specifically to give the old cross-join bug something to spuriously match against, giving the test real teeth rather than passing vacuously.
Location: lemur/tests/test_certificates.py
Source: Both Claude (code-reviewer, pr-test-analyzer)

Issue #19: _default_rotation_days() exception handling is properly scoped

Description: Uses except (TypeError, ValueError) rather than a bare except:/except Exception:, and logs before falling back — the right shape, even though severity/alerting (Issue #7) and test coverage (Issue #5) still need work.
Location: lemur/certificates/models.py:79-90
Source: Claude (silent-failure-hunter)

Issue #20: is_attached_to_endpoint tests cover all three logical branches well

Description: The three new tests correctly exercise missing-method, present+match, and present+no-match cases, using MagicMock(spec=[]) appropriately to simulate a plugin that genuinely lacks the method rather than one that just returns a falsy value.
Location: lemur/tests/test_certificates.py
Source: Claude (pr-test-analyzer)

Engine-Specific Notes

Claude-Only Findings

Issues #3–16 (comment accuracy, test-coverage gaps beyond the core regression, design/invariant concerns on RotationPolicy, simplification opportunities, and the .gitignore nit) were surfaced only by the Claude review agents — Codex's review focused narrowly on the single critical regression.

Codex-Only Findings

None — Codex's one finding (Issue #1) was independently corroborated by three Claude agents.

Review Metadata

• Claude review: completed (6/6 agents: code-reviewer, code-simplifier, comment-analyzer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer)
• Codex review: completed (exit code 0)
• Duplicates merged: 17
• Total unique findings: 20

[evan-datadog]

Addressed the dual-review findings (comment 5257447900) and validated in the sandbox:

• Issue Latest Upstream changes  #1 (is_attached_to_endpoint fail-open): now fails-closed — when a source plugin lacks get_endpoint_certificate_names() (GCP/Azure/COA), returns True (assume attached) + error log + capture_exception(), so a possibly still-deployed cert is never silently revoked. Updated the unit test to assert True.
• Issue Create ci-dd.yml #2 (30/60 divergence): manage.py now seeds the default RotationPolicy via _default_rotation_days() (was a hardcoded 30 fallback) — seed and NULL-policy fallback can't diver anymore.
• Issue Setting up Build for Lemur #3/Set default code owners to network edge #4: corrected the default.conf.py comment (lemur init, not create_config) and the local/src example (30 → 60).
• Issue Support displaying SNI certificates to user in web UI #8: fixed the self-contradictory test docstring.

Sandbox validation (validate-70d-rota): deployed this branch to lemur-sandbox, confirmed the 60-day default window for NULL-policy certs (30d/60d IN, 90d OUT, explicit-policy no-cross-join), rotation candidates end-to-end, then restored the sandbox to mutable-latest-prod and cleaned up. Full results in the PR description.

Keeping rotation at 60 days per your recommendation (no window shortening in this change).

[evan-datadog]

Follow-up to Issue #2: the previous commit only unified the default for new seeds — an existing "default" RotationPolicy row (seeded by the historical migration at 30d, or custom like the sandbox's 35d) was left untouched, so it still diverged from the 60d NULL-policy fallback.

Now lemur init calls the new sync_default_rotation_policy() (in policies/service.py): creates the policy if missing, or updates an existing row's days in place to _default_rotation_days() (config-driven, default 60). Unit tests cover create/update/no-op. Existing deployments just need a lemur init re-run to sync their "default" policy to 60.

[evan-datadog]

Important correction to the previous comment: lemur init is NOT called on conductor deploy — the k8s deployment containers run only lemur start, celery worker, celery beat (initContainers just copy static assets + consul-template config). Neither lemur init nor lemur db upgrade runs automatically.

So the sync_default_rotation_policy() in InitializeApp alone would not reach existing deployments. Added data migration c3d4e5f6a7b8 that updates the named "default" RotationPolicy to 60 days, applied via the documented lemur db upgrade deploy step (downgrade reverts to 30). Verified the SQL: 30→60, custom policies untouched, downgrade→30.

Net: fresh seeds get 60 via InitializeApp; existing deployments get 60 via the migration on lemur db upgrade; the NULL-policy fallback is 60 everywhere.

[evan-datadog]

Ran a review swarm (6 lenses × 3 cheap models + aggregator) over the full diff. Verdict: LGTM with minor changes. Applied the one legitimate finding:

• Finding C — is_attached_to_endpoint() endpoint None guard: endpoint_service.get_by_name() can return None; previously endpoint.source would raise AttributeError. Now logs error + capture_exception() and returns False (nothing to be attached to) instead of crashing. Added a unit test for the endpoint-not-found case.

Findings assessed as not actionable:

• as_scalar() → scalar_subquery() (finding A): false positive — this repo pins SQLAlchemy 1.3.24, where .as_scalar() is the correct API (scalar_subquery() is 1.4+). The PR comment documents this compat requirement.
• App-context safety, init race, migration robustness, orphaned-policy (B/D/E/F): theoretical/low-risk; lemur init is a one-time manual command and the correlated subquery is a PK lookup (≤1 row; orphaned → NULL → fallback, which is correct).

Branch now: ce5b77986, da657ca6b, 9d5309be1, 36e8958d3, c5de74e1e.

[evan-datadog]

https://github.com/DataDog/k8s-resources/pull/171269 not strictly necessary difference betwwen being where we want things to be configured.

[evan-datadog]

Split out: the fail-closed guard (review Issue #1) is now in PR #361 (#361). This PR (#355) is scoped to the rotation-window fix only — service.py is back to master here.

[evan-datadog]

Split out: the is_attached_to_endpoint fail-closed guard (review Issue #1) is now in PR #361 (#361). This PR (#355) is scoped to the rotation-window fix only — lemur/certificates/service.py is back to master here.

[maperu]

mind simplifying this to just a if statement for the existence of a rotation_policy_id or not