feat(secret): alter secret in catalog #19495
Conversation
There was a problem hiding this comment.
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_0andtopic_1,- fill some data, eg. message in
topic_0starts with 0 and message intopic_1starts with 1.
- fill some data, eg. message in
- create a table with kafka connector, using secret to spec the topic to
topic_0- check the data from
topic_0
- check the data from
- 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
| secret.value = encrypted_payload; | ||
| self.metadata_manager | ||
| .catalog_controller | ||
| .alter_secret(secret, secret_plain_payload) |
There was a problem hiding this comment.
why we need a plain secret here?
There was a problem hiding this comment.
To update the local secret manager.
| pub enum AlterSecretOperation { | ||
| ChangeCredential { new_credential: Value }, | ||
| } |
There was a problem hiding this comment.
what else operation can alter secret do?
There was a problem hiding this comment.
SET SCHEMA, change WITH option etc..
| @@ -40,6 +40,23 @@ create secret secret_1 with ( | |||
| backend = 'meta' | |||
| ) as 'demo_secret'; | |||
|
|
|||
| statement ok | |||
| alter secret secret_1 with ( | |||
| backend = 'meta' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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!( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is actually intentional or something we have to do.
- add_secret is called using the
LocalSecretManageron 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
| 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!( |
There was a problem hiding this comment.
Does it trigger the existing actors to upgrade to the new secret?
There was a problem hiding this comment.
Does it trigger the existing actors to upgrade to the new secret?
No. Already doced this limitation in PR.
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
./risedev check(or alias,./risedev c)Documentation
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.