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_sql_server - support for the extended_auditing_policy property #5036

Merged
merged 32 commits into from Mar 9, 2020

Conversation

yupwei68
Copy link
Contributor

@yupwei68 yupwei68 commented Dec 2, 2019

1.To add block code blob_extended_auditing_policy in resource_arm_sql_server to permit blob auditing policy stored in storage account.

  1. The features in blob_auditing_policy are all inclusive in blob_extended_auditing_policy. And blob_extended_auditing_policy has one more feature predicate_expression

3.is_storage_secondary_key_in_use is available on portal(show as belows), so I have reserved this feature.
image
Fixes #2217

sql server auditing pass
2020.3.5
sql server auditing pass

@katbyte
Copy link
Collaborator

katbyte commented Dec 3, 2019

continuation of #4755

@tombuildsstuff
Copy link
Member

hi @yupwei68

Thanks for this PR.

Since there's still review comments from #4755 pending (which I'm assuming you're working through) I'm going to hold off reviewing this for the moment - once those have been addressed we'll take another look through :)

Thanks!

@yupwei68
Copy link
Contributor Author

yupwei68 commented Dec 4, 2019

Hi @tombuildsstuff , thanks for your comments. I have replied them in #4755. I have made the corresponding changes in this PR and DB PR #5049. Please review

@yupwei68
Copy link
Contributor Author

hi @tombuildsstuff , any problem?

@ghost ghost removed the waiting-response label Dec 11, 2019
@ghost ghost added size/XL and removed size/L labels Mar 5, 2020
@yupwei68
Copy link
Contributor Author

yupwei68 commented Mar 5, 2020

Hi @katbyte Thanks! Corresponding changes are pushed. I'll update sql database auditing after this is merged.

@ghost ghost removed the waiting-response label Mar 5, 2020
@yupwei68 yupwei68 requested a review from katbyte March 5, 2020 05:25
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.

Thanks for the changes @yupwei68, we're getting this close, left a couple more comments inline

azurerm/internal/services/sql/resource_arm_sql_server.go Outdated Show resolved Hide resolved
website/docs/r/sql_server.html.markdown Outdated Show resolved Hide resolved
website/docs/r/sql_server.html.markdown Outdated Show resolved Hide resolved
@yupwei68 yupwei68 requested a review from katbyte March 5, 2020 08:10
@yupwei68
Copy link
Contributor Author

yupwei68 commented Mar 5, 2020

Hi @katbyte Sure! Once merged, I shall update 'extending_auditing' for sql_db

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.

Thanks @yupwei68, this is real close now with just a couple minor comments left that need to be addressed before merge

go.sum Outdated Show resolved Hide resolved
Comment on lines +106 to +108
* `storage_endpoint` - (Required) Specifies the blob storage endpoint (e.g. https://MyAccount.blob.core.windows.net).

* `storage_account_access_key_is_secondary` - (Optional) Specifies whether `storage_account_access_key` value is the storage's secondary key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we order these alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't need to put required fields in advance? We just order all required and optional fields alphabetically, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

WHoops missed the optional, yes typically alphabetically, but sometimes i'll order for aesthetics ie storage_endpoint -> storage_account_access_key -> storage_account_access_key_is_secondary

Comment on lines 25 to 31
"storage_endpoint": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validate.URLIsHTTPS,
},

"storage_account_access_key_is_secondary": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we order these alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same concern. So we don't need to put required fields in advance? We just order all required and optional fields alphabetically, right?

@yupwei68 yupwei68 requested a review from katbyte March 9, 2020 09:59
@yupwei68
Copy link
Contributor Author

yupwei68 commented Mar 9, 2020

Hi @katbyte , some concerns of the last two comments are left. Looking forward to get this PR merged!^ ^

@ghost ghost removed the waiting-response label Mar 9, 2020
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.

Thank for the change @yupwei68! LGTM now 👍

@katbyte katbyte changed the title New block blob_extended_auditing_policy in resource: resource_arm_sql_server azurerm_sql_server - support for the extended_auditing_policy property Mar 9, 2020
@katbyte katbyte merged commit 4ddb0e0 into hashicorp:master Mar 9, 2020
katbyte added a commit that referenced this pull request Mar 9, 2020
r0bnet pushed a commit to r0bnet/terraform-provider-azurerm that referenced this pull request Mar 9, 2020
r0bnet pushed a commit to r0bnet/terraform-provider-azurerm that referenced this pull request Mar 9, 2020
@ghost
Copy link

ghost commented Mar 11, 2020

This has been released in version 2.1.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.1.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 9, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Apr 9, 2020
@yupwei68 yupwei68 deleted the wyp-sqlserverauditing branch July 10, 2020 02:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_sql_server: Audit Configuration
3 participants