Skip to content

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Oct 14, 2019

Fixes #5511.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

@ericharmeling ericharmeling force-pushed the configure-zone-permissions branch from 3ab6974 to 1b8bdd3 Compare October 14, 2019 18:43
@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Eric, can you please get an engineering review for this first? I tried reading the related CRDB PR but didn't immediately understand how this doc update connects with that PR, so I'd be more comfortable looking at this after a technical review.

@ericharmeling ericharmeling requested a review from rohany October 16, 2019 19:36
@ericharmeling
Copy link
Contributor Author

Hey Eric, can you please get an engineering review for this first? I tried reading the related CRDB PR but didn't immediately understand how this doc update connects with that PR, so I'd be more comfortable looking at this after a technical review.

Sure thing! @rohany Can you look over this PR for technical accuracy?

@rohany
Copy link

rohany commented Oct 16, 2019

@solongordon is a better reviewer for this one -- handing it off.

@rohany rohany requested a review from solongordon October 16, 2019 20:11
@ericharmeling
Copy link
Contributor Author

@solongordon Friendly reminder to review :)

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @rohany, and @solongordon)


v19.2/configure-zone.md, line 43 at r1 (raw file):

## Required privileges

If the target is a [`system`](show-databases.html#preloaded-databases) range, the `system` database, or a table in the `system` database, the user must be an [`admin`](authorization.html#create-and-manage-roles). For all other databases and tables, the user must have the [CREATE](grant.html#supported-privileges) privilege on the target database or table.

I think the link you have on the system range should actually be used for the system database. For the system range it would be better to link to #create-a-replication-zone-for-a-system-range.


v19.2/configure-zone.md, line 46 at r1 (raw file):

{{site.data.alerts.callout_info}}
Required privileges for `CONFIGURE ZONE` statements in CockroachDB v19.2 may be backward-incompatible for users running scripted statements with restricted permissions in versions earlier than v19.1.<br/>To add the necessary permissions, use [`GRANT` &lt;privileges&gt;](../v19.2/grant.html) or [`GRANT` &lt;roles&gt;](../v19.2/grant-roles.html) as a user with an admin role. For example, to grant a user the admin role, run `GRANT admin TO <user>`. To grant the `CREATE` privilege on a database or table, run `GRANT CREATE ON [DATABASE | TABLE] <name> TO <user>`.

Rather than "versions earlier than v19.1", I think it should say "v19.1 and earlier" or just "earlier versions."

@ericharmeling ericharmeling force-pushed the configure-zone-permissions branch from 1b8bdd3 to e865465 Compare October 21, 2019 16:01
Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)


v19.2/configure-zone.md, line 43 at r1 (raw file):

Previously, solongordon (Solon) wrote…

I think the link you have on the system range should actually be used for the system database. For the system range it would be better to link to #create-a-replication-zone-for-a-system-range.

Done.


v19.2/configure-zone.md, line 46 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Rather than "versions earlier than v19.1", I think it should say "v19.1 and earlier" or just "earlier versions."

Done.

@cockroach-teamcity
Copy link
Member

@ericharmeling
Copy link
Contributor Author

@rmloveland This should be ready for you to review.

Per @solongordon's advice, I'm going to open a separate PR to update example output of the SHOW ZONE CONFIGURATION statement, and the doc for system ranges.

If the target is a [`system` range](#create-a-replication-zone-for-a-system-range), the [`system` database](show-databases.html#preloaded-databases), or a table in the `system` database, the user must be an [`admin`](authorization.html#create-and-manage-roles). For all other databases and tables, the user must have the [CREATE](grant.html#supported-privileges) privilege on the target database or table.

{{site.data.alerts.callout_info}}
Required privileges for `CONFIGURE ZONE` statements in CockroachDB v19.2 may be backward-incompatible for users running scripted statements with restricted permissions in v19.1 and earlier.<br/>To add the necessary permissions, use [`GRANT` &lt;privileges&gt;](../v19.2/grant.html) or [`GRANT` &lt;roles&gt;](../v19.2/grant-roles.html) as a user with an admin role. For example, to grant a user the admin role, run `GRANT admin TO <user>`. To grant the `CREATE` privilege on a database or table, run `GRANT CREATE ON [DATABASE | TABLE] <name> TO <user>`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~I think this should be a red box since this is potentially a breaking change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tad awkward to have all this inside a blue box, but I agree with the choice. Suggest adding line breaks before "For example" and again before "To grant the CREATE privilege" to make it easier to scan (since those lines are each separate tasks for the user).

@ericharmeling ericharmeling force-pushed the configure-zone-permissions branch from e865465 to cf34e24 Compare October 24, 2019 16:26
Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rmloveland and @solongordon)


v19.2/configure-zone.md, line 46 at r2 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

It's a tad awkward to have all this inside a blue box, but I agree with the choice. Suggest adding line breaks before "For example" and again before "To grant the CREATE privilege" to make it easier to scan (since those lines are each separate tasks for the user).

Done.

@cockroach-teamcity
Copy link
Member

@ericharmeling ericharmeling deleted the configure-zone-permissions branch November 12, 2019 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: check privileges in CONFIGURE ZONE commands
5 participants