-
Notifications
You must be signed in to change notification settings - Fork 224
fix: nil pointer dereferences in private cluster creation and reconciliation #1591
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
base: main
Are you sure you want to change the base?
fix: nil pointer dereferences in private cluster creation and reconciliation #1591
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pkieszcz The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
|
Welcome @pkieszcz! |
|
Hi @pkieszcz. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
fb148d8 to
12954c9
Compare
salasberryfin
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.
/ok-to-test
Fixes two nil pointer dereference issues when creating/reconciling private GKE clusters: 1. Creation path: Initialize NetworkConfig before accessing DefaultEnablePrivateNodes. Also set EnablePrivateNodes on PrivateClusterConfig to match (GCP SDK requires both to be equal). 2. Reconciliation path: Initialize DesiredControlPlaneEndpointsConfig and IpEndpointsConfig before assigning AuthorizedNetworksConfig in checkDiffAndPrepareUpdate. Both issues occur when using private clusters with PSC (Private Service Connect) mode, i.e., enablePrivateEndpoint: true without specifying controlPlaneCidrBlock. Signed-off-by: Piotr Kieszczyński <piotr.kieszczynski@gmail.com>
12954c9 to
b42c715
Compare
salasberryfin
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.
Thanks @pkieszcz. It would also be nice to have a test for this specific reconcile loop, as it would make it easy to spot simple errors like this one.
| // DesiredMasterAuthorizedNetworksConfig | ||
| // When desiredMasterAuthorizedNetworksConfig is nil, it means that the user wants to disable the feature. | ||
| desiredMasterAuthorizedNetworksConfig := convertToSdkMasterAuthorizedNetworksConfig(s.scope.GCPManagedControlPlane.Spec.MasterAuthorizedNetworksConfig) | ||
| if !compareMasterAuthorizedNetworksConfig(desiredMasterAuthorizedNetworksConfig, existingCluster.GetControlPlaneEndpointsConfig().GetIpEndpointsConfig().GetAuthorizedNetworksConfig()) { |
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.
It's probably worth changing this too:
existingCluster.GetControlPlaneEndpointsConfig().GetIpEndpointsConfig().GetAuthorizedNetworksConfig()compareMasterAuthorizedNetworksConfig() deals with nil values and it's safe to use but what if any of the intermediate methods on existingCluster returns nil?
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes two nil pointer dereference issues when creating/reconciling private GKE clusters with PSC (Private Service Connect) mode - i.e., when
enablePrivateEndpoint: truewithout specifyingcontrolPlaneCidrBlock.Fix 1: Creation path (related to #1503)
In
createCluster,cluster.NetworkConfig.DefaultEnablePrivateNodeswas accessed beforeNetworkConfigwas initialized.Changes:
NetworkConfigbefore accessingDefaultEnablePrivateNodesEnablePrivateNodesonPrivateClusterConfigto match (GCP SDK requires both fields to have the same value)Fix 2: Reconciliation path
In
checkDiffAndPrepareUpdate,clusterUpdate.DesiredControlPlaneEndpointsConfig.IpEndpointsConfig.AuthorizedNetworksConfigwas assigned without initializing the parent structs.Changes:
DesiredControlPlaneEndpointsConfigandIpEndpointsConfigbefore assigningAuthorizedNetworksConfigWhich issue(s) this PR fixes:
Fixes #1497
Special notes for your reviewer:
This PR combines the fix from #1503 (which was closed) with an additional fix for the reconciliation path. Both issues have the same root cause and affect private cluster functionality.
Reproduction steps:
enablePrivateEndpoint: trueand nocontrolPlaneCidrBlock(PSC mode)master_authorized_networks_configdiffersTODOs:
Release note: