-
Notifications
You must be signed in to change notification settings - Fork 39
fix: Fix nil pointer in Cluster #1358
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR aims to fix a nil pointer deference issue during Cluster creation by adding appropriate nil checks.
- Corrects a mistaken nil check on the label’s Value field.
- Adds a nil check for AdvancedSettings to prevent nil pointer issues.
- Ensures proper pointer handling for CustomOpensslCipherConfigTls12 in the advanced configuration.
Comments suppressed due to low confidence (2)
cfn-resources/cluster/cmd/resource/mappings.go:204
- Changing the check from labels[i].Key to labels[i].Value correctly prevents a nil pointer dereference when accessing the Value field. Confirm that similar nil checks are applied consistently in related logic if necessary.
if labels[i].Value != nil {
cfn-resources/cluster/cmd/resource/mappings.go:619
- The added nil check for processArgs.CustomOpensslCipherConfigTls12 ensures safe pointer handling. Please verify that pointer assignments follow the same pattern consistently across similar configurations.
if processArgs.CustomOpensslCipherConfigTls12 != nil {
| clusterRequest.TerminationProtectionEnabled = currentModel.TerminationProtectionEnabled | ||
|
|
||
| clusterRequest.AdvancedConfiguration = expandClusterAdvancedConfiguration(*currentModel.AdvancedSettings) | ||
| if currentModel.AdvancedSettings != nil { |
Copilot
AI
Jun 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the nil check for currentModel.AdvancedSettings prevents a potential nil pointer deference during cluster creation. This approach improves the robustness of the cluster setup.
oarbusi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!!
| clusterRequest.TerminationProtectionEnabled = currentModel.TerminationProtectionEnabled | ||
|
|
||
| clusterRequest.AdvancedConfiguration = expandClusterAdvancedConfiguration(*currentModel.AdvancedSettings) | ||
| if currentModel.AdvancedSettings != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a contract test which does not define AdvancedSettings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ticket created: CLOUDP-321624
Proposed changes
Fix nil pointer in Cluster. Cluster creation would fail for cluster configs without
AdvancedSettings.Fix regression introduced in #1344.
Issue was not caught in E2E or contract tests because they use
AdvancedSettings.Jira ticket: CLOUDP-321622
Type of change:
expected)
Manual QA performed:
Required Checklist:
make fmtand formatted my codeworks in Atlas
Further comments