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

Fixed transparentDataEncryption property status to state #1505

Merged
merged 5 commits into from
Jul 5, 2022
Merged

Fixed transparentDataEncryption property status to state #1505

merged 5 commits into from
Jul 5, 2022

Conversation

Dylan-Prins
Copy link
Contributor

PR Summary

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • Change is not breaking
  • This PR is ready to merge and is not Work in Progress
  • Rule changes
    • Unit tests created/ updated
    • Rule documentation created/ updated
    • Link to a filed issue
    • Change log has been updated with change under unreleased section
  • Other code changes
    • Unit tests created/ updated
    • Link to a filed issue
    • Change log has been updated with change under unreleased section

@Dylan-Prins Dylan-Prins requested a review from a team as a code owner July 4, 2022 20:50
@BernieWhite
Copy link
Collaborator

BernieWhite commented Jul 4, 2022

@Dylan-Prins Thanks for the PR. Looks like there is a difference between various API versions or the provider documentation is wrong. The newer use state where the older is status. If we can verify that status was an error, happy to accept your change. Otherwise we should update it to accept either status or state as Enabled you can do this with an if block or using AnyOf.

If the later, to get the tests to pass we need to add an entry for database-C that uses the state property here:

{
"Name": "database-B",
"ResourceId": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Sql/servers/server-A/databases/database-B",
"ResourceName": "server-A/database-B",
"ResourceType": "Microsoft.Sql/servers/databases",
"Kind": "v12.0,user",
"ResourceGroupName": "test-rg",
"Location": "region",
"SubscriptionId": "00000000-0000-0000-0000-000000000000",
"Properties": {
"edition": "Standard",
"status": "Online",
"serviceLevelObjective": "S2",
"collation": "SQL_Latin1_General_CP1_CI_AS",
"maxSizeBytes": "268435456000",
"requestedServiceObjectiveName": "S2",
"sampleName": null,
"defaultSecondaryLocation": "region-B",
"earliestRestoreDate": "2019-01-01T00:00:00Z",
"elasticPoolName": null,
"containmentState": 2,
"readScale": "Disabled",
"failoverGroupId": null,
"zoneRedundant": false,
"isUpgradeRequested": false
},
"resources": [
{
"ResourceId": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Sql/servers/server-A/databases/database-B/transparentDataEncryption/current",
"Id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Sql/servers/server-A/databases/database-B/transparentDataEncryption/current",
"Identity": null,
"Kind": null,
"Location": "region",
"ManagedBy": null,
"ResourceName": "current",
"Name": "current",
"ExtensionResourceName": null,
"ParentResource": null,
"Plan": null,
"Properties": {
"status": "Enabled"
},
"ResourceGroupName": "test-rg",
"Type": "Microsoft.Sql/servers/databases/transparentDataEncryption",
"ResourceType": "Microsoft.Sql/servers/databases/transparentDataEncryption",
"ExtensionResourceType": null,
"Sku": null,
"Tags": null,
"TagsTable": null,
"SubscriptionId": "00000000-0000-0000-0000-000000000000",
"CreatedTime": null,
"ChangedTime": null,
"ETag": null
}
]
}

2014-04-01

latest

@ms-sambell Did you notice something similar?

@Dylan-Prins
Copy link
Contributor Author

Dylan-Prins commented Jul 5, 2022 via email

@ghost
Copy link

ghost commented Jul 5, 2022

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@BernieWhite BernieWhite left a comment

Choose a reason for hiding this comment

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

@Dylan-Prins Nice work on the test resource. I think your suggestion is better to conditionally detect based on API version, but there is some specific cases around it.

Based on your suggestion, I am fine with removing database-C and just updating database-B with state as an export from Azure will be using a newer API version the specific case we are testing should always be state.

@@ -94,7 +94,7 @@ Rule 'Azure.SQL.TDE' -Ref 'AZR-000191' -Type 'Microsoft.Sql/servers/databases' -
return $Assert.Fail($LocalizedData.SubResourceNotFound, 'Microsoft.Sql/servers/databases/transparentDataEncryption');
}
foreach ($config in $configs) {
$Assert.HasFieldValue($config, 'Properties.status', 'Enabled');
$Assert.HasFieldValue($config, 'Properties.state', 'Enabled');
Copy link
Collaborator

Choose a reason for hiding this comment

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

The property apiVersion will not exist if exported with in-flight scenarios, but latest API version should be used. We only need to catch the case when older IaC is used.

Suggested change
$Assert.HasFieldValue($config, 'Properties.state', 'Enabled');
if ($Assert.HasFieldValue($config, 'apiVersion', '2014-04-01').Result) {
$Assert.HasFieldValue($config, 'Properties.status', 'Enabled');
}
else {
$Assert.HasFieldValue($config, 'Properties.state', 'Enabled');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have added the if statement, removed database-c and changed database-b to 'state'

Copy link
Collaborator

@BernieWhite BernieWhite left a comment

Choose a reason for hiding this comment

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

@Dylan-Prins Great, all good to go. I'll update the branch for main and merge. Thanks!

@BernieWhite BernieWhite reopened this Jul 5, 2022
@BernieWhite BernieWhite merged commit 8e23b3c into Azure:main Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants