Mounting secret - reading credentials from file #746

GunaKKIBM · 2024-09-16T04:25:18Z

What this PR does / why we need it:
Rather than getting apikey from env, the secret needs to be mounted inside controller and node server pod according CIS kubernetes benchmark

Which issue(s) this PR fixes:
#727

k8s-ci-robot · 2024-09-16T04:25:27Z

Welcome @GunaKKIBM!

It looks like this is your first PR to kubernetes-sigs/ibm-powervs-block-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/ibm-powervs-block-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-09-16T04:25:28Z

Hi @GunaKKIBM. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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-sigs/prow repository.

GunaKKIBM · 2024-09-16T04:29:31Z

Will update the tests if the code changes are ok.

mkumatag · 2024-09-18T07:58:24Z

pkg/cloud/powervs.go

@@ -60,7 +61,16 @@ func NewPowerVSCloud(cloudInstanceID, zone string, debug bool) (Cloud, error) {
 }

 func newPowerVSCloud(cloudInstanceID, zone string, debug bool) (Cloud, error) {
-	apikey := os.Getenv("IBMCLOUD_API_KEY")


I see you are completely removing the reading the API key from the env which is a breaking change, please keep both the ways.

mkumatag · 2024-09-18T08:07:13Z

/ok-to-test

mkumatag · 2024-09-19T04:53:37Z

pkg/cloud/powervs.go

@@ -252,3 +256,31 @@ func (p *powerVSCloud) GetDiskByID(volumeID string) (disk *Disk, err error) {
 		CapacityGiB: int64(*v.Size),
 	}, nil
 }
+
+func readCredentials() (string, error) {
+	apiKey, err := readCredentialsFromFile()


you are ignoring all the errors which might be problematic when program genuinely failed to read the file content.

https://github.com/kubernetes-sigs/ibm-powervs-block-csi-driver/pull/746/files#diff-27059fbf8e5d8f0370d7993eabd010230eb1cef170edd78aea391fb6f55953b2R285 - updated here, other than, is not exists, any other reading error, it is returning error, but ideally, If API_KEY_PATH is defined and if the file does not exist, that should also error out I think. Please let me know your thought.

yes, file not found should also be error

mkumatag · 2024-09-19T04:54:31Z

pkg/cloud/powervs.go

+		return apiKey, nil
+	}
+
+	apikey := os.Getenv("IBMCLOUD_API_KEY")


same variable name with just a smallcase may create a confusion, you can reuse the first variable itself

Sorry, I didn't understand this comment, earlier the env variable that was used in the code (in current main branch), is IBMCLOUD_API_KEY itself

I meant this

Suggested change

apikey := os.Getenv("IBMCLOUD_API_KEY")

apiKey = os.Getenv("IBMCLOUD_API_KEY")

Missed it for a typo. Updated

mkumatag · 2024-09-19T04:55:23Z

pkg/cloud/powervs.go

@@ -252,3 +256,31 @@ func (p *powerVSCloud) GetDiskByID(volumeID string) (disk *Disk, err error) {
 		CapacityGiB: int64(*v.Size),
 	}, nil
 }
+
+func readCredentials() (string, error) {
+	apiKey, err := readCredentialsFromFile()


adding a warning logging is better instead of no logs

https://github.com/kubernetes-sigs/ibm-powervs-block-csi-driver/pull/746/files#diff-27059fbf8e5d8f0370d7993eabd010230eb1cef170edd78aea391fb6f55953b2R280 - added for this

mkumatag · 2024-09-19T05:01:50Z

pkg/cloud/powervs.go

@@ -42,6 +42,7 @@ const (
 	PollInterval         = 5 * time.Second
 	VolumeInUseState     = "in-use"
 	VolumeAvailableState = "available"
+	secretPath           = "/var/secrets/IBMCLOUD_API_KEY"


where is this been used? I'm not sure why such mistakes aren't caught in the CI..

created #751 to track

mkumatag · 2024-09-19T05:02:39Z

deploy/kubernetes/base/controller.yaml

-                  key: IBMCLOUD_API_KEY
-                  optional: true
+            - name: API_KEY_PATH
+              value: /var/secrets/IBMCLOUD_API_KEY


is /var a standard path for the credential?

As far as I know, there's no one standard path as such, it could be /etc, /var, /mnt.

/etc looks to be better place..

mkumatag · 2024-09-20T10:00:02Z

pkg/cloud/powervs.go

+
+func readCredentialsFromFile() (string, error) {
+	apiKeyPath := os.Getenv("API_KEY_PATH")
+	if len(apiKeyPath) == 0 {


Suggested change

if len(apiKeyPath) == 0 {

if apiKeyPath == "" {

mkumatag · 2024-09-20T10:03:02Z

pkg/cloud/powervs.go

+	if len(apiKey) != 0 {
+		return apiKey, nil
+	}
+


Suggested change

klog.Info("Falling back to read IBMCLOUD_API_KEY environment variable for the key")

mkumatag · 2024-09-20T11:03:00Z

overall LGTM, please squash all the commits.

@yussufsh @kishen-v ptal..

kishen-v · 2024-09-20T13:55:28Z

Hi @GunaKKIBM,
Thanks for the PR.
/lgtm

k8s-ci-robot · 2024-09-20T13:55:33Z

@kishen-v: changing LGTM is restricted to collaborators

In response to this:

Hi @GunaKKIBM,
Thanks for the PR.
/lgtm

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-sigs/prow repository.

kishen-v · 2024-09-20T14:09:16Z

pkg/cloud/powervs.go

+	klog.Info("Falling back to read IBMCLOUD_API_KEY environment variable for the key")
+	apiKey = os.Getenv("IBMCLOUD_API_KEY")
+	if apiKey == "" {
+		return "", fmt.Errorf("IBMCLOUD_API_KEY is not provided")


nit:
We could switch to errors.New as the error string is static + has minor gains in op time. We can track this in a separate issue, altogether.

Suggested change

return "", fmt.Errorf("IBMCLOUD_API_KEY is not provided")

return "", errors.New("IBMCLOUD_API_KEY is not provided")

#755 - created a GHE to track this

yussufsh · 2024-09-24T11:28:35Z

deploy/kubernetes/base/controller.yaml

+          env:
+            - name: API_KEY_PATH
+              value: /etc/secrets/IBMCLOUD_API_KEY


Is there any reason for adding the key to this controller?

After looking unto the code, it looks like, the node update controller also needs the apikey (it is also initialising the authenticator), this apikey is needed to GetPVMInstancedetails. Hence, added here too

yussufsh · 2024-09-25T13:39:05Z

@GunaKKIBM please squash your commits. Overall the changes looks good.

yussufsh

/lgtm

k8s-ci-robot · 2024-09-27T00:55:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GunaKKIBM, yussufsh

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

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

k8s-ci-robot requested review from mkumatag and yussufsh September 16, 2024 04:25

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 16, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 16, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 16, 2024

GunaKKIBM mentioned this pull request Sep 16, 2024

CSI driver can't read credentials from file #727

Closed

mkumatag requested changes Sep 18, 2024

View reviewed changes

k8s-ci-robot 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 Sep 18, 2024

mkumatag requested changes Sep 19, 2024

View reviewed changes

mkumatag requested changes Sep 20, 2024

View reviewed changes

kishen-v reviewed Sep 20, 2024

View reviewed changes

yussufsh reviewed Sep 24, 2024

View reviewed changes

GunaKKIBM mentioned this pull request Sep 25, 2024

Use errors.New instead of fmt.Errorf #755

Closed

Mounting secret - reading credentials from file

d57e884

GunaKKIBM force-pushed the Mount-secret branch from 7810d21 to d57e884 Compare September 26, 2024 23:46

yussufsh approved these changes Sep 27, 2024

View reviewed changes

k8s-ci-robot assigned yussufsh Sep 27, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2024

k8s-ci-robot merged commit 7838d84 into kubernetes-sigs:main Sep 27, 2024
6 checks passed

yussufsh linked an issue Oct 9, 2024 that may be closed by this pull request

CSI driver can't read credentials from file #727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mounting secret - reading credentials from file #746

Mounting secret - reading credentials from file #746

GunaKKIBM commented Sep 16, 2024

k8s-ci-robot commented Sep 16, 2024

k8s-ci-robot commented Sep 16, 2024

GunaKKIBM commented Sep 16, 2024

mkumatag Sep 18, 2024

GunaKKIBM Sep 19, 2024

mkumatag commented Sep 18, 2024

mkumatag Sep 19, 2024

GunaKKIBM Sep 19, 2024

mkumatag Sep 19, 2024

GunaKKIBM Sep 26, 2024

mkumatag Sep 19, 2024

GunaKKIBM Sep 19, 2024

mkumatag Sep 19, 2024

GunaKKIBM Sep 20, 2024

mkumatag Sep 19, 2024

GunaKKIBM Sep 19, 2024

mkumatag Sep 19, 2024

mkumatag Sep 19, 2024

GunaKKIBM Sep 19, 2024

mkumatag Sep 19, 2024

mkumatag Sep 20, 2024

mkumatag Sep 20, 2024

GunaKKIBM Sep 26, 2024

mkumatag commented Sep 20, 2024

kishen-v commented Sep 20, 2024

k8s-ci-robot commented Sep 20, 2024

kishen-v Sep 20, 2024 •

edited

Loading

GunaKKIBM Sep 25, 2024

yussufsh Sep 24, 2024

GunaKKIBM Sep 25, 2024

yussufsh commented Sep 25, 2024

yussufsh left a comment

k8s-ci-robot commented Sep 27, 2024

	apikey := os.Getenv("IBMCLOUD_API_KEY")
	apiKey = os.Getenv("IBMCLOUD_API_KEY")


	klog.Info("Falling back to read IBMCLOUD_API_KEY environment variable for the key")

	return "", fmt.Errorf("IBMCLOUD_API_KEY is not provided")
	return "", errors.New("IBMCLOUD_API_KEY is not provided")

Mounting secret - reading credentials from file #746

Mounting secret - reading credentials from file #746

Conversation

GunaKKIBM commented Sep 16, 2024

k8s-ci-robot commented Sep 16, 2024

k8s-ci-robot commented Sep 16, 2024

GunaKKIBM commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag commented Sep 18, 2024

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

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

Choose a reason for hiding this comment

mkumatag commented Sep 20, 2024

kishen-v commented Sep 20, 2024

k8s-ci-robot commented Sep 20, 2024

kishen-v Sep 20, 2024 • edited Loading

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

yussufsh commented Sep 25, 2024

yussufsh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 27, 2024

kishen-v Sep 20, 2024 •

edited

Loading