-
Notifications
You must be signed in to change notification settings - Fork 559
[v2] Add versioned endpoint discovery #3435
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
base: v2
Are you sure you want to change the base?
Conversation
we hit this issue in terraform-provider-openstack/terraform-provider-openstack#1929 |
Not from me. I'm happy to merge this now. It's been stable on |
The merge-base changed after approval.
we do this in main branch and I have concerns for having this in the final v3 release or in v2. |
@stephenfin here is my solution for aliases terraform-provider-openstack/utils#1 |
@stephenfin can you rebase this PR on v2 to ease the review after the #3465 and #3466 are merged? |
@stephenfin #3465 and #3466 are merged, please rebase |
I'm sorry, I misunderstood what you were saying. If a user requests e.g. I'll rebase this now. |
We do not need to check the validity of the provided opts more than once so don't. We can also simplify our handling of multiple endpoints (though not as much as we'd like in the v2 case, due to forthcoming patches). Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 4329495)
We will use this to do proper discovery across multiple versions. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit bae4c6f)
This retrieves the base endpoint without stripping the version information. This is helpful for service catalog entries that include project IDs, like those historically preferred by Cinder for some reason. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 2bf97ce)
This is an re-implementation of GetSupportedMicroversions that supports use with a ProviderClient and an explicit endpoint URL, allowing us to use this during discovery. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 8cbcb2f)
For now these are exact duplicates of the V2EndpointURL and V3EndpointURL helpers but with different names. diff -u openstack/endpoint_location.go openstack/endpoint.go This is done to ease review. We will introduce further differences shortly. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit ed23d9b)
This is simpler than expected. Effectively, for each potential endpoint, we retrieve the version document from the root URL and parse the version information, if any, from it, comparing it against the version expected. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit f28c963)
Avoid unnecessary lookups for when we are encoding the version in our service type. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 07d1ee0)
And deprecate the old ones since they are no longer used. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 8139f99)
Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 82c2ca0)
Ahead of the addition of some new tests. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 774a8b8)
Right now these are effectively duplicates of the tests for GetSupportedMicroversions, which is also expanded here to test against the version documents of other services, but that will change shortly. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 9c18abd)
In commit f28c963, we added support for discovering API versions. To do this, we added a dependency on the 'version' and 'min_version' headers, which are used by services with microversion support to document the maximum and minimum API versions supported, respectively. However, not all services support API microversions: services like Glance and Designate use API versions as a signal of a new feature (or, in Glance's case, as a signal that a feature is not enabled), others like Keystone just support a single version, while Neutron does its own thing with API extensions [1]. Given this fact, relying on these fields is a mistake. Instead, we should be relying on the 'id' field. Per the api-sig guidelines [2], this should be the API major version [3]. We continue parsing the microversion-related headers, since it will be useful later on if/when we want to do versioned discovery. [1] https://that.guru/blog/api-versioning-in-openstack/ [2] https://specs.openstack.org/openstack/api-sig/guidelines/discoverability.html [3] The studious among you may notice that the api-sig guidelines indicate that the maximum API microversion should be exposed via the 'max_version' header. However, in practice, virtually everyone uses 'version' instead. Why? Who knows. Best to just take these things on the chin and move on with our lives. [4] The term "major version" is a bit loaded. Consider Nova: at the time of writing, it exposes two "major versions": v2.0 and v2.1. v2.0 does not support microversions. v2.1 does. For v2.1 you therefore also have microversions to content with and at the time of writing it supports a minimum microversion of 2.1 and a maximum microversion of 2.100 (no 'v' prefix here). Normalizing these as we've done here gives us a major version of 2, a minor version of 1, a major maximum microversion of 2, a minor maximum microversion of 100, a major minimum microversion of 2, and a minor minimum microversion of 1 🤯. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 5b89285)
While the expected format of these documents is defined by a spec [1], most services do things slightly differently. Here, we add support for the Keystone and Barbican-style documents, which embed version objects inside another object with a single 'values' key, like so: {"versions": {"values": [{"id": "v3.14", ...}]}} And the Magnum-style document, which doesn't envelope individual version objects at all: {"id": "v2", ...} These are in contrast to the format used by Nova, Cinder etc.: {"versions": [{"id": "v2", ...}]} We also add support for 'max_version' key, which is used by Magnum and is what the spec actually recommends. [1] https://specs.openstack.org/openstack/api-sig/guidelines/discoverability.html Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 6e92c22)
Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit 37ec207)
Signed-off-by: Stephen Finucane <stephenfin@redhat.com> (cherry picked from commit b6be984)
14f6c35
to
b4c6a45
Compare
Done |
(cherry picked from commit 9e5efb7)
@stephenfin can you add #3442 into this PR and make a rebase one more time? regarding the redundant API calls, this is what I mean, see a terraform example: data "openstack_networking_port_v2" "port_1" {
count= 100
name = "port_1"
} dev provider compiled with gophercloud with version discovery:
recent release provider:
UPD: if you still insist on discovery, then we need to reopen a #1987 to have a way to create a client only once and clone it for each terraform resource (to properly support microversion) |
This is a manual backport of #3351 and #3420, with #3108 also included to avoid tricky merge conflicts. A backport of #3108 is proposed separately at #3434 to ease reviews. We should probably merge that first and then come back to rebase and merge this.