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

feat(meta): watch and reload license key from file #18768

Merged
merged 10 commits into from
Oct 9, 2024
Merged

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Sep 30, 2024

Signed-off-by: Bugen Zhao [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Add a CLI argument of --license-key-file for the meta node, which enables a background task to watch and reload license key from that file, simplifying license key rotation in Cloud.

Some notes:

  • Only take effect on the meta node, as the license key will still be propagated through system parameters to all other nodes.
  • Can also be configured with env var RW_LICENSE_KEY_PATH.
  • Manually specifying the license key via system parameters (license_key), initial configuration (system.license_key), or the RW_LICENSE_KEY environment variable is still possible but not recommended when using --license-key-file, as it can easily get overwritten if the key file's content changes will be rejected when --license-key-file is specified. (see refactor(meta): reject directly setting license key when it's managed by watching a file #18823)

Added a unit test.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

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

Release note

See description above.

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao added the user-facing-changes Contains changes that are visible to users label Sep 30, 2024
@BugenZhao BugenZhao requested a review from a team as a code owner September 30, 2024 09:48
Copy link
Member

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Bugen Zhao <[email protected]>
@fuyufjh
Copy link
Member

fuyufjh commented Oct 9, 2024

Manually specifying the license key via system parameters (license_key), initial configuration (system.license_key), or the RW_LICENSE_KEY environment variable is still possible but not recommended when using --license-key-file, as it can easily get overwritten if the key file's content changes.

How about adding an error message for ALTER SYSTEM SET license_key TO '...' to notify users they are using it incorrectly?

@BugenZhao BugenZhao added this pull request to the merge queue Oct 9, 2024
Merged via the queue into main with commit 2478845 Oct 9, 2024
30 of 31 checks passed
@BugenZhao BugenZhao deleted the bz/watch-license-key branch October 9, 2024 05:20
@BugenZhao
Copy link
Member Author

How about adding an error message for ALTER SYSTEM SET license_key TO '...' to notify users they are using it incorrectly?

Oops. Missed this. Will refine in the following PR.

@@ -192,6 +193,10 @@ pub struct MetaNodeOpts {
#[override_opts(path = system.license_key)]
pub license_key: Option<LicenseKey>,

/// The path of the license key file to be watched and hot-reloaded.
#[clap(long, env = "RW_LICENSE_KEY_PATH")]
Copy link
Contributor

Choose a reason for hiding this comment

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

typo found 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed with @BugenZhao elsewhere, here's the conclusion: RW_LICENSE_KEY_PATH will be kept instead of being changed to the RW_LICENSE_KEY_FILE in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants