Skip to content

fix(opensearch): enforce ES/OS endpoint separation at the OS index-creation chokepoint (#36419)#36421

Open
swicken wants to merge 4 commits into
mainfrom
issue-36419-os-separation-guard-ordering
Open

fix(opensearch): enforce ES/OS endpoint separation at the OS index-creation chokepoint (#36419)#36421
swicken wants to merge 4 commits into
mainfrom
issue-36419-os-separation-guard-ordering

Conversation

@swicken

@swicken swicken commented Jul 3, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Fixes the endpoint-separation guard so it actually prevents OpenSearch .os shadow indices from being created when the ES and OS clients point at the same cluster.

Root cause: the separation check only ran in InitServlet → checkAndInitializeIndex, which executes after the empty-DB starter-load path (Task00004LoadStarter → bootstrapAndPoint → bootstrapAndPointOS) has already created the .os indices. With ES==OS the guard halted the migration but too late — the .os indices and their indicies rows already existed.

  • ContentletIndexAPIImpl.bootstrapAndPointOS — add a config-only endpoint-separation gate at the single OS index-creation chokepoint, before the existing connection gate. On ES==OS overlap it logs, calls haltMigration() (ES-only fallback), and skips OS bootstrap — so no .os artifacts are created on any startup path (starter-load or InitServlet).
  • IndexStartupValidator — extract the overlap logic into a shared assertEndpointsSeparate(config) and add a network-free endpointsAreSeparate() gate reused by both startup validation and the chokepoint (no duplicated comparison logic).
  • IndexStartupValidatorTest — new unit tests for the separation gate (same-address → false, different → true, scheme-normalized overlap → false, assert throws with the overlap message).

Closes #36419

Verification

Unit: IndexStartupValidatorTest 4/4 pass.

E2E (single-node-os-migration rig, branch image, DOT_ES_ENDPOINTS == DOT_OS_ENDPOINTS == https://opensearch3:9200, Phase 1):

Check Before After
.os rows in indicies 2 0
.os cluster indices 2 0
.os createIndex attempts created 0 (prevented)
Migration halted → Phase 0
dotCMS serving on ES

Log confirms the gate fires at the chokepoint before any OS creation:

ERROR IndexStartupValidator - OpenSearch endpoint-separation check failed: ES and OS clients point to the same endpoint(s): [opensearch3:9200]...
WARN  ContentletIndexAPIImpl - Skipping OpenSearch index bootstrap (...): OS endpoints overlap ES (same cluster). Migration halted (now ES-only)...
WARN  MigrationPhase - Migration phase reset to PHASE_0_MIGRATION_NOT_STARTED (was PHASE_1_DUAL_WRITE_ES_READS)...

Checklist

  • Tests
  • Translations
  • Security Implications Contemplated — config-only comparison, no new inputs; strengthens an existing safety guard (prevents dual-write to a shared cluster).

Additional Info

Found during QA of #36218 (TC-037), part of the OpenSearch migration QA epic #35476. The existing halt behavior (reset to Phase 0, ES-only fallback, "same endpoint(s)" log) is preserved; this only closes the ordering window on the fresh-install/starter-load path.

Known limitations / follow-ups (from review)

  • Runtime phase-flip (out of scope for OpenSearch: endpoint-separation guard runs after index bootstrap, so .os shadow indices are still created when ES==OS #36419): raising FEATURE_FLAG_OPEN_SEARCH_PHASE 0→1+ at runtime without a restart and then running a full reindex creates .os reindex slots via initAndPointReindex → createContentIndex fan-out, which does not pass through this bootstrap gate. Startup validation remains the primary guard.
  • Pre-existing (not introduced here): the halt is runtime-only and subject to system-table shadowing (MigrationPhase.reset() javadoc). If the phase flag lives in the DB system table, the reset is a no-op from current()'s perspective and dual-writes could continue.

…eation chokepoint (#36419)

The endpoint-separation guard only ran in InitServlet -> checkAndInitializeIndex,
which executes after the empty-DB starter-load path (Task00004LoadStarter ->
bootstrapAndPoint -> bootstrapAndPointOS) has already created the .os shadow
indices. With ES==OS configured, the guard correctly halted the migration but too
late: the .os indices and their `indicies` rows already existed (fails TC-037).

Move the separation check to the single OS index-creation chokepoint
(bootstrapAndPointOS), alongside the existing connection gate:

- IndexStartupValidator: extract the overlap logic into a shared
  assertEndpointsSeparate(config) and add a config-only, network-free
  endpointsAreSeparate() gate reused by both startup validation and the chokepoint.
- bootstrapAndPointOS: refuse OS bootstrap and haltMigration() on ES==OS overlap,
  before the connection gate, so no .os artifacts are created on any startup path.

Verified e2e: with ES==OS, 0 .os rows / 0 .os cluster indices, migration halts to
Phase 0, dotCMS serves on ES. Adds IndexStartupValidatorTest.
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @swicken's task in 1m 44s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

🤖 dotBot Review (Bedrock)

Reviewed 3 file(s); 4 candidate(s) → 3 confirmed, 0 uncertain (unverified, kept for review).

Confirmed findings

  • 🟡 Medium dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImpl.java:1029 — Race condition in haltMigration() exacerbated by new call site
    The new call to haltMigration() in ContentletIndexAPIImpl.bootstrapAndPointOS() (line 1029) increases exposure to an existing race condition in IndexStartupValidator.haltMigration(). Evidence from IndexStartupValidator.java shows haltMigration() calls MigrationPhase.reset() which modifies shared static state without synchronization. Concurrent calls could leave migration phase in inconsistent state during startup/bootstrap operations.
  • 🟡 Medium dotCMS/src/main/java/com/dotcms/content/index/opensearch/IndexStartupValidator.java:255 — Config resolution errors incorrectly block OS index creation
    The method endpointsAreSeparate() catches all RuntimeExceptions (including config errors unrelated to endpoint overlap) and returns false. This incorrectly prevents OS index creation for non-overlap issues like invalid configs, misleading operators. Evidence: line 255 returns false after catching any RuntimeException from assertEndpointsSeparate(), which may throw other exceptions beyond endpoint overlap.
  • 🟡 Medium dotCMS/src/test/java/com/dotcms/content/index/opensearch/IndexStartupValidatorTest.java:85 — Test expects exception from method that doesn't throw
    In IndexStartupValidatorTest.java line 85, the test testEndpointsAreSeparate_EmptyOsEndpoints_Throws asserts that IndexStartupValidator.endpointsAreSeparate() throws IllegalArgumentException when passed empty OS endpoints. However, the production method endpointsAreSeparate() is a boolean check that doesn't throw exceptions. This mismatch causes the test to fail erroneously.

us.deepseek.r1-v1:0 · Run: #28676689037 · tokens: in: 50825 · out: 21399 · total: 72224 · calls: 9 · est. ~$0.184

…nt overlap (#36419)

Review follow-up: endpointsAreSeparate() caught all exceptions and returned false,
so a configFromProperties() parse failure looked identical to an ES==OS overlap.
The caller then logged a hardcoded "OS endpoints overlap ES (same cluster)",
misdirecting the operator to a same-endpoint problem that may not exist.

- endpointsAreSeparate(): resolve config and run the overlap check in separate
  try blocks, each with a distinct ERROR (config-resolution failure vs overlap).
- bootstrapAndPointOS: soften the WARN to defer to the preceding error for the
  cause instead of asserting an overlap.
…ets; add tests (#36419)

Address review on PR #36421:

- Phase 3 (isMigrationComplete): the gate threw away the codebase's "never silently
  fall back to stale ES" invariant by calling haltMigration() unconditionally. Now it
  throws in Phase 3 (matching checkAndInitializeIndex and the connection gate); the
  non-Phase-3 path still halts to ES-only. Bites initOSCatchup Case 1 (Phase-3 DR).
- Restore the resolved ES/OS sets in the "Endpoint separation check passed" log line
  (dropped in the extraction); it now lives in the shared assertEndpointsSeparate.
- Add tests: multi-endpoint partial overlap, and config-resolution failure (fail-closed).
- Soften the "single chokepoint" comment — it covers working/live bootstrap, not
  reindex-slot creation (initAndPointReindex).
/**
* Unit tests for {@link IndexStartupValidator} endpoint-separation logic.
*
* <p>Covers the config-only separation gate ({@link IndexStartupValidator#endpointsAreSeparate()})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This validation must be phase-aware and consider that during Phase 3, ES_ENDPOINTS isn't required

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then I guess that if we're on phase 3, we shouldn't run this check at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

OpenSearch: endpoint-separation guard runs after index bootstrap, so .os shadow indices are still created when ES==OS

2 participants