Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Use CAPV util function to retrieve VSphere cluster credentials. #3391

Merged

Conversation

HanFa
Copy link
Contributor

@HanFa HanFa commented Sep 16, 2022

What this PR does / why we need it

To retrieve the credential from a VSphereCluster, instead of just getting it from the associated secret object, we use CAPV util function to cover the case of multi-tenancy.

Which issue(s) this PR fixes

Fixes #

#3392

Describe testing done for PR

Addon Controller integration test with KUBEBUILDER_ASSETS=/Users/hanfa/workspace/tanzu-framework-HanFa/hack/tools/bin/kubebuilder/bin go test ./controllers -v

Manually testing for non-paravirt uTKG

Release note

package-based-lcm: Use CAPV util function to retrieve VSphere cluster credentials.

Additional information

Special notes for your reviewer

@HanFa HanFa requested review from a team as code owners September 16, 2022 23:04
@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch from 77d4711 to dd76d2c Compare September 16, 2022 23:10
@HanFa HanFa changed the title [WIP] Use CAPV util function to retrieve VSphere cluster credentials. Use CAPV util function to retrieve VSphere cluster credentials. Sep 16, 2022
@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch 2 times, most recently from 1d8b269 to 78f3c96 Compare September 17, 2022 00:29
@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #3391 (f783cbc) into main (142aa6f) will increase coverage by 0.85%.
The diff coverage is 90.05%.

@@            Coverage Diff             @@
##             main    #3391      +/-   ##
==========================================
+ Coverage   53.12%   53.97%   +0.85%     
==========================================
  Files         103       91      -12     
  Lines       10419    10006     -413     
==========================================
- Hits         5535     5401     -134     
+ Misses       4429     4171     -258     
+ Partials      455      434      -21     
Impacted Files Coverage Δ
...sphere-template-resolver/templateresolver/types.go 42.10% <42.10%> (ø)
...ter/vsphere-template-resolver/template/resolver.go 89.91% <89.91%> (ø)
...emplate-resolver/templateresolver/resolver_impl.go 99.02% <99.02%> (ø)
...re-template-resolver/templateresolver/interface.go 100.00% <100.00%> (ø)
addons/controllers/clusterbootstrap_controller.go 64.50% <0.00%> (-0.05%) ⬇️
capabilities/client/pkg/discovery/cluster_gvr.go
...apabilities/client/pkg/discovery/cluster_object.go
...apabilities/client/pkg/discovery/cluster_schema.go
capabilities/client/pkg/discovery/discovery.go
capabilities/client/pkg/discovery/fake.go
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch from 78f3c96 to 4366568 Compare September 19, 2022 18:26
Copy link
Contributor

@lubronzhan lubronzhan left a comment

Choose a reason for hiding this comment

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

lgtm

@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch 5 times, most recently from 4a3767b to f315547 Compare September 19, 2022 23:17
@randomvariable
Copy link
Contributor

Split the testutil changes into a separate commit, otherwise lgtm

@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch from f315547 to e30e6c4 Compare September 22, 2022 20:16
@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch 2 times, most recently from 3eaabbb to 219b535 Compare September 23, 2022 19:04
@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch 4 times, most recently from 9f46115 to 59a8953 Compare September 26, 2022 16:46
@HanFa HanFa force-pushed the topic/fhan/fix-cpi-csi-to-support-multi-tenancy branch from 59a8953 to f783cbc Compare September 26, 2022 18:33
@maralavi
Copy link
Contributor

@HanFa Will a resource of kind VSphereClusterIdentity or an identityRef on a VsphereCluster exist for sure in a cluster? How credentials would be fetched if they don’t?

@HanFa
Copy link
Contributor Author

HanFa commented Sep 26, 2022

Will a resource of kind VSphereClusterIdentity or an identityRef on a VsphereCluster exist for sure in a cluster? How credentials would be fetched if they don’t?

Hi @maralavi , not necessarily. If the VSphereClusterIdentity doesn't exist, the secretRef type should be Secret instead, the controller shall grab the credentials from the ref-ed secret object.

@maralavi
Copy link
Contributor

maralavi commented Sep 26, 2022

Will a resource of kind VSphereClusterIdentity or an identityRef on a VsphereCluster exist for sure in a cluster? How credentials would be fetched if they don’t?

Hi @maralavi , not necessarily. If the VSphereClusterIdentity doesn't exist, the secretRef type should be Secret instead, the controller shall grab the credentials from the ref-ed secret object.

Thanks for the clarification @HanFa . Can we expect that capvidentity.GetCredentials() take care of that case too? (I ask as now getSecret() is removed?)

@HanFa
Copy link
Contributor Author

HanFa commented Sep 26, 2022

Will a resource of kind VSphereClusterIdentity or an identityRef on a VsphereCluster exist for sure in a cluster? How credentials would be fetched if they don’t?

Hi @maralavi , not necessarily. If the VSphereClusterIdentity doesn't exist, the secretRef type should be Secret instead, the controller shall grab the credentials from the ref-ed secret object.

Thanks for the clarification @HanFa . Can we expect that capvidentity.GetCredentials() take care of that case too? (I ask as now getSecret() is removed?)

yes, that's exactly the case

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/9492c4bb01f03a0cec534311ed1d61515e94eddf/pkg/identity/identity.go#L53

@maralavi
Copy link
Contributor

Changes look good to me, thanks for the changes. Lets wait for the test pipeline pass before merging.

@vmware-tanzu vmware-tanzu deleted a comment from HanFa Sep 26, 2022
@maralavi maralavi added the ok-to-merge PRs should be labelled with this before merging label Sep 26, 2022
@HanFa HanFa merged commit 47eb5ff into vmware-tanzu:main Sep 27, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants