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

azurerm_kubernetes_cluster - support for upgrade_override_setting property #27962

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6134a08
Update kubernetes_cluster.html.markdown
hqhqhqhqhqhqhqhqhqhqhq Nov 11, 2024
d17f687
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Nov 11, 2024
b8ff724
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Nov 11, 2024
e341b33
update test case to include update operations for upgradeOverrideSetting
hqhqhqhqhqhqhqhqhqhqhq Nov 11, 2024
ac5136a
fix: upgrade upgrade_override_setting to upgrade_override
hqhqhqhqhqhqhqhqhqhqhq Nov 22, 2024
21c5107
Update kubernetes_cluster_resource_test.go
hqhqhqhqhqhqhqhqhqhqhq Dec 2, 2024
5514082
update
hqhqhqhqhqhqhqhqhqhqhq Dec 2, 2024
925c72a
Update website/docs/r/kubernetes_cluster.html.markdown
hqhqhqhqhqhqhqhqhqhqhq Jan 20, 2025
656975a
update
hqhqhqhqhqhqhqhqhqhqhq Jan 20, 2025
0e21eff
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Jan 20, 2025
01a3af9
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Jan 20, 2025
f175ce7
update
hqhqhqhqhqhqhqhqhqhqhq Jan 20, 2025
07b3b1e
Update website/docs/r/kubernetes_cluster.html.markdown
hqhqhqhqhqhqhqhqhqhqhq Jan 22, 2025
d28c5e3
Update website/docs/r/kubernetes_cluster.html.markdown
hqhqhqhqhqhqhqhqhqhqhq Jan 22, 2025
a3dcc8c
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Jan 22, 2025
a0f05dc
Merge branch 'feat/container_service-upgrade-settings' of https://git…
hqhqhqhqhqhqhqhqhqhqhq Jan 22, 2025
aecf203
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Jan 23, 2025
f3cf8bd
Update website/docs/r/kubernetes_cluster.html.markdown
hqhqhqhqhqhqhqhqhqhqhq Jan 29, 2025
a33befb
Update internal/services/containers/kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Jan 29, 2025
5ac6fdd
Update kubernetes_cluster_resource.go
hqhqhqhqhqhqhqhqhqhqhq Jan 29, 2025
1eb8891
Merge branch 'feat/container_service-upgrade-settings' of https://git…
hqhqhqhqhqhqhqhqhqhqhq Jan 29, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions internal/services/containers/kubernetes_cluster_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,26 @@ func resourceKubernetesCluster() *pluginsdk.Resource {

"tags": commonschema.Tags(),

"upgrade_override": {
Type: pluginsdk.TypeList,
Optional: true,
MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"force_upgrade_enabled": {
Type: pluginsdk.TypeBool,
Required: true,
},

"effective_until": {
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the guideline suggestions, we may make it a top level property.

Finally there are instances where the addtional fields/properties for a object/feature are optional or few in number, as shown below.
Example C.
```go
type ManagedClusterStorageProfileDiskCSIDriver struct {
Enabled *bool `json:"enabled,omitempty"`
Version *string `json:"version,omitempty"`
}
```
In cases like these one option is to flatten the block into two top level properties:

Copy link
Collaborator

@WodansSon WodansSon Nov 22, 2024

Choose a reason for hiding this comment

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

@wuxu92, totally agree with you here, however it appears that not all of the possible fields have been exposed here as well based on the REST API documentation for this property. We should also drop the _settings part from the name of the exposed field in the provider, also per the contributing guide. 🙂

  • For blocks avoid redundant words in the name that don't add informational value e.g.

firewall_properties can be shortened to firewall, the same can apply to individual properties e.g. email_address to email.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other field in the block is forceUpgrade as a boolean, which doesn't need to be exposed. So we may only need to expose one property named upgrade_override_until and keep the expand/flatten logic as it is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am hesistant to add this as a top level as service team indicated there may be additional properties relevant to upgrade_override_setting added later. We may, in future, need to wrap around these properties a layer down in upgrade_override_setting as it doesn't make sense to expose all of them in top level.

I will update the naming to upgrade_override.

@wuxu92 @WodansSon

Type: pluginsdk.TypeString,
Optional: true,
ValidateFunc: validation.IsRFC3339Time,
},
},
},
},

"windows_profile": {
Type: pluginsdk.TypeList,
Optional: true,
Expand Down Expand Up @@ -1641,6 +1661,9 @@ func resourceKubernetesClusterCreate(d *pluginsdk.ResourceData, meta interface{}
storageProfileRaw := d.Get("storage_profile").([]interface{})
storageProfile := expandStorageProfile(storageProfileRaw)

upgradeOverrideSettingRaw := d.Get("upgrade_override").([]interface{})
upgradeOverrideSetting := expandKubernetesClusterUpgradeOverrideSetting(upgradeOverrideSettingRaw)

// assemble securityProfile (Defender, WorkloadIdentity, ImageCleaner, AzureKeyVaultKms)
securityProfile := &managedclusters.ManagedClusterSecurityProfile{}

Expand Down Expand Up @@ -1723,6 +1746,7 @@ func resourceKubernetesClusterCreate(d *pluginsdk.ResourceData, meta interface{}
OidcIssuerProfile: oidcIssuerProfile,
SecurityProfile: securityProfile,
StorageProfile: storageProfile,
UpgradeSettings: upgradeOverrideSetting,
WorkloadAutoScalerProfile: workloadAutoscalerProfile,
},
Tags: tags.Expand(t),
Expand Down Expand Up @@ -2281,6 +2305,19 @@ func resourceKubernetesClusterUpdate(d *pluginsdk.ResourceData, meta interface{}
}
}

if d.HasChange("upgrade_override") {
upgradeOverrideSettingRaw := d.Get("upgrade_override").([]interface{})

err = validateKubernetesClusterUpgradeOverrideSetting(d.GetChange("upgrade_override"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense to put this logic inline rather than in a helper function to be more consistent with the other resources in the provider? I might be wrong but I believe this is the only place this function is called, so to me it would make more sense to make that logic inline, here in the update fuction, to be more readable for future maintainers is they need to modify this code.

if err != nil {
return fmt.Errorf("expanding `upgrade_override`: %+v", err)
}

updateCluster = true
upgradeOverrideSetting := expandKubernetesClusterUpgradeOverrideSetting(upgradeOverrideSettingRaw)
existing.Model.Properties.UpgradeSettings = upgradeOverrideSetting
}

if d.HasChange("web_app_routing") {
updateCluster = true
existing.Model.Properties.IngressProfile = expandKubernetesClusterIngressProfile(d, d.Get("web_app_routing").([]interface{}))
Expand Down Expand Up @@ -2531,6 +2568,19 @@ func resourceKubernetesClusterUpdate(d *pluginsdk.ResourceData, meta interface{}
return resourceKubernetesClusterRead(d, meta)
}

func validateKubernetesClusterUpgradeOverrideSetting(_, new interface{}) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this function and make it inline in the update function? I think we should be able to use something like d.GetRawConfig so we could compare the old values of the fields against the new values of the fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to inline function, but left it as d.Get("upgrade_override") rather than d.GetRawConfig because it is easier to work with the expand function below (easier to parse to []interface{}).

if new == nil {
return fmt.Errorf("upgrade_override cannot be unset")
hqhqhqhqhqhqhqhqhqhqhq marked this conversation as resolved.
Show resolved Hide resolved
}

newOverrideSetting, canConvert := new.([]interface{})
if !canConvert || len(newOverrideSetting) == 0 {
return fmt.Errorf("upgrade_override cannot be unset")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here...

Suggested change
return fmt.Errorf("upgrade_override cannot be unset")
return fmt.Errorf("`upgrade_override` cannot be unset")

}

return nil
}

func resourceKubernetesClusterRead(d *pluginsdk.ResourceData, meta interface{}) error {
client := meta.(*clients.Client).Containers.KubernetesClustersClient
ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d)
Expand Down Expand Up @@ -2726,6 +2776,11 @@ func resourceKubernetesClusterRead(d *pluginsdk.ResourceData, meta interface{})
return fmt.Errorf("setting `windows_profile`: %+v", err)
}

upgradeOverrideSetting := flattenKubernetesClusterUpgradeOverrideSetting(props.UpgradeSettings)
if err := d.Set("upgrade_override", upgradeOverrideSetting); err != nil {
return fmt.Errorf("setting `upgrade_override`: %+v", err)
}

workloadAutoscalerProfile := flattenKubernetesClusterWorkloadAutoscalerProfile(props.WorkloadAutoScalerProfile)
if err := d.Set("workload_autoscaler_profile", workloadAutoscalerProfile); err != nil {
return fmt.Errorf("setting `workload_autoscaler_profile`: %+v", err)
Expand Down Expand Up @@ -4590,3 +4645,34 @@ func retrySystemNodePoolCreation(ctx context.Context, client *agentpools.AgentPo

return err
}

func expandKubernetesClusterUpgradeOverrideSetting(input []interface{}) *managedclusters.ClusterUpgradeSettings {
if len(input) == 0 || input[0] == nil {
return &managedclusters.ClusterUpgradeSettings{
OverrideSettings: &managedclusters.UpgradeOverrideSettings{
ForceUpgrade: pointer.To(false),
},
}
}

raw := input[0].(map[string]interface{})
return &managedclusters.ClusterUpgradeSettings{
OverrideSettings: &managedclusters.UpgradeOverrideSettings{
ForceUpgrade: pointer.To(raw["force_upgrade_enabled"].(bool)),
Until: pointer.To(raw["effective_until"].(string)),
},
}
}

func flattenKubernetesClusterUpgradeOverrideSetting(input *managedclusters.ClusterUpgradeSettings) []interface{} {
if input == nil || input.OverrideSettings == nil {
return []interface{}{}
}

return []interface{}{
map[string]interface{}{
"effective_until": pointer.From(input.OverrideSettings.Until),
"force_upgrade_enabled": pointer.From(input.OverrideSettings.ForceUpgrade),
},
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,35 @@ func TestAccKubernetesCluster_updateNetworkProfileOutboundType(t *testing.T) {
})
}

func TestAccKubernetesCluster_upgradeOverrideSetting(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test")
r := KubernetesClusterResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.upgradeOverrideSetting(data, false),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
Config: r.upgradeOverrideSetting(data, true),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
Config: r.upgradeOverrideSetting(data, false),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
})
}

func (t KubernetesClusterResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := commonids.ParseKubernetesClusterID(state.ID)
if err != nil {
Expand Down Expand Up @@ -923,3 +952,41 @@ resource "azurerm_kubernetes_cluster" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, controlPlaneVersion)
}

func (KubernetesClusterResource) upgradeOverrideSetting(data acceptance.TestData, isUpgradeOverrideSettingEnabled bool) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-aks-%[1]s"
location = "%[2]s"
}

resource "azurerm_kubernetes_cluster" "test" {
name = "acctestaks%[1]s"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
dns_prefix = "acctestaks%[1]s"

default_node_pool {
name = "default"
node_count = 1
vm_size = "Standard_DS2_v2"
upgrade_settings {
max_surge = "10%%"
}
}

identity {
type = "SystemAssigned"
}

upgrade_override {
effective_until = "%[3]s"
force_upgrade_enabled = %[4]t
}
}
`, data.RandomString, data.Locations.Primary, time.Now().UTC().Add(8*time.Minute).Format(time.RFC3339), isUpgradeOverrideSettingEnabled)
}
16 changes: 16 additions & 0 deletions website/docs/r/kubernetes_cluster.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ resource "azurerm_kubernetes_cluster" "example" {

* `tags` - (Optional) A mapping of tags to assign to the resource.

* `upgrade_override` - (Optional) A `upgrade_override` block as defined below.

* `web_app_routing` - (Optional) A `web_app_routing` block as defined below.

* `windows_profile` - (Optional) A `windows_profile` block as defined below.
Expand Down Expand Up @@ -875,6 +877,20 @@ A `sysctl_config` block supports the following:

---

The `upgrade_override` block supports the following:

-> **Note:** Once set, the `upgrade_override` block cannot be removed from the configuration.

* `force_upgrade_enabled` - (Required) Whether to force upgrade the cluster. Note that this option instructs the upgrade operation to bypass upgrade protections such as checking for deprecated API usage.
hqhqhqhqhqhqhqhqhqhqhq marked this conversation as resolved.
Show resolved Hide resolved

!> **Important: The `force_upgrade_enabled` field instructs the upgrade operation to bypass upgrade protections (e.g. checking for deprecated API usage) which may render the cluster inoperative after the upgrade process has completed. Use the `force_upgrade_enabled` option with extreme caution only.**
hqhqhqhqhqhqhqhqhqhqhq marked this conversation as resolved.
Show resolved Hide resolved

* `effective_until` - (Optional) Specifies the duration, in RFC 3339 format (e.g., `2025-10-01T13:00:00Z`), the `upgrade_override` values are effective. This field must be set for the `upgrade_override` values to take effect. The date-time must be within the next 30 days.

-> **Note:** This only matches the start time of an upgrade, and the effectiveness won't change once an upgrade starts even if the `effective_until` value expires as the upgrade proceeds.

---

A `web_app_routing` block supports the following:

* `dns_zone_ids` - (Required) Specifies the list of the DNS Zone IDs in which DNS entries are created for applications deployed to the cluster when Web App Routing is enabled. If not using Bring-Your-Own DNS zones this property should be set to an empty list.
Expand Down
Loading