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_security_center_server_vulnerability_assessment - deprecate in favour of azurerm_security_center_vm_server_vulnerability_assessment #15747

Merged
merged 11 commits into from
Mar 23, 2022

Conversation

catriona-m
Copy link
Member

No description provided.

Comment on lines 49 to 50
"azurerm_security_center_vm_server_vulnerability_assessment": resourceVMServerVulnerabilityAssessment(),
"azurerm_security_center_hybrid_vm_server_vulnerability_assessment": resourceHybridVMServerVulnerabilityAssessment(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it make sense to name these

Suggested change
"azurerm_security_center_vm_server_vulnerability_assessment": resourceVMServerVulnerabilityAssessment(),
"azurerm_security_center_hybrid_vm_server_vulnerability_assessment": resourceHybridVMServerVulnerabilityAssessment(),
"azurerm_security_center_vm_vulnerability_assessment_server": resourceVMServerVulnerabilityAssessment(),
"azurerm_security_center_server_vulnerability_assessment_hybrid_vm": resourceHybridVMServerVulnerabilityAssessment(),

so its service/thing/catagory of thing and when sorting things alphabetically they end up next to each other?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea - changed now!

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @catriona-m

Thanks for pushing those changes - I've taken a look through and left some comments inline but this is otherwise looking good 👍

Thanks!

"azurerm_security_center_automation": resourceSecurityCenterAutomation(),
"azurerm_security_center_auto_provisioning": resourceSecurityCenterAutoProvisioning(),
"azurerm_security_center_server_vulnerability_assessment": resourceServerVulnerabilityAssessment(),
"azurerm_security_center_server_vulnerability_assessment_hybrid_vm": resourceServerVulnerabilityAssessmentHybridVM(),
Copy link
Contributor

Choose a reason for hiding this comment

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

"Hybrid" is actually "Arc", so this wants to be:

Suggested change
"azurerm_security_center_server_vulnerability_assessment_hybrid_vm": resourceServerVulnerabilityAssessmentHybridVM(),
"azurerm_security_center_server_vulnerability_arc_virtual_machine_assessment": resourceServerVulnerabilityAssessmentHybridVM(),

"azurerm_security_center_auto_provisioning": resourceSecurityCenterAutoProvisioning(),
"azurerm_security_center_server_vulnerability_assessment": resourceServerVulnerabilityAssessment(),
"azurerm_security_center_server_vulnerability_assessment_hybrid_vm": resourceServerVulnerabilityAssessmentHybridVM(),
"azurerm_security_center_server_vulnerability_assessment_vm": resourceServerVulnerabilityAssessmentVM(),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we spell this out a little:

Suggested change
"azurerm_security_center_server_vulnerability_assessment_vm": resourceServerVulnerabilityAssessmentVM(),
"azurerm_security_center_server_vulnerability_virtual_machine_assessment": resourceServerVulnerabilityAssessmentVM(),

Comment on lines 31 to 34
// TODO: replace this with an importer which validates the ID during import
Importer: &schema.ResourceImporter{
StateContext: schema.ImportStatePassthroughContext,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add a custom importer which validates the Resource ID and confirms that this is a Hybrid VM? I can't make a suggestion for some reason, but that'd be pluginsdk.ImporterValidatingResourceId

Comment on lines 63 to 81
vulnerabilityAssessment, err := client.Get(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
if !utils.ResponseWasNotFound(vulnerabilityAssessment.Response) {
return fmt.Errorf("checking for presence of existing Advanced Threat Protection for %s: %+v", *hybridMachineId, err)
}
}

if vulnerabilityAssessment.ID != nil && *vulnerabilityAssessment.ID != "" {
return tf.ImportAsExistsError("azurerm_security_center_server_vulnerability_assessment_hybrid_vm", *vulnerabilityAssessment.ID)
}

vulnerabilityAssessment, err = client.CreateOrUpdate(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
return fmt.Errorf("create Server Vulnerability Assessment for %s: %+v", *hybridMachineId, err)
}

if vulnerabilityAssessment.ID != nil {
d.SetId(*vulnerabilityAssessment.ID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than using the Resource ID returned from the API here can we instead define a Resource ID (resourceids.go) and use that here instead?

Comment on lines 97 to 102
hybridMachineId, err := computeParse.HybridMachineID(d.Get("hybrid_machine_id").(string))
if err != nil {
return err
}

resp, err := client.Get(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
Copy link
Contributor

Choose a reason for hiding this comment

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

(similar to above) can we use the HybridVM Resource ID and then parse d.ID() instead?

We can't guarantee in the Read function that the schema fields are available, so there's a crash here

return fmt.Errorf("retrieving Server Vulnerability Assessment status for %s: %+v", *virtualMachineId, err)
}

d.SetId(*resp.ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above)

Suggested change
d.SetId(*resp.ID)

ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d)
defer cancel()

virtualMachineId, err := computeParse.VirtualMachineID(d.Get("virtual_machine_id").(string))
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above)

Comment on lines 128 to 135
// Can not delete if still in provisioning state. Wait for it to complete.
for retry := 1; retry <= 20; retry++ {
response, err := client.Get(ctx, virtualMachineId.ResourceGroup, computeProvider, vmType, virtualMachineId.Name)
if err != nil || response.ProvisioningState != security.ProvisioningState1Provisioning {
break
}
time.Sleep(time.Duration(retry) * time.Second)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above)

time.Sleep(time.Duration(retry) * time.Second)
}

future, err := client.Delete(ctx, virtualMachineId.ResourceGroup, computeProvider, vmType, virtualMachineId.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above)

Manages an Azure Server Vulnerability Assessment (Qualys) to a VM.

-> **NOTE** Azure Defender has to be enabled on the subscription in order for this resource to work.
See this [documentation](https://docs.microsoft.com/en-us/azure/security-center/security-center-get-started) to get started.
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw if we remove the language here it'll redirect to the users locale if available in that language

Suggested change
See this [documentation](https://docs.microsoft.com/en-us/azure/security-center/security-center-get-started) to get started.
See this [documentation](https://docs.microsoft.com/azure/security-center/security-center-get-started) to get started.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @catriona-m

Thanks for pushing those changes - I've taken a look through and left some more comments inline, most of them are similar between the two resources but if we can fix those up then this should otherwise be good to go 👍

Thanks!

Comment on lines 63 to 92
vulnerabilityAssessment, err := client.Get(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
if !utils.ResponseWasNotFound(vulnerabilityAssessment.Response) {
return fmt.Errorf("checking for presence of existing Advanced Threat Protection for %s: %+v", *hybridMachineId, err)
}
}

if vulnerabilityAssessment.ID != nil && *vulnerabilityAssessment.ID != "" {
return tf.ImportAsExistsError("azurerm_security_center_server_vulnerability_assessment_arc_virtual_machine", *vulnerabilityAssessment.ID)
}

vulnerabilityAssessment, err = client.CreateOrUpdate(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
return fmt.Errorf("create Server Vulnerability Assessment for %s: %+v", *hybridMachineId, err)
}

id := parse.NewVulnerabilityAssessmentVmID(hybridMachineId.SubscriptionId, hybridMachineId.ResourceGroup, hybridMachineId.MachineName, "Default")
d.SetId(id.ID())

timeout, _ := ctx.Deadline()
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Succeeded"},
Refresh: serverVulnerabilityAssessmentArcVirtualMachineStateRefreshFunc(ctx, client, id.ResourceGroup, id.VirtualMachineName),
PollInterval: 10 * time.Second,
Timeout: time.Until(timeout),
}

if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("waiting for the completion of the creating/updating of %s: %+v", id, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A few things to note here:

  1. The Resource ID defined in TF should be defined at the top so it can be used through the other items (error messages, requires import etc)
  2. The ID shouldn't be set until we're sure the Resource has been created (consistency with other resources)
  3. We can lean on the ID for error messages here fwiw
Suggested change
vulnerabilityAssessment, err := client.Get(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
if !utils.ResponseWasNotFound(vulnerabilityAssessment.Response) {
return fmt.Errorf("checking for presence of existing Advanced Threat Protection for %s: %+v", *hybridMachineId, err)
}
}
if vulnerabilityAssessment.ID != nil && *vulnerabilityAssessment.ID != "" {
return tf.ImportAsExistsError("azurerm_security_center_server_vulnerability_assessment_arc_virtual_machine", *vulnerabilityAssessment.ID)
}
vulnerabilityAssessment, err = client.CreateOrUpdate(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
return fmt.Errorf("create Server Vulnerability Assessment for %s: %+v", *hybridMachineId, err)
}
id := parse.NewVulnerabilityAssessmentVmID(hybridMachineId.SubscriptionId, hybridMachineId.ResourceGroup, hybridMachineId.MachineName, "Default")
d.SetId(id.ID())
timeout, _ := ctx.Deadline()
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Succeeded"},
Refresh: serverVulnerabilityAssessmentArcVirtualMachineStateRefreshFunc(ctx, client, id.ResourceGroup, id.VirtualMachineName),
PollInterval: 10 * time.Second,
Timeout: time.Until(timeout),
}
if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("waiting for the completion of the creating/updating of %s: %+v", id, err)
}
id := parse.NewVulnerabilityAssessmentVmID(hybridMachineId.SubscriptionId, hybridMachineId.ResourceGroup, hybridMachineId.MachineName, "Default")
vulnerabilityAssessment, err := client.Get(ctx, id.ResourceGroup, hybridProvider, hybridType, id.MachineName)
if err != nil {
if !utils.ResponseWasNotFound(vulnerabilityAssessment.Response) {
return fmt.Errorf("checking for presence of existing %s: %+v", id, err)
}
}
if !utils.ResponseWasNotFound(vulnerabilityAssessment.Response) {
return tf.ImportAsExistsError("azurerm_security_center_server_vulnerability_assessment_arc_virtual_machine", id.ID())
}
vulnerabilityAssessment, err = client.CreateOrUpdate(ctx, hybridMachineId.ResourceGroup, hybridProvider, hybridType, hybridMachineId.MachineName)
if err != nil {
return fmt.Errorf("creating %s: %+v", id, err)
}
timeout, _ := ctx.Deadline()
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Succeeded"},
Refresh: serverVulnerabilityAssessmentArcVirtualMachineStateRefreshFunc(ctx, client, id.ResourceGroup, id.VirtualMachineName),
PollInterval: 10 * time.Second,
Timeout: time.Until(timeout),
}
if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("waiting for the completion of the creating/updating of %s: %+v", id, err)
}
d.SetId(id.ID())

Comment on lines 108 to 118
var hybridMachineId *computeParse.HybridMachineId
if id.ID() != "" {
newHybridMachineId := computeParse.NewHybridMachineID(id.SubscriptionId, id.ResourceGroup, id.VirtualMachineName)
hybridMachineId = &newHybridMachineId
} else {
newHybridMachineId, err := computeParse.HybridMachineID(d.Get("hybrid_machine_id").(string))
if err != nil {
return err
}
hybridMachineId = newHybridMachineId
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the Read function should only be using the ID field, other fields in the state can't be guaranteed (import/read time)

Suggested change
var hybridMachineId *computeParse.HybridMachineId
if id.ID() != "" {
newHybridMachineId := computeParse.NewHybridMachineID(id.SubscriptionId, id.ResourceGroup, id.VirtualMachineName)
hybridMachineId = &newHybridMachineId
} else {
newHybridMachineId, err := computeParse.HybridMachineID(d.Get("hybrid_machine_id").(string))
if err != nil {
return err
}
hybridMachineId = newHybridMachineId
}
hybridMachineId := computeParse.NewHybridMachineID(id.SubscriptionId, id.ResourceGroup, id.VirtualMachineName)

Comment on lines 147 to 159
// Cannot delete if still in provisioning state. Wait for it to complete.
timeout, _ := ctx.Deadline()
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Succeeded"},
Refresh: serverVulnerabilityAssessmentArcVirtualMachineStateRefreshFunc(ctx, client, id.ResourceGroup, id.VirtualMachineName),
PollInterval: 10 * time.Second,
Timeout: time.Until(timeout),
}

if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("waiting for the completion of the creation of %s: %+v", id, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this would indicate a bug elsewhere we need to track down, so this needs to be removed:

Suggested change
// Cannot delete if still in provisioning state. Wait for it to complete.
timeout, _ := ctx.Deadline()
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Succeeded"},
Refresh: serverVulnerabilityAssessmentArcVirtualMachineStateRefreshFunc(ctx, client, id.ResourceGroup, id.VirtualMachineName),
PollInterval: 10 * time.Second,
Timeout: time.Until(timeout),
}
if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("waiting for the completion of the creation of %s: %+v", id, err)
}

Comment on lines +37 to +39
Create: pluginsdk.DefaultTimeout(5 * time.Minute),
Read: pluginsdk.DefaultTimeout(5 * time.Minute),
Delete: pluginsdk.DefaultTimeout(10 * time.Minute),
Copy link
Contributor

Choose a reason for hiding this comment

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

just double-checking: given the polling below are these times accurate?


The following arguments are supported:

* `hybrid_machine_id` - (Required) The ID of the Azure ARC server to be monitored by vulnerability assessment. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `hybrid_machine_id` - (Required) The ID of the Azure ARC server to be monitored by vulnerability assessment. Changing this forces a new resource to be created.
* `hybrid_machine_id` - (Required) The ID of the Arc Virtual Machine which should have a Vulnerability Assessment configured. Changing this forces a new resource to be created.

Comment on lines 35 to 47
* `id` - The ID of the Vulnerability Assessment resource.

## Timeouts

The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions:

* `create` - (Defaults to 5 minutes) Used when creating the Advanced Threat Protection.
* `read` - (Defaults to 5 minutes) Used when retrieving the Advanced Threat Protection.
* `delete` - (Defaults to 10 minutes) Used when deleting the Advanced Threat Protection.

## Import

Server Vulnerability Assessments can be imported using the `resource id`, e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

there's three different names listed here, can we standardize this on "Arc Virtual Machine Server Vulnerability Assessment"?

Comment on lines 84 to 102
* `virtual_machine_id` - (Required) The ID of the virtual machine to be monitored by vulnerability assessment. Changing this forces a new resource to be created.

## Attributes Reference

In addition to all arguments above, the following attributes are exported:

* `id` - The ID of the Vulnerability Assessment resource.

## Timeouts

The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions:

* `create` - (Defaults to 5 minutes) Used when creating the Advanced Threat Protection.
* `read` - (Defaults to 5 minutes) Used when retrieving the Advanced Threat Protection.
* `delete` - (Defaults to 10 minutes) Used when deleting the Advanced Threat Protection.

## Import

Server Vulnerability Assessments can be imported using the `resource id`, e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) can we make these names consistent?

Comment on lines 6 to 11
Manages an Azure Vulnerability Assessment (Qualys) to a VM.
---

# azurerm_security_center_vm_server_vulnerability_assessment

Manages an Azure Server Vulnerability Assessment (Qualys) to a VM.
Copy link
Contributor

Choose a reason for hiding this comment

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

we expand vm to virtual_machine fwiw

Manages an Azure Server Vulnerability Assessment (Qualys) to a VM.

-> **NOTE** Azure Defender has to be enabled on the subscription in order for this resource to work.
See this [documentation](https://docs.microsoft.com/en-us/azure/security-center/security-center-get-started) to get started.
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) removing the language here will redirect to the article in the users locale if available

Suggested change
See this [documentation](https://docs.microsoft.com/en-us/azure/security-center/security-center-get-started) to get started.
See this [documentation](https://docs.microsoft.com/azure/security-center/security-center-get-started) to get started.

@tombuildsstuff
Copy link
Contributor

Rebased.

…lity_assessment_arc_virtual_machine` for the moment

We can't currently test `security_center_server_vulnerability_assessment_arc_virtual_machine` as it requires provisioning
a Hybrid Virtual Machine which we don't currently support - so I've opened #15990
to track support for that.
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Left a couple of comments that I've pushed a couple of commits for - but this otherwise LGTM 👍

I've removed the azurerm_security_center_server_vulnerability_assessment_arc_virtual_machine resource since we can't current provision Hybrid Virtual Machines (which is a dependency for testing this) - support for which is being tracked in #15990

@tombuildsstuff tombuildsstuff requested a review from katbyte March 23, 2022 21:10
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.

LGTM ☔

@tombuildsstuff tombuildsstuff changed the title azurerm_security_center_server_vulnerability_assessment - deprecate in favour of azurerm_security_center_vm_server_vulnerability_assessment and azurerm_security_center_hybrid_vm_server_vulnerability_assessment azurerm_security_center_server_vulnerability_assessment - deprecate in favour of azurerm_security_center_vm_server_vulnerability_assessment Mar 23, 2022
@katbyte katbyte merged commit fbf0c92 into main Mar 23, 2022
@katbyte katbyte deleted the cm/split_sc_vuln branch March 23, 2022 21:47
katbyte added a commit that referenced this pull request Mar 23, 2022
@github-actions
Copy link

This functionality has been released in v3.0.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2022
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.

3 participants