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.

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

🐛 CAPV CSI driver isn't passing TLS thumbprint #1819

Merged
merged 5 commits into from Jul 20, 2023

Conversation

PatrickLaabs
Copy link
Contributor

@PatrickLaabs PatrickLaabs commented Mar 8, 2023

What this PR does / why we need it:
With this PR I changed a few lines of code.

packaging/flavorgen/flavors/crs/csi.go

config.Global.Thumbprint = env.VSphereThumbprint
config.Global.Insecure = env.VSphereInsecure

packaging/flavorgen/flavors/env/envsubts_consts.go

VSphereInsecure              = true

With this, I was able to create a new 'cluster-template.yaml' inside .cluster-api/overrides... where the Global-Section of the csi-vsphere-config Secret gets the thumbprint and insecure flag passed.

  stringData:
     csi-vsphere.conf: |+
        [Global]
        insecure-flag = true
        thumbprint = "${VSPHERE_TLS_THUMBPRINT}"
        cluster-id = "${NAMESPACE}/${CLUSTER_NAME}"

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1162

Special notes for your reviewer:

Newly created PR, since on my last one(PR 1812) , I encountered some Issues with my CLA.

Notes from the last PR:

Since the Insecure-Flag is defined as a bool inside packaging/flavorgen/flavors/crs/types/cloudprovider_types.go
at line 88, I was not able to make this Insecure-Flag configurable via the clusterctl.yaml under ~/.clusterctl-api.

Changing it to a string, would make it configurable.
Like so: Insecure string gcfg:"insecure-flag,omitempty" json:"insecure,omitempty"`` but comes with the side-effect, that the file packaging/flavorgen/flavors/crs/types/cloudprovider_encoding_test.go throws me an error cannot use true (untyped bool constant) as string...
Since changing the type of the Insecure-Flag causes some impact on the testing file (And I am not sure, which side-effects this could lead to) I did not changed the typed and let it remain as a bool.
Maybe a maintainer could have a look at this and maybe help me out with this 😄

Release note:

Passing Thumbprint and Insecure-Flag (set as true in default) to the generated Cluster-Manifests file

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @PatrickLaabs. 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 8, 2023
@srm09
Copy link
Contributor

srm09 commented Mar 9, 2023

/ok-to-test

@k8s-ci-robot 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 Mar 9, 2023
@hideakihal
Copy link

hideakihal commented Mar 23, 2023

Hi.

The vSphere Container Storage Plug-in documentation states that the thumbprint option is ignored when you are using an unsecured setup or when you provide a ca-file. It seems that it is not recognized when the insecure-flag is set to true.

https://docs.vmware.com/en/VMware-vSphere-Container-Storage-Plug-in/2.0/vmware-vsphere-csp-getting-started/GUID-BFF39F1D-F70A-4360-ABC9-85BDAFBE8864.html

@srm09
Copy link
Contributor

srm09 commented Apr 25, 2023

I am wary that the shipped cluster manifests with CAPV, by default support a non secure mode of communication. You could run an alternate mechanism to overlay these changes on top of the CAPV cluster manifests instead of chaining this directly in the cluster templates.

@PatrickLaabs
Copy link
Contributor Author

@srm09 I guess I understand you thoughts.

I believe removing the insecure flag hard-coded on the templates could make sense. But the Thumbprint-passing should stay, I guess.

Letting the user decide to edit the yaml afterwards and strictly enable / disable the insecure-flag might be a better approach, isnt it?

Best,
Patrick

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 28, 2023
@PatrickLaabs
Copy link
Contributor Author

/check-cla

…humbprint passing to node-ipam-template

Author:    PatrickLaabs <patrick.laabs@me.de>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 28, 2023
@PatrickLaabs
Copy link
Contributor Author

The insecure-flag has been removed.
The Thumbprint will now be passed from the cluster-api-configfile to the yaml-files.

@randomvariable
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2023
@PatrickLaabs
Copy link
Contributor Author

/retest

@PatrickLaabs
Copy link
Contributor Author

I'll take a look at this in the next few days.
I guess I have to update my long-living branch with the latest changes to get this to work 😀

@sbueringer
Copy link
Member

/tide merge-method-squash

It's just a flake

/retest
/lgtm

Thx for fixing this!

Let's cherry-pick

@sbueringer
Copy link
Member

/cherry-pick release-1.7

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.7

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.

@sbueringer
Copy link
Member

/cherry-pick release-1.6

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.6

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.

@sbueringer
Copy link
Member

/cherry-pick release-1.5

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.5

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.

@k8s-ci-robot k8s-ci-robot merged commit 901740c into kubernetes-sigs:main Jul 20, 2023
8 checks passed
@k8s-infra-cherrypick-robot

@sbueringer: new pull request created: #2062

In response to this:

/cherry-pick release-1.6

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.

@k8s-infra-cherrypick-robot

@sbueringer: new pull request created: #2063

In response to this:

/cherry-pick release-1.5

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.

@k8s-infra-cherrypick-robot

@sbueringer: new pull request created: #2064

In response to this:

/cherry-pick release-1.7

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CAPV CSI driver isn't passing TLS thumbprint
7 participants