🐛 do not propagate the cloud field to clientconfig.AuthOptions

[sbueringer]

What this PR does / why we need it:

This PR stops propagating the cloud field to clientconfig.AuthOptions. If this field is actually set, we hit a branch in AuthOptions which always fails: (I don't think we have to validate that the field is not set, when we can just change the code that it doesn't matter if it is set or not)

cluster-api-provider-openstack/pkg/cloud/services/provider/provider.go

Lines 80 to 91 in 31df024

	if cloud.AuthInfo != nil {
	clientOpts.AuthInfo = cloud.AuthInfo
	clientOpts.AuthType = cloud.AuthType
	clientOpts.Cloud = cloud.Cloud
	clientOpts.RegionName = cloud.RegionName
	}
	opts, err := clientconfig.AuthOptions(clientOpts)
	if err != nil {
	return nil, nil, fmt.Errorf("auth option failed for cloud %v: %v", cloud.Cloud, err)
	}
	opts.AllowReauth = true

https://github.com/gophercloud/utils/blob/7b186010c04f90ca48b5ce471a222911ff076e00/openstack/clientconfig/requests.go#L352-L370

https://github.com/gophercloud/utils/blob/7b186010c04f90ca48b5ce471a222911ff076e00/openstack/clientconfig/requests.go#L192

https://github.com/gophercloud/utils/blob/7b186010c04f90ca48b5ce471a222911ff076e00/openstack/clientconfig/requests.go#L119

there's not cloud.yaml there which could be found...

Alternative would be to duplicate the code from AuthOptions we actually want, but I wouldn't like to maintain that much code on our side:

https://github.com/gophercloud/utils/blob/7b186010c04f90ca48b5ce471a222911ff076e00/openstack/clientconfig/requests.go#L390-L396

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #825

Special notes for your reviewer:

1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• if necessary:
  • includes documentation
  • adds unit tests

/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

/assign @blencoff
/assign @jichenjc

WDYT?

[k8s-ci-robot]

@sbueringer: GitHub didn't allow me to assign the following users: blencoff.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @blencoff
/assign @jichenjc

WDYT?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jichenjc]

what about a clouds.yaml contains multiple cloud ?

[sbueringer]

I assume this does not matter. This is already handled in: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/829/files#diff-dbf706fca1006fa16e85a98f355508c0891e685f846ff079d5ac1d6e6b308148L155

We're only passing one cloud (clientconfig.Cloud) into the NewClient func.

[sbueringer]

Also the code path in AuthOptions never works when we set the cloud to a non empty string, because it always searches for a cloud.yaml file at the default locations which we don't have (and we don't need because clientconfig.Cloud is already one parsed cloud of a cloud.yaml).

[sbueringer]

@jichenjc wdyt, okay to merge?

[jichenjc]

give me another day, I want to read it more (run out of time today)

[jichenjc]

we have a set of logics at line 52 / 70 above , such as
cloud, caCert, err = getCloudFromSecret(ctrlClient, namespace, openStackCluster.Spec.CloudsSecret.Name, openStackCluster.Spec.CloudName)

so basically, we are providing info based on cluster.Spec.CloudName

if we don't want to honor this field, should we consider remove
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/api/v1alpha3/openstackcluster_types.go#L38?

actually we created a secret and this secret is a clouds.yaml and it might contains multiple clouds
I also think we don't want to (technically ?) support 1 CAPI + multiple openstack solution ?
if that's the case, maybe we need reconsider adjust the whole Cloud related items and manage expecations of users

[sbueringer]

Hm we are using the CloudName here:

• https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/829/files#diff-dbf706fca1006fa16e85a98f355508c0891e685f846ff079d5ac1d6e6b308148R150-R154

We are even checking that it's not empty here:

• https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/829/files#diff-dbf706fca1006fa16e85a98f355508c0891e685f846ff079d5ac1d6e6b308148R125-R127

So in my opinion we need the property and it does exactly what we want. It picks the right cloud if we have multiple in the cloud.yaml. The only problem is that we don't need it in clientconfig.AuthOptions. clientconfig.AuthOptions is not the func which parses the cloud.yaml so it doesn't need the cloud.
In fact the AuthOptions func searches for some other cloud.yaml at some default location when we hand it over a cloud

I don\t really understand what you mean, but we cannot drop the property. We need it, just not here.

[sbueringer]

(Also cloud.Cloud is not the same as the cloudName in our spec)

[jichenjc]

ok, you are right, I was confused by cloud and cloudName variable at first then I misunderstand the logic
after further read I think you are right , sorry for the confusion

[sbueringer]

No problem, it's confusing :). I also would like to use not that much of the AuthOptions func unfortunately the only way to do this is to copy it :/

[jichenjc]

/lgtm
/hold cancel