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_mssql_server and azurerm_mssql_database - upgrade from v5.0/sql to 2023-02-01-preview #23721

Merged
merged 56 commits into from
Nov 23, 2023

Conversation

WodansSon
Copy link
Collaborator

@WodansSon WodansSon commented Oct 28, 2023

Packages which are upgraded in this PR:

  • 2023-02-01-preview/backupshorttermretentionpolicies
  • 2023-02-01-preview/databases
  • 2023-02-01-preview/geobackuppolicies
  • 2023-02-01-preview/LogicalDatabaseTransparentDataEncryption
  • 2023-02-01-preview/longtermretentionpolicies
  • 2023-02-01-preview/replicatonlinks
  • 2023-02-01-preview/restorabledroppeddatabases
  • 2023-02-01-preview/serverazureadadministrators
  • 2023-02-01-preview/serverazureadonlyauthentications
  • 2023-02-01-preview/serverconnectionpolicies
  • 2023-02-01-preview/servers
  • 2023-02-01-preview/serversecurityalertpolicies

(fixes #23807)

@WodansSon WodansSon changed the title azurerm_mssql_server - upgrade mssql server from v5.0/sql to 2023-02-01-preview/servers and 2023-02-01-preview/serverconnectionpolicies azurerm_mssql_server - upgrade mssql server from v5.0/sql to 2023-02-01-preview Oct 28, 2023
@WodansSon WodansSon changed the title azurerm_mssql_server - upgrade mssql server from v5.0/sql to 2023-02-01-preview azurerm_mssql_server - upgrade from v5.0/sql to 2023-02-01-preview Oct 30, 2023
@WodansSon
Copy link
Collaborator Author

All test that can pass, do pass... the last four failures are due to a regression in the RP and the service team is currently working on a fix for that, current ETA end of November...

image

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @WodansSon

Thanks for pushing those changes - I've taken a look through and left some comments inline, but if we can fix those up then this otherwise LGTM 👍

Thans!

internal/services/mssql/mssql_server_data_source.go Outdated Show resolved Hide resolved
Comment on lines 442 to 487
// NOTE: This call does not return a future it returns a response, but you will get a future back if the status code is 202...
// https://learn.microsoft.com/en-us/rest/api/sql/server-azure-ad-only-authentications/delete?view=rest-sql-2023-05-01-preview&tabs=HTTP
if response.WasStatusCode(resp.HttpResponse, 202) {
// It was accepted but not completed, it is now an async operation...
log.Printf("[INFO] Delete Azure Active Directory Only Administrators response was a 202 WaitForStateContext...")

// NOTE: the resp.Poller.PollUntilDone(ctx) gets stuck in a loop and never returns even when the status code
// being retuned by the service is a 200... could not figure out how to get the resourcemanager.PollerFromResponse()
// to work, opted for a waitforstate instead...
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}

// NOTE: Internal x-ref, this is another case of hashicorp/go-azure-sdk#307 so this can be removed once that's fixed
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"InProgress"},
Target: []string{"Succeeded"},
Refresh: func() (interface{}, string, error) {
resp, err := aadOnlyAuthenticationsClient.Get(ctx, pointer.From(id))
if err != nil {
return nil, "Failed", fmt.Errorf("polling for the delete status of the Azure Active Directory Only Administrator: %+v", err)
}

if model := resp.Model; model != nil {
if model.Name != nil {
if props := model.Properties; props != nil {
if !props.AzureADOnlyAuthentication && strings.EqualFold(pointer.From(model.Name), "Default") {
return resp, "Succeeded", nil
}
}
}
}

return resp, "InProgress", nil
},
ContinuousTargetOccurence: 2,
MinTimeout: 5 * time.Second,
Timeout: time.Until(deadline),
}

if _, err = stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("waiting for the deletion of the Azure Active Directory Only Administrator: %+v", err)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(as discussed offline) we should be able to use a Custom Poller for this instead - example: https://gist.github.com/tombuildsstuff/70a174c4dc8a3e6111e659a9d28df0ea

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Took me a bit to figure out I needed to implement the custom poller itself, but I got there... lol... 🙂

Comment on lines 361 to 362
if d.HasChange("key_vault_key_id") {
keyVaultKeyId := d.Get(("transparent_data_encryption_key_vault_key_id")).(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bug, can we fix this:

Suggested change
if d.HasChange("key_vault_key_id") {
keyVaultKeyId := d.Get(("transparent_data_encryption_key_vault_key_id")).(string)
if d.HasChange("transparent_data_encryption_key_vault_key_id") {
keyVaultKeyId := d.Get("transparent_data_encryption_key_vault_key_id").(string)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 284 to 288
if resp.Model == nil {
return nil, fmt.Errorf("server model was nil")
}

return utils.Bool(resp.Model.Id != nil), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one's still pending?

@@ -225,7 +228,7 @@ func TestAccMsSqlServer_azureadAdminWithAADAuthOnly(t *testing.T) {
})
}

func TestAccMsSqlServer_updateAzureadAuthenticationOnlyWithIdentity(t *testing.T) {
func TestAccMsSqlServer_azureadAuthenticationOnlyWithIdentityUpdate(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a new test creating a server with a CMK, and then (in a second update) changing just the tags? That would catch the KeyId bug above/ensure that's fixed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines +561 to +563
if props.MaxSizeBytes != nil {
d.Set("max_size_gb", int32((*props.MaxSizeBytes)/int64(1073741824)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to make this:

Suggested change
if props.MaxSizeBytes != nil {
d.Set("max_size_gb", int32((*props.MaxSizeBytes)/int64(1073741824)))
}
maxSizeGb := 0
if props.MaxSizeBytes != nil {
maxSizeGb = *props.MaxSizeBytes/int64(1073741824)
}
d.Set("max_size_gb", maxSizeGb)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, actually we can't, else the compiler complains cannot use *props.MaxSizeBytes / int64(1073741824) (value of type int64) as int value in assignmentcompiler

return tags.FlattenAndSet(d, resp.Tags)
d.Set("transparent_data_encryption_enabled", tdeState)

return tags.FlattenAndSet(d, resp.Model.Tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move this up:

Suggested change
return tags.FlattenAndSet(d, resp.Model.Tags)
return nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


t := d.Get("tags").(map[string]interface{})
metadata := tags.Expand(t)
if model := existing.Model; model != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're nil-checking this below/relying on this having a value - we can remove this if statement:

Suggested change
if model := existing.Model; model != nil {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 405 to 416
future, err := client.CreateOrUpdate(ctx, *id, payload)
if err != nil {
return fmt.Errorf("issuing update request for %s: %+v", id, err)
}

return fmt.Errorf("waiting for update of %s: %+v", id.String(), err)
}
if err = future.Poller.PollUntilDone(ctx); err != nil {
if response.WasConflict(future.HttpResponse) {
return fmt.Errorf("SQL Server names need to be globally unique and %q is already in use", id.ServerName)
}

d.SetId(id.ID())
return fmt.Errorf("waiting for update of %s: %+v", pointer.From(id), err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is an Update, the SQL Server name will already be reserved, so I think we should be able to reduce this too:

Suggested change
future, err := client.CreateOrUpdate(ctx, *id, payload)
if err != nil {
return fmt.Errorf("issuing update request for %s: %+v", id, err)
}
return fmt.Errorf("waiting for update of %s: %+v", id.String(), err)
}
if err = future.Poller.PollUntilDone(ctx); err != nil {
if response.WasConflict(future.HttpResponse) {
return fmt.Errorf("SQL Server names need to be globally unique and %q is already in use", id.ServerName)
}
d.SetId(id.ID())
return fmt.Errorf("waiting for update of %s: %+v", pointer.From(id), err)
}
future, err := client.CreateOrUpdateThenPoll(ctx, *id, payload)
if err != nil {
return fmt.Errorf("issuing update request for %s: %+v", id, err)
}

Copy link
Collaborator Author

@WodansSon WodansSon Nov 15, 2023

Choose a reason for hiding this comment

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

Fixed.

@WodansSon
Copy link
Collaborator Author

These tests are also failing in main...

image

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

A couple of minor comments but this otherwise LGTM 👍

Comment on lines 542 to 544
if props.RequestedBackupStorageRedundancy != nil {
d.Set("storage_account_type", string(pointer.From(props.RequestedBackupStorageRedundancy)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this one is still pending?

Comment on lines 511 to 512
// NOTE: In the new API this just a string instead of a *string, shouldn't we just call Normalize?
d.Set("location", location.NormalizeNilable(pointer.To(model.Location)))
Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, this can become:

Suggested change
// NOTE: In the new API this just a string instead of a *string, shouldn't we just call Normalize?
d.Set("location", location.NormalizeNilable(pointer.To(model.Location)))
d.Set("location", location.Normalize(model.Location))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Thanks for pushing those changes @WodansSon - this LGTM 👍

@tombuildsstuff tombuildsstuff added this to the v3.82.0 milestone Nov 23, 2023
@tombuildsstuff tombuildsstuff removed their assignment Nov 23, 2023
@tombuildsstuff tombuildsstuff merged commit c39b06e into main Nov 23, 2023
23 checks passed
@tombuildsstuff tombuildsstuff deleted the e_sql_upgrade_deux branch November 23, 2023 08:16
opentofu-provider-sync-service-account pushed a commit to opentofu/terraform-provider-azurerm that referenced this pull request Nov 23, 2023
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Nov 24, 2023
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.81.0&#34; to
&#34;3.82.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.82.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.82.0&#xA;FEATURES:&#xA;&#xA;*
New Data Source: `azurerm_monitor_workspace`
([#23928](hashicorp/terraform-provider-azurerm#23928
New Resource: `azurerm_application_load_balancer_subnet_association`
([#23628](https://github.com/hashicorp/terraform-provider-azurerm/issues/23628))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20231117.1130141` of
`github.com/hashicorp/go-azure-sdk`
([#23945](hashicorp/terraform-provider-azurerm#23945
`azurestackhci`: updating to API Version `2023-08-01`
([#23939](hashicorp/terraform-provider-azurerm#23939
`dashboard`: updating to API Version `2023-09-01`
([#23929](hashicorp/terraform-provider-azurerm#23929
`hpccache`: updating to API version `2023-05-01`
([#24005](hashicorp/terraform-provider-azurerm#24005
`mssql`: updating resources using `hashicorp/go-azure-sdk` to API
Version `2023-02-01-preview`
([#23721](hashicorp/terraform-provider-azurerm#23721
`templatespecversions`: updating to API Version `2022-02-01`
([#24007](hashicorp/terraform-provider-azurerm#24007
Data Source: `azurerm_template_spec_version` - refactoring to use
`hashicorp/go-azure-sdk`
([#24007](hashicorp/terraform-provider-azurerm#24007
`azurerm_cosmosdb_postgresql_cluster` -
`coordinator_storage_quota_in_mb` and `coordinator_vcore_count` are no
longer required for read replicas
([#23928](hashicorp/terraform-provider-azurerm#23928
`azurerm_dashboard_grafana` - `sku` can now be set to `Essential`
([#23934](hashicorp/terraform-provider-azurerm#23934
`azurerm_gallery_application_version` - add support for the
`config_file`, `package_file` and `target_region.exclude_from_latest`
properties
([#23816](hashicorp/terraform-provider-azurerm#23816
`azurerm_hdinsight_hadoop_cluster` - `script_actions` is no longer Force
New
([#23888](hashicorp/terraform-provider-azurerm#23888
`azurerm_hdinsight_hbase_cluster` - `script_actions` is no longer Force
New
([#23888](hashicorp/terraform-provider-azurerm#23888
`azurerm_hdinsight_interactive_query_cluster` - `script_actions` is no
longer Force New
([#23888](hashicorp/terraform-provider-azurerm#23888
`azurerm_hdinsight_kafka_cluster` - `script_actions` is no longer Force
New
([#23888](hashicorp/terraform-provider-azurerm#23888
`azurerm_hdinsight_spark_cluster` - `script_actions` is no longer Force
New
([#23888](hashicorp/terraform-provider-azurerm#23888
`azurerm_kubernetes_cluster` - add support for the `gpu_instance`
property
([#23887](hashicorp/terraform-provider-azurerm#23887
`azurerm_kubernetes_cluster_node_pool` - add support for the
`gpu_instance` property
([#23887](hashicorp/terraform-provider-azurerm#23887
`azurerm_log_analytics_workspace` - add support for the `identity`
property
([#23864](hashicorp/terraform-provider-azurerm#23864
`azurerm_linux_function_app` - add support for dotnet 8
([#23638](hashicorp/terraform-provider-azurerm#23638
`azurerm_linux_function_app_slot` - add support for dotnet 8
([#23638](hashicorp/terraform-provider-azurerm#23638
`azurerm_managed_lustre_file_system` - export attribute `mgs_address`
([#23942](hashicorp/terraform-provider-azurerm#23942
`azurerm_mssql_database` - support for Hyperscale SKUs
([#23974](hashicorp/terraform-provider-azurerm#23974
`azurerm_mssql_database` - refactoring to use `hashicorp/go-azure-sdk`
([#23721](hashicorp/terraform-provider-azurerm#23721
`azurerm_mssql_server` - refactoring to use `hashicorp/go-azure-sdk`
([#23721](hashicorp/terraform-provider-azurerm#23721
`azurerm_shared_image` - add support for `trusted_launch_supported`
([#23781](hashicorp/terraform-provider-azurerm#23781
`azurerm_spring_cloud_container_deployment` - add support for the
`application_performance_monitoring_ids` property
([#23862](hashicorp/terraform-provider-azurerm#23862
`azurerm_spring_cloud_customized_accelerator` - add support for the
`accelerator_type` and `path` properties
([#23797](hashicorp/terraform-provider-azurerm#23797
`azurerm_point_to_site_vpn_gateway` - allow multiple
`connection_configurations` blocks
([#23936](hashicorp/terraform-provider-azurerm#23936
`azurerm_private_dns_cname_record` - `ttl` can now be set to 0
([#23918](hashicorp/terraform-provider-azurerm#23918
`azurerm_windows_function_app` - add support for dotnet 8
([#23638](hashicorp/terraform-provider-azurerm#23638
`azurerm_windows_function_app_slot` - add support for dotnet 8
([#23638](https://github.com/hashicorp/terraform-provider-azurerm/issues/23638))&#xA;&#xA;BUG
FIXES:&#xA;* `azurerm_api_management` - correct a bug with additional
location zones within the `additional_location` block
([#23943](hashicorp/terraform-provider-azurerm#23943
`azurerm_dev_test_linux_virtual_machine` - `storage_type` is now
ForceNew to match the updated API behaviour
([#23973](hashicorp/terraform-provider-azurerm#23973
`azurerm_dev_test_windows_virtual_machine` - `storage_type` is now
ForceNew to match the updated API behaviour
([#23973](hashicorp/terraform-provider-azurerm#23973
`azurerm_disk_encryption_set` - resource will recreate if `identity`
changes from `SystemAssigned` to `UserAssigned`
([#23904](hashicorp/terraform-provider-azurerm#23904
`azurerm_eventhub_cluster`: `sku_name` is no longer ForceNew
([#24009](hashicorp/terraform-provider-azurerm#24009
`azurerm_firewall` - recasing the value for `firewall_policy_id` to
workaround the API returning the incorrect casing
([#23993](hashicorp/terraform-provider-azurerm#23993
`azurerm_security_center_subscription_pricing` - fix a bug preventing
removal of `extensions` and downgrading `tier` to `Free`
([#23821](hashicorp/terraform-provider-azurerm#23821
`azurerm_windows_web_app` - fix an issue of incorrect application stack
settings during update
([#23372](https://github.com/hashicorp/terraform-provider-azurerm/issues/23372))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/896/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
Copy link

github-actions bot commented May 6, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for id for data source mssql_database
2 participants