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.

Sign up for GitHub

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

Jump to bottom

New Data Source: `azurerm_public_ips` #304

Merged

tombuildsstuff merged 15 commits into hashicorp:master from hbuckle:ds_public_ip_ids

Mar 7, 2018

Contributor

hbuckle commented Sep 4, 2017

Adds a new data source to retrieve the IDs of unassigned public IP addresses from a resource group. We have a pool of public IP addresses that have been pre-whitelisted by third parties so this will allow us to pick them up dynamically instead of needing to hardcode the IDs into our configuration.
This is my first time working with go and terraform so any feedback is appreciated!

Member

tombuildsstuff commented Sep 7, 2017

Thanks for opening this PR :)

Taking a look at this, I think this Data Source might be more useful were it to return a list of Public IP's (and most of their properties) rather than just their ID's. We can apply the same filter logic to this so that it's possible to return only Public IP's which aren't attached to a VM/LB etc (and thus your workflow should still work 😄) - but the advantage here would be that the Data Source would work for multiple use-cases, here's an example of what I mean:

data "azurerm_public_ips" "test" {
  resource_group_name = "foo"
  attached = false
}

output "public_ip_address_ids" {
  value = "${data.azurerm_public_ips.*.id}"
}

What do you think? :)

Thanks!

tombuildsstuff added enhancement new-data-source labels

Contributor Author

hbuckle commented Sep 10, 2017

I have started looking at adding more properties, the only issue is that DNS settings are optional for public IPs, and for dynamic IPs they will only have an IP address if they are assigned. What's the right way to handle this, just add empty values to the results?

Member

tombuildsstuff commented Oct 4, 2017

Sorry for the delayed response here - I'd missed we'd not replied to this!

I have started looking at adding more properties, the only issue is that DNS settings are optional for public IPs, and for dynamic IPs they will only have an IP address if they are assigned. What's the right way to handle this, just add empty values to the results?

So if there's no value we can either not set the object (in the case of a simple type like a String/Integer) - or set an empty value (in the case of a complex object). For instance, in the azurerm_public_ip resource we only set the fqdn if there's a value - otherwise (given it's a string) where it's not being set, it means that it'll get a default value of an empty string.

Hopefully that helps? :)

Thanks!

tombuildsstuff added the waiting-response label

Contributor Author

hbuckle commented Oct 8, 2017

I've updated the data source so that it returns a list of public IPs. Each public IP is represented by a map with the IP's properties.

tombuildsstuff removed the waiting-response label

Contributor Author

hbuckle commented Oct 31, 2017

Hi - does the updated version fit in with the suggested changes? Is it possible to merge?

Member

tombuildsstuff commented Nov 9, 2017

hey @hbuckle - sorry for the delayed review here - I'm taking another look at this now :)

tombuildsstuff previously requested changes

View reviewed changes

Member

tombuildsstuff left a comment

Apologies for the delay in reviewing this PR - I've taken a look through and left some comments inline.

Thanks!

azurerm/data_source_arm_public_ips.go Outdated

+              			"public_ips": {
+              				Type:     schema.TypeList,
+              				Computed: true,
+              				Elem:     &schema.Schema{Type: schema.TypeMap},

Member

tombuildsstuff Nov 9, 2017

Given this is a list - we need to specify each schema for the fields as in this example - could we update this to match?

azurerm/data_source_arm_public_ips.go Outdated

+              		if resp.StatusCode == http.StatusNotFound {
+              			d.SetId("")
+              		}
+              		return fmt.Errorf("Error making Read request on Azure resource group %s: %s", resGroup, err)

Member

tombuildsstuff Nov 13, 2017

could we make these formatting arguments%q and %+v?

azurerm/data_source_arm_public_ips.go Outdated

+              	attachedOnly := d.Get("attached").(bool)
+              	resp, err := publicIPClient.List(resGroup)
+              	if err != nil {
+              		if resp.StatusCode == http.StatusNotFound {

Member

tombuildsstuff Nov 13, 2017

can we update this to use the helper method utils.ResponseWasNotFound(resp) - which also handles connection drops?

azurerm/data_source_arm_public_ips.go Outdated

+              	publicIPClient := meta.(*ArmClient).publicIPClient
+              	resGroup := d.Get("resource_group_name").(string)
+              	minimumCount, minimumCountOk := d.GetOk("minimum_count")

Member

tombuildsstuff Nov 13, 2017

I think we can remove this field?

azurerm/data_source_arm_public_ips.go Outdated

+              			filteredIps = append(filteredIps, element)
+              		}
+              	}
+              	if minimumCountOk && len(filteredIps) < minimumCount.(int) {

Member

tombuildsstuff Nov 13, 2017

I think now this is a more generic Data Source - that we can remove this?

azurerm/data_source_arm_public_ips.go Outdated

+              				m["fqdn"] = *element.PublicIPAddressPropertiesFormat.DNSSettings.Fqdn
+              			} else {
+              				m["fqdn"] = ""
+              			}

Member

tombuildsstuff Nov 13, 2017

I think we can remove the else statement here - since it'll be set to an empty string automatically?

azurerm/data_source_arm_public_ips.go Outdated

+              			if element.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel != nil && *element.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel != "" {
+              				m["domain_name_label"] = *element.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel
+              			} else {
+              				m["domain_name_label"] = ""

Member

tombuildsstuff Nov 13, 2017

I think we can remove the else statement here - since it'll be set to an empty string automatically?

azurerm/data_source_arm_public_ips.go Outdated

+              	for _, element := range filteredIps {
+              		m := make(map[string]string)
+              		m["public_ip_address_id"] = *element.ID
+              		m["name"] = *element.Name

Member

tombuildsstuff Nov 13, 2017

can we add an if check around this, in-case it's nil?

azurerm/data_source_arm_public_ips.go Outdated

+              	var results []map[string]string
+              	for _, element := range filteredIps {
+              		m := make(map[string]string)
+              		m["public_ip_address_id"] = *element.ID

Member

tombuildsstuff Nov 13, 2017

can we add an if check around this, in-case it's nil?

website/docs/d/public_ips.html.markdown Outdated

+              * `resource_group_name` - (Required) Specifies the name of the resource group.
+              * `attached` - (Required) Whether to return public IPs that are attached or not.
+              * `minimum_count` - (Optional) Specifies the minimum number of IP addresses that

Member

tombuildsstuff Nov 13, 2017

(as above) can we remove this field?

tombuildsstuff added the waiting-response label

Contributor Author

hbuckle commented Feb 1, 2018

Sorry for the delay, finally got around to updating this

Contributor Author

hbuckle commented Mar 5, 2018

@tombuildsstuff any chance of going over this again?

Member

tombuildsstuff commented Mar 6, 2018

@hbuckle sorry, we're still trying to work through the backlog of pending PR reviews; I'll take a look into this shortly

hbuckle added 7 commits

March 6, 2018 12:53


          add public_ip_ids data source

2d005bb


          add docs

e342509


          rename ds

7d774fe


          return list of maps with pip details

694de65


          update tests

2ce4273


          update docs

4f80fcc


          pr review updates

056c6df

tombuildsstuff force-pushed the ds_public_ip_ids branch from 93953cd to 056c6df Compare

March 6, 2018 19:54

Member

tombuildsstuff commented Mar 6, 2018 •

edited

@hbuckle heads up that I've just rebased this on top of master so it merges cleanly, taking a look now :)

tombuildsstuff added 2 commits

March 6, 2018 13:03


          Refactoring the code

dc4ceaf


          Ditching the arm prefix

tombuildsstuff self-assigned this

tombuildsstuff added 2 commits

March 6, 2018 13:32


          Fixing up the tests:

8e1eafe

```
$ acctests azurerm TestAccDataSourceAzureRMPublicIPs_
=== RUN   TestAccDataSourceAzureRMPublicIPs_basic
--- PASS: TestAccDataSourceAzureRMPublicIPs_basic (85.74s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_mixed
--- PASS: TestAccDataSourceAzureRMPublicIPs_mixed (82.73s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_count
--- PASS: TestAccDataSourceAzureRMPublicIPs_count (79.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	247.767s
```


          Support for filtering by name prefix and ip allocation method

d496463

Member

tombuildsstuff commented Mar 6, 2018

@hbuckle I hope you don't mind, but I've pushed some commits to add support for also filtering by the prefix of the name and the allocation_type for the Public IP Address in order to expand the use-case of this data source

tombuildsstuff removed the waiting-response label

tombuildsstuff added 3 commits

March 6, 2018 15:47


          Fixing the tests

Tests pass:

```
$ acctests azurerm TestAccDataSourceAzureRMPublicIPs_
=== RUN   TestAccDataSourceAzureRMPublicIPs_namePrefix
--- PASS: TestAccDataSourceAzureRMPublicIPs_namePrefix (86.67s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_assigned
--- PASS: TestAccDataSourceAzureRMPublicIPs_assigned (90.43s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_allocationType
--- PASS: TestAccDataSourceAzureRMPublicIPs_allocationType (82.07s)
```


          Documenting the new fields

c180c9e


          Adding a sidebar link

c238706

tombuildsstuff added this to the 1.3.0 milestone

tombuildsstuff dismissed their stale review

March 6, 2018 22:51

pushed changes

tombuildsstuff changed the title ~~Public IP IDs data source~~ New Data Source: azurerm_public_ips

tombuildsstuff added service/public-ip and removed enhancement labels

tombuildsstuff requested a review from mbfrahry

March 6, 2018 23:01

mbfrahry approved these changes

View reviewed changes

Member

mbfrahry left a comment

LGTM minus the one comment and if the tests pass

website/docs/d/public_ips.html.markdown Outdated

+              ## Argument Reference
+              * `resource_group_name` - (Required) Specifies the name of the resource group.
+              * `attached` - (Optional) Should we only return IP Addresses which are attached to a device (VM/LB) etc?

Member

mbfrahry Mar 6, 2018

I wouldn't frame this as a question and instead have it read like Returns IP Address that are attached to a device or not attached to a device or something to that effect.


          Review feedback

6a91d19

tombuildsstuff merged commit fc98c0a into hashicorp:master

tombuildsstuff added a commit that referenced this pull request


          Updating to include #304

3cd42a1

ghost commented Mar 31, 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 locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.