Repository navigation
harden: the connector must be read-only against its MySQL source - #1407
Merged
Merged
Conversation
The keyless-table preflight refused to start whenever the SOURCE held a
table with no row identity, even when the operator had already excluded
that table from replication. A table the connector never reads cannot be
replicated incorrectly, so it has no business blocking startup.
The practical effect was that the only ways past a keyless table were to
add a primary key to a table you may not own, or to set
keyless.table.check.skip=true and lose the protection for every OTHER
table on the source. Neither is right when the table is already excluded.
Concretely: txnrepo uat and staging carry alembic_version in 18
databases (Alembic schema bookkeeping, 0-1 rows, no key). Excluding it is
the correct fix, but before this change the connector would still refuse
to start.
withoutExcluded() drops out-of-scope tables before the refusal decision,
honouring both Debezium list properties with Debezium semantics:
* entries are REGULAR EXPRESSIONS, full-matched against the
fully-qualified db.table -- not substring, so an exclusion written
for one table cannot silently swallow a neighbour;
* table.include.list, when set, means anything NOT listed is out of
scope.
Fails safe in the same direction as databaseFilter(): an uncompilable
pattern is IGNORED rather than treated as a match. This list can only
ever REMOVE tables from the refusal set, so a broken pattern that matched
everything would silently drop a genuinely keyless table from the check -
a false pass, which is worse than a false refusal.
Tests: KeylessTablePreflightTest 20 (5 new) - an excluded keyless table
no longer blocks (using the exact ".*.temp_.*,.*[.]alembic_version"
pattern shipped for txnrepo uat/staging, so the test fails if that
production exclusion ever stops covering the table it was written for);
exclusion is anchored not substring; a table outside table.include.list
is out of scope; no lists configured excludes nothing; a malformed
pattern is ignored while the valid entry beside it still applies.
Failing-first verified: with withoutExcluded() short-circuited, 3 of the
5 fail, each showing the excluded table still blocking.
Full suite green: 44 across KeylessTablePreflightTest,
SequenceSeedOverflowTest, SourceTsVersionAnchorTest,
DdlReplayIdempotencyTest, CreateTableNoKeySortKeyTest.
Co-authored-by: Jenkins Continuous Build server <jenkins@fpif1-dbal2.w2k.jumptrading.com>
CI run 32819328349 failed java-tests-lightweight with 3 failures and 19
errors across 15 suites. Two causes, both from the keyless preflight.
1. THE PREFLIGHT WROTE A GLOBAL ON THE SOURCE (a real defect, not a test
problem). enableGipk() ran
SET GLOBAL sql_generate_invisible_primary_key = ON
With that global on, MySQL REJECTS every subsequent keyless
CREATE TABLE ... PARTITION BY on the WHOLE server:
ERROR 1235 (42000): This version of MySQL doesn't yet support
'generating invisible primary key for the partitioned tables'
So merely starting the connector broke partitioned-table DDL for every
application sharing that MySQL -- an outage on the SOURCE caused by a
read-only consumer, persisting after the connector stopped, and lasting
until someone found the global and reset it. TableOperationsIT:111 is
exactly that: a CREATE TABLE ... PARTITION BY KEY(joined) rejected on a
server the connector had reconfigured.
The global is now never written. The session setting is kept (harmless;
the connector creates no source tables). When the global is OFF the
passing path logs a WARN with the exact statements and states the
partitioned-table consequence, so enabling it stays the server owner's
decision, made with knowledge of what else uses that MySQL. The refusal
message already carried the same advice.
2. THE SHARED IT FIXTURE IS KEYLESS ON PURPOSE. sql/data_types.sql declares
ship_class and add_test with no PRIMARY KEY and no UNIQUE key because the
DDL-parser suites exercise ALTER/RENAME/TRUNCATE against them. The
preflight is right to refuse such a source, so the ITs opt out via
keyless.table.check.skip in src/test/resources/config.yml, with a comment
explaining why. The check itself stays covered directly by
KeylessTablePreflightTest, which does not depend on that fixture.
The remaining 17 errors in the run are downstream of these two: once the
connector refuses to start, the ITs' ClickHouse tables are never created and
the assertions fail with UNKNOWN_TABLE / UNKNOWN_IDENTIFIER, and one suite
reports "Mapped port can only be obtained after the container is started"
after its container teardown.
Tests: KeylessTablePreflightTest 22 (2 new).
- testPreflightNeverSetsAGlobalOnTheSource asserts against the source text,
because the failure mode is the PRESENCE of the statement: any execution
path reaching it is the bug, so no runtime scenario has to be guessed at.
- testItConfigOptsOutOfTheKeylessCheck pins the opt-out so a future edit
cannot silently reintroduce a 19-error run.
Both locate their file from the compiled class location and fail loudly if
it is missing, so neither can pass vacuously.
Failing-first verified. Restoring the SET GLOBAL call and removing the
opt-out fails exactly the two new tests:
testPreflightNeverSetsAGlobalOnTheSource:370
the preflight must not execute SET GLOBAL on the source server:
st.execute("SET GLOBAL sql_generate_invisible_primary_key = ON");
testItConfigOptsOutOfTheKeylessCheck:401
keyless.table.check.skip must be true in .../config.yml
Full unit run green: 154 across KeylessTablePreflightTest,
SequenceSeedOverflowTest, SourceTsVersionAnchorTest, DdlReplayIdempotencyTest,
CreateTableNoKeySortKeyTest, MySqlDDLParserListenerImplTest.
Inherited from the base branch, where a test run let maven-shade regenerate the file: all 441 lines rewritten with CRLF endings plus a silent bump of the checked-in lombok pin from 1.18.22 to 1.18.34. Neither belongs in this change, and against upstream 2.10.0 it showed up as a 441-line third file in the diff. Restored to the 2.10.0 content. Verified before reverting that after CRLF normalisation the lombok <version> line was the only content difference, so nothing else is lost.
Review findings from round 2. 1. gipkEnabledGlobally() ran the same SELECT twice whenever the first comparison did not match -- i.e. on every server where the global is OFF, exactly the path that exists in order to warn. Now one round trip into a local, with both renderings (ON/OFF and 1/0) still accepted. 2. All three judges flagged a lombok skew in dependency-reduced-pom.xml, and they were right to look: sink-connector-lightweight/pom.xml declares 1.18.34 in BOTH the dependency and the compiler annotationProcessorPaths, while the checked-in generated dependency-reduced-pom.xml carries 1.18.34 in the processor path and 1.18.22 in the dependency block. The committed generated file is the stale, internally inconsistent one. No change is made here. That file is build-generated, its inconsistency predates this work, and it is already byte-identical to the base commit on this branch, so it does not appear in the diff. Regenerating or gitignoring it is a separate call for the repo's owners; this PR should not carry an unrelated dependency change either direction. Unit run green: 154 across KeylessTablePreflightTest, SequenceSeedOverflowTest, SourceTsVersionAnchorTest, DdlReplayIdempotencyTest, CreateTableNoKeySortKeyTest, MySqlDDLParserListenerImplTest.
A replication consumer must never change the server it reads from. #59 fixed one violation of that rule; this makes the rule structural, so the next one cannot be introduced silently. SCOPE. Swept both modules for statements executed against the SOURCE. KeylessTablePreflight is the connector's ONLY code path that opens its own connection to MySQL -- everything else either targets ClickHouse (the sink, where writing is the job) or is Debezium reading the binlog. So the guarantee is enforced at that one chokepoint. WHAT WAS STILL THERE. One write remained: SET SESSION sql_generate_invisible_primary_key = ON. It is harmless in effect -- the setting only applies to CREATE TABLE, and the preflight only reads information_schema, so it changed nothing on any server. But it is still a write issued by a read-only program, and leaving it meant the class could not claim, or enforce, read-only. Removed. Nothing depended on it: it was already dead weight after #59 dropped the global. THREE LAYERS, so the guarantee does not rest on reviewers noticing: 1. Connection.setReadOnly(true) on the preflight's connection. The MySQL driver sends SET SESSION TRANSACTION READ ONLY, so the SERVER rejects a write even if future code asks for one. This is the only layer that does not depend on the connector behaving. Requested and then VERIFIED via isReadOnly(), because a driver that silently ignored it would leave the guarantee resting on the other two without saying so. Note this is a session property of a short-lived connection owned by the connector -- it changes nothing for any other client, which is precisely the distinction the old SET GLOBAL got wrong. 2. assertReadOnlySql() on every statement, refusing a write before it is sent, with a message naming the rule instead of a driver error. Deliberately an ALLOWLIST (SELECT only): a denylist is the wrong shape for a safety check, because every statement a future MySQL adds defaults to permitted -- and the verb that actually caused the incident, SET, is easy to omit precisely because it does not read like a write. Leading /* */, -- and # comments are stripped first, so a commented statement cannot slip past the prefix test. 3. testPreflightIssuesNoWritesToTheSource fails the BUILD if any of 25 mutating verbs appears in an executed string literal in this class. This is the general form of #59's SET GLOBAL test: that one pins a statement, this pins the principle, so the next write fails even though nobody predicted its verb. The operator-advice message still quotes SET GLOBAL, because telling the operator how to enable GIPK themselves is the whole point of that message. It is log text, never executed, and the scan skips only that shape (a concatenated message fragment ending in ";\n") -- an actual write would not look like it. Behaviour is otherwise unchanged: the check still refuses a source with pre-existing keyless tables, still refuses below 8.0.30, and still reports whether GIPK is ON with the statements to enable it. Tests: KeylessTablePreflightTest 27 (5 new) -- the no-writes scan, writes rejected, the reads this class issues allowed, comment-disguised writes rejected, and non-SELECT statements refused rather than assumed safe. Failing-first verified with a verb the guard's call sites never mention: adding probe.execute("FLUSH TABLES WITH READ LOCK") fails the build with the connector is read-only against its MySQL source, but this class contains a 'FLUSH' statement: probe.execute("FLUSH TABLES WITH READ LOCK"); Unit run green: 159 across KeylessTablePreflightTest, SequenceSeedOverflowTest, SourceTsVersionAnchorTest, DdlReplayIdempotencyTest, CreateTableNoKeySortKeyTest, MySqlDDLParserListenerImplTest. Destructive-Op-Check: sites=2; result=pass
A replication consumer must never change the server it reads from. #1406 fixed one violation of that rule; this makes the rule structural, so the next one cannot be introduced silently. SCOPE. Swept both modules for statements executed against the SOURCE. KeylessTablePreflight is the connector's ONLY code path that opens its own connection to MySQL -- everything else either targets ClickHouse (the sink, where writing is the job) or is Debezium reading the binlog. So the guarantee is enforced at that one chokepoint. WHAT WAS STILL THERE. One write remained: SET SESSION sql_generate_invisible_primary_key = ON. It is harmless in effect -- the setting only applies to CREATE TABLE, and the preflight only reads information_schema, so it changed nothing on any server. But it is still a write issued by a read-only program, and leaving it meant the class could not claim, or enforce, read-only. Removed. Nothing depended on it: it was already dead weight after #1406 dropped the global. THREE LAYERS, so the guarantee does not rest on reviewers noticing: 1. Connection.setReadOnly(true) on the preflight's connection. The MySQL driver sends SET SESSION TRANSACTION READ ONLY, so the SERVER rejects a write even if future code asks for one. This is the only layer that does not depend on the connector behaving. Requested and then VERIFIED via isReadOnly(), because a driver that silently ignored it would leave the guarantee resting on the other two without saying so. Note this is a session property of a short-lived connection owned by the connector -- it changes nothing for any other client, which is precisely the distinction the old SET GLOBAL got wrong. 2. assertReadOnlySql() on every statement, refusing a write before it is sent, with a message naming the rule instead of a driver error. Deliberately an ALLOWLIST (SELECT only): a denylist is the wrong shape for a safety check, because every statement a future MySQL adds defaults to permitted -- and the verb that actually caused the incident, SET, is easy to omit precisely because it does not read like a write. Leading /* */, -- and # comments are stripped first, so a commented statement cannot slip past the prefix test. 3. testPreflightIssuesNoWritesToTheSource fails the BUILD if any of 25 mutating verbs appears in an executed string literal in this class. This is the general form of #59's SET GLOBAL test: that one pins a statement, this pins the principle, so the next write fails even though nobody predicted its verb. The operator-advice message still quotes SET GLOBAL, because telling the operator how to enable GIPK themselves is the whole point of that message. It is log text, never executed, and the scan skips only that shape (a concatenated message fragment ending in ";\n") -- an actual write would not look like it. Behaviour is otherwise unchanged: the check still refuses a source with pre-existing keyless tables, still refuses below 8.0.30, and still reports whether GIPK is ON with the statements to enable it. Tests: KeylessTablePreflightTest 27 (5 new) -- the no-writes scan, writes rejected, the reads this class issues allowed, comment-disguised writes rejected, and non-SELECT statements refused rather than assumed safe. Failing-first verified with a verb the guard's call sites never mention: adding probe.execute("FLUSH TABLES WITH READ LOCK") fails the build with the connector is read-only against its MySQL source, but this class contains a 'FLUSH' statement: probe.execute("FLUSH TABLES WITH READ LOCK"); Unit run green: 159 across KeylessTablePreflightTest, SequenceSeedOverflowTest, SourceTsVersionAnchorTest, DdlReplayIdempotencyTest, CreateTableNoKeySortKeyTest, MySqlDDLParserListenerImplTest. Destructive-Op-Check: sites=2; result=pass
#1405 and #1406 merged upstream while this PR was open, so this branch's original base carried those same commits and the PR showed CONFLICTING. Rebuilt on the new 2.10.0 head (aa00431) by applying only this change's two files. Force-push is never used here, so the previous tip is reconciled with a -s ours merge instead. Verified byte-identical to the tested branch: the diff of both files against the 159-test-green commit is empty.
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.
A replication consumer must never change the server it reads from. #1406 fixed one violation of that rule; this makes the rule structural, so the next one cannot be introduced silently.
Scope of the sweep
Swept both modules for statements executed against the source.
KeylessTablePreflightis the connector's only code path that opens its own connection to MySQL — everything else either targets ClickHouse (the sink, where writing is the job) or is Debezium reading the binlog. So the guarantee is enforced at that one chokepoint.What was still there
One write remained:
Harmless in effect — the setting only applies to
CREATE TABLE, and the preflight only readsinformation_schema, so it changed nothing on any server. But it is still a write issued by a program that is supposed to be read-only, and leaving it meant the class could not claim read-only, let alone enforce it. Removed. Nothing depended on it: it was already dead weight once #1406 dropped the global.Three layers
So the guarantee does not rest on a reviewer noticing.
1.
Connection.setReadOnly(true). The MySQL driver sends a read-only transaction setting, so the server rejects a write even if future code asks for one. This is the only layer that does not depend on the connector behaving.Requested and then verified via
isReadOnly()— a driver that silently ignored the request would leave the guarantee resting on the other two layers without saying so.This is a session property of a short-lived connection the connector owns. It changes nothing for any other client — precisely the distinction the old
SET GLOBALgot wrong.2.
assertReadOnlySql()on every statement, refusing a write before it is sent, with a message naming the rule rather than a driver error.Deliberately an allowlist (SELECT only). A denylist is the wrong shape for a safety check: every statement a future MySQL version adds defaults to permitted, and the verb that actually caused the incident —
SET— is easy to omit precisely because it does not read like a write. Leading/* */,--and#comments are stripped first, so a commented statement cannot slip past the prefix test.3.
testPreflightIssuesNoWritesToTheSourcefails the build if any of 25 mutating verbs appears in an executed string literal in this class. This is the general form of #1406's regression test: that one pins a statement, this pins the principle.The one thing deliberately not caught
The operator-advice message still quotes
SET GLOBAL ..., because telling the operator how to enable GIPK themselves is the entire point of that message. It is log text, never executed, and the scan skips only that shape — a concatenated message fragment. An actual write would not look like it.Behaviour unchanged
The check still refuses a source with pre-existing keyless tables, still refuses below MySQL 8.0.30, and still reports whether GIPK is ON with the statements to enable it.
Tests — 27 in the class, 5 new
The no-writes scan; writes rejected; the reads this class actually issues allowed; comment-disguised writes rejected; and non-SELECT statements refused rather than assumed safe.
Failing-first verified with a verb the guard's call sites never mention. Adding a
FLUSH TABLESprobe fails the build:That is the point of layer 3 — it catches a write nobody predicted, which is exactly how the original defect got in.
Unit run green: 159 across
KeylessTablePreflightTest,SequenceSeedOverflowTest,SourceTsVersionAnchorTest,DdlReplayIdempotencyTest,CreateTableNoKeySortKeyTest,MySqlDDLParserListenerImplTest.Stacked on #1406 (base branch
2.10.0).Integration suites need Docker and were not run here; CI exercises them on this branch.