Skip to content

[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

Open
wants to merge 16 commits into
base: v2
Choose a base branch
from

Conversation

stephenfin
Copy link
Contributor

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.

@github-actions github-actions bot added edit:networking This PR updates networking code edit:openstack This PR updates common OpenStack code edit:dns This PR updates dns code edit:common This PR updates common code edit:actions This PR updates GitHub Actions code edit:image This PR updates image code edit:orchestration This PR updates orchestration code edit:db This PR updates db code edit:loadbalancer This PR updates loadbalancer code edit:compute This PR updates compute code edit:keymanager This PR updates keymanager code edit:utils This PR updates utils code v2 This PR targets v2 edit:messaging This PR updates messaging code edit:containerinfra This PR updates containerinfra code edit:placement This PR updates placement code edit:objectstorage This PR updates objectstorage code edit:workflow This PR updates workflow code edit:sharedfilesystems This PR updates sharedfilesystems code edit:baremetal This PR updates baremetal code edit:gophercloud This PR updates common Gophercloud code edit:blockstorage This PR updates blockstorage code edit:identity This PR updates identity code edit:container This PR updates container code edit:baremetalintrospection This PR updates baremetalintrospection code edit:testinfra This PR updates testing infrastructure code semver:major Breaking change labels Jun 19, 2025
@kayrus
Copy link
Contributor

kayrus commented Jul 15, 2025

we hit this issue in terraform-provider-openstack/terraform-provider-openstack#1929
any objections on merging this? this will also require #3442 backport

kayrus
kayrus previously approved these changes Jul 15, 2025
@stephenfin
Copy link
Contributor Author

we hit this issue in terraform-provider-openstack/terraform-provider-openstack#1929 any objections on merging this? this will also require #3442 backport

Not from me. I'm happy to merge this now. It's been stable on main.

@stephenfin stephenfin dismissed kayrus’s stale review July 16, 2025 10:38

The merge-base changed after approval.

@kayrus
Copy link
Contributor

kayrus commented Jul 16, 2025

This is already what we do

we do this in main branch and I have concerns for having this in the final v3 release or in v2.

@kayrus
Copy link
Contributor

kayrus commented Jul 16, 2025

@stephenfin here is my solution for aliases terraform-provider-openstack/utils#1
see also #3462

@kayrus kayrus mentioned this pull request Jul 16, 2025
@kayrus
Copy link
Contributor

kayrus commented Jul 16, 2025

@stephenfin can you rebase this PR on v2 to ease the review after the #3465 and #3466 are merged?

@kayrus
Copy link
Contributor

kayrus commented Jul 18, 2025

@stephenfin #3465 and #3466 are merged, please rebase

@stephenfin
Copy link
Contributor Author

This is already what we do

we do this in main branch and I have concerns for having this in the final v3 release or in v2.

I'm sorry, I misunderstood what you were saying. If a user requests e.g. volumev3 then we shortcut the API lookup but, as you note, those versioned service types are not the default service types used anymore. With that said, I'm inclined to say this is okay, as we are following the specs and, as you've shown, users can always override this service type as needed.

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)
@stephenfin stephenfin force-pushed the endpoint-discovery-v2 branch from 14f6c35 to b4c6a45 Compare July 21, 2025 15:22
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:major Breaking change labels Jul 21, 2025
@stephenfin
Copy link
Contributor Author

I'll rebase this now.

Done

@coveralls
Copy link

coveralls commented Jul 21, 2025

Coverage Status

coverage: 63.557% (-0.08%) from 63.637%
when pulling a3efa14 on shiftstack:endpoint-discovery-v2
into 015f396 on gophercloud:v2.

@kayrus
Copy link
Contributor

kayrus commented Jul 23, 2025

@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:

$ TF_LOG=DEBUG terraform apply 2>&1 | grep 'GET ' | wc -l
200

recent release provider:

$ TF_LOG=DEBUG terraform apply 2>&1 | grep 'GET ' | wc -l
100

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
edit:actions This PR updates GitHub Actions code edit:baremetal This PR updates baremetal code edit:baremetalintrospection This PR updates baremetalintrospection code edit:blockstorage This PR updates blockstorage code edit:common This PR updates common code edit:compute This PR updates compute code edit:container This PR updates container code edit:containerinfra This PR updates containerinfra code edit:db This PR updates db code edit:dns This PR updates dns code edit:gophercloud This PR updates common Gophercloud code edit:identity This PR updates identity code edit:image This PR updates image code edit:keymanager This PR updates keymanager code edit:loadbalancer This PR updates loadbalancer code edit:messaging This PR updates messaging code edit:networking This PR updates networking code edit:objectstorage This PR updates objectstorage code edit:openstack This PR updates common OpenStack code edit:orchestration This PR updates orchestration code edit:placement This PR updates placement code edit:sharedfilesystems This PR updates sharedfilesystems code edit:testinfra This PR updates testing infrastructure code edit:utils This PR updates utils code edit:workflow This PR updates workflow code hold Do not merge semver:minor Backwards-compatible change v2 This PR targets v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants