`azurerm_kubernetes_cluster` - support for the `revisions` for istio addon #26546

hqhqhqhqhqhqhqhqhqhqhq · 2024-07-05T05:48:33Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Adds new property revisions forazurerm_kubernetes_cluster istio addon. This configuration allows users to bring set istio control plane revisions for managing upgrades the minor revision using canary upgrade process.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_kubernetes_cluster - support for the revisions property for service_mesh_profile.

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Closes #25724

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

stephybun · 2024-07-05T07:08:44Z

internal/services/containers/kubernetes_cluster_resource.go

+							Optional: true,
+							Computed: true,


Is there a reason this needs to be Computed? We avoid introducing new properties that are both Optional+Computed (O+C), in particular lists of primitives unless there is a good reason to. This is because the provider is sometimes unable to tell whether a change has happened to an O+C property when a user tries to update it. We have a more detailed explanation over in the contributor docs in case you're interested.

Updated in latest commit

stephybun · 2024-07-05T07:08:57Z

internal/services/containers/kubernetes_cluster_resource.go

+							Type:     pluginsdk.TypeSet,
+							Optional: true,
+							Computed: true,
+							MinItems: 1,


This would be a breaking change for users that have this block set.

Sorry, are you referring to MinItems? As in it would be a breaking change when in future this MinItems changes? And vice versa for MaxItems?

I was referring to the MinItems, but my suspicion that it would be a breaking change was incorrect. Specifying a MinItems would have implied that the property must be set with at least one element in the list, but because it's Optional is appears to really only be a validation that's applied if the property has been set. Good to know.

Unfortunately adding this property is still going to be a breaking change since users will need to add this property to ignore_changes to prevent a diff when they upgrade to a version that will include this change.

From the sounds of it it also seems like this property might actually want to be Required instead of Optional?

stephybun · 2024-07-05T07:11:34Z

internal/services/containers/kubernetes_cluster_resource.go

+							Computed: true,
+							MinItems: 1,
+							MaxItems: 2,
+							Set:      set.HashStringIgnoreCase,


Does this need to be a set?

No need, but Set feels more suitable in this case to stop users from inputting duplicates, can update to List if preferred.

We only use sets if the API doesn't preserve the ordering.

Regarding duplicates, you could add some validation in the expand function to check for duplicates and can error at apply time if that helps?

stephybun · 2024-07-05T07:14:16Z

internal/services/containers/kubernetes_cluster_resource.go

+		if existing != nil && existing.Istio != nil && existing.Istio.Revisions != nil {
+			profile.Istio.Revisions = existing.Istio.Revisions


What is this for?

Previously, if the service_mesh_profile is set with istio, it will be deployed fine, but any updates for service_mesh_profile properties later will cause error returned by the AKS API because the revisions has not been specified.
I have added this block so that if user wants to update the service_mesh_profile without revisions, it will by default use whatever revisions it already has as returned by the AKS API.

Based on this it sounds like this property might actually want to be Required instead of Optional

internal/services/containers/kubernetes_cluster_network_resource_test.go

website/docs/r/kubernetes_cluster.html.markdown

…data source feat: add revisions setting in azurerm_kubernetes_cluster resource & data source feat: add doc & acceptance tests for new property feat: add revisions setting in azurerm_kubernetes_cluster resource & data source Update kubernetes_cluster_network_resource_test.go fix: fix issues based on pr reviews Update website/docs/r/kubernetes_cluster.html.markdown Co-Authored-By: stephybun <[email protected]>

github-actions bot added documentation size/L waiting-response and removed waiting-response labels Jul 5, 2024

hqhqhqhqhqhqhqhqhqhqhq force-pushed the feat/aks-istio.components branch from 680512f to d516c68 Compare July 5, 2024 06:01

github-actions bot added the waiting-response label Jul 5, 2024

stephybun requested changes Jul 5, 2024

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Jul 5, 2024

hqhqhqhqhqhqhqhqhqhqhq force-pushed the feat/aks-istio.components branch from 0089c72 to 34371c9 Compare July 8, 2024 00:07

github-actions bot added the waiting-response label Jul 8, 2024

hqhqhqhqhqhqhqhqhqhqhq marked this pull request as ready for review July 9, 2024 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_kubernetes_cluster` - support for the `revisions` for istio addon #26546

`azurerm_kubernetes_cluster` - support for the `revisions` for istio addon #26546

hqhqhqhqhqhqhqhqhqhqhq commented Jul 5, 2024 •

edited by stephybun

Loading

stephybun Jul 5, 2024

hqhqhqhqhqhqhqhqhqhqhq Jul 8, 2024

stephybun Jul 5, 2024

hqhqhqhqhqhqhqhqhqhqhq Jul 5, 2024

stephybun Jul 9, 2024

stephybun Jul 5, 2024

hqhqhqhqhqhqhqhqhqhqhq Jul 5, 2024

stephybun Jul 9, 2024

stephybun Jul 5, 2024

hqhqhqhqhqhqhqhqhqhqhq Jul 5, 2024

stephybun Jul 9, 2024

		if existing != nil && existing.Istio != nil && existing.Istio.Revisions != nil {
		profile.Istio.Revisions = existing.Istio.Revisions

azurerm_kubernetes_cluster - support for the revisions for istio addon #26546

Are you sure you want to change the base?

azurerm_kubernetes_cluster - support for the revisions for istio addon #26546

Conversation

hqhqhqhqhqhqhqhqhqhqhq commented Jul 5, 2024 • edited by stephybun Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`azurerm_kubernetes_cluster` - support for the `revisions` for istio addon #26546

`azurerm_kubernetes_cluster` - support for the `revisions` for istio addon #26546

hqhqhqhqhqhqhqhqhqhqhq commented Jul 5, 2024 •

edited by stephybun

Loading