KMIP Upgrade - Fix for empty StringData after calling KubeUpdate#1654
KMIP Upgrade - Fix for empty StringData after calling KubeUpdate#1654jackyalbo merged 1 commit intonoobaa:masterfrom
Conversation
Signed-off-by: jackyalbo <jacky.albo@gmail.com>
WalkthroughThe update introduces explicit validation of the Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/kms/kms_kmip_storage.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/util/kms/kms_kmip_storage.go (1)
pkg/util/util.go (1)
KubeCheck(562-583)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-cli-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-hac-test
- GitHub Check: auto-update
- GitHub Check: cnpg-deployment-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-key-rotate-test
🔇 Additional comments (2)
pkg/util/kms/kms_kmip_storage.go (2)
422-422: Good improvement to error logging.The enhanced error message now includes both the missing key name (
lookfor) and the secret name (k.secret.Name), providing much better diagnostic information compared to the previous version.
267-267: KMIP secret storage now calls KubeCheck before accessing StringDataAll code paths in pkg/util/kms/kms_kmip_storage.go that read from k.secret.StringData after an update now invoke util.KubeCheck(k.secret), ensuring StringData is repopulated:
- GetSecret (around line 267): added
util.KubeCheck(k.secret)before readingactiveKeyID/uniqueIdentifier- DeleteSecret (around line 419): added
util.KubeCheck(k.secret)before locating the key IDNo other flows in this file read StringData post‐update, so the root cause is fully addressed.
| lookfor = NewKMIPUniqueID | ||
| } | ||
| // Find the key ID | ||
| util.KubeCheck(k.secret) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling KubeCheck return value for consistency.
Similar to the GetSecret method, the KubeCheck return value should be handled for better error handling and consistency.
Consider handling the KubeCheck return value:
- util.KubeCheck(k.secret)
+ if !util.KubeCheck(k.secret) {
+ log.Errorf("KMIPSecretStorage.DeleteSecret() failed to validate secret %v", k.secret.Name)
+ return fmt.Errorf("failed to validate secret %v", k.secret.Name)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| util.KubeCheck(k.secret) | |
| - util.KubeCheck(k.secret) | |
| + if !util.KubeCheck(k.secret) { | |
| + log.Errorf("KMIPSecretStorage.DeleteSecret() failed to validate secret %v", k.secret.Name) | |
| + return fmt.Errorf("failed to validate secret %v", k.secret.Name) | |
| + } |
🤖 Prompt for AI Agents
In pkg/util/kms/kms_kmip_storage.go at line 419, the call to
util.KubeCheck(k.secret) currently ignores its return value. To improve error
handling and maintain consistency with the GetSecret method, capture the return
value from KubeCheck and handle any error it returns appropriately, such as
returning the error or logging it, depending on the function context.
|
|
||
| lookfor := KMIPUniqueID // Addition to upgrade | ||
| var activeKeyID string | ||
| util.KubeCheck(k.secret) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling KubeCheck return value for robustness.
The KubeCheck call correctly addresses the StringData restoration issue described in the PR objectives. However, the return value (boolean) is not being checked, which could lead to proceeding with operations even if the secret validation fails.
Consider handling the KubeCheck return value:
- util.KubeCheck(k.secret)
+ if !util.KubeCheck(k.secret) {
+ log.Errorf("KMIPSecretStorage.GetSecret() failed to validate secret %v", k.secret.Name)
+ return nil, secrets.NoVersion, fmt.Errorf("failed to validate secret %v", k.secret.Name)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| util.KubeCheck(k.secret) | |
| if !util.KubeCheck(k.secret) { | |
| log.Errorf("KMIPSecretStorage.GetSecret() failed to validate secret %v", k.secret.Name) | |
| return nil, secrets.NoVersion, fmt.Errorf("failed to validate secret %v", k.secret.Name) | |
| } |
🤖 Prompt for AI Agents
In pkg/util/kms/kms_kmip_storage.go at line 267, the call to
util.KubeCheck(k.secret) returns a boolean that indicates success or failure,
but this return value is currently ignored. Modify the code to check the boolean
result and handle the failure case appropriately, such as returning an error or
aborting further processing if the check fails, to ensure robustness and prevent
proceeding with invalid secrets.
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit