Create read and write access level roles and give grants by atanasovskib · Pull Request #69 · timescale/promscale · GitHub

This repository was archived by the owner on Apr 2, 2024. It is now read-only.

Contributor

atanasovskib commented Apr 30, 2020 •

edited

Loading

The prom_reader and prom_writer roles are created
and granted specific access to all the different schemas,
tables and functions. prom_writer inherits from prom_reader.
prom_reader has read-only access.


          Craete read and write access level roles and give grants

17978f6

The prom_reader and prom_writer roles are created
and granted specific access to all the different schemas,
tables and functions. prom_writer inherits from prom_reader.
prom_reader has read-only access.

atanasovskib requested review from cevian and davidkohn88

April 30, 2020 16:16

cevian reviewed

View reviewed changes

Contributor

cevian left a comment

Some comments, mostly look good.

We need to add the idempotent role creation
Let's remove the grants for admin functions but leave a comment there for later so easy to find
We need to do idempotent role creation in extension sql as well for prom_reader and add grants to that role for the functions there (https://github.com/timescale/timescale-prometheus/blob/master/extension/sql/timescale-prometheus.sql)

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              GRANT USAGE ON SCHEMA SCHEMA_CATALOG TO prom_reader;

              GRANT SELECT ON ALL TABLES IN SCHEMA SCHEMA_CATALOG TO prom_reader;

              ALTER DEFAULT PRIVILEGES IN SCHEMA SCHEMA_CATALOG GRANT SELECT ON TABLES TO prom_reader;

              GRANT CREATE ON SCHEMA SCHEMA_CATALOG TO prom_writer;

Contributor

cevian Apr 30, 2020

this should be an admin thing. A writer is only for writing prom data it does not need to add catalog tables.

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              GRANT SELECT ON ALL TABLES IN SCHEMA SCHEMA_CATALOG TO prom_reader;

              ALTER DEFAULT PRIVILEGES IN SCHEMA SCHEMA_CATALOG GRANT SELECT ON TABLES TO prom_reader;

              GRANT CREATE ON SCHEMA SCHEMA_CATALOG TO prom_writer;

              GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA SCHEMA_CATALOG TO prom_writer;

Contributor

cevian Apr 30, 2020

Privileges should be SELECT, INSERT, UPDATE, DELETE not all. Same on line below

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              CREATE SCHEMA IF NOT EXISTS SCHEMA_PROM; -- data tables + public functions

              GRANT USAGE ON SCHEMA SCHEMA_PROM TO prom_reader;

              GRANT SELECT ON ALL TABLES IN SCHEMA SCHEMA_PROM TO prom_reader;

Contributor

cevian Apr 30, 2020

actually there are no tables in here any more (need too change comment) so I think this should just be USAGE on the schema for both reader and writer

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              CREATE SCHEMA IF NOT EXISTS SCHEMA_EXT; -- optimized versions of functions created by the extension

              GRANT USAGE ON SCHEMA SCHEMA_EXT TO prom_reader;

              GRANT SELECT ON ALL TABLES IN SCHEMA SCHEMA_EXT TO prom_reader;

Contributor

cevian Apr 30, 2020

again no tables in here so we just need usage for both reader and writer.

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              GRANT SELECT ON ALL TABLES IN SCHEMA SCHEMA_SERIES TO prom_reader;

              ALTER DEFAULT PRIVILEGES IN SCHEMA SCHEMA_SERIES GRANT SELECT ON TABLES TO prom_reader;

              GRANT CREATE ON SCHEMA SCHEMA_SERIES TO prom_writer;

              GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA SCHEMA_SERIES TO prom_writer;

Contributor

cevian Apr 30, 2020

should probably be SELECT only since these are just views

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              LANGUAGE SQL VOLATILE;

              COMMENT ON FUNCTION SCHEMA_PROM.set_default_retention_period(INTERVAL)

              IS 'set the retention period for any metrics (existing and new) without an explicit override';

              GRANT EXECUTE ON FUNCTION SCHEMA_PROM.set_default_retention_period(INTERVAL) TO prom_writer;

Contributor

cevian Apr 30, 2020

this should be an admin func

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
                  SELECT true;

              $func$

              LANGUAGE SQL VOLATILE;

              GRANT EXECUTE ON FUNCTION SCHEMA_PROM.set_metric_retention_period(TEXT, INTERVAL) TO prom_writer;

Contributor

cevian Apr 30, 2020

admin function

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              LANGUAGE SQL VOLATILE;

              COMMENT ON FUNCTION SCHEMA_PROM.reset_metric_retention_period(TEXT)

              IS 'resets the retention period for a specific metric to using the default';

              GRANT EXECUTE ON FUNCTION SCHEMA_PROM.reset_metric_retention_period(TEXT) TO prom_writer;

Contributor

cevian Apr 30, 2020

admin func

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              END

              $func$

              LANGUAGE PLPGSQL VOLATILE;

              GRANT EXECUTE ON FUNCTION SCHEMA_CATALOG.drop_metric_chunks(TEXT, TIMESTAMPTZ) TO prom_writer;

Contributor

cevian Apr 30, 2020

admin func

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              $$ LANGUAGE PLPGSQL;

              COMMENT ON PROCEDURE SCHEMA_PROM.drop_chunks()

              IS 'drops data according to the data retention policy. This procedure should be run regularly in a cron job';

              GRANT EXECUTE ON PROCEDURE SCHEMA_PROM.drop_chunks() TO prom_writer;

Contributor

cevian Apr 30, 2020

admin func

davidkohn88 reviewed

View reviewed changes

Contributor

davidkohn88 left a comment

This looks good. I don't know whether we also need to modify grants in the extension bit though?

pkg/pgmodel/migrations/sql/1_base_schema.up.sql Outdated

    
              GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA SCHEMA_INFO TO prom_writer;

              ALTER DEFAULT PRIVILEGES IN SCHEMA SCHEMA_INFO GRANT ALL PRIVILEGES ON TABLES TO prom_writer;

              GRANT prom_reader TO prom_writer;

Contributor

davidkohn88 Apr 30, 2020

Doesn't horribly matter, but I'd put the grant of prom reader to prom writer right after you create the roles, just makes everything else make more sense.

Blagoj Atanasovski added 3 commits

May 1, 2020 13:38


          Remove excess GRANTS given to prom_writer

0933a9d


          Make CREATE ROLE idempotent

0e4b92a


          Remove EXECUTE GRANT for prom_writer on set_*_chunk_interval funcs

a329015

Also fixed SELECT GRANT not given to prom_reader on SCHEMA_PROM

atanasovskib force-pushed the create-access-roles branch from 10c9b9d to a329015 Compare

May 1, 2020 11:56

Blagoj Atanasovski added 2 commits

May 1, 2020 14:06


          Create prom_reader and add GRANTS in extension

f38ff2c


          run generate on migration files

3faa547

Contributor Author

atanasovskib commented May 1, 2020

Committed changes per comment suggestions. Additionally:

Also assumed set_chunk_interval functions are admin as set_retention
Granted only select on SCHEMA_METRIC since it has only views as SCHEMA_SERIES

atanasovskib changed the title ~~WIP: Craete read and write access level roles and give grants~~ Craete read and write access level roles and give grants

atanasovskib changed the title ~~Craete read and write access level roles and give grants~~ Create read and write access level roles and give grants

cevian approved these changes

View reviewed changes

Contributor

cevian left a comment

Looks good. I will want some of our integration test to run as users for prom_reader and prom_writer users. But I'll leave it up to you if that should be part of this or a a separate PR. Also, I think some of the commits can be squashed before merge.

atanasovskib merged commit 02eb5aa into master

atanasovskib deleted the create-access-roles branch

May 4, 2020 14:03

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

Labels

None yet