-
Notifications
You must be signed in to change notification settings - Fork 44
Better NPE handling of updateComputeConfig #182
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?
Better NPE handling of updateComputeConfig #182
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kunhwiko The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @kunhwiko. Thanks for your PR. I'm waiting for a aws-controllers-k8s 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. Instructions 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/test-infra repository. |
defer exit(err) | ||
|
||
// Convert []*string to []string for NodePools | ||
nodePools := make([]string, 0, len(r.ko.Spec.ComputeConfig.NodePools)) |
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.
So updateComputeConfig
seems to be triggered when one of these conditions are met:
- delta.DifferentAt("Spec.ComputeConfig")
- delta.DifferentAt("Spec.StorageConfig")
- delta.DifferentAt("Spec.KubernetesNetworkConfig")
As an example here, users that aren't using EKS auto mode might be using Spec.KubernetesNetworkConfig
but are likely to leave Spec.ComputeConfig
and Spec.StorageConfig
empty. This leads to a NilPointerException.
11028df
to
b19ab8f
Compare
dcfd48a
to
f8c7d61
Compare
/ok-to-test |
f8c7d61
to
f433496
Compare
f433496
to
de98d00
Compare
latestConfig := ko.Spec.KubernetesNetworkConfig | ||
|
||
// ElasticLoadBalancing can by default be initialized as false even when ACK is providing an empty input. | ||
// This condition prevents unnecessary deltas when the desired value is empty and ElasticLoadBalancing is already disabled. | ||
if latestConfig != nil && latestConfig.ElasticLoadBalancing != nil && latestConfig.ElasticLoadBalancing.Enabled != nil { | ||
if *latestConfig.ElasticLoadBalancing.Enabled == false && r.ko.Spec.KubernetesNetworkConfig.ElasticLoadBalancing == nil { | ||
latestConfig.ElasticLoadBalancing = 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.
One thing that I noticed is even with an empty elasticloadbalancing input:
spec:
accessConfig:
authenticationMode: CONFIG_MAP
kubernetesNetworkConfig:
ipFamily: ipv4
serviceIPv4CIDR: 172.20.0.0/14
The AWS API sometimes is returning this field initialized with some default:
aws eks describe-cluster --name <cluster-name>
"kubernetesNetworkConfig": {
"serviceIpv4Cidr": "172.20.0.0/14",
"ipFamily": "ipv4",
"elasticLoadBalancing": {
"enabled": false
}
},
This is causing a delta to always occur between desired (i.e. user input) / latest (i.e. fetched from AWS). Anyways either nil or false here should indicate we don't want EKS auto mode to be turned on.
go_version: go1.24.0 | ||
version: v0.33.0-102-g3756200 |
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.
Something wrong with my local setup..?
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.
You just need to pull the git tags
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.
@a-hilaly That did it :D Any tips for the go version as well?...
/test eks-kind-e2e |
d7d3ecd
to
c648716
Compare
80bcdab
to
6569a97
Compare
@kunhwiko: The following test failed, say
Full PR test history. Your PR dashboard. Instructions 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/test-infra repository. I understand the commands that are listed here. |
Issue #, if available:
N/A
Description of changes:
updateComputeConfig
seems to currently be susceptible to NPE errors, especially when the user isn't using EKS Auto Mode.The function is executed when any one of these conditions are met:
There is no guarantee that all 3 fields are fully populated, so we may need to gracefully handle some inputs.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.