Initialize the cluster client cache from the kubeconfig in secrets

[metlos]

This changes the cluster cache to initialize the connections to the clusters from the kubeconfig stored in the toolchain cluster secret.

The information from the cluster cache is also used to populate the new informational fields in the ToolchainCluster status.

Related PRs:

• host-operator: Create tc based on creds secret host-operator#1058
• toolchain-e2e: Check that ToolchainCluster status is populated toolchain-e2e#1012

[codecov]

Codecov Report

Attention: Patch coverage is 76.74419% with 10 lines in your changes missing coverage. Please review.

Project coverage is 78.57%. Comparing base (cd921d6) to head (0196128).
Report is 1 commits behind head on master.

Files	Patch %	Lines
pkg/cluster/service.go	72.72%	5 Missing and 4 partials ⚠️
...rs/toolchaincluster/toolchaincluster_controller.go	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   78.54%   78.57%   +0.03%     
==========================================
  Files          49       49              
  Lines        2023     2054      +31     
==========================================
+ Hits         1589     1614      +25     
- Misses        376      379       +3     
- Partials       58       61       +3     

Files	Coverage Δ
controllers/toolchaincluster/healthchecker.go	100.00% <ø> (ø)
...rs/toolchaincluster/toolchaincluster_controller.go	83.18% <90.00%> (+0.94%)	⬆️
pkg/cluster/service.go	83.22% <72.72%> (-1.46%)	⬇️

[MatousJobanek]

I probably know why you kept the support for the legacy config. There could be a problem as soon as you would merge a support for kubeconfig only. The reason is that we load the ToolchainCluster CRs and their configs inside main.go before we start the controllers. This means that the ToolchainCluster controller that does the migration wouldn't be started yet and it wouldn't be able to migrate the legacy fields in the secret to the kubeconfig one.
Anyway, at some point we need to make the switch, so I decided to not keep postponing this action and I updated the add-cluster.sh script so it populates the kubeconfig right away:
codeready-toolchain/toolchain-cicd#117

This means that we are ready to drop the migration logic from the ToolchainCluster controller and support kubeconfig only.

[MatousJobanek]

Looks good, but since the "kubeconfig` field is the preferred one now, could you please update the unit tests so they load the ToolchainCluster clients from kubeconfig instead of from the legacy fields?
I guess that it should be enough to update this helper function:

toolchain-common/pkg/test/cluster.go

Lines 19 to 34 in 14c41a9

	func NewToolchainClusterWithEndpoint(name, tcNs, secName, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus, labels map[string]string) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) {
	gock.New(apiEndpoint).
	Get("api").
	Persist().
	Reply(200).
	BodyString("{}")
	secret := &corev1.Secret{
	ObjectMeta: v1.ObjectMeta{
	Name: secName,
	Namespace: tcNs,
	},
	Type: corev1.SecretTypeOpaque,
	Data: map[string][]byte{
	"token": []byte("mycooltoken"),
	},
	}

[metlos]

Looks good, but since the "kubeconfig` field is the preferred one now, could you please update the unit tests so they load the ToolchainCluster clients from kubeconfig instead of from the legacy fields? I guess that it should be enough to update this helper function.

For this to make most sense, I guess we should also not use any legacy fields in the code as well as in the tests. When I do that, I get quite a substantial diff (on top of the changes in this PR). The main reason for that diff is that the function you mention now needs to contain more parameters (namely operator namespace and insecureTls) because those are now part of the kubeconfig and cannot be easily set as before when they were just part of the returned toolchain cluster spec.

$ git diff --stat
 controllers/toolchaincluster/healthchecker_test.go               | 22 ++++++++++------------
 controllers/toolchaincluster/toolchaincluster_controller.go      | 41 -----------------------------------------
 controllers/toolchaincluster/toolchaincluster_controller_test.go | 65 ++++++++++++++---------------------------------------------------
 pkg/cluster/service.go                                           | 43 +------------------------------------------
 pkg/cluster/service_test.go                                      | 80 ++++++++------------------------------------------------------------------------
 pkg/cluster/service_whitebox_test.go                             |  8 +++-----
 pkg/test/cluster.go                                              | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
 pkg/test/verify/cluster.go                                       | 80 +++++++++++++++++---------------------------------------------------------------
 8 files changed, 97 insertions(+), 291 deletions(-)

I think it makes more sense to merge this PR as is and move the test updates and removal of the migration logic to a new PR.. WDYT?

[mfrancisc]

Looks good !

I have one minor comment only.

[mfrancisc]

do we need to add a test case that uses the legacyTc?

basically:

                 tc := legacyTc()
		// Combine the kubeconfig and the token in the same secret.
		// We should see auth from the kubeconfig used...
		secret := kubeconfigSecret(t)
		secret.Data["token"] = []byte("not-the-token-we-want")

[metlos]

This essentially checks that if we detect a kubeconfig data field in the secret then we use that instead of the data in the ToolchainCluster and the token in the secret.

So as soon as we see the data coming from the kubeconfig, we know that happened.

Now, you have a very good idea for a "defensive test" that would make sure any future changes wouldn't change that behavior. But we plan to completely remove the loading from the legacy fields in the upcoming PRs so I think we can afford to not have that and instead just make sure we use the new over the old.

[mfrancisc]

Right, I was asking since as soon as you push this PR to prod, the tc will still have the legacy format, and later will have the new one. But I guess it's not a concern since the only thing that matters is the loading mechanism and not the tc format.

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
14.7% Duplication on New Code

See analysis details on SonarCloud

[metlos]

The e2e tests for this PR embedded in the host-operator are passing, so I'm merging this. See https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/codeready-toolchain_toolchain-e2e/1012/pull-ci-codeready-toolchain-toolchain-e2e-master-e2e/1828041777043476480