[v2] Add versioned endpoint discovery#3435
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 |
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)
a3efa14 to
a7234bb
Compare
I rebased, but can we add #3442 in here? That looks like a breaking change?
I don't see any way to avoid discovery: it's what OpenStack insists on. However, yes, we should be caching the discovery documents. I can work on this. |
yes, without this change the discovery will be broken. |
Indeed. I think we need an alternative option so. From the linked issue, the issue is here: gophercloud/openstack/client.go Lines 286 to 288 in c3046e0 Could we hardcode |
This is a v2-only alternative to gophercloud#3442 [1] that avoids introducing a breaking function signature change while still resolving gophercloud#3441 [2]. [1] gophercloud#3442 [2] gophercloud#3441 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
I've done this. |
|
@stephenfin is this PR ready to merge? I'd like to cut a new v2 release soon and wondering if this PR should be included. |
|
I think so, yes |
|
OK, let's get it in then. |
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.