feat(secret): alter secret in catalog #19495

yuhao-su · 2024-11-20T20:12:22Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR introduced alter secret in the catalog. But for secrets used in running jobs, they will not be changed until the executor has been rebuilt (by restarting the cluster)

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

You can alter a secret value by using ALTER SECRET your_secret_name WITH (backend = 'meta') AS 'your_new_secret'.

Note that altering a secret will not result in a change of secret being used in a running job until the job is restarted (e.g. by restarting the cluster).

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

…hao/alter-secret-catalog

tabVersion

The impl LGTM, please add an e2e check if we can apply a secret change to running actor by calling recover.

my thoughts about the test

two kafka topic with 1 partition: topic_0 and topic_1,
- fill some data, eg. message in topic_0 starts with 0 and message in topic_1 starts with 1.
create a table with kafka connector, using secret to spec the topic to topic_0
- check the data from topic_0
pause the cluster, alter the secret pointing to topic_1, pushing new data to both topics, then resume the cluster
check the new data is from topic_1

tabVersion · 2024-11-21T07:48:20Z

src/meta/src/rpc/ddl_controller.rs

+        secret.value = encrypted_payload;
+        self.metadata_manager
+            .catalog_controller
+            .alter_secret(secret, secret_plain_payload)


why we need a plain secret here?

To update the local secret manager.

tabVersion · 2024-11-21T07:49:06Z

src/sqlparser/src/ast/ddl.rs

+pub enum AlterSecretOperation {
+    ChangeCredential { new_credential: Value },
+}


what else operation can alter secret do?

SET SCHEMA, change WITH option etc..

fuyufjh · 2024-11-21T08:13:34Z

e2e_test/ddl/secret.slt

@@ -40,6 +40,23 @@ create secret secret_1 with (
  backend = 'meta'
 ) as 'demo_secret';

+statement ok
+alter secret secret_1 with (
+  backend = 'meta'


The backend option should not appear in the alter secret statement. To follow the convention of other alter commands, only the changed part should be provided. Here the backend option is obviously untouched, so users should not write it here.

But, furthermore, I think backend shouldn't be a per-secret option. In my mind it should be a global-wise config and can't be changed after cluster initialized.

But, furthermore, I think backend shouldn't be a per-secret option. In my mind it should be a global-wise config and can't be changed after cluster initialized.

It's a bit off-topic. We can create a new issue

I understand the concern here. I was thinking the same. But there are 2 problems

We are doing something unusual here. The backend info in with option is also encrypted. We need to decrypt the original secret to write the new one. I can do this.

For hashvault, there is no value we can alter, we can only alter the with. For meta backend, user can alter the secret to using hashvault or just the value. So I try to ask user to write both WITH and AS so they know what they are doing.

So anyway, I think get rid of the WITH here is also a good choice. Will change this.

fuyufjh

Generally LGTM

fuyufjh · 2024-11-21T09:50:57Z

src/common/secret/src/secret_manager.rs

@@ -74,7 +74,23 @@ impl LocalSecretManager {

    pub fn add_secret(&self, secret_id: SecretId, secret: Vec<u8>) {
        let mut secret_guard = self.secrets.write();
-        secret_guard.insert(secret_id, secret);
+        if secret_guard.insert(secret_id, secret).is_some() {
+            tracing::error!(


Good point. I tend to make it a real error i.e. return Err(...) and reject the add_secret

The status quo is somehow weird to me - an error log is printed, but the action actually succeeded.

This is actually intentional or something we have to do.

add_secret is called using the LocalSecretManager on each worker node asynchronizly by notification service, so we can't return an error just like any other notification serivces.

alter secret result was already persisted to meta before we call add_secret. So returning an error here can confuse users unless we decide to roll back the meta commit. Besides, this secret_id cames from meta catalog, so we should trust the data from meta instead of LocalSecretManager

It is still a little weird to me. I understand the LocalSecretManager cannot reject adding a new secret but we can always do such check on the meta node.

fuyufjh · 2024-11-21T09:53:20Z

src/common/secret/src/secret_manager.rs

+    pub fn update_secret(&self, secret_id: SecretId, secret: Vec<u8>) {
+        let mut secret_guard = self.secrets.write();
+        if secret_guard.insert(secret_id, secret).is_none() {
+            tracing::error!(


Does it trigger the existing actors to upgrade to the new secret?

Does it trigger the existing actors to upgrade to the new secret?

No. Already doced this limitation in PR.

yuhao-su · 2024-11-26T02:43:32Z

we can apply a secret change to running actor by calling recover

Added the test in CDC test. A potential problem with your proposal is those 2 topics won't share the offset and it's not a real use case.

tabVersion · 2024-11-26T04:00:40Z

we can apply a secret change to running actor by calling recover

Added the test in CDC test. A potential problem with your proposal is those 2 topics won't share the offset and it's not a real use case.

Yes, a little hack here. Anyway, it is good to have a test here.

tabVersion · 2024-11-26T04:12:53Z

src/common/secret/src/secret_manager.rs

@@ -74,7 +74,23 @@ impl LocalSecretManager {

    pub fn add_secret(&self, secret_id: SecretId, secret: Vec<u8>) {
        let mut secret_guard = self.secrets.write();
-        secret_guard.insert(secret_id, secret);
+        if secret_guard.insert(secret_id, secret).is_some() {
+            tracing::error!(


It is still a little weird to me. I understand the LocalSecretManager cannot reject adding a new secret but we can always do such check on the meta node.

tabVersion · 2024-11-26T04:13:51Z

src/meta/src/controller/catalog.rs

+        let secret: secret::ActiveModel = pb_secret.clone().into();
+        Secret::update(secret).exec(&txn).await?;


Maybe do a secret existance check here?

Already checked in frontend. But I guess it won't hurt to check again on meta.

yuhao-su added 3 commits November 20, 2024 11:18

init

ea6821d

Merge branch 'main' of github.com:singularity-data/risingwave into yu…

3cef5fb

…hao/alter-secret-catalog

refine

b784e27

github-actions bot added type/feature ci/run-e2e-single-node-tests ci/run-e2e-test-other-backends labels Nov 20, 2024

yuhao-su added 2 commits November 20, 2024 14:24

fix

fd51880

fix

61b57f7

graphite-app bot requested a review from a team November 20, 2024 20:50

fix

dc6d3d8

graphite-app bot requested a review from a team November 21, 2024 02:42

yuhao-su requested review from fuyufjh, tabVersion and st1page November 21, 2024 03:34

tabVersion reviewed Nov 21, 2024

View reviewed changes

fuyufjh reviewed Nov 21, 2024

View reviewed changes

fuyufjh approved these changes Nov 22, 2024

View reviewed changes

yuhao-su added 3 commits November 25, 2024 14:53

add recover in test

90377f1

allow no with

c7b2c11

inc mysql backend e2e timeout

b7624e7

yuhao-su requested review from tabVersion and fuyufjh November 26, 2024 02:44

tabVersion reviewed Nov 26, 2024

View reviewed changes

st1page approved these changes Nov 26, 2024

View reviewed changes

yuhao-su added 3 commits November 26, 2024 08:59

check secret exist on meta

bde3335

conflict

8b12e4a

fmt

a8e2a5a

refine

d28fc1e

yuhao-su enabled auto-merge November 26, 2024 15:49

yuhao-su added this pull request to the merge queue Nov 26, 2024

Merged via the queue into main with commit 5070856 Nov 26, 2024
33 of 34 checks passed

yuhao-su deleted the yuhao/alter-secret-catalog branch November 26, 2024 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(secret): alter secret in catalog #19495

feat(secret): alter secret in catalog #19495

yuhao-su commented Nov 20, 2024 •

edited

Loading

tabVersion left a comment

tabVersion Nov 21, 2024

yuhao-su Nov 21, 2024

tabVersion Nov 21, 2024

yuhao-su Nov 21, 2024

fuyufjh Nov 21, 2024

fuyufjh Nov 21, 2024

yuhao-su Nov 21, 2024 •

edited

Loading

fuyufjh left a comment

fuyufjh Nov 21, 2024

yuhao-su Nov 21, 2024 •

edited

Loading

tabVersion Nov 26, 2024

fuyufjh Nov 21, 2024

fuyufjh Nov 21, 2024

yuhao-su Nov 21, 2024

yuhao-su commented Nov 26, 2024 •

edited

Loading

tabVersion commented Nov 26, 2024

tabVersion Nov 26, 2024

tabVersion Nov 26, 2024

yuhao-su Nov 26, 2024 •

edited

Loading

		let secret: secret::ActiveModel = pb_secret.clone().into();
		Secret::update(secret).exec(&txn).await?;

feat(secret): alter secret in catalog #19495

feat(secret): alter secret in catalog #19495

Conversation

yuhao-su commented Nov 20, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

tabVersion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

fuyufjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su commented Nov 26, 2024 • edited Loading

tabVersion commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

yuhao-su commented Nov 20, 2024 •

edited

Loading

yuhao-su Nov 21, 2024 •

edited

Loading

yuhao-su Nov 21, 2024 •

edited

Loading

yuhao-su commented Nov 26, 2024 •

edited

Loading

yuhao-su Nov 26, 2024 •

edited

Loading