Skip to content
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

fix nil panic when SecurityProfile is nil #23392

Closed
wants to merge 1 commit into from
Closed

fix nil panic when SecurityProfile is nil #23392

wants to merge 1 commit into from

Conversation

yangzuo0621
Copy link

As not all clusters have property SecurityProfile, when SecurityProfile is nil, it will panic with nil pointer.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @yangzuo0621.

I have a request to change where we nil check SecurityProfile. If you can take a look at that when we can give this another look and get this merged.

Thanks!

Comment on lines +2619 to 2622
if props.SecurityProfile == nil {
props.SecurityProfile = &managedclusters.ManagedClusterSecurityProfile{}
}
customCaTrustCertList := flattenCustomCaTrustCerts(props.SecurityProfile.CustomCATrustCertificates)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We nil check SecurityProfile at multiple points in the Read where we set security property values into state.

For consistency could we actually update flattenCustomCaTrustCerts to do the nil check instead of doing a general one here

Suggested change
if props.SecurityProfile == nil {
props.SecurityProfile = &managedclusters.ManagedClusterSecurityProfile{}
}
customCaTrustCertList := flattenCustomCaTrustCerts(props.SecurityProfile.CustomCATrustCertificates)
customCaTrustCertList := flattenCustomCaTrustCerts(props.SecurityProfile)

flattenCustomCaTrustCerts would then become

func flattenCustomCaTrustCerts(input *managedclusters.ManagedClusterSecurityProfile) []interface{} {
	if input == nil || input.CustomCATrustCertificates == nil {
		return make([]interface{}, 0)
	}

	customCaTrustCertInterface := make([]interface{}, len(*input.CustomCATrustCertificates))

	for index, value := range *input.CustomCATrustCertificates {
		customCaTrustCertInterface[index] = value
	}

	return customCaTrustCertInterface
}

@github-actions
Copy link

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

@stephybun
Copy link
Member

Since it's been over a month I looked into finalising this PR for you but stumbled onto some other issues with the custom_ca_trust_certificates_base64 property. As a result I ended up opening a PR to make the changes to the nil check as well as fix the behaviour in the provider for this property.

I'm going to close this PR in favour of #23737. Thanks again for your contribution.

@stephybun stephybun closed this Oct 31, 2023
@github-actions github-actions bot removed the stale label Oct 31, 2023
@yangzuo0621 yangzuo0621 deleted the zuya/fix-nil branch February 6, 2024 06:42
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants