Fix change detection condition on KeeperMap read-only step

[aris-aiven]

Fix change detection condition on KeeperMap read-only step

Unfortunately looking at system.grants to check whether specific grants have
been successfully revoked in ClickHouse is not perfect.

Using the system table or parsing the output or SHOW GRANTS FOR <user> makes
the step more brittle, and doesn't necessarily lead to better garantees. The fix
is to remove the watcher.

More details:

Let a user with a set of initial grants, including some ALTER statements. Doing
REVOKE INSERT, ALTER UPDATE, ALTER DELETE ON <user> FROM <table> on this
user creates rows in system.grants for each of the grants with is_partial_revoke=1.

If the user doesn't have ALTER statements in their initial grants, then
after executing REVOKE the system.grants table simply has no
corresponding rows.

[joelynch]

Doesn't look like it works but I might be misunderstanding.

[joelynch]

Can this return if the user has a grant on the database but before the revoke goes through?

[aris-aiven]

If the user has an explicit grant for any of INSERT, ALTER UPDATE,ALTER DELETE the count will be greater than zero and the waiter won't return directly no.

[aris-aiven]

Ok actually checking that the revoke statement is in the response returned by SHOW GRANTS FOR user might be better.

[joelynch]

If I comment out

# await asyncio.gather(*(client.execute(revoke_statement.encode()) for client in self.clients))

this line still returns in the test test_keeper_map_table_read_only_step.

Would it make more sense to actually just check for the partial revoke. Rather than filtering out those rows.

[joelynch]

sql = """
SELECT access_type
FROM system.grants
WHERE ...
AND is_partial_revoke
"""
revokes = (await client.execute(sql)).data
for type in ('INSERT', 'ALTER UPDATE', 'ALTER DELETE'):
    assert type in revokes

[aris-aiven]

Well not really either. Here is the result for SHOW GRANTS after the execution of the grant altering statement during tests.integration.coordinator.plugins.clickhouse.test_steps.test_keeper_map_table_read_only_step

INFO     astacus.coordinator.plugins.clickhouse.steps:steps.py:227 Grants for user b'foobar': [['GRANT SHOW, SELECT, INSERT, ALTER UPDATE, ALTER DELETE, ALTER COLUMN, ALTER MODIFY COMMENT, ALTER INDEX, ALTER PROJECTION, ALTER CONSTRAINT, ALTER TTL, ALTER SETTINGS, ALTER VIEW, CREATE TABLE, DROP TABLE, TRUNCATE, OPTIMIZE, SYSTEM SYNC REPLICA ON keeperdata.* TO foobar WITH GRANT OPTION'], ['REVOKE INSERT, ALTER UPDATE, ALTER DELETE ON keeperdata.keepertable FROM foobar']]
INFO     astacus.coordinator.plugins.clickhouse.steps:steps.py:227 Grants for user b'alice': [['GRANT SELECT ON default.* TO alice'], ['GRANT bob TO alice']]
INFO     astacus.coordinator.plugins.clickhouse.steps:steps.py:227 Grants for user b'z_\x80_enjoyer': []
INFO     astacus.coordinator.plugins.clickhouse.steps:steps.py:227 Grants for user b'foobar': [['GRANT SHOW, SELECT, INSERT, ALTER UPDATE, ALTER DELETE, ALTER COLUMN, ALTER MODIFY COMMENT, ALTER INDEX, ALTER PROJECTION, ALTER CONSTRAINT, ALTER TTL, ALTER SETTINGS, ALTER VIEW, CREATE TABLE, DROP TABLE, TRUNCATE, OPTIMIZE, SYSTEM SYNC REPLICA ON keeperdata.* TO foobar WITH GRANT OPTION'], ['REVOKE GRANT OPTION FOR INSERT, ALTER UPDATE, ALTER DELETE ON keeperdata.keepertable FROM foobar']]
INFO     astacus.coordinator.plugins.clickhouse.steps:steps.py:227 Grants for user b'alice': [['GRANT SELECT ON default.* TO alice'], ['GRANT INSERT, ALTER UPDATE, ALTER DELETE ON keeperdata.keepertable TO alice'], ['GRANT bob TO alice']]
INFO     astacus.coordinator.plugins.clickhouse.steps:steps.py:227 Grants for user b'z_\x80_enjoyer': [['GRANT INSERT, ALTER UPDATE, ALTER DELETE ON keeperdata.keepertable TO `z_�_enjoyer`']]

[joelynch]

Yea okay I see. Feels like it should be an upstream feature.

[aris-aiven]

But that's the catch, is_partial_revoke isn't a good discriminating variable. If you filter by it, and you have the grants as below (which will happen after the first backup has been taken), you might return before the grants are revoked (and those rows are gone).

user_name         role_name    access_type    database    table       column      is_partial_revoke    grant_option
----------------  -----------  -------------  ----------  ----------  --------  -------------------  --------------
aiven_monitoring               INSERT         default     keeper_map                              0               0
aiven_monitoring               ALTER UPDATE   default     keeper_map                              0               0
aiven_monitoring               ALTER DELETE   default     keeper_map                              0               0
alice                          INSERT         default     keeper_map                              0               0
alice                          ALTER UPDATE   default     keeper_map                              0               0
alice                          ALTER DELETE   default     keeper_map                              0               0
bob                            INSERT         default     keeper_map                              1               1
bob                            ALTER UPDATE   default     keeper_map                              1               1
bob                            ALTER DELETE   default     keeper_map                              1               1

Removed the watcher and improved the integration test