-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to separate infrastructure kubeconfig from CCM config #86
Update to separate infrastructure kubeconfig from CCM config #86
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
@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
to
I like the cloud config idea better, what do you think? |
As we are already use the cloud-config, there is no reason why not use it to hold the kubeconfig path |
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. |
@sankalp-r so please proceed with kubeconfig path inside the cloud config. |
fc1f705
to
18d599e
Compare
18d599e
to
c672ba2
Compare
looking at the cloud-provider interface more closely, i think this is the right choice. |
@@ -4,6 +4,7 @@ import ( | |||
"bytes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sankalp-r let's rewrite these tests to Ginkgo, it's a good opportunity :)
pkg/provider/cloud.go
Outdated
@@ -177,3 +182,16 @@ func (c *cloud) ProviderName() string { | |||
func (c *cloud) HasClusterID() bool { | |||
return true | |||
} | |||
|
|||
var getInfraKubeConfig = func(infraKubeConfigPath string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c672ba2
to
787d753
Compare
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, just a few minor things
pkg/provider/cloud_test.go
Outdated
Expect(config).To(Equal(expectedCloudConfig)) | ||
Expect(err).To(BeNil()) | ||
}, | ||
Entry("With Minimal Configuration", minimalConf, makeCloudConfig(infraKubeConfigPath, "", true, true, 5), nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to have every word uppercase, could you adjust to instancesV2 tests style?
pkg/provider/cloud_test.go
Outdated
_, 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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/provider/cloud_test.go
Outdated
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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
3edceeb
to
214ef79
Compare
214ef79
to
6bee399
Compare
Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>
6bee399
to
55dd8ab
Compare
/approve |
[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:
Approvers can indicate their approval by writing |
Fixes: #77
Signed-off-by: Sankalp Rangare sankalprangare786@gmail.com