Skip to content

Update LDR privileges for v25.2 #19518

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

Merged
merged 3 commits into from
May 7, 2025
Merged

Update LDR privileges for v25.2 #19518

merged 3 commits into from
May 7, 2025

Conversation

katmayb
Copy link
Contributor

@katmayb katmayb commented Apr 11, 2025

Fixes DOC-12962, DOC-12786

This PR updates the LDR privileges for v25.2.

Adds the REPLICATIONDEST and REPLICATIONSOURCE privilege. Deprecates the REPLICATION privilege.

Updates the SQL ref pages, tutorial for LDR, and general Auth page of privileges.

The tutorial content required some shuffling round to ensure the privilege instructions made logical sense.

Preview

Tutorial
Create logical replication stream
Create logically replicated

Copy link

github-actions bot commented Apr 11, 2025

Copy link

netlify bot commented Apr 11, 2025

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit 76b77c7
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/681b8a5c3d28410008f0d41d

Copy link

netlify bot commented Apr 11, 2025

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit 76b77c7
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/681b8a5cf6dd3e00081a3487


<image src="{{ 'images/v25.2/bidirectional-stream.svg' | relative_url }}" alt="Diagram showing bidirectional LDR from cluster A to B and back again from cluster B to A." style="width:70%" />

For more details on use cases, refer to the [Logical Data Replication Overview]({% link {{ page.version.version }}/logical-data-replication-overview.md %}).

## Syntax
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not new content, I moved this up from one of the steps. Since the privilege instructions would have preceded these syntax descriptions, it made sense to pull this Syntax section up to the introduction.

Copy link

netlify bot commented Apr 11, 2025

Netlify Preview

Name Link
🔨 Latest commit 76b77c7
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/681b8a5cf379b20008517470
😎 Deploy Preview https://deploy-preview-19518--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.

@katmayb katmayb requested review from msbutler and alicia-l2 April 11, 2025 20:30
Copy link

@alicia-l2 alicia-l2 left a comment

Choose a reason for hiding this comment

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

This LGTM - I have a bit of confusion about which user needs which privilege - I'm going to let Michael have the final say on that though.

Use the [`GRANT SYSTEM`]({% link {{ page.version.version }}/grant.md %}) statement:
- The table-level `REPLICATIONSOURCE` privilege on the source table(s).

This is the user provided in the source URI when you start a LDR stream.

Choose a reason for hiding this comment

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

Do you mean that the user provided in the source must have this privilege? This line doesn't have a bullet point on it so it was a bit confusing to read.


On the destination cluster:

- The table-level `REPLICATIONDEST` privilege on the destination table(s).

Choose a reason for hiding this comment

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

do we also need to call out that the user here should have the privilege?

Copy link

@msbutler msbutler left a comment

Choose a reason for hiding this comment

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

Thank you for doing this and sorry for such a slow review! I've been a bit under water lately.


For bidirectional LDR:

- The user in the original source URI, who begins the reverse LDR stream, requires the table-level `REPLICATIONDEST` privilege.

Choose a reason for hiding this comment

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

the CREATE LOGICAL REPLICATION STREAM syntax does not automatically set up a reverse stream. so i think this line can be rephrased. For "manually set up" bidi replication, the user essentially has to do the same auth exercises on both sides. E.g.

For setting up the stream from A to B, the user passed in for the URI to A must have the REPLICATIONSOURCE priv, and the user executing the command from B, needs to have the REPLICATIONDEST priv.

For setting up the stream from B to A, the user passed in for the URI to B must have the REPLICATIONSOURCE priv, and the user executing the command from A, needs to have the REPLICATIONDEST priv.


For bidirectional LDR:

- The user in the original source URI, who begins the reverse LDR stream, requires the table-level `REPLICATIONDEST` privilege.

Choose a reason for hiding this comment

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

we should also document that the user provided in the Dest URI must be the same user executing the CREATE LOGICALLY REPLICATED table cmd. For example, in this unit test:
https://github.com/jeffswenson/cockroach/blob/jeffswenson-workload-generate-decimals/pkg/crosscluster/logical/logical_replication_job_test.go#L2180
CREATE LOGICALLY REPLICATED TABLES (tab_clone_2, tab2_clone_2) FROM TABLES (tab, tab2) ON $1 WITH BIDIRECTIONAL ON $2
the user in the URI supplied at $2 must be the same user executing the CREATE LOGICALLY REPLICATED table cmd.

(I need to add a quick patch to assert this but higher priority things have gotten in the way)

For details on which syntax to use, refer to the [Syntax](#syntax) section at the beginning of this tutorial.

{{site.data.alerts.callout_info}}
If you are setting up bidirectional LDR, each cluster must **authorize both stream directions** using the table-level privileges. Ensure that you also grant privileges to users running the LDR stream in the reverse direction (from the original destination to the original source).

Choose a reason for hiding this comment

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

given my comments above on the slightly different reverse stream auth stories for bidi replication, I think this callout could be rephrased.

@katmayb katmayb force-pushed the ldr-privileges-25.2 branch 4 times, most recently from f5b0559 to 29b7b42 Compare April 30, 2025 20:55
@katmayb katmayb requested a review from msbutler April 30, 2025 21:02
Copy link

@msbutler msbutler left a comment

Choose a reason for hiding this comment

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

This looks much clearer! Left some minor corrections.

B ➔ A | B | User in the LDR connection string. | `REPLICATIONSOURCE`
B ➔ A | A | User running the command. | `REPLICATIONDEST`

Grant a table-level privilege with the [`GRANT`]({% link {{ page.version.version }}/grant.md %}) statement to a [user or a role]({% link {{ page.version.version }}/security-reference/authorization.md %}#users-and-roles):
Copy link

Choose a reason for hiding this comment

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

nit: a user can still grant these privs at the system level if they'd like. If a user was running LDR over a 1000 tables, perhaps they'd rather use the system level priv as oppose to granting the priv on each table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed!

----------------------+---------+-----------+-------------------
A ➔ B | A | User in source connection string. | `REPLICATIONSOURCE` on A's tables.
A ➔ B | B | User running `CREATE LOGICALLY REPLICATED` from the destination cluster. The destination table will be created and the user given the `REPLICATIONDEST` privilege on the new table automatically.<br>**Note:** Must match the user in the destination connection string for bidirectional LDR. | `CREATE` on B's parent database.
B ➔ A (reverse stream) | B | User in the new source connection string. | `REPLICATIONSOURCE` on B's tables.
Copy link

Choose a reason for hiding this comment

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

this isn't necessary. Becuase the user in the reverse stream is the user that ran the CREATE LDR stmt, it has CREATE on the database and is implicitly granted REPLICATIONSOURCE on B's tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, removed that line!

Use the [`GRANT SYSTEM`]({% link {{ page.version.version }}/grant.md %}) statement:
LDR from cluster A to B represents a _unidirectional_ setup from a source to a destination cluster. LDR from cluster B to A is the reverse stream for a _bidirectional_ setup:

Replication direction | Cluster | User role | Required privileges
Copy link

Choose a reason for hiding this comment

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

i think this table explains things really well, and I wonder if it should be supplemented with a toy command, especially for the bidi case:

user baz executes the following statement
CREATE LOGICALLY REPLICATED TABLES B.tab FROM TABLES A.tab ON 'uriToA/user=foo' WITH BIDIRECTIONAL ON 'uriToB/user=baz

For this to work

  • baz requires CREATE on database B, implicitly gets REPLICATIONDEST and REPLICATIONSOURCE on b.tab
  • foo requires REPLICATIONSOURCE and REPLICATIONDEST on A.tab
  • baz must be the user in the BIDIRECTIONAL ON uri.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this example under the table

@katmayb katmayb force-pushed the ldr-privileges-25.2 branch from 29b7b42 to a9b2d1c Compare May 2, 2025 18:52
@katmayb katmayb requested a review from msbutler May 2, 2025 19:30
Copy link

@msbutler msbutler left a comment

Choose a reason for hiding this comment

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

LGTM!

@katmayb katmayb requested a review from rmloveland May 6, 2025 12:45
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!

@katmayb katmayb force-pushed the ldr-privileges-25.2 branch from a9b2d1c to 76b77c7 Compare May 7, 2025 16:29
@katmayb katmayb merged commit 6da99f1 into main May 7, 2025
6 checks passed
@katmayb katmayb deleted the ldr-privileges-25.2 branch May 7, 2025 16:36
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.

4 participants