Allow to read openstack cloud provider config from a secret #89885

Fedosin · 2020-04-06T10:38:28Z

This patch introduces new changes to allow reading openstack cloud provider config from a secret.

This change includes:

Ability to read metadata values for kubelet. Since the service does not have access to the secret to read the configuration, but it needs data to download (e.g. hostname or flavor), we are trying to get it from the metadata server.
Deprecation of kubeConfig parameter. Now we read the file that was provided with --kubeconfig option.

/kind feature

Allow to read openstack cloud provider config from a secret

k8s-ci-robot · 2020-04-06T10:40:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

staging/src/k8s.io/legacy-cloud-providers/openstack/OWNERS

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

sttts · 2020-04-06T11:45:33Z

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack.go

 	}

-	secret, err := k8sClient.CoreV1().Secrets(secretNamespace).Get(context.TODO(), secretName, metav1.GetOptions{})
+	secretName := os.secretName
+	secretNamespace := os.secretNamespace


why copy values? just access directly

sttts · 2020-04-06T11:45:58Z

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack.go

 		return err
 	}

+	cfg := &Config{}


why have this here outside of the if clause if it is only used inside?

sttts · 2020-04-06T11:47:59Z

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack.go

+	if err != nil {
+		return nil, false
+	}
+


why does it matter to initialize here?

yep, I did it for consistency, but there is no need to initialize here.

sttts · 2020-04-06T11:48:53Z

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack_instances.go

@@ -44,10 +44,36 @@ const (
 	instanceShutoff = "SHUTOFF"
 )

+func (os *OpenStack) getCompute() *gophercloud.ServiceClient {


where is this used?

nowhere, removed

sttts · 2020-04-06T11:51:26Z

staging/src/k8s.io/legacy-cloud-providers/openstack/metadata.go

+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		err = fmt.Errorf("unexpected status code when reading instance type from %s: %s", instanceTypeURL, resp.Status)


why assign to err and not just return?

sttts · 2020-04-06T11:51:41Z

staging/src/k8s.io/legacy-cloud-providers/openstack/metadata.go

+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		err = fmt.Errorf("unexpected status code when reading instance address from %s: %s", url, resp.Status)


why assign to err and not just return?

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack.go

sttts · 2020-04-06T11:54:44Z

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack.go

 	}

-	err = checkOpenStackOpts(&os)
+	err := checkOpenStackOpts(&os)
 	if err != nil {


if err := ...; err != nil {

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack_instances.go

Fedosin · 2020-04-06T14:45:14Z

/test pull-kubernetes-node-e2e-containerd

k8s-ci-robot · 2020-04-06T15:53:26Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	33714e47e79df7bb5e4973aa043cbfe77a492f97	link	`/test pull-kubernetes-node-e2e-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

... when provider is uninitialised. This is a fix to downstream-only code which was originally proposed upstream as kubernetes#89885 but did not merge. It is therefore not relevant upstream. Given that we will replace the openstack legacy cloud provider in 4.12 we will not re-propose kubernetes#89885 or this fix to it. Causes all openstack.Instances() methods which require more than the local metadata service to return NotImplemented instead of crashing if the provider is not initialised.

… secret This patch brings back the downstream changes that were introduced to allow reading openstack cloud provider config from a secret. They are available in release-4.4, but were reverted in master with openshift/origin#24719 This change includes: - Ability to read metadata values for kubelet. Since the service does not have access to the secret to read the configuration, but it needs data to download (e.g. hostname or flavor), we are trying to get it from the metadata server. - Deprecation of kubeConfig parameter. Now we read the file that was provided with --kubeconfig option. Origin-commit: f95edc26155a29769b3c5b80c03755a01a87b5fc UPSTREAM: 89885: legacy-cloud-provider/openstack: include / prefix in instance ID output When we want to read an instance ID from the metadata service, cloud provider doesn't include "/" prefix, which is required for successful parsing of provider the ID later. This commit adds the missing "/" prefix to the output. UPSTREAM: 89885: SQUASH: Fix Cinder provisioning crashing on nil cloud provider OpenStack cloud provider must not use nil when provisioning a Cinder volume. UPSTREAM: 89885: SQUASH: Report OpenStack cloud initialization errors openshift-rebase(v1.24):source=dbe70e455ee UPSTREAM: <carry>: Set informer for openstack Set informer for the openstack cloud provider to ensure it is properly initialized when reading config from a secret. Upstream 89885 was closed in favor of 96750. Co-authored-by: Hemant Kumar <hekumar@redhat.com> openshift-rebase(v1.24):source=d7ecbd903e2 UPSTREAM: 89885: SQUASH: Retry fetching clouds.conf The OpenStack secret is not guaranteed to be present at the time kube-controller-manager is initialised. Co-authored-by: Martin André <m.andre@redhat.com> Co-authored-by: Pierre Prinetti <pierreprinetti@redhat.com> openshift-rebase(v1.24):source=8bc9dd29ef0 UPSTREAM: 89885: Fix panic in openstack.InstanceExistsByProviderID() ... when provider is uninitialised. This is a fix to downstream-only code which was originally proposed upstream as kubernetes#89885 but did not merge. It is therefore not relevant upstream. Given that we will replace the openstack legacy cloud provider in 4.12 we will not re-propose kubernetes#89885 or this fix to it. Causes all openstack.Instances() methods which require more than the local metadata service to return NotImplemented instead of crashing if the provider is not initialised.

k8s-ci-robot requested review from anguslees and FengyunPan2 April 6, 2020 10:39

Fedosin force-pushed the openstack_cloud_provider_secret branch from c47a87a to e0f3ae2 Compare April 6, 2020 11:15

sttts reviewed Apr 6, 2020

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack.go Show resolved Hide resolved

sttts reviewed Apr 6, 2020

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack_instances.go Show resolved Hide resolved

sttts reviewed Apr 6, 2020

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/openstack/openstack_instances.go Show resolved Hide resolved

Fedosin force-pushed the openstack_cloud_provider_secret branch from e0f3ae2 to 33714e4 Compare April 6, 2020 12:28

openshift-ci-robot mentioned this pull request Jan 11, 2022

Bug 2039377: UPSTREAM: 89885: SQUASH: Retry fetching clouds.conf openshift/kubernetes#1108

Merged

This was referenced Feb 15, 2022

[release-4.7] Bug 2054669: UPSTREAM: 89885: SQUASH: Retry fetching clouds.conf openshift/kubernetes#1178

Merged

SDN 4.11 kubernetes 1.23.0 rebase openshift/kubernetes#1186

Closed

openshift-ci-robot mentioned this pull request Mar 1, 2022

[release-4.6] Bug 2059677: UPSTREAM: 89885: SQUASH: Retry fetching clouds.conf openshift/kubernetes#1198

Merged

This was referenced Apr 20, 2022

[WIP] update to k8s(v1.24.0-rc.0) openshift/kubernetes#1248

Closed

Fix "GCE L4 load balancer: enable migration of Instance Group management out of K/K." openshift/kubernetes#1249

Closed

openshift-ci-robot mentioned this pull request May 4, 2022

Bug 2086092: update kube to v1.24.0 openshift/kubernetes#1252

Merged

This was referenced May 25, 2022

Sdn 4.11 kubernetes 1.24 openshift/kubernetes#1275

Closed

Sdn 4.11 kubernetes 1.24.0 openshift/kubernetes#1276

Closed

mdbooth mentioned this pull request Jul 8, 2022

Bug 2102383: UPSTREAM: 89885: Fix panic in openstack.InstanceExistsByProviderID() openshift/kubernetes#1315

Merged

openshift-ci-robot mentioned this pull request Jul 20, 2022

[release-4.11] Bug 2109270: UPSTREAM: 89885: Fix panic in openstack.InstanceExistsByProviderID() openshift/kubernetes#1332

Merged

openshift-ci-robot mentioned this pull request Jul 29, 2022

Updating openshift-enterprise-hyperkube images to be consistent with ART openshift/kubernetes#1309

Closed

openshift-ci-robot mentioned this pull request Aug 10, 2022

[release-4.10] Bug 2117321: UPSTREAM: 89885: Fix panic in openstack.InstanceExistsByProviderID() openshift/kubernetes#1341

Closed

This was referenced Aug 19, 2022

[WIP] k8s 1.25.0-rc.1 openshift/kubernetes#1348

Closed

WIP: Rebase 1.25 w/ etcd health check reverted openshift/kubernetes#1355

Closed

openshift-ci-robot mentioned this pull request Aug 26, 2022

WIP: Rebase 1.25 openshift/kubernetes#1357

Closed

openshift-ci-robot mentioned this pull request Sep 1, 2022

k8s 1.25.0 openshift/kubernetes#1360

Merged

openshift-ci-robot mentioned this pull request Sep 16, 2022

[WIP] 1.25 rebase openshift/kubernetes#1367

Closed

openshift-ci-robot mentioned this pull request Oct 4, 2022

Bug 2099800: bump to k8s 1.24.6 openshift/kubernetes#1381

Merged

mandre mentioned this pull request Oct 11, 2022

UPSTREAM: 89885: revert: allow to read openstack cloud provider config from a secret openshift/kubernetes#1389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to read openstack cloud provider config from a secret #89885

Allow to read openstack cloud provider config from a secret #89885

Fedosin commented Apr 6, 2020 •

edited

k8s-ci-robot commented Apr 6, 2020

sttts Apr 6, 2020

Fedosin Apr 6, 2020

sttts Apr 6, 2020

Fedosin Apr 6, 2020

sttts Apr 6, 2020

Fedosin Apr 6, 2020

sttts Apr 6, 2020

Fedosin Apr 6, 2020

sttts Apr 6, 2020 •

edited

Fedosin Apr 6, 2020

sttts Apr 6, 2020 •

edited

Fedosin Apr 6, 2020

sttts Apr 6, 2020

Fedosin Apr 6, 2020

Fedosin commented Apr 6, 2020

k8s-ci-robot commented Apr 6, 2020 •

edited

Allow to read openstack cloud provider config from a secret #89885

Allow to read openstack cloud provider config from a secret #89885

Conversation

Fedosin commented Apr 6, 2020 • edited

k8s-ci-robot commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Apr 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Apr 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedosin commented Apr 6, 2020

k8s-ci-robot commented Apr 6, 2020 • edited

Fedosin commented Apr 6, 2020 •

edited

sttts Apr 6, 2020 •

edited

sttts Apr 6, 2020 •

edited

k8s-ci-robot commented Apr 6, 2020 •

edited