Skip to content

Refine endpoint discovery #3420

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

Merged
merged 8 commits into from
Jun 19, 2025
Merged

Conversation

stephenfin
Copy link
Contributor

@stephenfin stephenfin commented Jun 11, 2025

This is a follow-up for #3351. In that PR we added support for discovering API versions. To do this, we added a dependency on the version and min_version headers. These 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. Given this fact, relying on these fields is a mistake.

Instead, we should attempt to infer the version from the URL and only attempt version discovery if we need more information. If we do attempt version discovery, then we should be relying on the id field which, per the api-sig guidelines, 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.

Note

This appears with a semver:major label, but in reality the function it's modifying has not been released/backported yet so it's not actually.

@github-actions github-actions bot added edit:openstack This PR updates common OpenStack code edit:utils This PR updates utils code semver:major Breaking change labels Jun 11, 2025
@stephenfin stephenfin force-pushed the endpoint-discovery branch 2 times, most recently from 79d9d87 to fb39f9f Compare June 11, 2025 17:26
@github-actions github-actions bot added edit:actions This PR updates GitHub Actions code edit:testinfra This PR updates testing infrastructure code labels Jun 11, 2025
@stephenfin
Copy link
Contributor Author

keymanager failures look legit. It seems that service does use the max_version field, unlike nova, cinder etc. https://docs.openstack.org/barbican/latest/api/microversions.html

@coveralls
Copy link

coveralls commented Jun 11, 2025

Coverage Status

coverage: 63.704% (+0.08%) from 63.623%
when pulling a574a3e on shiftstack:endpoint-discovery
into 83e0c32 on gophercloud:main.

@stephenfin stephenfin force-pushed the endpoint-discovery branch from dd13d3f to 12a7c8e Compare June 12, 2025 10:07
@stephenfin stephenfin force-pushed the endpoint-discovery branch 2 times, most recently from c94c9a7 to 2eaf8c6 Compare June 17, 2025 12:19
@stephenfin
Copy link
Contributor Author

stephenfin commented Jun 17, 2025

The functional-orchestration and functional-workflow jobs are failing because Mistral and Heat don't expose a proper version document. For example, for Heat:

❯ curl -g -X GET -H "X-Auth-Token: {token}" http://10.0.110.168/heat-api/
{"versions": [{"id": "v1.0", "status": "CURRENT", "links": [{"rel": "self", "href": "http://10.0.110.168/heat-api/v1/"}]}]}
❯ curl -g -X GET -H "X-Auth-Token: {token}" http://10.0.110.168/heat-api/v1/
<html>
 <head>
  <title>404 Not Found</title>
 </head>
 <body>
  <h1>404 Not Found</h1>
  The resource could not be found.<br /><br />



 </body>
</html>

I've proposed a fix for heat here but idk if that'll be accepted and we'll need to live with older versions of both services, so I'll respin to address both of these.

The functional-networking (master) test is newly failing because the vlan-transparent extension is now enabled by default which means we're finally running this test, and the test is broken since we're attempting to update the vlan_transparent field but that is read-only.

The functional-baremetal (master) and functional-messaging (epoxy, master) are long-term failures that we're working on separately.

@stephenfin stephenfin force-pushed the endpoint-discovery branch from 2eaf8c6 to e243293 Compare June 18, 2025 16:08
@github-actions github-actions bot added the edit:networking This PR updates networking code label Jun 18, 2025
Copy link
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a can of worms! Makes you appreciate the value of consistency even more. Thanks a lot for navigating this mess and put it in order.

Thanks also for updating all the workflows to trigger CI jobs when updating common files. This will be useful to avoid breaking main for next time we'll make a global change like this one.

@mandre
Copy link
Contributor

mandre commented Jun 19, 2025

For the other reviewers, note the semver bot flags this as major change, but this isn't exactly true. From the output of go-apidiff:

   Incompatible changes:
  - GetServiceVersions: changed from func(context.Context, *github.com/gophercloud/gophercloud/v2.ProviderClient, string) (SupportedMicroversions, error) to func(context.Context, *github.com/gophercloud/gophercloud/v2.ProviderClient, string, bool) ([]SupportedVersion, error)

However, GetServiceVersions() was introduced in #3351 and is not yet part of a release.

It should be possible to backport this to v2 together with #3351 if we want to.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Ahead of the addition of some new tests.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
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>
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>
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>
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
The 'vlan-transparent' extension is now enabled by default [1]. This is
highlighting a bug in the aforementioned test: namely, that we are
trying to update an attribute which is read-only [2]. Remove the update
step of the job and fix the test.

[1] openstack/neutron@11ff4f2
[2] https://github.com/openstack/neutron-lib/blob/fd011c955dfae1072555c69b6ba742b85f041736/neutron_lib/api/definitions/vlantransparent.py#L49

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin stephenfin force-pushed the endpoint-discovery branch from e243293 to a574a3e Compare June 19, 2025 11:48
@stephenfin
Copy link
Contributor Author

For the other reviewers, note the semver bot flags this as major change, but this isn't exactly true. From the output of go-apidiff:

   Incompatible changes:
  - GetServiceVersions: changed from func(context.Context, *github.com/gophercloud/gophercloud/v2.ProviderClient, string) (SupportedMicroversions, error) to func(context.Context, *github.com/gophercloud/gophercloud/v2.ProviderClient, string, bool) ([]SupportedVersion, error)

However, GetServiceVersions() was introduced in #3351 and is not yet part of a release.

It should be possible to backport this to v2 together with #3351 if we want to.

Yeah, I meant to note this in the description. I'll add it now.

Copy link
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it good to go now?

@mandre mandre merged commit 992e8b7 into gophercloud:main Jun 19, 2025
78 of 82 checks passed
@mandre mandre deleted the endpoint-discovery branch June 19, 2025 14:17
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:networking This PR updates networking code edit:openstack This PR updates common OpenStack code edit:testinfra This PR updates testing infrastructure code edit:utils This PR updates utils code semver:major Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants