[Feature] [License] Master Management #1975
Conversation
ajanikow
force-pushed
the
feature/license/master_management
branch
from
836066b to
2dbc103
Compare
ajanikow
force-pushed
the
feature/license/master_management
branch
from
2dbc103 to
4650dec
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements License Activation API Integration, enabling automatic license generation and renewal through the ArangoDB License Manager API. The key changes include:
- Introduction of dual license modes: "key" (static license) and "api" (dynamically generated license)
- New actions for license generation and cleanup
- Support for automatic license regeneration based on expiration grace periods
- Refactoring of license-related code including package renaming from
pkg/license/managertopkg/license_manager
Reviewed Changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/k8sutil/license.go | Added LicenseSecretMaster struct and API field to support license API credentials |
| pkg/util/constants/constants.go | Added constants for license client ID and secret keys |
| pkg/util/cli/lm.go | Updated imports to use renamed license_manager package |
| pkg/util/arangod/request.go | Refactored AcceptCode method to use extracted error evaluation logic |
| pkg/util/arangod/error.go | Added InvalidCode error type and evaluation utilities |
| pkg/apis/deployment/v1/license_spec.go | Added LicenseMode, TTL, ExpirationGracePeriod, Telemetry, and Inventory fields |
| pkg/apis/deployment/v1/deployment_status_license.go | New file defining license status tracking |
| pkg/deployment/reconcile/action_license_generate.go | New action for automatic license generation via API |
| pkg/deployment/reconcile/action_license_clean.go | New action for cleaning license references |
| pkg/deployment/reconcile/plan_builder_license.go | Enhanced license plan builder with mode discovery and API support |
| pkg/platform/inventory/*.go | Refactored inventory fetching into reusable function |
| pkg/license_manager/*.go | Renamed from pkg/license/manager and updated types |
| internal/actions.yaml | Added LicenseGenerate and LicenseClean action definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if expires.After(license.Expires()) { | ||
| // License will expire before grace period, reduce to 90% | ||
| expires = time.Now().Add(time.Duration(math.Round(float64(time.Since(license.Expires())) * api.LicenseExpirationGraceRatio))) |
There was a problem hiding this comment.
The calculation uses time.Since(license.Expires()) which will be negative if the license hasn't expired yet. This should be time.Until(license.Expires()) to get a positive duration representing the time remaining until expiration.
| expires = time.Now().Add(time.Duration(math.Round(float64(time.Since(license.Expires())) * api.LicenseExpirationGraceRatio))) | |
| expires = time.Now().Add(time.Duration(math.Round(float64(time.Until(license.Expires())) * api.LicenseExpirationGraceRatio))) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/platform/inventory/consts_test.go:21
- The file was completely rewritten from a license package with interface definitions to an inventory package with a UUID test. This appears to be an incorrect replacement - the original content (Loader interface and MockLoader) seems unrelated to consts_test.go and may have been misplaced. Verify this file change is intentional.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return api.Plan{actions.NewClusterAction(api.ActionTypeLicenseClean, "Removing license reference - Invalid Hash")} | ||
| } | ||
|
|
||
| if status.License.Regenerate.After(time.Now()) { |
There was a problem hiding this comment.
The condition is inverted - it should trigger regeneration when Regenerate time is before (not after) time.Now(). Currently, it will clean the license when regeneration is NOT yet needed. Change to if status.License.Regenerate.Before(time.Now()) or if !status.License.Regenerate.After(time.Now()).
| if status.License.Regenerate.After(time.Now()) { | |
| if status.License.Regenerate.Before(time.Now()) { |
| expires := time.Now().Add(expiration) | ||
|
|
||
| if expires.After(license.Expires()) { | ||
| // License will expire before grace period, reduce to 90% |
There was a problem hiding this comment.
The comment states 'License will expire before grace period, reduce to 90%' but LicenseExpirationGraceRatio is 0.75 (75%), not 0.90. The comment should be updated to say 'reduce to 75%' or use a named constant for clarity.
| // License will expire before grace period, reduce to 90% | |
| // License will expire before grace period, reduce to 75% |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/platform/inventory/consts_test.go:21
- The test file appears to have been completely replaced with only a UUID validation test. The original file contained interface and mock definitions that may have been used elsewhere. Verify that removing the Loader interface and MockLoader doesn't break other tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if inv.DeploymentId != did { | ||
| a.log.Err(err).Error("Invalid deployment ID in inventory") |
There was a problem hiding this comment.
Error logging references 'err' but the variable 'err' is nil at this point (line 98 shows err is only used for FetchInventorySpec). This will log a nil error. Should either create a proper error or remove the .Err(err) call.
| a.log.Err(err).Error("Invalid deployment ID in inventory") | |
| a.log.Error("Invalid deployment ID in inventory") |
| return api.LicenseModeAPI, nil | ||
| } | ||
|
|
||
| return "", errors.Errorf("Unable to discover LicenseAPIKey mode") |
There was a problem hiding this comment.
Error message says 'LicenseAPIKey mode' but should be 'License mode' for consistency with the rest of the codebase and to avoid confusion.
| return "", errors.Errorf("Unable to discover LicenseAPIKey mode") | |
| return "", errors.Errorf("Unable to discover License mode") |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if l.API == nil { | ||
| return LicenseSecret{}, errors.Errorf("Key (%s, %s, %s, or %s+%s) is missing in the license secret (%s)", | ||
| utilConstants.SecretKeyToken, utilConstants.SecretKeyV2License, utilConstants.SecretKeyV2Token, utilConstants.SecretKeyLicenseClientID, utilConstants.SecretKeyLicenseClientSecret, name) |
There was a problem hiding this comment.
The error message lists individual secret keys but the logic only requires either V2 license keys OR API credentials (not all of them). The message should clarify that it needs 'either (token/license-v2/token-v2) or (client-id + client-secret)' rather than listing all keys with 'or'.
| return api.Plan{actions.NewClusterAction(api.ActionTypeLicenseClean, "Removing license reference - Invalid Hash")} | ||
| } | ||
|
|
||
| if status.License.Regenerate.Time.Before(time.Now()) { |
There was a problem hiding this comment.
The time comparison is inverted. When Regenerate.Time.Before(time.Now()) is true, it means the regeneration time is in the past, so regeneration should be triggered. However, the current logic triggers regeneration when the time is in the past, which is correct. But note that this check should use !After or proper time comparison to handle edge cases. Consider using time.Now().After(status.License.Regenerate.Time) for clarity.
| if status.License.Regenerate.Time.Before(time.Now()) { | |
| if time.Now().After(status.License.Regenerate.Time) { |
| expires := time.Now().Add(expiration) | ||
|
|
||
| if expires.After(license.Expires()) { | ||
| // License will expire before grace period, reduce to 75% |
There was a problem hiding this comment.
The comment on line 166 is misleading. The condition checks if expires.After(license.Expires()), which means the calculated regeneration time is AFTER the license expiration. This is when we need to reduce the time, but the comment says 'License will expire before grace period' which describes the opposite scenario. The comment should say 'Grace period extends beyond license expiration, reduce to 75% of remaining license time'.
| // License will expire before grace period, reduce to 75% | |
| // Grace period extends beyond license expiration, reduce to 75% of remaining license time |
| s.TTL = util.NewTypeOrNil(other.TTL) | ||
| s.ExpirationGracePeriod = util.NewTypeOrNil(other.ExpirationGracePeriod) |
There was a problem hiding this comment.
Lines 143-144 unconditionally overwrite TTL and ExpirationGracePeriod from other regardless of whether they are already set in s. This is inconsistent with the SecretName logic (lines 140-142) which only sets the value if not already present. These fields should follow the same pattern with conditional checks.
| s.TTL = util.NewTypeOrNil(other.TTL) | |
| s.ExpirationGracePeriod = util.NewTypeOrNil(other.ExpirationGracePeriod) | |
| if s.TTL == nil { | |
| s.TTL = util.NewTypeOrNil(other.TTL) | |
| } | |
| if s.ExpirationGracePeriod == nil { | |
| s.ExpirationGracePeriod = util.NewTypeOrNil(other.ExpirationGracePeriod) | |
| } |
| expiration := time.Until(license.Expires()) | ||
| if expiration <= 0 { | ||
| a.log.Error("Unable to get license - invalid timestamp") | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
The error message 'Unable to get license - invalid timestamp' is misleading because the license was successfully retrieved. The issue is that the license has already expired. The message should be 'License has already expired' or 'Invalid license expiration time'.
| if c, ok := arangod.IsInvalidCode(err); ok && c.Got == goHttp.StatusForbidden { | ||
| // Fallback to the Deployment ID Single | ||
| return FixedSingleDeploymentID, nil |
There was a problem hiding this comment.
The comment 'Fallback to the Deployment ID Single' is unclear. It should explain why StatusForbidden triggers a fallback to a fixed deployment ID (e.g., 'Use fixed single deployment ID when cluster health endpoint returns Forbidden (single server mode)').
| func (i InvalidCode) Error() string { | ||
| return fmt.Sprintf("Code %d not allowed in expected status codes: %s", i.Got, goStrings.Join(util.FormatList(i.Expected, func(a int) string { | ||
| return fmt.Sprintf("%d", a) | ||
| }), ", ")) | ||
| } |
There was a problem hiding this comment.
[nitpick] The error message could be more informative by indicating what the expected vs actual codes represent (HTTP status codes). Consider: 'HTTP status code %d not allowed, expected one of: %s'.
No description provided.