-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Changing AKS load_balancer_profile settings after creation (#5846) #5847
Conversation
When the `load_balancer_profile` settings are calculated, changing (for example) from managed IPs to provided outbound IP addresses is not possible. The `effective_outbound_ips` are still `Computed`, but as only one of the three options `outbound_ip_address_ids`, `outbound_ip_prefix_ids` and `managed_outbound_ip_count` and at least one of them should be set, `Computed` doesnt make sense to me. If there is a better option to make a change possible, let me know
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.
Hey @aristosvo,
I've offered some suggestions on how we can not remove computed (as that would change behaviour), but could you describe a little more why the cluster needs to be recreated when computed=true
?
azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go
Outdated
Show resolved
Hide resolved
@katbyte Thanks for your remarks! I've added a test to describe better what I want and implemented your suggestions. I implemented some logic to replace edit: I think I found a better solution |
@katbyte Can you give it another review? I have reworked it a few times and I've finaly a solution which works for me:) With the solution I implemented we can keep both I'm still learning a lot about Go/TF and enjoying it, don't hold back if you've any improvements!:) |
If I can find the time I'll try to solve the conflicts |
@aristosvo awesome, thanks for rebasing this 👍 |
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.
hey @aristosvo
Thanks for this PR :)
Taking a look through this mostly LGTM - so that we can run the tests for this I hope you don't mind but I'm going to push a commit to fix the failing build/pending comments here - once that's done we should be able to run the tests and 🤞hopefully get this into v2.5 of the Azure Provider :)
Thanks!
azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go
Outdated
Show resolved
Hide resolved
dismissing since changes have been pushed
Acceptance test passed!
|
Are these expected failures tracked somewhere? I’m only running targeted acceptance tests, but I was unaware of the failing ones. |
@aristosvo unfortunately that varies test-run to test run due to capacity issues on the Azure side, so that's not easily possible - but we're checking these as we go |
This has been released in version 2.6.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 = "~> 2.6.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fixes #5846
Issue
When the
load_balancer_profile
settings forservice/kubernetes-cluster
are all computed, changing (for example) from managed IPs to provided outbound IP addresses is not possible with only TerraForm without recreating your cluster.Changes
With this change applied
effective_outbound_ips
->Computed: true
(no change)outbound_ip_address_ids
->Computed: false
outbound_ip_prefix_ids
->Computed: false
managed_outbound_ip_count
->Computed: false
As only one of the three options (
outbound_ip_address_ids
,outbound_ip_prefix_ids
,outbound_ip_prefix_ids
) could be set and at least one of them should be set,Computed: true
doesn't make sense to me for the last three and it prevented changes.If there is a better option to make a change possible, let me know