OpenStack: Add cloud provider secret key #216

Fedosin · 2020-06-30T20:55:22Z

Now we include only clouds.yaml key in CCO minted secrets. But starting from OpenShift 4.6 we also need clouds.conf key data for Cinder CSI driver.

Both keys are generated by the installer: https://github.com/openshift/installer/blob/master/data/data/manifests/openshift/cloud-creds-secret.yaml.template#L33-L34

This commit adds clouds.conf key in CCO generated secrets.

Now we include only clouds.yaml key in CCO minted secrets. But starting from OpenShift 4.6 we also need clouds.conf key data for Cinder CSI driver. This commit adds clouds.conf key in generated secrets.

openshift-ci-robot · 2020-06-30T20:55:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Fedosin
To complete the pull request process, please assign twiest
You can assign the PR to them by writing /assign @twiest in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

bertinatto · 2020-07-01T10:07:05Z

CC @openshift/storage

bertinatto · 2020-07-01T10:07:35Z

/retest

mandre · 2020-07-01T10:13:45Z

/test e2e-openstack

bertinatto · 2020-07-01T12:05:12Z

I tested this patch and the operator correctly created a secret with a clouds.conf file. Then I used that secret with the Cinder CSI driver and it was able to successfully create/attach a volume.

Thanks for your work on this, @Fedosin!

joelddiaz · 2020-07-01T14:58:10Z

Is there an example of the kind of data that is stored in clouds.conf (I've never come across this in my limited OpenStack experience)?

Just as I wrote that I think I came across an example https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/csi/cinder/etc/cloud.conf .

joelddiaz · 2020-07-01T15:40:06Z

IIUC, clouds.conf is basically just the same contents as clouds.yaml but in INI-style format?

mandre · 2020-07-01T18:36:02Z

IIUC, clouds.conf is basically just the same contents as clouds.yaml but in INI-style format?

Not exactly - they're actually configuration files for different projects. The clouds.conf (should be cloud.conf really, I do not know why we chose to name it clouds.conf here) is the configuration for the cloud provider, while clouds.yaml is the configuration for the openstack client, that is also used by gophercloud.

The change looks good to me, we can merge it when it passes e2e-openstack CI.

joelddiaz · 2020-07-01T19:17:54Z

IIUC, clouds.conf is basically just the same contents as clouds.yaml but in INI-style format?

Not exactly - they're actually configuration files for different projects. The clouds.conf (should be cloud.conf really, I do not know why we chose to name it clouds.conf here) is the configuration for the cloud provider, while clouds.yaml is the configuration for the openstack client, that is also used by gophercloud.

Perhaps I've misread things. My understanding came from how the installer builds this INI-looking content https://github.com/openshift/installer/blob/master/pkg/asset/manifests/openstack/cloudproviderconfig.go#L29

So is it more accurate to say that this clouds.conf file is really useful only for Cinder?

What I'm trying to understand is if in the future we decide to implement "mint" mode for cloud-cred-operator for OpenStack (today we only support "passthrough" mode where we blindly copy the contents of the secret kube-system/openstack-credentials), what do all these fields in the Secret that cloud-cred-operator would need to populate if CCO was making OpenStack API calls to create new credentials while processing a CredentialsRequest object?

Fedosin · 2020-07-01T21:16:52Z

clouds.conf contains the in-tree cloud provider configuration. It includes auth parameters, cinder configuration and other sections: https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#typical-configuration

For Cinder CSI driver they decided to reuse this file as it already has everything they need. Other sections are ignored.

clouds.yaml has just auth parameters in the predefined format: https://docs.openstack.org/python-openstackclient/latest/configuration/index.html#configuration-files

bertinatto · 2020-07-02T07:40:08Z

/retest

openshift-ci-robot · 2020-07-02T08:36:58Z

@Fedosin: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-openstack	`ae43093`	link	`/test e2e-openstack`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

mandre · 2020-07-02T12:05:29Z

FYI e2e-openstack is currently broken due to https://bugzilla.redhat.com/show_bug.cgi?id=1853298. Patch at openshift/machine-config-operator#1773 should solve it.

joelddiaz · 2020-07-07T15:28:53Z

clouds.conf contains the in-tree cloud provider configuration. It includes auth parameters, cinder configuration and other sections: https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#typical-configuration

For Cinder CSI driver they decided to reuse this file as it already has everything they need. Other sections are ignored.

clouds.yaml has just auth parameters in the predefined format: https://docs.openstack.org/python-openstackclient/latest/configuration/index.html#configuration-files

@dgoodwin WDYT? It seems like if we ever need to implement mint mode for OpenStack, it's going to be more involved than "give me creds" -> "save to secret" as there are files with specific formats that need to be built. I think my primary concern here is that we might be building files with storage configuration data/sections when the credentials request is for the ability to create/delete instances.

dgoodwin · 2020-07-07T15:42:03Z

How does it differ from the json blobs we do for GCP?

joelddiaz · 2020-07-07T16:32:05Z

How does it differ from the json blobs we do for GCP?

Perhaps I simply do not understand how things are normally done in OpenStack, but to me the primary difference is that the GCP auth secret contains the auth info (urls, username, private keys), but not service configuration info (eg. no storage bucket encryption settings).

In my mental model, I would expect the operator that managed running ServiceX to ask for creds, and then combine the creds with the desired ServiceX configuration (assuming ServiceX's config and auth were both bundled into a single file).

With the present situation, for OpenStack mint-mode it feels like CCO would need to know:

This CredentialsRequest is for Cinder (or nova or whatever other OpenStack service)
Create a user/password that can do the actions requested in the CredRequest
Create a Cinder/service-specific secret payload

(and every new service could potentially need CCO-specific handling)

dgoodwin

Very good question, OpenStack folks, is Joel correct here? If we start supporting minting specific credentials for OpenStack by examining the CredentialsRequest, what is that secret expected to contain? Do we assume it would contain both of these two keys? Could there be additional keys and would we have enough into to always assume to populate them as well?

dgoodwin · 2020-07-07T17:26:56Z

pkg/openstack/actuator.go

 const (
-	rootOpenStackCredsSecretKey = "clouds.yaml"
+	rootOpenStackCredsSecretKey          = "clouds.yaml"
+	cloudProviderOpenStackCredsSecretKey = "clouds.conf"


Please add some comments here for the differences between the two keys, as this is definitely going to be confusing.

dgoodwin · 2020-07-07T17:33:04Z

pkg/openstack/actuator.go

+		return err
+	}
+
+	cloudsCONF, err := a.getCloudCredentialsSecretData(cloudCredSecret, cloudProviderOpenStackCredsSecretKey, logger)


What's going to happen on a 4.5 to 4.6 upgrade here, presumably this key will not be populated, will all cred requests start failing?

Fedosin · 2020-07-15T09:14:22Z

/hold
we need some time to consider if we really need this key in the CCO output

openshift-bot · 2020-12-15T22:28:31Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

OpenStack: Add cloud provider secret key

ae43093

Now we include only clouds.yaml key in CCO minted secrets. But starting from OpenShift 4.6 we also need clouds.conf key data for Cinder CSI driver. This commit adds clouds.conf key in generated secrets.

openshift-ci-robot requested review from csrwng and staebler June 30, 2020 20:55

dgoodwin reviewed Jul 7, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 15, 2020

Fedosin closed this Jan 6, 2021

OpenStack: Add cloud provider secret key #216

OpenStack: Add cloud provider secret key #216

Uh oh!

Conversation

Fedosin commented Jun 30, 2020

Uh oh!

openshift-ci-robot commented Jun 30, 2020

Uh oh!

bertinatto commented Jul 1, 2020

Uh oh!

bertinatto commented Jul 1, 2020

Uh oh!

mandre commented Jul 1, 2020

Uh oh!

bertinatto commented Jul 1, 2020

Uh oh!

joelddiaz commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelddiaz commented Jul 1, 2020

Uh oh!

mandre commented Jul 1, 2020

Uh oh!

joelddiaz commented Jul 1, 2020

Uh oh!

Fedosin commented Jul 1, 2020

Uh oh!

bertinatto commented Jul 2, 2020

Uh oh!

openshift-ci-robot commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandre commented Jul 2, 2020

Uh oh!

joelddiaz commented Jul 7, 2020

Uh oh!

dgoodwin commented Jul 7, 2020

Uh oh!

joelddiaz commented Jul 7, 2020

Uh oh!

dgoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

dgoodwin Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

dgoodwin Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Fedosin commented Jul 15, 2020

Uh oh!

openshift-bot commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joelddiaz commented Jul 1, 2020 •

edited

Loading

openshift-ci-robot commented Jul 2, 2020 •

edited

Loading