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

virtual_machine_scale_set datasource - support network_interfaces #10585

Conversation

shurikk
Copy link
Contributor

@shurikk shurikk commented Feb 15, 2021

Knowing scale set network_profile is important when creating load balancers as a separate process/state because the scale set and the load balancer must be on the same virtual network.

@ghost ghost added the size/XS label Feb 15, 2021
@shurikk shurikk marked this pull request as draft February 15, 2021 02:25
@katbyte katbyte added enhancement service/vmss Virtual Machine Scale Sets labels Apr 6, 2021
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 pr @shurikk - could we update the docs to reflect this new property? thanks

@shurikk
Copy link
Contributor Author

shurikk commented Apr 15, 2021

yes, will do, was planning to but got distracted

@ghost ghost added size/S documentation and removed size/XS labels Apr 17, 2021
@shurikk shurikk marked this pull request as ready for review April 17, 2021 22:19
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 @shurikk - overall this looks good but instead of referring to the schema in another resource could we pull that out? I think the new windows|linux virtual resources already have done that so you should be able to use "network_interface": VirtualMachineScaleSetNetworkInterfaceSchema(),

return &schema.Schema{
Type: schema.TypeSet,
Computed: true,
Elem: resourceVirtualMachineScaleSet().Schema["network_profile"].Elem,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should pull that out into a shared schema function instead of doing this

…ta_source.go

Co-authored-by: kt <kt@katbyte.me>
@shurikk
Copy link
Contributor Author

shurikk commented Apr 26, 2021

@katbyte just to be on the same page, we want this change to be aligned with linux_virtual_machine_scale_set and windows_virtual_machine_scale_set rather than virtual_machine_scale_set? If yes, then the docs should be updated as well.

perhaps new data sources should be introduced rather than adding new properties to the virtual_machine_scale_set?

@ghost ghost removed the waiting-response label Apr 26, 2021
@katbyte
Copy link
Collaborator

katbyte commented Apr 27, 2021

@shurikk - correct, those are the new resources that will replace the old virtual_machine_scale_set. they share the VirtualMachineScaleSetNetworkInterfaceSchema so it should be fine to add this to the virtual_machine_scale_set data source

@shurikk
Copy link
Contributor Author

shurikk commented Apr 27, 2021

@katbyte updated, still need schemaNetworkInterfaceForDataSource to set Computed: true, cannot use VirtualMachineScaleSetNetworkInterfaceSchema() directly without that. Perhaps there is a better way?

e.g.

data azurerm_virtual_machine_scale_set vmss {
  name                = "test-vmss"
  resource_group_name = "test-group"
}

output test {
  value = data.azurerm_virtual_machine_scale_set.vmss
}

results in

Error: "network_interface": required field is not set                                                                                                         
                                                                                                                                                              
  on main.tf line 18, in data "azurerm_virtual_machine_scale_set" "vmss":                                                                                     
  18: data azurerm_virtual_machine_scale_set vmss {

@katbyte
Copy link
Collaborator

katbyte commented Apr 27, 2021

oh right 🤦‍♀️🤦‍♀️ sorry i totally blanked on this, this is a datasource so we need to have everything be computed, which means copying that function's schema and changing everything to be computed - however you should beable to reuse the flatten function

@shurikk
Copy link
Contributor Author

shurikk commented Apr 27, 2021

@katbyte how should we proceed from here? I've updated docs and the code to be aligned with linux and windows scale sets.

@ghost ghost removed the waiting-response label Apr 27, 2021
@shurikk
Copy link
Contributor Author

shurikk commented Apr 27, 2021

config

data azurerm_virtual_machine_scale_set vmss {
  name                = "test-vmss"
  resource_group_name = "test"
}

output x { 
  value = data.azurerm_virtual_machine_scale_set.vmss
}

terraform output

x = {
  "id" = "/subscriptions/.../.../Microsoft.Compute/virtualMachineScaleSets/test-vmss"
  "identity" = tolist([])
  "location" = tostring(null)
  "name" = "test-vmss"
  "network_interface" = tolist([
    {
      "dns_servers" = tolist([])
      "enable_accelerated_networking" = true
      "enable_ip_forwarding" = false
      "ip_configuration" = tolist([
        {
          "application_gateway_backend_address_pool_ids" = toset([])
          "application_security_group_ids" = toset([])
          "load_balancer_backend_address_pool_ids" = toset([
            "/subscriptions/.../.../backendAddressPools/test-lb",
          ])
          "load_balancer_inbound_nat_rules_ids" = toset([])
          "name" = "test-vmss"
          "primary" = true
          "public_ip_address" = tolist([])
          "subnet_id" = "/subscriptions/.../.../subnets/subnet12345"
          "version" = "IPv4"
        },
      ])
      "name" = "test-vmss"
      "network_security_group_id" = ""
      "primary" = true
    },
  ])
  "resource_group_name" = "test"
  "timeouts" = null /* object */
}

@ghost ghost added size/L and removed size/S labels Apr 27, 2021
@katbyte katbyte changed the title add network_profile to virtual_machine_scale_set data virtual_machine_scale_set datasource - support network_interfaces Apr 27, 2021
@katbyte
Copy link
Collaborator

katbyte commented Apr 27, 2021

@shurikk - i hope you don't mind but I just pushed the changes required to get this merged, i'll have someone else review it and get it into this weeks release 🙂

@shurikk
Copy link
Contributor Author

shurikk commented Apr 27, 2021

@shurikk - i hope you don't mind but I just pushed the changes required to get this merged, i'll have someone else review it and get it into this weeks release 🙂

thanks, works, it's not super urgent. also noticed some doc updates, still work in progress?

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@katbyte, thank you for this PR... I left one super minor comment and there are a few lint errors. Other than that this LGTM! 🚀

…ta_source.go

Co-authored-by: WS <20408400+WodansSon@users.noreply.github.com>
Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@katbyte katbyte merged commit d23845d into hashicorp:master Apr 28, 2021
katbyte added a commit that referenced this pull request Apr 28, 2021
alvintang pushed a commit to alvintang/terraform-provider-azurerm that referenced this pull request Apr 29, 2021
…shicorp#10585)

Co-authored-by: kt <kt@katbyte.me>
Co-authored-by: WS <20408400+WodansSon@users.noreply.github.com>
Co-authored-by: Alexander Kabanov <akabanov@linkedin.com>
alvintang pushed a commit to alvintang/terraform-provider-azurerm that referenced this pull request Apr 29, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 2.57.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.57.0"
}
# ... other configuration ...

@shurikk shurikk deleted the add-network-profile-to-virtual-machine-scale-set-data branch April 30, 2021 14:41
Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

Shall we change the schemas above to be Computed: true?

func virtualMachineScaleSetIPConfigurationSchemaForDataSource() *schema.Schema {
return &schema.Schema{
Type: schema.TypeList,
Required: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to Computed: true?

func virtualMachineScaleSetPublicIPAddressSchemaForDataSource() *schema.Schema {
return &schema.Schema{
Type: schema.TypeList,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to Computed: true?

@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 Jun 14, 2021
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.

5 participants