-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
base: main
Are you sure you want to change the base?
Changes from 14 commits
6134a08
d17f687
b8ff724
e341b33
ac5136a
21c5107
5514082
925c72a
656975a
0e21eff
01a3af9
f175ce7
07b3b1e
d28c5e3
a3dcc8c
a0f05dc
aecf203
f3cf8bd
a33befb
5ac6fdd
1eb8891
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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": { | ||||||
Type: pluginsdk.TypeString, | ||||||
Optional: true, | ||||||
ValidateFunc: validation.IsRFC3339Time, | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
|
||||||
"windows_profile": { | ||||||
Type: pluginsdk.TypeList, | ||||||
Optional: true, | ||||||
|
@@ -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{} | ||||||
|
||||||
|
@@ -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), | ||||||
|
@@ -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")) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
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{})) | ||||||
|
@@ -2531,6 +2568,19 @@ func resourceKubernetesClusterUpdate(d *pluginsdk.ResourceData, meta interface{} | |||||
return resourceKubernetesClusterRead(d, meta) | ||||||
} | ||||||
|
||||||
func validateKubernetesClusterUpgradeOverrideSetting(_, new interface{}) error { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove this function and make it inline in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to inline function, but left it as |
||||||
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") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here...
Suggested change
|
||||||
} | ||||||
|
||||||
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) | ||||||
|
@@ -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) | ||||||
|
@@ -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), | ||||||
}, | ||||||
} | ||||||
} |
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.
According to the guideline suggestions, we may make it a top level property.
terraform-provider-azurerm/contributing/topics/schema-design-considerations.md
Lines 75 to 85 in 778c935
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.
@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. 🙂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.
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 namedupgrade_override_until
and keep the expand/flatten logic as it is now.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.
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 inupgrade_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