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

AKS: Support outbound IPs when using standard loadbalancer #4400

Closed
wants to merge 6 commits into from

Conversation

@evenh
Copy link
Contributor

evenh commented Sep 20, 2019

This PR implements #4322.

Fixes #4322

@hashibot hashibot bot added size/XXL dependencies and removed size/XL labels Sep 23, 2019
Copy link
Contributor

tombuildsstuff left a comment

hey @evenh

Thanks for this PR - taking a look through this mostly looks good - if we can clarify/fix the comments (and the tests pass) then this otherwise LGTM 👍

Thanks!

azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff added this to the v1.35.0 milestone Sep 23, 2019
@hashibot hashibot bot added size/XL and removed size/XXL labels Sep 23, 2019
@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Sep 23, 2019

Thanks for the review @tombuildsstuff! I've pushed a commit to address your comments.

Copy link
Contributor

tombuildsstuff left a comment

hey @evenh

Thanks for pushing those changes - taking a look through this mostly LGTM - if we can fix up the remaining comments (and the tests pass) this should otherwise be good to merge 👍

Thanks!

azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Sep 26, 2019

Fixes applied @tombuildsstuff 👍

@hashibot hashibot bot removed the waiting-response label Sep 26, 2019
@tombuildsstuff tombuildsstuff self-assigned this Oct 1, 2019
@tombuildsstuff

This comment has been minimized.

Copy link
Contributor

tombuildsstuff commented Oct 1, 2019

@evenh unfortunately I've merged a PR which introduced conflicts into this PR - apologies. I hope you don't mind but since this otherwise LGTM I'm going to push a commit to rebase this / fix the merge conflicts so that we can get this merged 👍

@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Oct 1, 2019

No worries. Go ahead @tombuildsstuff :-)


if profile.LoadBalancerProfile.OutboundIPs != nil {
if profile.LoadBalancerProfile.OutboundIPs.PublicIPs != nil {
lb["outbound_ip_address_ids"] = profile.LoadBalancerProfile.OutboundIPs.PublicIPs

This comment has been minimized.

Copy link
@tombuildsstuff

tombuildsstuff Oct 1, 2019

Contributor

taking a closer look into this since this returns a *[]ResourceReference we'll need to flatten this to a []string so that this sets correctly - I hope you don't mind but I'm going to do this as a part of the rebase


if profile.LoadBalancerProfile.OutboundIPPrefixes != nil {
if profile.LoadBalancerProfile.OutboundIPPrefixes.PublicIPPrefixes != nil {
lb["outbound_ip_prefixes_ids"] = profile.LoadBalancerProfile.OutboundIPPrefixes.PublicIPPrefixes

This comment has been minimized.

Copy link
@tombuildsstuff

tombuildsstuff Oct 1, 2019

Contributor

taking a closer look into this since this returns a *[]ResourceReference we'll need to flatten this to a []string so that this sets correctly - I hope you don't mind but I'm going to do this as a part of the rebase

@tombuildsstuff

This comment has been minimized.

Copy link
Contributor

tombuildsstuff commented Oct 1, 2019

@evenh actually appears I can't push to that fork 🙃 - I'll push this to a local branch/another PR and close this PR in favour of that PR (which will include all these commits, so you'll still get attribution)

Thanks for this contribution - this otherwise LGTM 👍

@tombuildsstuff

This comment has been minimized.

Copy link
Contributor

tombuildsstuff commented Oct 1, 2019

Closing in favour of #4472

@hashibot

This comment has been minimized.

Copy link

hashibot bot commented Oct 4, 2019

This has been released in version 1.35.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.35.0"
}
# ... other configuration ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.