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

VSphereCPIConfig derives the insecure flags from the TLS thumbprint in cluster variable. #3300

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

HanFa
Copy link
Contributor

@HanFa HanFa commented Sep 9, 2022

What this PR does / why we need it

This PR changes the data values reconcile logics in the VSphereCPIConfig controller.

First, in the reconcile loop, the tlsthumbprint will be resolved in the following priority

  • spec.vsphereCPI.thumbprint in the VSphereCPIConfig CR that user specified
  • thumbprint derived from the cluster variables

Second, the controller then determine the value of insecureFlag, and make it possible to be overriden if user specifies in spec.vsphereCPI.insecure.

Which issue(s) this PR fixes

#3381

This PR aims to fix cpi package reconcilation error vsphereCPI tlsThumbprint should be provided when insecureFlag is False

Describe testing done for PR

Release note

package-based-lcm: VSphereCPIConfig derives the insecure flags from the TLS thumbprint in cluster variable.

Additional information

Special notes for your reviewer

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #3300 (06011a6) into main (81ab277) will increase coverage by 4.87%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3300      +/-   ##
==========================================
+ Coverage   46.70%   51.58%   +4.87%     
==========================================
  Files         281      122     -159     
  Lines       29654    11197   -18457     
==========================================
- Hits        13850     5776    -8074     
+ Misses      14553     4942    -9611     
+ Partials     1251      479     -772     
Impacted Files Coverage Δ
addons/controllers/machine_controller.go 65.65% <0.00%> (-3.04%) ⬇️
addons/controllers/clusterbootstrap_controller.go 63.52% <0.00%> (-1.63%) ⬇️
...ons/controllers/packageinstallstatus_controller.go 77.99% <0.00%> (-1.16%) ⬇️
pkg/v1/tkg/clusterclient/clusterclient.go
pkg/v1/tkg/cmd/common.go
pkg/v1/tkg/cmd/create.go
cmd/cli/plugin/feature/list.go
pkg/v1/cli/carvelhelpers/registry.go
pkg/v1/tkg/cmd/get_cluster.go
pkg/v1/tkg/tkgctl/describe_cluster.go
... and 152 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 changed the title Turn off the insecure flag only if both insecure and TLS thumbprint are specified in the CPIConfig VSphereCPIConfig derives the insecure flags from the TLS thumbprint in cluster variable. Sep 9, 2022
Copy link

@adduarte adduarte left a comment

Choose a reason for hiding this comment

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

Please add link to isue number this fixes with description of original problem.
Also not sure what will happen if

spec.vsphereCPI.thumbprint in the VSphereCPIConfig CR that user specified ="" (not provided)
&
thumbprint derived from the cluster variables = "" (not provided)
&
and user did not specify spec.vsphereCPI.insecure.

@HanFa HanFa self-assigned this Sep 16, 2022
@HanFa
Copy link
Contributor Author

HanFa commented Sep 16, 2022

Also not sure what will happen if

In that case, the final values of thumbprint is resolved to be empty. Hence, the insecure flag will be set to true with d.InsecureFlag = d.TLSThumbprint == ""

Copy link

@adduarte adduarte left a comment

Choose a reason for hiding this comment

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

Please make the error message a bit clearer, right now they don't quite match the logic. thanks.

@adduarte
Copy link

Please provide testing details to verify this patches fixes problem.

@adduarte
Copy link

Currently this patch will turn insecureFlag=True if thumbprintTLS is not available.
This is probably not a behavior we want. I could result in the connection being insecure without user knowing..
i.e. the absense of thumbprintTLS could be an oversight or bug.

@HanFa
Copy link
Contributor Author

HanFa commented Sep 20, 2022

per @srm09, the insecure flag from CAPV will be deprecated. An empty string in thumbprint should imply to use insecure connection.

@vijaykatam vijaykatam added the ok-to-merge PRs should be labelled with this before merging label Sep 20, 2022
Copy link
Contributor

@vuil vuil 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 merged commit 43a5968 into vmware-tanzu:main Sep 21, 2022
chandrareddyp pushed a commit that referenced this pull request Sep 25, 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.

6 participants