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

kubernetes_cluster_resource support network_profile.ip_versions #16088

Conversation

qiqingzhang
Copy link
Contributor

@qiqingzhang qiqingzhang commented Mar 28, 2022

create a public pull request for kubernetes_cluster_resource support network_profile.ip_families

Link to #15708

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 @heiazuo, this is off to a good start. Aside from the comments left in-line I also have two additional suggestions:

  1. We should create an additional test that tests dual-stack networking with the value ["IPv4", "IPv6"]
  2. In the create method could we add a check that throws an error if a user tries to specify ["IPv6"], the validation for the property allows it, but it isn't valid for the API.

Once the above is addressed we can take another look through this.

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
@qiqingzhang qiqingzhang force-pushed the branch-add_ip_families_to_kubernetes_cluster branch from b71dce8 to b58db9c Compare April 1, 2022 06:13
@qiqingzhang
Copy link
Contributor Author

Thanks @stephybun, thanks for your suggestion, I made the changes as suggested

@stephybun stephybun changed the title kubernetes_cluster_resource support network_profile.ip_families kubernetes_cluster_resource support network_profile.ip_versions Apr 1, 2022
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 renaming the property and adding the test for dual-stack networking @heiazuo, I still have a question and suggestion regarding the check on the value.

Comment on lines 2380 to 2390
func expandIPVersions(input []interface{}) *[]containerservice.IPFamily {
if len(input) == 0 {
return nil
}
ipf := make([]containerservice.IPFamily, 0)
for _, data := range input {
ipf = append(ipf, containerservice.IPFamily(data.(string)))
}

return &ipf
}
Copy link
Member

Choose a reason for hiding this comment

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

Like so

Suggested change
func expandIPVersions(input []interface{}) *[]containerservice.IPFamily {
if len(input) == 0 {
return nil
}
ipf := make([]containerservice.IPFamily, 0)
for _, data := range input {
ipf = append(ipf, containerservice.IPFamily(data.(string)))
}
return &ipf
}
func expandIPVersions(input []interface{}) (*[]containerservice.IPFamily, error) {
if len(input) == 0 {
return nil, nil
}
ipf := make([]containerservice.IPFamily, 0)
for _, data := range input {
ipf = append(ipf, containerservice.IPFamily(data.(string)))
}
if len(ipf) == 1 && ipf[0] == containerservice.IPFamilyIPv6 {
return nil, fmt.Errorf("`ip_versions` must be `IPv4` or `IPv4` and `IPv6`. `IPv6` alone is not supported")
}
return &ipf, nil
}

Comment on lines 2296 to 2304
ipVersions := config["ip_versions"].([]interface{})

networkProfile := containerservice.NetworkProfile{
NetworkPlugin: containerservice.NetworkPlugin(networkPlugin),
NetworkMode: containerservice.NetworkMode(networkMode),
NetworkPolicy: containerservice.NetworkPolicy(networkPolicy),
LoadBalancerSku: containerservice.LoadBalancerSku(loadBalancerSku),
OutboundType: containerservice.OutboundType(outboundType),
IPFamilies: expandIPVersions(ipVersions),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ipVersions := config["ip_versions"].([]interface{})
networkProfile := containerservice.NetworkProfile{
NetworkPlugin: containerservice.NetworkPlugin(networkPlugin),
NetworkMode: containerservice.NetworkMode(networkMode),
NetworkPolicy: containerservice.NetworkPolicy(networkPolicy),
LoadBalancerSku: containerservice.LoadBalancerSku(loadBalancerSku),
OutboundType: containerservice.OutboundType(outboundType),
IPFamilies: expandIPVersions(ipVersions),
ipVersions, err := expandIPVersions(config["ip_versions"].([]interface{}))
if err != nil {
return nil, err
}
networkProfile := containerservice.NetworkProfile{
NetworkPlugin: containerservice.NetworkPlugin(networkPlugin),
NetworkMode: containerservice.NetworkMode(networkMode),
NetworkPolicy: containerservice.NetworkPolicy(networkPolicy),
LoadBalancerSku: containerservice.LoadBalancerSku(loadBalancerSku),
OutboundType: containerservice.OutboundType(outboundType),
IPFamilies: ipVersions,
}

2:add ip_versions`IPv6` alone check logic at expandIPVersions function
@github-actions github-actions bot added size/M and removed size/L labels Apr 2, 2022
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 @heiazuo, LGTM 🥟

@stephybun stephybun merged commit bfaaefd into hashicorp:main Apr 5, 2022
@github-actions github-actions bot added this to the v3.1.0 milestone Apr 5, 2022
stephybun added a commit that referenced this pull request Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

This functionality has been released in v3.1.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented May 9, 2022

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 May 9, 2022
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