Skip to content

mz-deploy: stop rotating SSH tunnel keys on every apply (DEX-53) - #37374

Open
sjwiesman wants to merge 2 commits into
MaterializeInc:mainfrom
sjwiesman:seth/dex-53-ssh-tunnel-key-rotation
Open

mz-deploy: stop rotating SSH tunnel keys on every apply (DEX-53)#37374
sjwiesman wants to merge 2 commits into
MaterializeInc:mainfrom
sjwiesman:seth/dex-53-ssh-tunnel-key-rotation

Conversation

@sjwiesman

@sjwiesman sjwiesman commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

The connection-options diff is purely structural, so the server-generated
PUBLIC KEY 1/PUBLIC KEY 2 that SHOW CREATE reports for an SSH tunnel
connection (and that a user cannot write in project SQL) always looked
like drift. Every re-apply emitted ALTER CONNECTION ... DROP (PUBLIC KEY 1/2), which rotates the keypair and breaks bastions authorized against
the old keys. Exclude these server-managed options from both sides of the
diff so apply is idempotent.

Ticket: DEX-53

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

🤖 Generated with Claude Code

https://claude.ai/code/session_01VAcnVpSQi8ZgF5LekQRwvw

The connection-options diff is purely structural, so the server-generated
`PUBLIC KEY 1`/`PUBLIC KEY 2` that `SHOW CREATE` reports for an SSH tunnel
connection (and that a user cannot write in project SQL) always looked
like drift. Every re-apply emitted `ALTER CONNECTION ... DROP (PUBLIC KEY
1/2)`, which rotates the keypair and breaks bastions authorized against
the old keys. Exclude these server-managed options from both sides of the
diff so apply is idempotent.

Ticket: DEX-53

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sjwiesman
sjwiesman marked this pull request as ready for review July 1, 2026 14:16
@tonydu-mz

Copy link
Copy Markdown
Contributor

Confirmed the core assumption holds: PUBLIC KEY 1/2 are always server-produced, so ignoring them in the diff is correct.

  • They're valid options for exactly one connection type, SSH (connection.rs:165Ssh => &[Host, Port, User, PublicKey1, PublicKey2]), and appear in no other type's option list, so they never show up in a non-SSH SHOW CREATE.
  • Even for SSH the value is always overwritten by the planner, never taken from the user (plan_create_connection, ddl.rs:5627-5639): it retains out any user-supplied PublicKey1/2 and pushes key_1.public_key() / key_2.public_key(). There's no code path that persists a user-authored public key.

So this can't ever swallow a legitimate user change. 👍

One thing worth tightening before this becomes the durable answer: is_server_managed is a hardcoded name list, and the classification "which options are server-generated" now lives in two uncoupled places (here, and the planner's overwrite block). Renames are caught by the compiler, but the matches! has an implicit _ => false, so a newly added ConnectionOptionName variant silently defaults to "user-authored." If a future connection type ever surfaces a server-minted value in SHOW CREATE, this exact rotate-on-every-apply bug comes back with nothing to flag it.

Cheap guard, and it matches existing code on this enum (there's already an exhaustive match over ConnectionOptionName at ddl.rs:958-981): make this an exhaustive match with no wildcard, so a new variant fails to compile here until someone classifies it.

fn is_server_managed(name: ConnectionOptionName) -> bool {
    use ConnectionOptionName::*;
    match name {
        // Server-generated for SSH connections and overwritten by the planner on
        // every plan (plan_create_connection); never written by the user.
        PublicKey1 | PublicKey2 => true,
        // Everything else is user-authored. Enumerated explicitly (no `_`) so a
        // new ConnectionOptionName variant won't compile until it's classified —
        // otherwise a future server-generated option silently reintroduces the
        // rotate-on-every-apply bug this PR fixes.
        Host | Port | User | Broker | Brokers | /* …rest of the variants… */ => false,
    }
}

(The full variant list can be lifted straight from the exhaustive match at ddl.rs:958-981.)

It doesn't guarantee the next person classifies a new option correctly, but it turns a silent bug into a reviewed decision. Longer term the sturdier version is a single ConnectionOptionName::is_server_generated() in the parser crate that both this and the planner's overwrite consult (ideally with the planner's key push driven by it), so the two sides are the same list by construction — but that's a follow-up, not this PR.

Not blocking, the fix is correct as-is.

@tonydu-mz

tonydu-mz commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Follow-up on the longer-term direction I mentioned above (a separate PR or this one, your choice).

The exhaustive-match tweak is the cheap stopgap. The durable fix is to stop keeping "which options are server-generated" in two places: right now it lives both in the planner's SSH overwrite (ddl.rs:5627-5639) and in is_server_managed here, with nothing linking them.

Move it onto the enum in mz-sql-parser as the single source of truth:

impl ConnectionOptionName {
    /// Options whose values are generated and owned by the server (surfaced in
    /// SHOW CREATE) rather than written by the user. Today: the SSH tunnel
    /// public keys.
    pub fn is_server_generated(&self) -> bool {
        use ConnectionOptionName::*;
        match self {
            PublicKey1 | PublicKey2 => true,
            Host | Port | User | Broker | Brokers | /* …rest, no wildcard… */ => false,
        }
    }
}

Then is_server_managed here collapses to name.is_server_generated(), and the planner's strip becomes stmt.values.retain(|v| !v.name.is_server_generated()) (the generated push stays option-specific, since each server-generated value is produced differently).

The point is that the predicate becomes load-bearing for the planner's own strip, not just a list mz-deploy has to remember to mirror. Adding a new server-generated option then has to flip is_server_generated() to make the planner strip user input for it, and that same edit automatically makes mz-deploy ignore it in the diff. Same list by construction, so it can't drift.

The server-managed classification was a `matches!` with an implicit
`_ => false`, so a newly added `ConnectionOptionName` variant would
silently default to user-authored. If a future connection type surfaces a
server-generated value in `SHOW CREATE`, that would reintroduce the
rotate-on-every-apply bug this branch fixes, with nothing to flag it.

Enumerate every variant explicitly with no wildcard so a new variant
fails to compile here until it is classified, turning a silent bug into a
reviewed decision.

The durable fix (a single `ConnectionOptionName::is_server_generated()`
in mz-sql-parser that both this and the planner's SSH overwrite consult)
is a follow-up.

Reported-by: tonydu-mz
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sjwiesman

Copy link
Copy Markdown
Contributor Author

@tonydu-mz took your suggestion in the mz-deploy crate. I will leave it to the SQL team if they want to push into the parser crate.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants