Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow users to configure cloud provider and CSI Driver with different credentials #1730

Merged
merged 1 commit into from
Apr 14, 2022
Merged

Allow users to configure cloud provider and CSI Driver with different credentials #1730

merged 1 commit into from
Apr 14, 2022

Conversation

mdsgabriel
Copy link
Member

Issue #, if available:

Description of changes:
This will allow users to configure cloud provider and CSI Driver with different credentials using new environment variables.
The new environment variables are optional, and if not set, the required variables (EKSA_VSPHERE_USERNAME and EKSA_VSPHERE_PASSWORD) will be used.

Currently the same credentials are used when configuring CAPV, cloud provider and CSI driver. This change will allow users to set new environment variables to be used when configuring cloud provider and CSI credentials:

        // cloud provider credentials
	"EKSA_VSPHERE_CP_USERNAME"
	"EKSA_VSPHERE_CP_PASSWORD"
	// CSI driver credentials 
	"EKSA_VSPHERE_CSI_USERNAME"
	"EKSA_VSPHERE_CSI_PASSWORD"

The new credentials are optional, if not set then the following credentials will be used

     "EKSA_VSPHERE_USERNAME"
     "EKSA_VSPHERE_PASSWORD"

Testing (if applicable):
Added unit tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot eks-distro-bot added approved size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 31, 2022
@mdsgabriel mdsgabriel force-pushed the extra-vsphere-credentials branch 2 times, most recently from 16b372d to 1c039c4 Compare March 31, 2022 19:51
chrisdoherty4
chrisdoherty4 reviewed Apr 4, 2022
View reviewed changes
pkg/providers/vsphere/config/template-cp.yaml Outdated
@@ -1043,8 +1043,8 @@ stringData:
name: cloud-provider-vsphere-credentials
namespace: kube-system
stringData:
{{.vsphereServer}}.password: "{{.eksaVspherePassword}}"
{{.vsphereServer}}.username: "{{.eksaVsphereUsername}}"
{{.vsphereServer}}.password: "{{(or .eksaControlPlanePassword .eksaVspherePassword)}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a plan to migrate to 1 way of doing this? In general, 1 way is better than 2 but with env variables I feel like its more of a necessity as it can be confusing whats configuring what.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point this should be modified to use something other than environment variables.

jiayiwang7
jiayiwang7 reviewed Apr 6, 2022
View reviewed changes
pkg/providers/vsphere/config/template-cp.yaml Outdated
@@ -577,8 +577,8 @@ stringData:
thumbprint = "{{.thumbprint}}"

[VirtualCenter "{{.vsphereServer}}"]
user = "{{.eksaVsphereUsername}}"
password = "{{.eksaVspherePassword}}"
user = "{{( or .eksaCSIUsername .eksaVsphereUsername)}}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel it's more clear to handle this in vsphere.go when generating the vars instead of using or in template.

Here u can just user = "{{( .eksaCSIUsername )}}" and in vsphere.go if the env does not exist, use eksaCSIUsername=eksaVsphereUsername

Same thing for password.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@chrisdoherty4
Copy link
Contributor

Could we also change the username and password to be wrapped in single quotes making them literals so escape characters don't cause issues?

chrisdoherty4
chrisdoherty4 suggested changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@chrisdoherty4 chrisdoherty4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@mdsgabriel mdsgabriel force-pushed the extra-vsphere-credentials branch 2 times, most recently from 54c51de to baac31a Compare April 12, 2022 00:35
@mdsgabriel
Copy link
Member Author

Could we also change the username and password to be wrapped in single quotes making them literals so escape characters don't cause issues?

done

@abhay-krishna
Copy link
Member

/override eks-anywhere-release-tooling-test-presubmit

@eks-distro-bot
Copy link
Collaborator

@abhay-krishna: Overrode contexts on behalf of abhay-krishna: eks-anywhere-release-tooling-test-presubmit

In response to this:

/override eks-anywhere-release-tooling-test-presubmit

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.

@eks-distro-bot
Copy link
Collaborator

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

Test name Commit Details Rerun command
eks-anywhere-release-tooling-test-presubmit baac31a link /test eks-anywhere-release-tooling-test-presubmit

Full PR test history. Your PR dashboard.

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.

jiayiwang7
jiayiwang7 approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@jiayiwang7 jiayiwang7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@mdsgabriel
Copy link
Member Author

Could we also change the username and password to be wrapped in single quotes making them literals so escape characters don't cause issues?

done

decided not to use single quotes for now

@mdsgabriel
Allow users to pass extra vsphere credentials
c4df74f 
This will allow users to provide different credentials
for cloud provider and CSI driver
jiayiwang7
jiayiwang7 approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@jiayiwang7 jiayiwang7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@jiayiwang7
Copy link
Member

/approve
/lgtm

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiayiwang7

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@eks-distro-bot eks-distro-bot merged commit 98fb7c1 into aws:main Apr 14, 2022
@mdsgabriel mdsgabriel mentioned this pull request May 4, 2022
Merged
@mitalipaygude mitalipaygude added this to the next milestone May 4, 2022
@jaxesn jaxesn removed this from the next milestone May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisdoherty4 chrisdoherty4 chrisdoherty4 approved these changes

@jiayiwang7 jiayiwang7 jiayiwang7 approved these changes

@a-cool-train a-cool-train Awaiting requested review from a-cool-train

@vivek-koppuru vivek-koppuru Awaiting requested review from vivek-koppuru

Assignees

@chrisdoherty4 chrisdoherty4

@jiayiwang7 jiayiwang7

Labels
approved lgtm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mdsgabriel @chrisdoherty4 @abhay-krishna @eks-distro-bot @jiayiwang7 @jaxesn @mitalipaygude