-
Notifications
You must be signed in to change notification settings - Fork 560
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
Conversation
79d9d87
to
fb39f9f
Compare
keymanager failures look legit. It seems that service does use the |
dd13d3f
to
12a7c8e
Compare
c94c9a7
to
2eaf8c6
Compare
The
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 The |
2eaf8c6
to
e243293
Compare
There was a problem hiding this 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.
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:
However, 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>
e243293
to
a574a3e
Compare
Yeah, I meant to note this in the description. I'll add it now. |
There was a problem hiding this 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?
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
andmin_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.