provider/openstack: Add openstack_networking_network_v2 datasource

[yanndegat]

hi @jtopjian

i'm pushing this new datasource for the openstack provider.
sadly, i cannot run the acceptance tests because my openstack provider doesn't allow network creation through the neutron api.
hoping the tests pass

anyhow, i've used this datasource against a pre existing network and this works.

[jtopjian]

@yanndegat Thanks for making this :)

This looks good, but it's limited to only using the IDFromName function. I think a good approach would be to use the List call like the image data source does.

You can see the list of list options for networks here.

You don't need to add all of the possible options. For example, if all you need is is to search by "name", then just do:

listOpts := networks.ListOpts{
        Name: d.Get("name").(string),
}

This method is the same of what you have now, but at least by doing it this way, someone else can easily add Status, TenantID, etc in the future just by appending to networks.ListOpts.

Then in order to handle multiple returned networks, maybe add a most_recent parameter like is done here, here, and here.

This is the pattern that most of the OpenStack resources will take for data sources. The various ListOpts in Gophercloud map very well for data sources. :)

Thoughts?

Also, if you use the image data source to base the network data source off of, you'll see that the image is iterating through EachPage. This was due to a bug in Gophercloud at the time. Networks should be more simple:

allPages, err := networks.List(client, listOpts).AllPages()
if err != nil {
        return err
}
allNetworks, err := networks.ExtractNetworks(allPages)
if err != nil {
        return err
}

Then you should be able to pick up again starting here.

Let me know if you have any questions or comments! Thanks again for putting this together :)

[yanndegat]

@jtopjian hi joe

thanks a lot.
will do. makes a lot of sense.

One thing though : there's no way to sort a list of networks based on a pertinent criteria.
Moreover, in the specific case of a network, i think that if you have multiple results, the system shouldn't try to choose one by default.

There's only 2 cases where there could be more than one result:

• if a user filters on criterias other than name or subnet, which i think shouldn't be allowed.
  i mean that, i don't imagine a case where someone would want to, for instance, create a subnet in "one of the networks that are UP"
• if an ADMIN user filters on names or subnet without specifying a tenant_id, which i still think shouldn't be allowed. because i don't imagine a case where some admin would want to, for instance, create a subnet in "one of the networks named 'test' regardless of the tenant"

so if the list search returns more than one network, then there's obsviously missing criterias.
i've read networks data_source code for other providers, it seems that they don't allow multipe results either.

what do you think?

[jtopjian]

Can you move admin_state_up and shared to the bottom of the list like here?

[jtopjian]

@yanndegat I agree with everything :)

I left one comment on schema placement, but it's absolutely minor. I'll run some acceptance tests on this later today :)

[jtopjian]

@yanndegat I ran into an error with the acceptance test. The "network" one works, but it's failing on the subnet test. I'll look at it in more detail tomorrow. I can send you a patch once it's working.

[yanndegat]

yup, i saw whats wrong. resubmitting

[jtopjian]

@yanndegat Ah, I found the problem :)

By default, this data source will query all networks, even those created by an admin and marked as "shared". Normal users can see these networks, but it looks like they cannot perform a GET on some subnets. So when the data source is iterating through all networks and looking for a matching subnet, I was seeing an error about "subnet not found". In reality, it was simply querying a subnet the test user did not have access to.

I made a patch here that fixes the problem: https://gist.github.com/jtopjian/37accf201276cf18fb18c575f7094243

Let me know if this makes sense :)

[yanndegat]

thanks a lot,
i also added a default value in the schema based on the OS_TENANT_ID env var

[jtopjian]

That's a great idea, but it's better to use MultiEnvDefaultFunc for the project/tenant ID:

https://github.com/hashicorp/terraform/blob/master/builtin/providers/openstack/provider.go#L37

[yanndegat]

done

[jtopjian]

@yanndegat Awesome. This looks good to me! Thank you for making this and for your patience with the changes. :)

@stack72 do you want to do a quick review?

[stack72]

This LGTM :) Thanks for all the work here @yanndegat

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.