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: support license #709

Merged
merged 10 commits into from
Sep 5, 2024
Merged

feat: support license #709

merged 10 commits into from
Sep 5, 2024

Conversation

fuyufjh
Copy link
Member

@fuyufjh fuyufjh commented Sep 3, 2024

What's changed and what's your intention?

To use a license, users need to

  1. store the license (JWT Token) in K8s Secret with key license
  2. set the RisingWave CRD .license.secretName to the secret name

Checklist

  • I have written the necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@fuyufjh fuyufjh requested a review from a team as a code owner September 3, 2024 10:08
@fuyufjh
Copy link
Member Author

fuyufjh commented Sep 3, 2024

@arkbriar make generate-all it will make such a diff. Is it expected or did I did anything wrong?

image

@arkbriar
Copy link
Collaborator

arkbriar commented Sep 4, 2024

@arkbriar make generate-all it will make such a diff. Is it expected or did I did anything wrong?

image

Could you try remove the local bin dir and try again?

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

The kernel uses the term of license_key and accepts env var of RW_LICENSE_KEY to initialize the system parameter. Shall we unify the terms here?

type RisingWaveLicense struct {
// SecretName that contains the license. The license must be JWT formatted JSON.
// +optional
SecretName string `json:"secretName,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mark it a required field.

Suggested change
SecretName string `json:"secretName,omitempty"`
SecretName string `json:"secretName"`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also clarify the secret key that will be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mark it a required field.

Can you explain the motivation? I learned from

SecretName string `json:"secretName,omitempty"`

Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably makes the semantic more clear. The example you quoted seems untuned 😄

Currently,

# valid but has no effect
spec:
  license:
    secretName: ""
# valid and effective
spec:
  license:
    secretName: "abc"
# valid and won't be misunderstood
spec:
  # no license field

After change, the k8s API service will reject

# no longer valid
spec:
  license:
    secretName: ""

Copy link
Member Author

@fuyufjh fuyufjh Sep 5, 2024

Choose a reason for hiding this comment

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

Either LGTM, but I'd like to keep it consistent with TLS config (RisingWaveTLSConfiguration). Do you think we need to change it as well?

pkg/factory/risingwave_object_factory.go Outdated Show resolved Hide resolved
pkg/factory/risingwave_object_factory.go Show resolved Hide resolved

package v1alpha1

const RisingWaveLicenseLicenseSecretKey = "licenseKey"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const RisingWaveLicenseLicenseSecretKey = "licenseKey"
const RisingWaveLicenseKeySecretKey = "licenseKey"

? 🤔

fuyufjh and others added 2 commits September 4, 2024 17:49
Signed-off-by: arkbriar <arkbriar@gmail.com>
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 58.82353% with 14 lines in your changes missing coverage. Please review.

Project coverage is 55.03%. Comparing base (c6da95a) to head (25ffc48).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
apis/risingwave/v1alpha1/zz_generated.deepcopy.go 0.00% 13 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
+ Coverage   55.01%   55.03%   +0.01%     
==========================================
  Files          40       40              
  Lines        6645     6679      +34     
==========================================
+ Hits         3656     3676      +20     
- Misses       2864     2877      +13     
- Partials      125      126       +1     
Flag Coverage Δ
unittests 55.03% <58.82%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@arkbriar arkbriar left a comment

Choose a reason for hiding this comment

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

LGTM!

@arkbriar arkbriar added this pull request to the merge queue Sep 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 5, 2024
@arkbriar arkbriar added this pull request to the merge queue Sep 5, 2024
Merged via the queue into main with commit c2a24ad Sep 5, 2024
10 checks passed
@arkbriar arkbriar deleted the eric/feat_license branch September 5, 2024 08:57
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