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_cosmosdb_account add minimal_tls_version #24711

Closed
wants to merge 0 commits into from

Conversation

rizkybiz
Copy link
Contributor

@rizkybiz rizkybiz commented Jan 30, 2024

Test Output

make acctests SERVICE='cosmos' TESTARGS='-run=TestAccCosmosDBAccount_MinimalTlsVersion' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/cosmos -run=TestAccCosmosDBAccount_MinimalTlsVersion -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccCosmosDBAccount_MinimalTlsVersion
=== PAUSE TestAccCosmosDBAccount_MinimalTlsVersion
=== CONT  TestAccCosmosDBAccount_MinimalTlsVersion
--- PASS: TestAccCosmosDBAccount_MinimalTlsVersion (723.97s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/cosmos        730.340s

fixes #21295

@neil-yechenwei
Copy link
Contributor

neil-yechenwei commented Jan 31, 2024

CosmosDB Service team confirmed that there is a known issue that the default value of minimalTlsVersion of the source CosmosDB Account returned by service API is Tls12 but the default value of minimalTlsVersion of the restored CosmosDB Account returned by service API is Tls when it isn't set in the request payload. I assume that
it should be consolidated on a single default value. So service team is still investigating this issue.

Note: TestAccCosmosDBAccount_restoreCreateMode would fail once this PR is applied.

@jackofallops
Copy link
Member

HI @rizkybiz - Thanks for this PR. Unfortunately, as noted in an earlier PR here #24615 (comment) this property has an upstream API issue which we are working with MSFT to resolve. Due to this ongoing work with the service team, I'm going to close this for now and we can take a look again when the upstream issue is resolved.
Thanks again!

@katbyte katbyte reopened this Feb 13, 2024
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hey @rizkybiz - While there is an underlaying API bug this is something i think we can work around by in the create function once we’ve restored the CosmosDB account we can do an update on the other values (e.g. tls) to ensure they match the configuration

@@ -301,6 +301,12 @@ func resourceCosmosDbAccount() *pluginsdk.Resource {
}
}(),

"minimal_tls_version": {
Type: pluginsdk.TypeString,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we be specifying a default value here? or making it computed?

Comment on lines 754 to 763

var minimalTlsVersion cosmosdb.MinimalTlsVersion
switch v := d.Get("minimal_tls_version").(string); v {
case "Tls":
minimalTlsVersion = cosmosdb.MinimalTlsVersionTls
case "Tls11":
minimalTlsVersion = cosmosdb.MinimalTlsVersionTlsOneOne
case "Tls12":
minimalTlsVersion = cosmosdb.MinimalTlsVersionTlsOneTwo
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come we need this? shouldn't the allowed values match the constant?

Copy link

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 Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.