-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add security center setting resource #8783
Conversation
9b4239b
to
fd82d6a
Compare
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these timeouts be shorter?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
cedd4a2
to
5fd92cc
Compare
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 👉 [email protected]. 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