Update to separate infrastructure kubeconfig from CCM config #86

sankalp-r · 2022-06-29T14:34:16Z

Fixes: #77

Signed-off-by: Sankalp Rangare sankalprangare786@gmail.com

kubevirt-bot · 2022-06-29T14:34:27Z

Hi @sankalp-r. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mfranczy · 2022-07-01T10:36:26Z

@sankalp-r I think we should rework the PR a bit, but before that...

@nirarg as I mention in the past, we should avoid having a kubeconfig content in the cloud-config file that we can mount cloud-config as configmap and kubeconfig as a secret.

I am wondering now, maybe instead of passing kubeconfig infra as a parameter we could just keep the path in the cloud config?

The change would look like this for cloud config:

from

kubeconfig: |
  <kubeconfig content>
...

to

kubeconfig: <path-to-kubeconfig>
...

I like the cloud config idea better, what do you think?

nirarg · 2022-07-03T08:41:00Z

I am wondering now, maybe instead of passing kubeconfig infra as a parameter we could just keep the path in the cloud config?

As we are already use the cloud-config, there is no reason why not use it to hold the kubeconfig path
But I'm not sure what is the right convention here
I don't have strong opinion on this
@davidvossel do you have any thoughts here?

davidvossel · 2022-07-06T15:28:20Z

As we are already use the cloud-config, there is no reason why not use it to hold the kubeconfig path
But I'm not sure what is the right convention here
I don't have strong opinion on this
@davidvossel do you have any thoughts here?

I don't think it matters much either way. I've seen this done all kinds of ways in the past.

I've even seen in other CCM controllers that the entire cloud-config is a secret, and include credentials. For example, the azure ccm does this, https://github.com/kubernetes-sigs/cloud-provider-azure/blob/a756b8e6c99b7a8e895169fffdbbe3b84ad88110/examples/out-of-tree/cloud-controller-manager.yaml#L216 I know that's what we're requesting to move away from in this PR and issue #77. I'm just pointing out that there's precedence for what we have today as well even.

So, my take is as long as the kubeconfig is in a secret, then we're good to go. If we want an option to define a custom path to the kubeconfig, then either cli option or configmap value is fine. If there's no reason for someone to alter a cli option today, then using the configmap is probably the more consistent place to consolidate configuration.

mfranczy · 2022-07-07T10:23:25Z

@sankalp-r so please proceed with kubeconfig path inside the cloud config.

davidvossel · 2022-07-07T17:43:53Z

@sankalp-r so please proceed with kubeconfig path inside the cloud config.

looking at the cloud-provider interface more closely, i think this is the right choice.

mfranczy · 2022-07-12T09:37:00Z

pkg/provider/cloud.go

@@ -4,6 +4,7 @@ import (
 	"bytes"


@sankalp-r let's rewrite these tests to Ginkgo, it's a good opportunity :)

mfranczy · 2022-07-12T09:40:11Z

pkg/provider/cloud.go

@@ -177,3 +182,16 @@ func (c *cloud) ProviderName() string {
 func (c *cloud) HasClusterID() bool {
 	return true
 }
+
+var getInfraKubeConfig = func(infraKubeConfigPath string) (string, error) {


You don't have to use anonymous function. I understand you did this for tests but since you accept a path as a parameter for this function, you could do something similar like folks for configmap tests https://github.com/kubevirt/kubevirt/blob/main/pkg/config/config-map_test.go#L41, use a temp file and inject valid or invalid kubeconfig.

mfranczy · 2022-07-13T09:06:06Z

/ok-to-test

mfranczy

Overall looks good to me, just a few minor things

mfranczy · 2022-07-13T09:33:04Z

pkg/provider/cloud_test.go

+		Expect(config).To(Equal(expectedCloudConfig))
+		Expect(err).To(BeNil())
+	},
+		Entry("With Minimal Configuration", minimalConf, makeCloudConfig(infraKubeConfigPath, "", true, true, 5), nil),


I don't think we need to have every word uppercase, could you adjust to instancesV2 tests style?

mfranczy · 2022-07-13T09:37:08Z

pkg/provider/cloud_test.go

+				_, err = kubevirtCloudProviderFactory(strings.NewReader(fmt.Sprintf("kubeconfig: %s", infraKubeConfig.Name())))
+				Expect(err).To(HaveOccurred())
+				Expect(strings.Contains(err.Error(), "couldn't get version/kind; json parse error")).To(BeTrue())
+				os.Remove(infraKubeConfig.Name())


If because of some reasons the test fails (like code change) then os.Remove won't be executed.
Since you created a context with invalid infraKubeConfig then it's ok to implement BeforeEach and AfterEach functions to initialize and cleanup in there.

mfranczy · 2022-07-13T09:38:22Z

pkg/provider/cloud_test.go

+				infraKubeConfig.Write([]byte(invalidKubeconf))
+				_, err = kubevirtCloudProviderFactory(strings.NewReader(fmt.Sprintf("kubeconfig: %s", infraKubeConfig.Name())))
+				Expect(err).To(HaveOccurred())
+				Expect(strings.Contains(err.Error(), "couldn't get version/kind; json parse error")).To(BeTrue())


To be honest I would not test if string contains something. Messages can change, it's better to test type of the error if possible, if not then I would just remove this check.

Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>

mfranczy · 2022-07-13T14:12:41Z

/approve
/lgtm

kubevirt-bot · 2022-07-13T14:13:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfranczy

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~~ [mfranczy]

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

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jun 29, 2022

kubevirt-bot requested review from davidvossel and mfranczy June 29, 2022 14:34

kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2022

kubevirt-bot added the size/M label Jun 29, 2022

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2022

sankalp-r force-pushed the update-kubevirt-config-1 branch from fc1f705 to 18d599e Compare July 7, 2022 12:58

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 7, 2022

sankalp-r force-pushed the update-kubevirt-config-1 branch from 18d599e to c672ba2 Compare July 7, 2022 13:01

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 7, 2022

mfranczy requested changes Jul 12, 2022

View reviewed changes

kubevirt-bot assigned mfranczy Jul 12, 2022

sankalp-r force-pushed the update-kubevirt-config-1 branch from c672ba2 to 787d753 Compare July 12, 2022 16:39

kubevirt-bot added size/L and removed size/M labels Jul 12, 2022

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 13, 2022

mfranczy requested changes Jul 13, 2022

View reviewed changes

sankalp-r force-pushed the update-kubevirt-config-1 branch 2 times, most recently from 3edceeb to 214ef79 Compare July 13, 2022 12:58

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 13, 2022

sankalp-r force-pushed the update-kubevirt-config-1 branch from 214ef79 to 6bee399 Compare July 13, 2022 12:59

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 13, 2022

refactor ccm config and rewrite test with ginkgo

55dd8ab

Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>

sankalp-r force-pushed the update-kubevirt-config-1 branch from 6bee399 to 55dd8ab Compare July 13, 2022 13:47

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2022

kubevirt-bot merged commit b177e12 into kubevirt:main Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to separate infrastructure kubeconfig from CCM config #86

Update to separate infrastructure kubeconfig from CCM config #86

sankalp-r commented Jun 29, 2022

kubevirt-bot commented Jun 29, 2022

mfranczy commented Jul 1, 2022 •

edited

nirarg commented Jul 3, 2022

davidvossel commented Jul 6, 2022

mfranczy commented Jul 7, 2022

davidvossel commented Jul 7, 2022

mfranczy Jul 12, 2022

mfranczy Jul 12, 2022

mfranczy commented Jul 13, 2022

mfranczy left a comment

mfranczy Jul 13, 2022

mfranczy Jul 13, 2022 •

edited

mfranczy Jul 13, 2022

mfranczy commented Jul 13, 2022

kubevirt-bot commented Jul 13, 2022

Update to separate infrastructure kubeconfig from CCM config #86

Update to separate infrastructure kubeconfig from CCM config #86

Conversation

sankalp-r commented Jun 29, 2022

kubevirt-bot commented Jun 29, 2022

mfranczy commented Jul 1, 2022 • edited

nirarg commented Jul 3, 2022

davidvossel commented Jul 6, 2022

mfranczy commented Jul 7, 2022

davidvossel commented Jul 7, 2022

mfranczy Jul 12, 2022

Choose a reason for hiding this comment

mfranczy Jul 12, 2022

Choose a reason for hiding this comment

mfranczy commented Jul 13, 2022

mfranczy left a comment

Choose a reason for hiding this comment

mfranczy Jul 13, 2022

Choose a reason for hiding this comment

mfranczy Jul 13, 2022 • edited

Choose a reason for hiding this comment

mfranczy Jul 13, 2022

Choose a reason for hiding this comment

mfranczy commented Jul 13, 2022

kubevirt-bot commented Jul 13, 2022

mfranczy commented Jul 1, 2022 •

edited

mfranczy Jul 13, 2022 •

edited