rfc: create SQL query denylist RFC

[angelapwen]

Co-Authored-By: Oliver Tan [email protected]

View text at: https://github.com/angelapwen/cockroach/blob/denylist-rfc/docs/RFCS/20201020_sql_query_denylist.md

Thank you in advance for the reviews! ✨

[cockroach-teamcity]

This change is 

[otan]

(seems like you need to rebase and adjust your PR to have a release note for this to go green!)

[vy-ton]

time- and resource-intensive for developers to write their own

Clarify what "write their own" means? I believe this refers to developers modifying the application code to stop running certain queries which is not always possible.

[angelapwen]

Yes, this is what I meant — I'll modify to clarify.

[vy-ton]

DBAs would need file write permission to do this. What user/level of access do CC SREs have to CC clusters?

[angelapwen]

I don't know the answer to this — perhaps an SRE reviewing here could answer? @joshimhoff or @JuanLeon1?

[vy-ton]

Open question about whether we need a staging denylist.

Assuming we do, a separate file seems more error-prone instead of marking queries as "staged" in a single denylist fiile.

[angelapwen]

If I remember correctly, @JuanLeon1 mentioned in our call that having a separate file for the staging denylist would be preferable so that it could go through a separate pull request/review process for changes, for auditing purposes. Perhaps I misinterpreted and he meant that having a single file is preferable for efficiency?

[vy-ton]

"non-configurable" is confusing here, I assume you mean non-configurable through SQL

[angelapwen]

Hm, yes you're right because evidently it can be configured through modifying the YAML file 😄 I'll think of better wording for this or remove the word altogether.

[vy-ton]

If we're assuming an unresponsive cluster, the Admin UI and SQL console would be inaccessible? So you could only confirm staged queries with logs

[angelapwen]

You're right, in that situation they'd have to just confirm via logs. I'll update to clarify.

[vy-ton]

How frequently do we check the denylist for updates?

[rafiss]

i think it's mentioned later -- there's a file watcher that notifies when a file is modified

[rafiss]

it's mentioned later -- we get notified whenever the file is modified

[vy-ton]

Does this include all cluster settings?

[angelapwen]

I don't quite understand what you mean here. Do you mean that all cluster settings could be added to this denylist? I think that's right. They can always be removed.

[vy-ton]

This list doesn't include arbitary queries and is all features.

@joshimhoff or @JuanLeon1 Is the ability to dynamically turn off specific features what CC SRE really needs? We wouldn't need a denylist to support arbitrary query patterns if we just need to prevent backups, CDC, temp tables etc?

[vy-ton]

In the phase where we think a file-based approach is required to meet the use case, this wouldn't be needed

[vy-ton]

What happens if a user enters an invalid entry?

[angelapwen]

That's fair. I'd suggest that an invalid entry (specifically an invalid key, one that's not sql in this case) be surfaced as a warning/error to console and the logs so that the administrator knows that the denylist was not processed correctly. @otan thoughts here?

[rafiss]

this is kind of a nit, but perhaps a word other than retries would be helpful. that makes me think of a query being retried because it failed. but i think we are also just worried about applications running more instances of an entire class of "bad queries" that it executes in the course of handling regular requests

[rafiss]

to clarify: the deny list won't affect any currently running queries? what about queries that are retried internally?

[rafiss]

what does it mean for the entire denylist to longer be available?

[angelapwen]

I think this was a typo — I probably meant "if the entire denylist is no longer necessary" Thanks for the catch!

[rafiss]

i think it's mentioned later -- there's a file watcher that notifies when a file is modified

[rafiss]

kind of a nit, but i'd use a word other than retry here. retries make me think of a query being retried because the previous time failed. but we also are more concerned with the application generating more of a certain type of query in the process of handling more user requests.

[angelapwen]

Yeah, that makes sense. I hadn't thought about how the word retry is overloaded in the context of application code vs. our database internals.

[rafiss]

what does it mean for the entire denylist to longer be available?

[rafiss]

it's mentioned later -- we get notified whenever the file is modified

[rafiss]

so any currently running queries are not affected? what about queries that are retried internally?

[angelapwen]

That's correct. I don't believe that queries that are retried internally, or queries that are in the middle of running, should be affected — only new incoming queries.

[rafiss]

could we add a section about testing? would be nice to see what sort of automated tests we can do. and also maybe some manual test cases to verify it works at the end and is a good experience

[angelapwen]

I'll look into this. Thanks for the suggestion 💖

[awoods187]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @joshimhoff, @otan, @rafiss, and @vy-ton)

---

docs/RFCS/20201020_sql_query_denylist.md, line 10 at r1 (raw file):

# Summary

We propose a mechanism built into CockroachDB to prevent operator-defined SQL queries from being executed. This will immediately address the situations where clusters enter unresponsive states due to a query, or a query pattern, that has been identified. To bring the clusters back up as soon as possible and prevent them from going down again due to the same query, we need a query denylist on the database. This RFC details a file-managed SQL query denylist implementation that will allow database administrators to trigger query denial by uploading a YAML file comprising regular expression patterns to a customizable file location.  

We dont grant SSH access (and therefore the ability to use YAML) on CockroachCloud. As it is currently designed SREs could use this but our CC customers could not. What happens if a customer wants to override an SRE decision but can't configure this YAML file? This is straying beyond database-as-a-service and impinging on application management.

---

docs/RFCS/20201020_sql_query_denylist.md, line 11 at r1 (raw file):

We propose a mechanism built into CockroachDB to prevent operator-defined SQL queries from being executed. This will immediately address the situations where clusters enter unresponsive states due to a query, or a query pattern, that has been identified. To bring the clusters back up as soon as possible and prevent them from going down again due to the same query, we need a query denylist on the database. This RFC details a file-managed SQL query denylist implementation that will allow database administrators to trigger query denial by uploading a YAML file comprising regular expression patterns to a customizable file location.  
# Motivation

I'd like to see a discussion of what exists in Postgres and other database peers. My initial googling hasn't found a good comparison and it concerns me to deviate as heavily in this case. If they don't allow for this behavior perhaps we need to further reconsider.

---

docs/RFCS/20201020_sql_query_denylist.md, line 19 at r1 (raw file):

As blocking a query pattern is invasive and potentially dangerous, it is also important to add a staging denylist for administrators to test whether the query pattern accurately matches the queries they meant to deny or not. 

Having a query denylist will allow database administrators to mitigate these situations quickly and immediately without making any code changes. Additionally, this will give more flexibility and time for CockroachDB’s database and release engineers to work on database fixes relevant to the query. 

This feels very anti-developer in our thinking. I think we at least need some conversation in here about how operators and developers would interact. How would developers understand their query is blocked (I see this covered in part below but it feels incomplete)? Why can't developers continuously deploy a patch to the application that removes the problematic query? Or rollback to the previous version? It strikes me as dangerous to assume that operators (and particularly our SREs) know better than developers what the application needs can be. Why wouldn't we scale the cluster first and let developers/customers decide what the right action to take is (e.g., supporting the increase scale, rollingback or patching the problem).

---

docs/RFCS/20201020_sql_query_denylist.md, line 36 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

If we're assuming an unresponsive cluster, the Admin UI and SQL console would be inaccessible? So you could only confirm staged queries with logs

CC doesn't support log access for non-CRL labs employees so this also seems problematic. In addition, we would likely want to persist information in SQL/web console about what was denied and when, especially as we look toward persisting information in the future.

---

docs/RFCS/20201020_sql_query_denylist.md, line 124 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

In the phase where we think a file-based approach is required to meet the use case, this wouldn't be needed

you'd still likely want to know what was blocked historically when it came back

---

docs/RFCS/20201020_sql_query_denylist.md, line 145 at r1 (raw file):

### Pattern matching with regular expressions
There are a couple of drawbacks to consider due to the use of regular expressions to represent queries to be denied in our YAML file. The first is that it is slightly less user-friendly than exact fingerprint matching, because certain special characters in our queries will need to be escaped: eg. `SELECT \(\(a\)\) FROM b`. We accept this drawback because it gives the user writing the query much more fine-grained control than fingerprint matching does — with fingerprint matching, all queries matching a fingerprint will be blocked and the user would not be able to block, for example, queries for a specific database object.

YAML is a huge drawback. We ran into this via zone configs and people really disliked its usage. In part you could argue because they were using SQL and we forced a switch. I think this presents a similar problem.

---

docs/RFCS/20201020_sql_query_denylist.md, line 169 at r1 (raw file):

### Using file-managed denylist vs. cluster settings/system table
A file-managed system for the denylist was chosen over cluster settings and system table for this first iteration of the denylist. This is because the most urgent use case for our users is a situation in which clusters are entirely unresponsive, and therefore updating a denylist via cluster settings or system table would not be feasible. The file-managed denylist will mitigate the most extreme circumstance and therefore is the first denylist management system we will design and implement.

I still think it would be more natural to rollback an application change that caused this problem and/or scale the db resources.

---

docs/RFCS/20201020_sql_query_denylist.md, line 173 at r1 (raw file):

In the future, it is likely that we will add implementations of the denylist via cluster settings for broader use cases when the cluster is not unresponsive. At this point it would be possible to surface the functionality to the Web UI for an easier user experience.

### Denylist implementation in the database vs. SQL proxy

It strikes me as potentially not cloud-first to implement this now when SQL proxy is on the roadmap purely for on-prem users. Any work we do in SQL should not be motivated purely for on-prem users.

[otan]

---

docs/RFCS/20201020_sql_query_denylist.md, line 11 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I'd like to see a discussion of what exists in Postgres and other database peers. My initial googling hasn't found a good comparison and it concerns me to deviate as heavily in this case. If they don't allow for this behavior perhaps we need to further reconsider.

You're right in that most databases do not allow this kind of configurability - this is we brought up that this should really be done on a proxy layer instead. Though it evades me why we think this should go on the DB for now besides making it easier for CC to have something in the meantime. @joshimhoff is probably a good person to answer here.

[otan]

---

docs/RFCS/20201020_sql_query_denylist.md, line 11 at r1 (raw file):

this is we brought up that this should really be done on a proxy layer instead
oops, why is why we brought up on whether this should be done on a proxy layer instead*

[otan]

---

docs/RFCS/20201020_sql_query_denylist.md, line 145 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

YAML is a huge drawback. We ran into this via zone configs and people really disliked its usage. In part you could argue because they were using SQL and we forced a switch. I think this presents a similar problem.

Is there something that you would suggest as a replacement? Configuration of these items (as currently written) has to be file based due to the nature of the blocking. If we did this as a cluster setting / sql table, i can see that being different.

[otan]

---

docs/RFCS/20201020_sql_query_denylist.md, line 19 at r1 (raw file):

Why can't developers continuously deploy a patch to the application that removes the problematic query?

I can imagine a situation where the DB operators have no control over applications being pushed. It may be quicker to deny something and wait for the application rollback to be more gradual.

Why wouldn't we scale the cluster first

I believe SREs have had the problem where no matter the size of the cluster, a command may always break.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 19 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

Why can't developers continuously deploy a patch to the application that removes the problematic query?

I can imagine a situation where the DB operators have no control over applications being pushed. It may be quicker to deny something and wait for the application rollback to be more gradual.

Why wouldn't we scale the cluster first

I believe SREs have had the problem where no matter the size of the cluster, a command may always break.

I agree that the notification to developers that their query is being rejected needs to be solid and thought out, perhaps elaborated on a bit more.

I don't understand the rest. Let's say every developer carries a pager, and is able to diagnose and fix a problem and push a solution on their application as fast as an oncall SRE can add the query to the blacklist. The time to repair will see, in the first case, the whole database and all the other apps being down, and only the one app being down in the latter case. For a production system where downtime is money, the blacklist seems like profit. Even assuming all devs are always on call etc.

I don't think this is more than slightly "anti-developer". The developer can wake up at 3 am they want, and fix the problem, and remove the blacklist as they push the solution.

Further, a query of death often decimates any cluster of any size, because they are usually retried until the service stops responding. I got many fireplace stories to tell that all sound like that.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 60 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

This list doesn't include arbitary queries and is all features.

@joshimhoff or @JuanLeon1 Is the ability to dynamically turn off specific features what CC SRE really needs? We wouldn't need a denylist to support arbitrary query patterns if we just need to prevent backups, CDC, temp tables etc?

Well turning off specific features would also be useful, but not what we are trying to get here. It is queries that kill the system, which people would not predict would kill the system, that we need to block when system keeps dying on startup. Some of these queries may come from features like backups, CDC, etc, but many of them come from normal apps that got unlucky or are too aggressive or both.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 169 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I still think it would be more natural to rollback an application change that caused this problem and/or scale the db resources.

It is not always an app change, can be that a table size went over a threshold, that the DB version changed along with e.g, the optimizer. Also the operator may not have the ability to roll back the app change that may even be from a client in a different company in North Korea. Regarding scale, apps often retry, and a bad query can consume infinite resources unless throttled.

More natural, perhaps, to someone, but better able to keep a service up, no.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 29 at r1 (raw file):

Previously, angelapwen (Angela P Wen) wrote…

If I remember correctly, @JuanLeon1 mentioned in our call that having a separate file for the staging denylist would be preferable so that it could go through a separate pull request/review process for changes, for auditing purposes. Perhaps I misinterpreted and he meant that having a single file is preferable for efficiency?

I don't feel strongly but I think you want some people to have access to staging blocklists that you don't want to access production ones. I can see testing a list and then needing another approval from someone else to actually make it do its thing.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 19 at r1 (raw file):

Previously, JuanLeon1 wrote…

I agree that the notification to developers that their query is being rejected needs to be solid and thought out, perhaps elaborated on a bit more.

I don't understand the rest. Let's say every developer carries a pager, and is able to diagnose and fix a problem and push a solution on their application as fast as an oncall SRE can add the query to the blacklist. The time to repair will see, in the first case, the whole database and all the other apps being down, and only the one app being down in the latter case. For a production system where downtime is money, the blacklist seems like profit. Even assuming all devs are always on call etc.

I don't think this is more than slightly "anti-developer". The developer can wake up at 3 am they want, and fix the problem, and remove the blacklist as they push the solution.

Further, a query of death often decimates any cluster of any size, because they are usually retried until the service stops responding. I got many fireplace stories to tell that all sound like that.

My first paragraph was wrong, in both cases the whole DB would be down. So if all devs carry pages and can diagnose/push fixes as fast as an oncall SRE can apply a blocklist, there is no benefit to the blocklist other than a bunch of unhappy, unbelievably good, devs.

[angelapwen]

Thank you all for the great reviews 😸

Raphael, I would tend to agree with your recommendation that we should focus on the 1st use case you mentioned above, as our goal here was to create a "non-configurable" (by the application developer), global denylist. I hadn't realized that the two (or three, per Josh!) use cases were not so compatible until I looked through these reviews, but given that they are I agree that the first one is the one that we prefer to solve first.

It sounds like creating a global denylist like this will work as needed for the multi-tenant CC (serverless) case and, with few modifications surrounding communications, can be configured for the single-tenant CC (dedicated) case.

Is it possible to present this feature as an option for on-prem customers with their full knowledge that it does not come with end-user configurability and the option to "opt out"? If this is not acceptable, then we can exclude on-prem from our intended customers of this first version of the denylist implementation.

PS — the point about benchmarking is a good one @knz. I'll discuss further details with Oliver and update progress or implementation details via this PR/RFC.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @awoods187, @joshimhoff, @knz, @otan, @rafiss, and @vy-ton)

---

docs/RFCS/20201020_sql_query_denylist.md, line 29 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

We certainly need some way to test out additions to the denylist. This tool is too dangerous not to have that capability.

I think I actually prefer a single file. The benefit is that the query you are testing in the staging denylist is def the same as the one you are about to actually block in the prod one. Since same file. If two files, maybe you copy & paste wrong, and then your test via the staging denylist is a bad test.

Wonder what Juan thinks about ^^.

That's fair. I worry that there could be danger in using the same file as well (accidentally removing the 'staging' key before it is ready or something like that). Open to either.

---

docs/RFCS/20201020_sql_query_denylist.md, line 58 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

Should we allow adding text to include in the error message? This way, SRE can communicate with the user about why the query is being blocked?

That seems like a good idea to me. There can be default error message text (what is displayed now) and an optional field that will allow a custom error message.

---

docs/RFCS/20201020_sql_query_denylist.md, line 76 at r1 (raw file):

Previously, knz (kena) wrote…

To clarify my last comment on this point: I am not suggesting to turn this thing into a cluster setting. We can still use files. But the reloading of the file should not be automatic and instead there should be a mechanism to trigger it.

For example, we trigger rotation of TLS certificates using a unix signal, and we trigger other types of file reloads using HTTP/RPC APIs.

Could you clarify which comment you meant about notification? I am not understanding why fsnotify does not work well when files in a container mount are updated.

I understand the point about invalid intermediate states — most of these result in a different file extension that could be ignored by the watcher, right?

I didn't know that we already use a pattern of triggering reloads using HTTP/RPC APIs. Do you know off the top of your head where I can dig into these a bit more?

---

docs/RFCS/20201020_sql_query_denylist.md, line 106 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

Interesting!

That is interesting — at the current stage we are only checking against the canonical SQL inside CockroachDB after transformations. This is not always intuitive to the operator configuring the YAML file — though if CC SREs are the primary customer, they would perhaps be able to validate with database SQL engineers sooner than on-prem operators.

The latter case mentioned could be checked for by using the ^ and $ anchors, though this may require that the operator write pretty fine-grained, complex regular expressions for the file.

---

docs/RFCS/20201020_sql_query_denylist.md, line 124 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

Why would these graphs not be needed with a file-based approach? These graphs seem very useful to me.

I think @vy-ton meant that they wouldn't be accessible in case the cluster is fully down, but yes when they are back up they would be useful to check later on.

---

docs/RFCS/20201020_sql_query_denylist.md, line 145 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

This is a thorny problem. No one likes really any of the available languages for configuring applications. There is not really a "good" one.

YAML has comments so it is better than JSON for this use-case IMHO.

What we design ourselves very well may be worse than YAML. It is a common thing to do to invent a new (special-case) config language bc/ the existing ones are "bad"... then realize oh my ours is a mess also!

I think zone configs are logically part of the schema of an application. So should be SQL. That is not the case here.

I'd also like to avoid writing a new config language for this case if at all possible, for all the reasons you mentioned 😄

---

docs/RFCS/20201020_sql_query_denylist.md, line 181 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

I think this an important Q! I will be interested in following along & seeing how DB engineers answer Qs like this.

Thanks Josh! Will try to keep you updated as these benchmarks move forward. It looks like Raphael also wanted the benchmarking process to be included as part of the RFC.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 11 at r1 (raw file):

Previously, knz (kena) wrote…

FWIW one of our major customers ever since 2016 has been pushing for this feature, because they use an equivalent feature in Oracle.

We need to understand what Oracle provides and how they achieve it. That feature is loved by DBAs.

There are two things that I remember Robert (from sales eng) has been very adamant about:

• the filtering needs to be done per application (not globally): i.e. a query must be able to succeed for one value of application_name even when it is blocked for another
• the filtering needs to be disabled for queries sent by administrator users (I'd say, for users with a special privilege - we'd need to define a new privilege maybe, ask Solon)

More interesting than what Postgress does is, I think, what Managed SQL DBs do; e.g, Google Cloud SQL backed with postgress.
An on-prem, customer-controlled and -endured database would not need this. A DB that one party has an SLA to keep up without controlling workload from remote clients needs something like it.
E.g, spanner was built as an internal SasS and I am pretty sure Spanner SREs have this.
We can build it in the proxy, sure, but any on-prem customer of any size will want it. So we should be packaging the proxy with CRDB, which introduces some complexity.

I think "cloud-first" means managed-first, meaning there is a split of agency between the DB service operators and the app operators. Would we insist that the former have no control over the workload presented to them, and thereby relinquish control over their ability to maintain the service?

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 29 at r1 (raw file):

Previously, angelapwen (Angela P Wen) wrote…

That's fair. I worry that there could be danger in using the same file as well (accidentally removing the 'staging' key before it is ready or something like that). Open to either.

I can see Josh's point. The downside is that presumably production blocks require one of few people to approve, which means that even test and staging blocks will be starved for attention and approval.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 106 at r1 (raw file):

Previously, angelapwen (Angela P Wen) wrote…

That is interesting — at the current stage we are only checking against the canonical SQL inside CockroachDB after transformations. This is not always intuitive to the operator configuring the YAML file — though if CC SREs are the primary customer, they would perhaps be able to validate with database SQL engineers sooner than on-prem operators.

The latter case mentioned could be checked for by using the ^ and $ anchors, though this may require that the operator write pretty fine-grained, complex regular expressions for the file.

Is the canonical SQL what shows up in the table of queries? IN logs? That would be what operators would use to block.
CC SREs are not the primary customer -- all SREs in customer orgs will also want to use this if it is available.

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 145 at r1 (raw file):

Previously, angelapwen (Angela P Wen) wrote…

I'd also like to avoid writing a new config language for this case if at all possible, for all the reasons you mentioned 😄

+1 on YACL (yet-another-configuration-language). My rule of thumb for configs: 1) JSON if a human never needs to read or write 2) YAML is config is simple enough -- as is the case here and 3) JSONET if need inheritance, modules, and still human readable,

[rafiss]

moved comment to thread

[JuanLeon1]

---

docs/RFCS/20201020_sql_query_denylist.md, line 173 at r1 (raw file):

Previously, joshimhoff (Josh Imhoff) wrote…

I do think on-prem folks want this feature. I would if I was running CRDB on-prem. Raphael points out customers have asked for it.

OTOH, if putting it in the SQL proxy gets CC SRE what we want, I'd be okay with that too. The main thing we'll have to figure out if we can drop the SQL proxy into dedicated clusters; if we can't do that, then putting it there doesn't help SRE so much.

And on-prem customers will want this and would then have to get the proxy, so maybe need to ship proxy by default, which is not very different from putting the feature in CRDB.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @awoods187, @joshimhoff, @knz, @otan, @rafiss, and @vy-ton)

---

docs/RFCS/20201020_sql_query_denylist.md, line 173 at r1 (raw file):

Previously, JuanLeon1 wrote…

And on-prem customers will want this and would then have to get the proxy, so maybe need to ship proxy by default, which is not very different from putting the feature in CRDB.

Just my 2cents -- if we are explicitly deciding to address the CC SRE use-case, and not address the self-hosted DBA use-case, then I view that as an argument against building this feature inside the cockroach server. Otherwise, we will be releasing a feature to all our self-hosted users that was not intended for them, and we will bear the maintenance and support costs as they ask us to change the feature to their liking.

(moved my comment from the top level comments to this thread)

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @awoods187, @joshimhoff, @JuanLeon1, @knz, @otan, @rafiss, and @vy-ton)

---

docs/RFCS/20201020_sql_query_denylist.md, line 11 at r1 (raw file):

Previously, JuanLeon1 wrote…

More interesting than what Postgress does is, I think, what Managed SQL DBs do; e.g, Google Cloud SQL backed with postgress.
An on-prem, customer-controlled and -endured database would not need this. A DB that one party has an SLA to keep up without controlling workload from remote clients needs something like it.
E.g, spanner was built as an internal SasS and I am pretty sure Spanner SREs have this.
We can build it in the proxy, sure, but any on-prem customer of any size will want it. So we should be packaging the proxy with CRDB, which introduces some complexity.

I think "cloud-first" means managed-first, meaning there is a split of agency between the DB service operators and the app operators. Would we insist that the former have no control over the workload presented to them, and thereby relinquish control over their ability to maintain the service?

Juan my experience of other teams building hosted databases is that they implement two separate features in their infrastructure:

• there is a global filter that applies to every hosted service equally, which can be deployed "quickly" by SREs and is not controllable by customers
• there is a per-application / service filter entirely controllable by customers (with limited control for SREs)

I do not think that we wish Angela to work on both at this time. We should really focus.

---

docs/RFCS/20201020_sql_query_denylist.md, line 76 at r1 (raw file):
fsnotify is relying on the event notifcation from the linux kernel working properly, which it does not for certain ways docker runs volume mounts.
For example, a fsnotify-based solution would certainly not work when running docker on top of macOS.

most of these result in a different file extension that could be ignored by the watcher, right?

No that is not my experience. Text editors overwrite files in-place, especially when the person editing tends to press Ctrl+S regularly as they write, which is customary.

Do you know off the top of your head where I can dig into these a bit more?

cli/start.go for the signal handling

server/admin.go for the RPCs

that said I do not believe you need to do a deep analysis at this point of the RFC. You can simply explain "there's going to be a trigger mechanism".

---

docs/RFCS/20201020_sql_query_denylist.md, line 106 at r1 (raw file):

the canonical SQL inside CockroachDB after transformations. This is not always intuitive to the operator configuring the YAML file — though if CC SREs are the primary customer, they would perhaps be able to validate with database SQL engineers sooner than on-prem operators.

I think this paragraph is key to determine the next course of action. The story you're sketching in this explanation is the following:

1. malicious user sends "complicated" SQL to crdb
2. crdb parses and normalizes the SQL to something simpler
3. however during execution the node crashes / becomes starved for resources
4. the SRE gets notified of the situation and needs to "do something about it"

However at the point step 4 happens, crdb is already down and inoperative. Therefore, at that point in time the SRE cannot any more ask the crdb node to show what the "canonical" SQL looks like. It is too late. The only thing the SRE may see is the "raw" SQL that is incoming on client connections presumably as they are trying to disrupt more nodes.

What I am pointing out is that a SRE that needs to "take action" during an attack must be able to do so with the least dependencies on the system under attack. It is unreasonable to expect them to 1) log in to a crdb node 2) do query parsing and planning 3) show the resulting SQL just so they can figure out which regexp to apply.

This clearly and directly (to my eyes) redirects the design towards a pattern match on the raw SQL before it "enters" crdb. Either in the SQL proxy or at the level of "pgwire".

This also directly reminds me of another thing we need to deal with: if there are SQL strings that cause problems in pgwire or the SQL executor before they get planned, then it's possible for them to "crash" or disrupt a node before the code inside the "execution" function is reached, where you were planning to introduce the pattern match.

I am also reminded that if a SQL string is extremely long, or if it contains a lot of placeholders, then pgwire is going to consume a lot of resources just reading the SQL. So simply waiting for the SQL to be entirely received by a crdb node makes it already too late for the check.

So we can't do this deep inside the SQL layer, after query normalization. And even in pgwire may be too late.

So overall I think this makes it clear that pattern matching needs to apply on the "raw" SQL, and as early as possible, and if possible before it "enters" the crdb node. So that tends to point to the SQL proxy, and this way my reasoning gets me to the same place Rafi also reached in his last comment.

[angelapwen]

Thanks all for your input. Will regroup with @vy-ton and @otan today to discuss the great points brought up and consider the SQL proxy mechanism in more depth

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @awoods187, @joshimhoff, @JuanLeon1, @knz, @otan, @rafiss, and @vy-ton)

---

docs/RFCS/20201020_sql_query_denylist.md, line 11 at r1 (raw file):

Previously, knz (kena) wrote…

Juan my experience of other teams building hosted databases is that they implement two separate features in their infrastructure:

• there is a global filter that applies to every hosted service equally, which can be deployed "quickly" by SREs and is not controllable by customers
• there is a per-application / service filter entirely controllable by customers (with limited control for SREs)

I do not think that we wish Angela to work on both at this time. We should really focus.

I agree that we should be focusing on the former: the global filter that is deployed by SREs and does not have application developer configurability.

---

docs/RFCS/20201020_sql_query_denylist.md, line 76 at r1 (raw file):

Previously, knz (kena) wrote…

fsnotify is relying on the event notifcation from the linux kernel working properly, which it does not for certain ways docker runs volume mounts.
For example, a fsnotify-based solution would certainly not work when running docker on top of macOS.

most of these result in a different file extension that could be ignored by the watcher, right?

No that is not my experience. Text editors overwrite files in-place, especially when the person editing tends to press Ctrl+S regularly as they write, which is customary.

Do you know off the top of your head where I can dig into these a bit more?

cli/start.go for the signal handling

server/admin.go for the RPCs

that said I do not believe you need to do a deep analysis at this point of the RFC. You can simply explain "there's going to be a trigger mechanism".

Thank you, understood! 🙏

[angelapwen]

Hi everyone 👋 thank you for all your input on the RFC thus far. We've decided to work on a small prototype solely for the SRE use case to begin with, and once the SREs have played around with it a bit we can continue discussions on this RFC with more information.

To that end we've moved conversation to #tmp-sql-denylist on Slack, where you're welcome to join us. We've had some interesting suggestions by @jordanlewis and @andy-kimball in the channel to add to the mix that may inform some larger changes to the RFC and prototype.

[angelapwen]

Following up for interested parties. Based off conversation in the Slack channel, we decided to move forward with a simpler feature flag implementation for now to address CC SRE's use cases more quickly.

Thank you for your feedback again! We will leave this RFC draft if and when the denylist is revisited.

[knz]

Please translate the document you linked to a new rfc document in the repository. We need a public record of this conversation

[angelapwen]

Please translate the document you linked to a new rfc document in the repository. We need a public record of this conversation

Sure, I am happy to do this. I was under the impression that the feature flag implementation was not advanced enough to require its own RFC, but understand the need for public record somewhere outside of a Google Doc hosted in an intern's Google Drive. I will write a separate RFC document and link to this one.

[angelapwen]

Hi everyone 👋 as my internship winds down and I look to presenting on Thursday, I am providing an update here via comment on the status of the project and our design decisions thus far. As the current implementation was much simpler and did not require many comments, I am writing down the relevant notes here as opposed to another separate RFC.

Current Implementation — Feature Flags

Primary Differences

Instead of a denylist, we will move forward with an MVP of a feature flag-based way to block SQL queries based off of recent conversations in #tmp-sql-denylist. It is estimated that this mechanism will block >90% of the queries that SREs have needed to be blocked in the past and a finer-grained regex denylist is not indicated at this time. Additionally it seems that there will be a longer path to engineering consensus around designs for the regex denylist, and this implementation will allow us to more quickly address the most pressing issue at hand. The feature flag implementation means that we do not have need for a ‘staging’ version anymore, as there should not be ambiguity about whether a feature flag correctly matches the feature to be turned off (as compared to regular expression matching).

Another difference between this proposal and the previous one is that there will be a separation of concerns between what the SREs will handle on the Cockroach Cloud side vs. the database engineers on CRDB. Originally it was proposed that we would need a file-based mechanism so that SREs would be able to implement a denylist when clusters are unresponsive; but the consensus on #tmp-sql-denylist has been that a better design (proposed by @andy-kimball) would be for SREs to deny access for the particular user triggering the unresponsiveness; implement the query denying on the SQL side; and then allow access for the user again. This would allow for greater decoupling between the database and CC SRE work, and allows us to move forward using cluster settings for implementation.

Storage Mechanism — Cluster Settings

We use cluster settings to load in the flags we want to turn on. Every time the relevant cluster setting is set or changed, all nodes can parse the new configuration and store which features to block in a cache. Once the new configuration is cached/stored, the feature flag will be checked during the planning phase of command execution.

Our MVP includes boolean flags for each feature or feature category. So far we have: feature.backup.enabled, feature.restore.enabled, feature.import.enabled, feature.export.enabled, feature.changefeed.enabled, feature.stats.enabled, and feature.schema_change_enabled.

A sample PR for basic implementation on feature.stats.enabled is at #57076.

Metrics and Telemetry

A telemetry counter sql.feature_flag.denied has been added for each time a statement is denied due to a feature flag. Additionally, a metric counter sql.feature_flag_denial has been added in the same situation and will be displayed in a timeseries graph on the DB Console, in the SQL dashboard.

Future Extensions

Adding hierarchy to features

This option allows even greater ability to set and unset entire categories of features, such as schema changes; while maintaining the ability to set and unset more granular features as well.

Syntactic Sugar

This extension would wrap the same functionality mentioned above into a cleaner, more intuitive CLI command so that operators would not need to learn syntax around setting cluster settings.

Future Considerations

We consider many features out of scope for this MVP, but do want to consider how extending the MVP to allow for those features affect our design. These include:

• How on-prem users would interact with the feature flags
• How CC serverless (where only the system tenant can set a cluster setting) will be able to use this functionality on a per-tenant basis. For the purpose of this use case, only the file cluster setting or system table approach would be extensible, as there would need to be extra configurability around tenant-by-tenant settings/table entries.

I hope this is useful for any future readers who may be interested in expanding upon the feature flags or moving forward with the denylist! ✨

[joshimhoff]

Thank you for the writeup (& the feature flags)!!

[otan]

do you mind replacing the RFC with the write up, let's merge this (so it's off my stuff to review list :P )

[knz]

I asked angela to make the rfc. If you cannot review it then I'll do it.

[otan]

Ah the bit in the comment just needs to be translated into the markdown and we should be good.

[otan]

(no rush tho!)

[angelapwen]

Oh, sorry I think I had misunderstood. I had thought we were going to leave this RFC PR 'open' until/if a denylist or file-mounted path were to be revisited.

To clarify, I should replace the original markdown file on denylist with the path we ended up going (summarized via the last comment) and merge it? @otan @knz

[knz]

There are two possible situations:

• if the description of the work that was done is a natural / small deviation from the main text of the RFC today, then I recommedn to update the RFC with the description of the work done, then merge that.

• otherwise (large difference) you'd proceed as follows:

  1. edit the RFC that's already there with a small section that describes why that specific proposal was not realized
  2. mark the RFC (in the status field) as "rejected" or "postponed"
  3. add a new RFC doc with your description of the work done (or, at your leisure, a tech note in the tech note directory). If you go for an RFC, you would then mark it as "completed". You can add this 2nd document inside this PR.
  4. if at all possible, cross-link the two
  5. merge this PR.

[angelapwen]

I see! I will do the second option as I believe they are quite fundamentally different designs and would benefit from having two separate docs.

[cockroach-teamcity]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

angelapwen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}