Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docs for changefeed privileges in v25.1 #19290

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kathancox
Copy link
Contributor

@kathancox kathancox commented Jan 9, 2025

Fixes DOC-12058

This PR updates the changefeed privilege docs to deprecate the use of the CHANGEFEED privilege for viewing and managing changefeed jobs on tables users have the privilege on (from v25.1). Instead, documenting that users should use job ownership or global privileges to view and manage jobs.

This PR also adds the ALTER JOB statement, which can transfer job ownership between users or roles. (SQL diagram not live yet until the eng work is merged into master.)

Preview

https://deploy-preview-19290--cockroachdb-docs.netlify.app/docs/v25.1/alter-job.html
https://deploy-preview-19290--cockroachdb-docs.netlify.app/docs/v25.1/create-changefeed.html#required-privileges

Copy link

github-actions bot commented Jan 9, 2025

Files changed:

Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit 98bc23d
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/677ff4efa67c4300091350fe

Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit 98bc23d
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/677ff4ef2caa930008fab949

Copy link

netlify bot commented Jan 9, 2025

Netlify Preview

Name Link
🔨 Latest commit 98bc23d
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/677ff4ef549cb500088d3832
😎 Deploy Preview https://deploy-preview-19290--cockroachdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kathancox kathancox requested a review from dt January 13, 2025 19:18
@kathancox kathancox marked this pull request as ready for review January 13, 2025 19:18
@kathancox
Copy link
Contributor Author

Hey @dt, can you ptal? The SQL diagram for alter job will not yet render because the release branch is not ready — I'll have to wait to merge for this.

@kathancox kathancox marked this pull request as draft January 13, 2025 19:19
@dt
Copy link
Member

dt commented Jan 13, 2025

Thanks for picking this up!

@kathancox kathancox requested a review from rmloveland January 16, 2025 12:36
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.

LGTM overall but i really don't understand how we can do the retroactive statements w.r.t "legacy model" in this PR that reference previous versions without updating those previous versions' docs - also absolutely nothing about these retroactive statements are mentioned in the linked issue so I don't get it at all

{{site.data.alerts.callout_info}}
Starting in v22.2, CockroachDB introduces a new [system-level privilege model]({% link {{ page.version.version }}/security-reference/authorization.md %}#supported-privileges) that provides finer control over a user's privilege to work with the database, including creating and managing changefeeds.
{{site.data.alerts.callout_danger}}
As of v25.1, viewing and managing a changefeed job by users with the `CHANGEFEED` privilege is **deprecated**. This functionality of the `CHANGEFEED` privilege will be removed in a future release.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestions:

  1. link the term "privilege" to the list of privileges at https://www.cockroachlabs.com/docs/v25.1/security-reference/authorization#privileges
  2. on that page, add a note to the CHANGEFEED privilege stating that as of v25.1, it is deprecated; and also add a link from the description of the (now noted as deprecated) CHANGEFEED privilege back to this via e.g. create-changefeed.md#required-privileges (i know this is specifically an include but I think linking to any section that mentions the privilege change is fine)


There is continued support for the [legacy privilege model](#legacy-privilege-model) for changefeeds in v23.1, however it **will be removed** in a future release of CockroachDB. We recommend implementing the new privilege model that follows in this section for all changefeeds.
We recommend transitioning users that need to view and manage running changefeed jobs to [roles]({% link {{ page.version.version }}/create-role.md %}) that own the [jobs]({% link {{ page.version.version }}/show-jobs.md %}) or [granting]({% link {{ page.version.version }}/grant.md %}) them the `VIEWJOB` or `CONTROLJOB` privilege. For more details, refer to [View and manage changefeed jobs](#view-and-manage-changefeed-jobs).
Copy link
Contributor

Choose a reason for hiding this comment

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

this mention of "privilege" could also be a link to the list of privileges since presumably i'd want to read the docs for those privs before just granting them


Granted privileges | Usage
-------------------+-------
`CHANGEFEED` | Create changefeeds on tables. For details, refer to [`CHANGEFEED` privilege](#changefeed-privilege).<br>**Deprecated**: View and manage changefeed jobs on tables.
Copy link
Contributor

Choose a reason for hiding this comment

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

i can see these are deprecated but we don't say what to do instead

any chance of pulling in the text from above for each of these deprecated options, e.g.

Transition users that need to view and manage running changefeed jobs to [roles]({% link {{ page.version.version }}/create-role.md %}) that own the [jobs]({% link {{ page.version.version }}/show-jobs.md %}) or [granting]({% link {{ page.version.version }}/grant.md %}) them the VIEWJOB or CONTROLJOB privilege. For more details, refer to View and manage changefeed jobs.

I realize it's a bit tedious and maybe there's a better way using includes or so, but the current text assumes i've read the big warning in the previous section but maybe i haven't. as a user i'd expect that since you are describing this privilege here and saying it's deprecated you are also gonna tell me what to do instead

Granted privileges | Usage
-------------------+-------
`CHANGEFEED` | Create changefeeds on tables. For details, refer to [`CHANGEFEED` privilege](#changefeed-privilege).<br>**Deprecated**: View and manage changefeed jobs on tables.
`CHANGEFEED` + [`USAGE`]({% link {{ page.version.version }}/create-external-connection.md %}#required-privileges) on external connection | Create changefeeds on tables to an external connection URI. For details, refer to [`CHANGEFEED` privilege](#changefeed-privilege).<br>**Deprecated**: View and manage changefeed jobs on tables.<br><br>**Note:** If you need to manage access to changefeed sink URIs, set the `changefeed.permissions.require_external_connection_sink.enabled=true` cluster setting. This will mean that users with these privileges can **only** create changefeeds on external connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above re: if it's deprecated, pls tell me what to use instead?

`CONTROLJOB` | Manage changefeed jobs ([pause]({% link {{ page.version.version }}/pause-job.md %}), [resume]({% link {{ page.version.version }}/resume-job.md %}), and [cancel]({% link {{ page.version.version }}/cancel-job.md %})). For details, refer to [View and manage changefeed jobs](#view-and-manage-changefeed-jobs).
`VIEWJOB` | [View]({% link {{ page.version.version }}/show-jobs.md %}#show-changefeed-jobs) changefeed jobs. For details, refer to [View and manage changefeed jobs](#view-and-manage-changefeed-jobs).
`SELECT` | Create a sinkless changefeed that emits messages to a SQL client.
**Deprecated** `CONTROLCHANGEFEED` role option + `SELECT` | Create changefeeds on tables.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this deprecation also covered by the "use a role that owns the jobs, or these other privileges" text? if so it would be good to see that here somehow

maybe they all have like a little star/asterisk that points to a single note at the bottom of the table idk, defer to your expertise on it, i just wanna know what to do instead without reading backwards up the page


You can add `CHANGEFEED` to the user or role's [default privileges]({% link {{ page.version.version }}/security-reference/authorization.md %}#default-privileges) with [`ALTER DEFAULT PRIVILEGES`]({% link {{ page.version.version }}/alter-default-privileges.md %}#grant-default-privileges-to-a-specific-role):
To give a set of users access to a specific job, or set of jobs, you can assign them to a [role]({% link {{ page.version.version }}/security-reference/authorization.md %}#users-and-roles) that owns the job(s).
Copy link
Contributor

Choose a reason for hiding this comment

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

could drop "you can" and just say "assign them to a role that ..."


- The current job owner.
- A member of the role that is the current owner.
- An `admin`.
Copy link
Contributor

Choose a reason for hiding this comment

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


Unless the user is an `admin`, they can only transfer ownership of a job to themselves or to a role of which they are a member.

To add a user to a role, refer to the [`GRANT`]({% link {{ page.version.version }}/grant.md %}) statement.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could say "... use the GRANT statement"

@@ -23,6 +23,12 @@ The [examples](#examples) on this page provide the foundational syntax of the `C

### Legacy privilege model

{{site.data.alerts.callout_info}}
Starting in v22.2, CockroachDB introduces a new [system-level privilege model]({% link {{ page.version.version }}/security-reference/authorization.md %}#supported-privileges) that provides finer control over a user's privilege to work with the database, including creating and managing changefeeds.
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't understand this, this is referencing pretty far into the past to say "oh btw that was the legacy thing" but AFAICT this snippet of text cannot (meaningfully) retroactively accomplish this? even if it is "technically correct" etc?

also if this statement is really true would not the new docs we're adding here about the new model also be ported back to those previous versions? because otherwise how would this be a "legacy" model unless the new thing also existed in v22.2+ ?

whereas AFAICT in this PR the other docs about the non-legacy way of doing this are only being added to v25.1 ?

this feels "highly irregular", please enlighten me 🤓

{{site.data.alerts.callout_info}}
Starting in v22.2, CockroachDB introduces a new [system-level privilege model]({% link {{ page.version.version }}/security-reference/authorization.md %}#supported-privileges) that provides finer control over a user's privilege to work with the database, including creating and managing changefeeds.

There is continued support for the legacy privilege model for changefeeds in v23.1, however it **will be removed** in a future release of CockroachDB. We recommend implementing the [new privilege model](#required-privileges) for all changefeeds.
Copy link
Contributor

Choose a reason for hiding this comment

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

also perhaps implied by the above but it seems like this notice referring to previous versions should be backported to those versions since e.g. v24.3 currently says

To create a changefeed, the user must be a member of the admin role or have the CREATECHANGEFEED parameter set.

that said i know little about changefeeds and the associated roles but this seems like some very retroactive legislation of something and I don't understand what is happening here

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.

3 participants