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 4 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
61 changes: 61 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,21 @@ func resourceKubernetesCluster() *pluginsdk.Resource {

"tags": commonschema.Tags(),

"upgrade_override_setting": {
Type: pluginsdk.TypeList,
Optional: true,
MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"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,
},
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only configurable property in this block? If so, it might be worth flattening this down to a single property, for example upgrade_override_until?

What happens to this value when the specified point in time passes, is it removed from the response, or still returned?

Should we also validate that the supplied time is in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jackofallops

  1. There is only one property that user should configure, but service team indicated more may be added later.
  2. When the value is in the past in time, it will stay in the response
  3. I prefer not to add this check because if the time is in the past, users don't need to update it every single time; they can leave it as is (and it will not have any impact).


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

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

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

Expand Down Expand Up @@ -1723,6 +1741,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 +2300,13 @@ func resourceKubernetesClusterUpdate(d *pluginsdk.ResourceData, meta interface{}
}
}

if d.HasChange("upgrade_override_setting") {
updateCluster = true
upgradeOverrideSettingRaw := d.Get("upgrade_override_setting").([]interface{})
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 @@ -2726,6 +2752,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_setting", upgradeOverrideSetting); err != nil {
return fmt.Errorf("setting `upgrade_override_setting`: %+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 +4621,33 @@ 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(true),
Until: pointer.To(raw["effective_until"].(string)),
},
}
}

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

return []interface{}{
map[string]interface{}{
"effective_until": pointer.From(input.OverrideSettings.Until),
},
}
}
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,49 @@ resource "azurerm_kubernetes_cluster" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, controlPlaneVersion)
}

func (KubernetesClusterResource) upgradeOverrideSetting(data acceptance.TestData, isUpgradeOverrideSettingEnabled bool) string {

upgradeOverrideSetting := ""
if isUpgradeOverrideSettingEnabled {
upgradeOverrideSetting = `
upgrade_override_setting {
effective_until = "2024-01-01T00:00:00Z"
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to be testing a time in the past here? It's unlikely this will be representative of user input? Could we generate a value for this based on time.Now() plus some realistic duration?

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

}`
}

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"
}

%[3]s

}
`, data.RandomString, data.Locations.Primary, upgradeOverrideSetting)

}
8 changes: 8 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_setting` - (Optional) A `upgrade_override_setting` 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,12 @@ A `sysctl_config` block supports the following:

---

The `upgrade_override_setting` block supports the following:

* `effective_until` - (Optional) Specifies the date and time until when the upgrade overrides are effective. This should be in RFC 3339 format (e.g., `"2023-10-01T13:00:00Z"`). Note that this only matches the start time of an upgrade, and the effectiveness won't change once an upgrade starts even if the `effective_until` expires as the upgrade proceeds.
hqhqhqhqhqhqhqhqhqhqhq marked this conversation as resolved.
Show resolved Hide resolved

---

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