Add security center setting resource #8783
Conversation
ghost
added
beandrad
force-pushed
the
8679-asc-settings
branch
2 times, most recently
from
9b4239b
to
fd82d6a
Compare
There was a problem hiding this comment.
@beandrad Thanks for this PR! This mostly looks good to me. Just a few minor comments, if we can fix these up this looks ok to merge 👍
| features {} | ||
| } | ||
|
|
||
| resource "azurerm_security_center_setting" "example" { |
There was a problem hiding this comment.
To match the resource name in the tests
| resource "azurerm_security_center_setting" "example" { | |
| resource "azurerm_security_center_setting" "test" { |
| resource "azurerm_security_center_setting" "example" { | ||
| setting_name = "MCAS" | ||
| enabled = true | ||
| } |
There was a problem hiding this comment.
Could you add a newline at the end of this file?
| Create: schema.DefaultTimeout(60 * time.Minute), | ||
| Read: schema.DefaultTimeout(5 * time.Minute), | ||
| Update: schema.DefaultTimeout(60 * time.Minute), | ||
| Delete: schema.DefaultTimeout(60 * time.Minute), |
There was a problem hiding this comment.
Could these timeouts be shorter?
There was a problem hiding this comment.
Good question, how do you assess the value that the timeouts should have? In general, creating a resource doesn't take 30-60 mins, however, most of the resources seem to have this timeout.
There was a problem hiding this comment.
I don't believe there is a set formula. We err on the side of caution, you might surprised how long operations can sometimes take :D
Given this is (I think?) a straightforward logical setting, keeping it down to 5-10 mins should be safe?
| } | ||
|
|
||
| if _, err := client.Update(ctx, settingName, setting); err != nil { | ||
| return fmt.Errorf("Error creating/updating Security Center pricing: %+v", err) |
There was a problem hiding this comment.
The string "Error" is implicit
| return fmt.Errorf("Error creating/updating Security Center pricing: %+v", err) | |
| return fmt.Errorf("creating/updating Security Center pricing: %+v", err) |
| // https://github.com/Azure/azure-sdk-for-go/issues/12687 | ||
| resp, err := azuresdkhacks.GetSecurityCenterSetting(client, ctx, settingName) | ||
| if err != nil { | ||
| return fmt.Errorf("Error reading Security Center setting: %+v", err) |
There was a problem hiding this comment.
As above
| return fmt.Errorf("Error reading Security Center setting: %+v", err) | |
| return fmt.Errorf("reading Security Center setting: %+v", err) |
| // https://github.com/Azure/azure-sdk-for-go/issues/12687 (`Enabled` field missing) | ||
| resp, err := azuresdkhacks.GetSecurityCenterSetting(client, ctx, id.SettingName) | ||
| if err != nil { | ||
| return fmt.Errorf("Error reading Security Center setting: %+v", err) |
There was a problem hiding this comment.
As above
| return fmt.Errorf("Error reading Security Center setting: %+v", err) | |
| return fmt.Errorf("reading Security Center setting: %+v", err) |
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("Error reading Security Center setting: %+v", err) |
There was a problem hiding this comment.
As above
| return fmt.Errorf("Error reading Security Center setting: %+v", err) | |
| return fmt.Errorf("reading Security Center setting: %+v", err) |
azurerm/internal/services/securitycenter/azuresdkhacks/security_center_setting.go
Show resolved
Hide resolved
azurerm/internal/services/securitycenter/resource_arm_security_center_setting.go
Show resolved
Hide resolved
beandrad
force-pushed
the
8679-asc-settings
branch
from
fd82d6a
to
cedd4a2
Compare
So that we can disable and/or enabled the access of Microsoft App Security (MCAS) and Windows Defender ATP (WDATP) to the subscription data.
beandrad
force-pushed
the
8679-asc-settings
branch
from
cedd4a2
to
5fd92cc
Compare
|
Test result:
|
|
This has been released in version 2.32.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.32.0"
}
# ... other configuration ... |
|
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! |
So that we can disable and/or enabled the access of Microsoft
App Security (MCAS) and Windows Defender ATP (WDATP) to the subscription
data.
Fixes #8679