CASSANDRA-21139 - Feature/guardrail for misprepare statements

[omniCoder77]

Validation is posted on Jira Ticket

Feature suggestion:
Guardrail for miss-prepared statements

Description:

We have hundreds of application teams and several dozen of them miss-prepare statements by using literals instead of bind markers.

I.e.,

// wrong 
session.prepare("select * from users where ID = 996");
session.prepare("select * from users where ID = 997");
session.prepare("select * from users where ID = 998");
session.prepare("select * from users where ID = 999");

// correct
session.prepare("select * from users where ID = ?");

The problem causes the prepared statement cache to constantly overflow, and will print a prepared statements discarded WARN message in the Cassandra log. At present, we use a wack-a-mole approach to discuss the problem with each development team individually, and hope they fix it and train the entire team on how to prepare statements correctly.

Also, finding the root cause of the issue today requires having the knowledge and access to look at the system.prepared_statements table.

Guardrails would seem a good approach here, where the guard could WARN or REJECT when a statement was prepared using a WHERE clause and no bind markers.

Note, this should not prevent users from creating prepared statements without a WHERE clause or with one or more literal values so long as there was at least one bind marker. Thus, the following would remain valid:

session.prepare("select * from users");
session.prepare("select * from users where TYPE=5 and ID = ?");

Approach:
Introduced a boolean flag use_misprepare_statements_enabled (which can be configured from cassandra.yaml) whose default value is true (backward compatibility) and added functions to StorageServiceMBean to enable dynamic runtime configuration.
Added test cases to validate changes in parseAndPrepare function.

[omniCoder77]

@smiklosovic
I have addressed above comments and

Added new test cases verifying warning behaviour (warn only when misprepared_statements_enabled is true) and to bypass misprepare guardrail for system keyspaces.

Centralized onMisprepare logic to Guardrails.java

Refined test case to extract repeating statements to a function, increase readability.

[omniCoder77]

In my test cases I have tested

• System keyspaces, internal calls and super users are exempted from this guardrail check
• Confirmed literals in the SELECT clause are allowed as long as the WHERE clause is prepared.
• Verified use is warned if and only if misprepared_statement_use is set to true and statement is misprepared
• Verified no warning when guardrail is violated, instead GuardrailViolationException
• Verified blocks for literals in WHERE clauses, IN clauses, LWT IF conditions, and JSON inserts.
• Confirmed that blocked queries do not enter the statement cache
• Exempts super user and internal call from this guardrail

[smiklosovic]

I would rather just copy the content of that mehod here instead of having completely arbitrary method in Guardrails. There is no precedent to that.

You might also move it to superclass of these Statement classes so both can call it.

[omniCoder77]

public static void onMisprepared(ClientState state)
    {
        mispreparedStatementsEnabled.ensureEnabled(state);
        mispreparedStatementsEnabled.warn(MISPREPARED_STATEMENT_WARN_MESSAGE);
    }

here warn is a protected function, can't write it inside statement classes or interface, not sure where else to put it.

[smiklosovic]

@omniCoder77 thanks for the perseverance! I am just checking overall code / design etc. Havent deeply checked the actual logic around the very guardrail application.

[bschoening]

typo on ins't -- but why is super user relevant to the warning?

[bschoening]

I think this should be "if it contains only hardcoded liter values and without any bind markers"

[bschoening]

not heap exhaustion, but cache eviction

Prevents cache overflow and eviction caused by ...

[bschoening]

"misprepared statements create non-reusable query entries and cause cache overflow"