Add setup alert for no privileges on events table #75
Conversation
oguzhanunlu
force-pushed
the
no-priv-err
branch
2 times, most recently
from
ab8c424 to
5018058
Compare
oguzhanunlu
force-pushed
the
no-priv-err
branch
from
5018058 to
f8b785c
Compare
| implicit class FilteredSnowflakeSQLException(val sse: SnowflakeSQLException) extends AnyVal { | ||
| def isEligibleSetupError: Boolean = { | ||
| lazy val pattern = "^SQL compilation error:\nTable '(\\w+)' already exists, but current role has no privileges on it.*".r | ||
| sse.getErrorCode === 3041 && sse.getSQLState === "42710" && pattern.matches(sse.getMessage) |
There was a problem hiding this comment.
| sse.getErrorCode === 3041 && sse.getSQLState === "42710" && pattern.matches(sse.getMessage) | |
| pattern.matches(sse.getMessage) |
I would not be so precise. I think that matching on the error message is enough, for 2 reasons:
- What if Snowflake keeps the same error message, but changes the error code? Very unlikely I know
- Are we 100% sure that no other error code can lead to this error message?
There was a problem hiding this comment.
Oh, my instinct was to suggest the exact opposite!! I would match on the error code only, because I think the message is more likely to change over time, whereas error codes tend to stay the same over time.
I noticed this code in a Snowflake repo, which suggests all of the following error codes are authorization failures: (1063, 3001, 3003, 3005, 3007, 3011, 3041).
Notice that we already catch and handle 3001 on line 57. And here you add a catch for 3041. Could we consolidate those two matches into one matcher? i.e. amend line 57 to catch 3001 OR 3041 (and possibly the other linked error codes too).
There was a problem hiding this comment.
I suggest that we have two different checks (two case), one on the error codes, and one on the error message only. This way if error code or message changes, we still catch the error
| case sse: SnowflakeSQLException if sse.isEligibleSetupError => | ||
| // if loader role is not granted to SYSADMIN, snowflake throws the below | ||
| // Table 'EVENTS' already exists, but current role has no privileges on it. | ||
| "Loader role has no privileges on events table. Role must be granted to SYSADMIN." |
There was a problem hiding this comment.
| "Loader role has no privileges on events table. Role must be granted to SYSADMIN." | |
| s"Loader role has no privileges on $tableName table. Role must be granted to SYSADMIN." |
It'd be nice to extract the table name in the regex and put it in the message.
You could use unapply for that, like here.
This PR extends setup alert mechanism by adding a new error code check for
3041, one of authorization error codes from snowflake.An example exception for error code 3041 is as following:
and the alert message will be
ref: pdp-1792