-
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_cosmosdb_account add minimal_tls_version #24966
Changes from 86 commits
1c9050c
ab3fdc4
e8bb996
9ff23ad
9803cee
01bcf16
55aceb6
1d45589
1a0a550
45e9a54
71acbe2
7c9bad6
18a7147
59f8cab
ae0dd8b
bf4e418
9c86ac5
8eb73d7
b985d8b
15fa666
0a819f3
8045592
340b548
3780c81
3f153ac
62e4f4f
fac827b
f62f607
0d650c1
d10b679
1ce8844
b30bcd5
35c6066
bebfaab
c316f92
787f259
67cc29b
e466940
ac6f11e
d673b25
0c3459d
1fdaaa7
3ce1f01
b90ff89
787f22a
c3cfff6
bf22d8e
63b6b44
0b200ca
7be7e69
a8cd578
155fb35
3289b87
3d9f023
c88058d
71b01f5
e31c633
e8aaa9c
6981988
605644f
381fe4f
fe0c063
34b0aa4
98b1d7f
5a2b3c8
ec0c09b
acf73e0
e6a5c40
5635fac
2985be8
76f7c61
eefec1c
a85f7ab
2201135
0a3f809
07a2466
5685dea
19b8e8c
dead4d6
a5d3f32
212a96b
3fe589b
fd5816d
1d08aca
1fc55a3
db01d50
6ea8757
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 |
---|---|---|
|
@@ -242,6 +242,15 @@ func resourceCosmosDbAccount() *pluginsdk.Resource { | |
}, | ||
}, | ||
|
||
// TODO: 4.0 - set the default to Tls12 | ||
// per Microsoft's documentation, as of April 1 2023 the default minimal TLS version for all new accounts is 1.2 | ||
"minimal_tls_version": { | ||
Type: pluginsdk.TypeString, | ||
Optional: true, | ||
Computed: true, | ||
ValidateFunc: validation.StringInSlice(cosmosdb.PossibleValuesForMinimalTlsVersion(), false), | ||
}, | ||
|
||
"create_mode": { | ||
Type: pluginsdk.TypeString, | ||
Optional: true, | ||
|
@@ -852,6 +861,7 @@ func resourceCosmosDbAccountCreate(d *pluginsdk.ResourceData, meta interface{}) | |
ConsistencyPolicy: expandAzureRmCosmosDBAccountConsistencyPolicy(d), | ||
Locations: geoLocations, | ||
Capabilities: capabilities, | ||
MinimalTlsVersion: pointer.To(cosmosdb.MinimalTlsVersion(d.Get("minimal_tls_version").(string))), | ||
VirtualNetworkRules: expandAzureRmCosmosDBAccountVirtualNetworkRules(d), | ||
EnableMultipleWriteLocations: utils.Bool(enableMultipleWriteLocations), | ||
EnablePartitionMerge: pointer.To(partitionMergeEnabled), | ||
|
@@ -929,6 +939,15 @@ func resourceCosmosDbAccountCreate(d *pluginsdk.ResourceData, meta interface{}) | |
return fmt.Errorf("creating %s: %+v", id, err) | ||
} | ||
|
||
// NOTE: this is to work around the issue here: https://github.com/Azure/azure-rest-api-specs/issues/27596 | ||
// Once the above issue is resolved we shouldn't need this check and update anymore | ||
if d.Get("create_mode").(string) == string(cosmosdb.CreateModeRestore) { | ||
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. Is it easy to add a test for this? I think it might be better to pull this out into it's own PR 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. It would be difficult to pull out into it's own PR because it requires It's kind of a chicken/egg scenario, since without Minimal TLS, there's no action to be taken because the value won't be in the state. So far as writing a test, I believe might it is possible(checking state twice within a single action?) , but we would be checking for the presence of a bug. It would just be checking that the TLS version was first incorrectly set, then correctly set. This might work, but as soon as MS fixes it on their end, that test would break. |
||
err = resourceCosmosDbAccountApiCreateOrUpdate(client, ctx, id, account, d) | ||
if err != nil { | ||
return fmt.Errorf("updating %s: %+v", id, err) | ||
} | ||
} | ||
|
||
d.SetId(id.ID()) | ||
|
||
return resourceCosmosDbAccountRead(d, meta) | ||
|
@@ -1051,7 +1070,7 @@ func resourceCosmosDbAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) | |
"capacity", "create_mode", "restore", "key_vault_key_id", "mongo_server_version", | ||
"public_network_access_enabled", "ip_range_filter", "offer_type", "is_virtual_network_filter_enabled", | ||
"kind", "tags", "enable_free_tier", "enable_automatic_failover", "analytical_storage_enabled", | ||
"local_authentication_disabled", "partition_merge_enabled") { | ||
"local_authentication_disabled", "partition_merge_enabled", "minimal_tls_version") { | ||
updateRequired = true | ||
} | ||
|
||
|
@@ -1085,6 +1104,7 @@ func resourceCosmosDbAccountUpdate(d *pluginsdk.ResourceData, meta interface{}) | |
IsVirtualNetworkFilterEnabled: isVirtualNetworkFilterEnabled, | ||
EnableFreeTier: enableFreeTier, | ||
EnableAutomaticFailover: enableAutomaticFailover, | ||
MinimalTlsVersion: pointer.To(cosmosdb.MinimalTlsVersion(d.Get("minimal_tls_version").(string))), | ||
Capabilities: capabilities, | ||
ConsistencyPolicy: expandAzureRmCosmosDBAccountConsistencyPolicy(d), | ||
Locations: cosmosLocations, | ||
|
@@ -1381,6 +1401,7 @@ func resourceCosmosDbAccountRead(d *pluginsdk.ResourceData, meta interface{}) er | |
d.Set("analytical_storage_enabled", props.EnableAnalyticalStorage) | ||
d.Set("public_network_access_enabled", pointer.From(props.PublicNetworkAccess) == cosmosdb.PublicNetworkAccessEnabled) | ||
d.Set("default_identity_type", props.DefaultIdentity) | ||
d.Set("minimal_tls_version", pointer.From(props.MinimalTlsVersion)) | ||
d.Set("create_mode", pointer.From(props.CreateMode)) | ||
d.Set("partition_merge_enabled", pointer.From(props.EnablePartitionMerge)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -224,6 +224,28 @@ func TestAccCosmosDBAccount_updateTagsWithUserAssignedDefaultIdentity(t *testing | |
}) | ||
} | ||
|
||
func TestAccCosmosDBAccount_minimalTlsVersion(t *testing.T) { | ||
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. This value is update-able so we should test update in this test 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. Added a test for updates |
||
data := acceptance.BuildTestData(t, "azurerm_cosmosdb_account", "test") | ||
r := CosmosDBAccountResource{} | ||
|
||
data.ResourceTest(t, r, []acceptance.TestStep{ | ||
{ | ||
Config: r.basicMinimalTlsVersion(data, cosmosdb.MinimalTlsVersionTls), | ||
Check: acceptance.ComposeAggregateTestCheckFunc( | ||
check.That(data.ResourceName).Key("minimal_tls_version").HasValue("Tls"), | ||
), | ||
}, | ||
data.ImportStep(), | ||
{ | ||
Config: r.basicMinimalTlsVersion(data, cosmosdb.MinimalTlsVersionTlsOneOne), | ||
Check: acceptance.ComposeAggregateTestCheckFunc( | ||
check.That(data.ResourceName).Key("minimal_tls_version").HasValue("Tls11"), | ||
), | ||
}, | ||
data.ImportStep(), | ||
}) | ||
} | ||
|
||
func TestAccCosmosDBAccount_updateDefaultIdentity(t *testing.T) { | ||
data := acceptance.BuildTestData(t, "azurerm_cosmosdb_account", "test") | ||
r := CosmosDBAccountResource{} | ||
|
@@ -1295,6 +1317,7 @@ func TestAccCosmosDBAccount_restoreCreateMode(t *testing.T) { | |
Config: r.restoreCreateMode(data, cosmosdb.DatabaseAccountKindMongoDB, cosmosdb.DefaultConsistencyLevelSession), | ||
Check: acceptance.ComposeAggregateTestCheckFunc( | ||
checkAccCosmosDBAccount_basic(data, cosmosdb.DefaultConsistencyLevelSession, 1), | ||
check.That(data.ResourceName).Key("minimal_tls_version").HasValue("Tls12"), | ||
), | ||
}, | ||
data.ImportStep(), | ||
|
@@ -1452,6 +1475,37 @@ resource "azurerm_cosmosdb_account" "test" { | |
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, string(kind), string(consistency)) | ||
} | ||
|
||
func (CosmosDBAccountResource) basicMinimalTlsVersion(data acceptance.TestData, tls cosmosdb.MinimalTlsVersion) string { | ||
return fmt.Sprintf(` | ||
provider "azurerm" { | ||
features {} | ||
} | ||
|
||
resource "azurerm_resource_group" "test" { | ||
name = "acctestRG-cosmos-%d" | ||
location = "%s" | ||
} | ||
|
||
resource "azurerm_cosmosdb_account" "test" { | ||
name = "acctest-ca-%d" | ||
location = azurerm_resource_group.test.location | ||
resource_group_name = azurerm_resource_group.test.name | ||
offer_type = "Standard" | ||
kind = "GlobalDocumentDB" | ||
minimal_tls_version = "%s" | ||
|
||
consistency_policy { | ||
consistency_level = "Eventual" | ||
} | ||
|
||
geo_location { | ||
location = azurerm_resource_group.test.location | ||
failover_priority = 0 | ||
} | ||
} | ||
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, string(tls)) | ||
} | ||
|
||
func (CosmosDBAccountResource) basicMongoDB(data acceptance.TestData, consistency cosmosdb.DefaultConsistencyLevel) string { | ||
return fmt.Sprintf(` | ||
provider "azurerm" { | ||
|
@@ -4133,6 +4187,7 @@ resource "azurerm_cosmosdb_account" "test1" { | |
resource_group_name = azurerm_resource_group.test.name | ||
offer_type = "Standard" | ||
kind = "MongoDB" | ||
minimal_tls_version = "Tls12" | ||
|
||
capabilities { | ||
name = "EnableMongo" | ||
|
@@ -4166,7 +4221,15 @@ resource "azurerm_cosmosdb_mongo_collection" "test" { | |
|
||
index { | ||
keys = ["_id"] | ||
unique = true | ||
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 you share why this was flipped from true to false? It looks like the following is needed but I'm wondering why this needed to be flipped in the first place
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. My mistake for altering the original value. In the context of the test, it doesn't make a difference. The test itself fails due to
I known @rizkybiz as well as @katbyte both ran into issues on this particular test as well. Risking this PR becoming stuck due to another resource misbehaving, I opted to ignore the changing property. 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. So weird! But it does look it's not failing because of these changes! Thanks for following up! |
||
unique = false | ||
} | ||
|
||
// indices can cause test to be inconsistent | ||
// I believe there is a bug within the azurerm_cosmosdb_mongo_collection that causes inconsistent results on read | ||
lifecycle { | ||
ignore_changes = [ | ||
index | ||
] | ||
} | ||
} | ||
|
||
|
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.
Can we add a todo for 4.0 to add this in as a default?
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.
done