Configure and respect service type aliases by stephenfin · Pull Request #3209 · gophercloud/gophercloud

stephenfin · 2024-10-10T10:55:29Z

This means we can identify a service by either its type or any of its official aliases, which is the expectation when it comes to navigating the service catalog.

stephenfin · 2024-10-10T10:58:37Z

I'm not sure whether we want to expose the aliases via a new field in EndpointOpts like I've done here, or if it would be simpler to stick with a single Type field and do the mapping of service type to aliases inside V2EndpointURL / V3EndpointURL in openstack/endpoint_location.go. What we've done here is more obvious, but I'm not allowing users to override the aliases maybe exposing it is wholly unnecessary? 🤷 Open to thoughts here.

EDIT: Also, I'm surprised this has attracted the semver:major label. I only changed internal methods (various versions of initClientOpts) which I had assumed to be fair game. 😕

kayrus · 2024-10-10T11:00:27Z

@stephenfin I think that NewClient functions also need to support aliases, so users can override them for custom service like block-storage-beta

coveralls · 2024-10-10T11:01:02Z

coverage: 78.681% (-0.05%) from 78.726%
when pulling 5a3e9f6 on shiftstack:proper-service-discovery
into 24c9641 on gophercloud:master.

stephenfin · 2024-10-11T09:10:53Z

@stephenfin I think that NewClient functions also need to support aliases, so users can override them for custom service like block-storage-beta

If I do that, it'll be a breaking API change though, which means I can't fix this until V3?

kayrus · 2024-10-11T09:16:12Z

right. we can implement this in V3.

stephenfin · 2024-10-11T11:00:52Z

Okay. So is what we have here good enough for now, in v2? What are your thoughts on my questions above?

This allows us to request an endpoint by either its service type or one of its aliases. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

This demonstrates an unhappy path that we may wish to deprecate in the future, but no mechanism exists for doing aside from logging, which we don't do anywhere else. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

We now have alias support and Gophercloud will automatically negotiate on our behalf. This means we set the service types to their official value rather than one of the aliases. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

EmilienM · 2024-11-29T14:04:04Z

endpoint_search.go

 )

+// ServiceTypeAliases contains a mapping of service types to any aliases, as
+// defined by the OpenStack Service Types Authority. Only service types that


It would have been nice to provide a link (for each service) of the code where we can find the alias. But I'm fine as is now.

FWIW, many months later, if someone is curious:

https://service-types.openstack.org/

EmilienM · 2024-11-29T14:05:42Z

All the Cinder jobs have a failure, we need to check whether it's related here.

stephenfin · 2024-11-29T14:30:47Z

All the Cinder jobs have a failure, we need to check whether it's related here.

Fixed here 5a3e9f6

Reported here https://lists.openstack.org/archives/list/openstack-discuss@lists.openstack.org/thread/QQ7L44VE4MOWOQVOFCZQPTIVI35A6SCK/

Despite the official service type being block-storage, cinder only accepts volume as the service type in the OpenStack-API-Version header :( Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

stephenfin · 2024-11-29T14:43:02Z

EDIT: Also, I'm surprised this has attracted the semver:major label. I only changed internal methods (various versions of initClientOpts) which I had assumed to be fair game. 😕

Turns out this is because of my addition of the Aliases field to EndpointOpts. It's a slice which is not comparable, thus EndpointOpts is no longer comparable. I think this is an okay exception to the backport policy but I'd like others to weigh in before I propose said backport.

EmilienM

Our CI is awesome.

kayrus · 2025-03-06T16:54:21Z

Turns out this is because of my addition of the Aliases field to EndpointOpts. It's a slice which is not comparable, thus EndpointOpts is no longer comparable. I think this is an okay exception to the backport policy but I'd like others to weigh in before I propose said backport.

@stephenfin I'd like this to be backported.
@EmilienM @mandre @pierreprinetti objections?

…scovery Configure and respect service type aliases

Merge pull request #3209 from shiftstack/proper-service-discovery

SuperSandro2000 · 2025-04-15T14:13:54Z

openstack/client.go

 // NewSharedFileSystemV2 creates a ServiceClient that may be used to access the v2 shared file system service.
 func NewSharedFileSystemV2(client *gophercloud.ProviderClient, eo gophercloud.EndpointOpts) (*gophercloud.ServiceClient, error) {
-	return initClientOpts(client, eo, "sharev2")
+	return initClientOpts(client, eo, "shared-file-system")


I have the suspicion that this somehow reverted our usage to sharev1 from sharev2. Any hints how to properly debug this?

see terraform-provider-openstack/terraform-provider-openstack#1848

Would setting the Type on the endpoint opts also work?

diff --git a/internal/liquids/manila/liquid.go b/internal/liquids/manila/liquid.go index 2a15d01a..b524ec9a 100644 --- a/internal/liquids/manila/liquid.go +++ b/internal/liquids/manila/liquid.go @@ -25,7 +25,6 @@ "math" "regexp" "strconv" - "strings" "time" "github.com/gophercloud/gophercloud/v2" @@ -74,12 +73,11 @@ func (l *Logic) Init(ctx context.Context, provider *gophercloud.ProviderClient, } // initialize connection to Manila + eo.Type = "sharev2" l.ManilaV2, err = openstack.NewSharedFileSystemV2(provider, eo) if err != nil { return err } - // Workaround for gophercloud 2.7.0 which gets the first found OpenStack URL for Manila. - l.ManilaV2.Endpoint = strings.Replace(l.ManilaV2.Endpoint, "v1", "v2", 1) microversion, err := l.findMicroversion(ctx, l.ManilaV2) if err != nil { return err

majewsky · 2025-04-16T08:27:15Z

I'm confused why this patch was accepted at all. You're just aliasing volume, and volumev2 and volumev3 to each other, even though those are completely different APIs. How is this supposed to work on a theoretical level? If I'm asking for NewBlockStorageV3, giving back the v2 API is a categorically wrong answer.

mnaser · 2025-04-16T13:57:25Z

V3 is exactly V2 but with micro version support so requesting V3 without a micro version gets you V2 equivilant API.

majewsky · 2025-04-16T13:58:40Z

So then the problem is only with including the v1 APIs in the list (share and volume)?

stephenfin · 2025-04-16T20:49:45Z

The issue here is that we are missing service discovery, i.e. the equivalent of keystoneauth.discover. I am working on a fix plus a interim partial revert to tide us through until it's ready. A full revert is not an option as what gophercloud was doing previously was not compliant with deployments using the correct service type rather than an alias. The DevStack change merely exposed this incorrect behaviour.

stephenfin · 2025-04-17T14:19:57Z

The issue here is that we are missing service discovery, i.e. the equivalent of keystoneauth.discover. I am working on a fix plus a interim partial revert to tide us through until it's ready. A full revert is not an option as what gophercloud was doing previously was not compliant with deployments using the correct service type rather than an alias. The DevStack change merely exposed this incorrect behaviour.

The interim fix is available here #3350

github-actions bot added edit:openstack This PR updates common OpenStack code edit:gophercloud This PR updates common Gophercloud code edit:blockstorage This PR updates blockstorage code semver:major Breaking change labels Oct 10, 2024

stephenfin force-pushed the proper-service-discovery branch from c0e9fff to 84236e0 Compare October 11, 2024 11:18

stephenfin added 4 commits November 29, 2024 11:17

Add EndpointOpts.Aliases field, Types() method

eff7d91

This allows us to request an endpoint by either its service type or one of its aliases. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Extend EndpointOpts.ApplyDefaults to set Aliases

c507e2a

This demonstrates an unhappy path that we may wish to deprecate in the future, but no mechanism exists for doing aside from logging, which we don't do anywhere else. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Start respecting service types when searching for endpoints

0ba521a

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Use official service types

e394733

We now have alias support and Gophercloud will automatically negotiate on our behalf. This means we set the service types to their official value rather than one of the aliases. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

stephenfin force-pushed the proper-service-discovery branch from 84236e0 to e394733 Compare November 29, 2024 11:26

EmilienM reviewed Nov 29, 2024

View reviewed changes

EmilienM previously approved these changes Nov 29, 2024

View reviewed changes

stephenfin dismissed EmilienM’s stale review via 6428ace November 29, 2024 14:25

Set correct service name for microversion header

5a3e9f6

Despite the official service type being block-storage, cinder only accepts volume as the service type in the OpenStack-API-Version header :( Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

stephenfin force-pushed the proper-service-discovery branch from 6428ace to 5a3e9f6 Compare November 29, 2024 14:37

EmilienM approved these changes Nov 29, 2024

View reviewed changes

EmilienM merged commit fdb527e into gophercloud:master Nov 29, 2024

EmilienM deleted the proper-service-discovery branch November 29, 2024 15:26

EmilienM added the backport-v2 This PR will be backported to v2 label Mar 11, 2025

kayrus removed the semver:major Breaking change label Mar 11, 2025

kayrus added the semver:minor Backwards-compatible change label Mar 11, 2025

kayrus pushed a commit to kayrus/gophercloud that referenced this pull request Mar 11, 2025

Merge pull request gophercloud#3209 from shiftstack/proper-service-di…

42cbbca

…scovery Configure and respect service type aliases

kayrus mentioned this pull request Mar 11, 2025

Merge pull request #3209 from shiftstack/proper-service-discovery #3327

Merged

kayrus added semver:major Breaking change and removed semver:minor Backwards-compatible change labels Mar 11, 2025

kayrus pushed a commit to kayrus/gophercloud that referenced this pull request Mar 12, 2025

Merge pull request gophercloud#3209 from shiftstack/proper-service-di…

e0b454d

…scovery Configure and respect service type aliases

mandre added a commit that referenced this pull request Mar 12, 2025

Merge pull request #3327 from kayrus/backport-aliases

908ce17

Merge pull request #3209 from shiftstack/proper-service-discovery

SuperSandro2000 reviewed Apr 15, 2025

View reviewed changes

majewsky mentioned this pull request Apr 16, 2025

Regression in 2.7.0: openstack.NewSharedFilesystemV2 chooses v1 API #3347

Open

stephenfin mentioned this pull request Apr 17, 2025

Prefer versioned service type aliases #3350

Closed

mnaser mentioned this pull request May 1, 2025

Cinder CSI plugin doesn't work with latest OpenStack kubernetes/cloud-provider-openstack#2884

Closed

Conversation

stephenfin commented Oct 10, 2024

Uh oh!

stephenfin commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayrus commented Oct 10, 2024

Uh oh!

coveralls commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenfin commented Oct 11, 2024

Uh oh!

kayrus commented Oct 11, 2024

Uh oh!

stephenfin commented Oct 11, 2024

Uh oh!

EmilienM Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

mnaser Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

EmilienM commented Nov 29, 2024

Uh oh!

stephenfin commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenfin commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmilienM left a comment

Choose a reason for hiding this comment

Uh oh!

kayrus commented Mar 6, 2025

Uh oh!

SuperSandro2000 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

kayrus Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

majewsky commented Apr 16, 2025

Uh oh!

mnaser commented Apr 16, 2025

Uh oh!

majewsky commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenfin commented Apr 16, 2025

Uh oh!

stephenfin commented Apr 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stephenfin commented Oct 10, 2024 •

edited

Loading

coveralls commented Oct 10, 2024 •

edited

Loading

stephenfin commented Nov 29, 2024 •

edited

Loading

stephenfin commented Nov 29, 2024 •

edited

Loading

majewsky commented Apr 16, 2025 •

edited

Loading