From 28a92959306a31dc5c6017f7dc1c55eac165ec52 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 26 Aug 2024 17:00:13 +0200 Subject: [PATCH 1/6] Remove the migration step to kubeconfig and update the tests to not test for the legacy fields in ToolchainCluster. --- .../toolchaincluster/healthchecker_test.go | 22 +++-- .../toolchaincluster_controller.go | 41 ---------- .../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(-) diff --git a/controllers/toolchaincluster/healthchecker_test.go b/controllers/toolchaincluster/healthchecker_test.go index 8da6e932..5cd7c48b 100644 --- a/controllers/toolchaincluster/healthchecker_test.go +++ b/controllers/toolchaincluster/healthchecker_test.go @@ -14,21 +14,20 @@ import ( ) func TestClusterHealthChecks(t *testing.T) { - // given defer gock.Off() tcNs := "test-namespace" - gock.New("http://cluster.com"). + gock.New("https://cluster.com"). Get("healthz"). Persist(). Reply(200). BodyString("ok") - gock.New("http://unstable.com"). + gock.New("https://unstable.com"). Get("healthz"). Persist(). Reply(200). BodyString("unstable") - gock.New("http://not-found.com"). + gock.New("https://not-found.com"). Get("healthz"). Persist(). Reply(404) @@ -41,25 +40,25 @@ func TestClusterHealthChecks(t *testing.T) { }{ "HealthOkay": { tcType: "stable", - apiEndPoint: "http://cluster.com", + apiEndPoint: "https://cluster.com", healthCheck: true, }, "HealthNotOkayButNoError": { tcType: "unstable", - apiEndPoint: "http://unstable.com", + apiEndPoint: "https://unstable.com", healthCheck: false, }, "ErrorWhileDoingHealth": { tcType: "Notfound", - apiEndPoint: "http://not-found.com", + apiEndPoint: "https://not-found.com", healthCheck: false, err: fmt.Errorf("the server could not find the requested resource"), }, } for k, tc := range tests { t.Run(k, func(t *testing.T) { - //given - tcType, sec := newToolchainCluster(tc.tcType, tcNs, tc.apiEndPoint, toolchainv1alpha1.ToolchainClusterStatus{}) + // given + tcType, sec := newToolchainCluster(t, tc.tcType, tcNs, tc.apiEndPoint, toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, tcType, sec) reset := setupCachedClusters(t, cl, tcType) defer reset() @@ -68,17 +67,16 @@ func TestClusterHealthChecks(t *testing.T) { cacheClient, err := kubeclientset.NewForConfig(cachedTC.RestConfig) require.NoError(t, err) - //when + // when healthCheck, err := getClusterHealthStatus(context.TODO(), cacheClient) - //then + // then require.Equal(t, tc.healthCheck, healthCheck) if tc.err != nil { require.EqualError(t, err, tc.err.Error()) } else { require.NoError(t, err) } - }) } } diff --git a/controllers/toolchaincluster/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go index c64732b8..df2bf830 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -1,7 +1,6 @@ package toolchaincluster import ( - "bytes" "context" "fmt" "time" @@ -13,7 +12,6 @@ import ( kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" kubeclientset "k8s.io/client-go/kubernetes" - "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -66,10 +64,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } - if err = r.migrateSecretToKubeConfig(ctx, toolchainCluster); err != nil { - return reconcile.Result{}, err - } - clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) if err != nil { reqLogger.Error(err, "cannot create ClientSet for the ToolchainCluster") @@ -152,41 +146,6 @@ func clusterNotReadyCondition() toolchainv1alpha1.Condition { } } -func (r *Reconciler) migrateSecretToKubeConfig(ctx context.Context, tc *toolchainv1alpha1.ToolchainCluster) error { - if len(tc.Spec.SecretRef.Name) == 0 { - return nil - } - - secret := &corev1.Secret{} - if err := r.Client.Get(ctx, client.ObjectKey{Name: tc.Spec.SecretRef.Name, Namespace: tc.Namespace}, secret); err != nil { - return err - } - - token := secret.Data["token"] - apiEndpoint := tc.Spec.APIEndpoint - operatorNamespace := tc.Labels["namespace"] - insecureTls := len(tc.Spec.DisabledTLSValidations) == 1 && tc.Spec.DisabledTLSValidations[0] == "*" - // we ignore the Spec.CABundle here because we don't want it migrated. The new configurations are free - // to use the certificate data for the connections but we don't want to migrate the existing certificates. - kubeConfig := composeKubeConfigFromData(token, apiEndpoint, operatorNamespace, insecureTls) - - data, err := clientcmd.Write(kubeConfig) - if err != nil { - return err - } - - origKubeConfigData := secret.Data["kubeconfig"] - secret.Data["kubeconfig"] = data - - if !bytes.Equal(origKubeConfigData, data) { - if err = r.Client.Update(ctx, secret); err != nil { - return err - } - } - - return nil -} - func composeKubeConfigFromData(token []byte, apiEndpoint, operatorNamespace string, insecureTls bool) clientcmdapi.Config { return clientcmdapi.Config{ Contexts: map[string]*clientcmdapi.Context{ diff --git a/controllers/toolchaincluster/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go index 6f47a9fc..fd344e6e 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -9,17 +9,13 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/codeready-toolchain/toolchain-common/pkg/test" - "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" - "gotest.tools/assert/cmp" corev1 "k8s.io/api/core/v1" kubeclientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -32,24 +28,24 @@ func TestClusterControllerChecks(t *testing.T) { defer gock.Off() tcNs := "test-namespace" - gock.New("http://cluster.com"). + gock.New("https://cluster.com"). Get("healthz"). Persist(). Reply(200). BodyString("ok") - gock.New("http://unstable.com"). + gock.New("https://unstable.com"). Get("healthz"). Persist(). Reply(200). BodyString("unstable") - gock.New("http://not-found.com"). + gock.New("https://not-found.com"). Get("healthz"). Persist(). Reply(404) t.Run("ToolchainCluster not found", func(t *testing.T) { // given - NotFound, sec := newToolchainCluster("notfound", tcNs, "http://not-found.com", toolchainv1alpha1.ToolchainClusterStatus{}) + NotFound, sec := newToolchainCluster(t, "notfound", tcNs, "http://not-found.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, sec) reset := setupCachedClusters(t, cl, NotFound) @@ -66,7 +62,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("Error while getting ToolchainCluster", func(t *testing.T) { // given - tc, sec := newToolchainCluster("tc", tcNs, "http://tc.com", toolchainv1alpha1.ToolchainClusterStatus{}) + tc, sec := newToolchainCluster(t, "tc", tcNs, "http://tc.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, sec) cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { @@ -87,7 +83,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("reconcile successful and requeued", func(t *testing.T) { // given - stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + stable, sec := newToolchainCluster(t, "stable", tcNs, "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) @@ -106,7 +102,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("toolchain cluster cache not found", func(t *testing.T) { // given - unstable, _ := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) + unstable, _ := newToolchainCluster(t, "unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, unstable) controller, req := prepareReconcile(unstable, cl, requeAfter) @@ -121,7 +117,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("error while updating a toolchain cluster status on cache not found", func(t *testing.T) { // given - stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + stable, _ := newToolchainCluster(t, "stable", tcNs, "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, stable) cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { @@ -141,7 +137,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("error while updating a toolchain cluster status when health-check failed", func(t *testing.T) { // given - stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + stable, sec := newToolchainCluster(t, "stable", tcNs, "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) expectedErr := fmt.Errorf("my test error") cl := test.NewFakeClient(t, stable, sec) cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { @@ -162,46 +158,12 @@ func TestClusterControllerChecks(t *testing.T) { require.Equal(t, reconcile.Result{}, recResult) assertClusterStatus(t, cl, "stable") }) - - t.Run("migrates connection settings to kubeconfig in secret", func(t *testing.T) { - // given - tc, secret := newToolchainCluster("tc", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) - cl := test.NewFakeClient(t, tc, secret) - reset := setupCachedClusters(t, cl, tc) - defer reset() - - controller, req := prepareReconcile(tc, cl, requeAfter) - expectedKubeConfig := composeKubeConfigFromData([]byte("mycooltoken"), "http://cluster.com", "test-namespace", true) - - // when - _, err := controller.Reconcile(context.TODO(), req) - secretAfterReconcile := &corev1.Secret{} - require.NoError(t, cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(secret), secretAfterReconcile)) - actualKubeConfig, loadErr := clientcmd.Load(secretAfterReconcile.Data["kubeconfig"]) - - // then - require.NoError(t, err) - require.NoError(t, loadErr) - assert.Contains(t, secretAfterReconcile.Data, "kubeconfig") - - // we need to use this more complex equals, because we don't initialize the Extension fields (i.e. they're nil) - // while they're initialized and empty after deserialization, which causes the "normal" deep equals to fail. - result := cmp.DeepEqual(expectedKubeConfig, *actualKubeConfig, - cmpopts.IgnoreFields(clientcmdapi.Config{}, "Extensions"), - cmpopts.IgnoreFields(clientcmdapi.Preferences{}, "Extensions"), - cmpopts.IgnoreFields(clientcmdapi.Cluster{}, "Extensions"), - cmpopts.IgnoreFields(clientcmdapi.AuthInfo{}, "Extensions"), - cmpopts.IgnoreFields(clientcmdapi.Context{}, "Extensions"), - )() - - assert.True(t, result.Success()) - }) } func TestGetClusterHealth(t *testing.T) { t.Run("Check health default", func(t *testing.T) { // given - stable, sec := newToolchainCluster("stable", "test-namespace", "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + stable, sec := newToolchainCluster(t, "stable", "test-namespace", "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) @@ -222,7 +184,7 @@ func TestGetClusterHealth(t *testing.T) { }) t.Run("get health condition when health obtained is false ", func(t *testing.T) { // given - stable, sec := newToolchainCluster("stable", "test-namespace", "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + stable, sec := newToolchainCluster(t, "stable", "test-namespace", "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) @@ -242,6 +204,7 @@ func TestGetClusterHealth(t *testing.T) { assertClusterStatus(t, cl, "stable", clusterNotReadyCondition()) }) } + func TestComposeKubeConfig(t *testing.T) { // when kubeConfig := composeKubeConfigFromData([]byte("token"), "http://over.the.rainbow", "the-namespace", false) @@ -275,8 +238,8 @@ func setupCachedClusters(t *testing.T, cl *test.FakeClient, clusters ...*toolcha } } -func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { - toolchainCluster, secret := test.NewToolchainClusterWithEndpoint(name, tcNs, "secret", apiEndpoint, status, map[string]string{"namespace": "test-namespace"}) +func newToolchainCluster(t *testing.T, name, tcNs string, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus, insecureTls bool) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { + toolchainCluster, secret := test.NewToolchainClusterWithEndpoint(t, name, tcNs, "test-namespace", "secret", apiEndpoint, status, insecureTls) return toolchainCluster, secret } diff --git a/pkg/cluster/service.go b/pkg/cluster/service.go index 31ba528e..4ef2c924 100644 --- a/pkg/cluster/service.go +++ b/pkg/cluster/service.go @@ -174,48 +174,7 @@ func NewClusterConfig(cl client.Client, toolchainCluster *toolchainv1alpha1.Tool return nil, errors.Wrapf(err, "unable to get secret %s for cluster %s", name, toolchainCluster.Name) } - if _, ok := secret.Data["kubeconfig"]; ok { - return loadConfigFromKubeConfig(toolchainCluster, secret, timeout) - } else { - return loadConfigFromLegacyToolchainCluster(toolchainCluster, secret, timeout) - } -} - -func loadConfigFromLegacyToolchainCluster(toolchainCluster *toolchainv1alpha1.ToolchainCluster, secret *v1.Secret, timeout time.Duration) (*Config, error) { - clusterName := toolchainCluster.Name - - apiEndpoint := toolchainCluster.Spec.APIEndpoint - if apiEndpoint == "" { - return nil, errors.Errorf("the api endpoint of cluster %s is empty", clusterName) - } - - token, tokenFound := secret.Data[toolchainTokenKey] - if !tokenFound || len(token) == 0 { - return nil, errors.Errorf("the secret for cluster %s is missing a non-empty value for %q", clusterName, toolchainTokenKey) - } - - restConfig, err := clientcmd.BuildConfigFromFlags(apiEndpoint, "") - if err != nil { - return nil, err - } - - if len(toolchainCluster.Spec.DisabledTLSValidations) == 1 && - toolchainCluster.Spec.DisabledTLSValidations[0] == toolchainv1alpha1.TLSAll { - restConfig.Insecure = true - } - restConfig.BearerToken = string(token) - restConfig.QPS = toolchainAPIQPS - restConfig.Burst = toolchainAPIBurst - restConfig.Timeout = timeout - - return &Config{ - Name: clusterName, - APIEndpoint: apiEndpoint, - RestConfig: restConfig, - OperatorNamespace: toolchainCluster.Labels[labelNamespace], - OwnerClusterName: toolchainCluster.Labels[labelOwnerClusterName], - Labels: toolchainCluster.Labels, - }, nil + return loadConfigFromKubeConfig(toolchainCluster, secret, timeout) } func loadConfigFromKubeConfig(toolchainCluster *toolchainv1alpha1.ToolchainCluster, secret *v1.Secret, timeout time.Duration) (*Config, error) { diff --git a/pkg/cluster/service_test.go b/pkg/cluster/service_test.go index b8630bdf..21c551ec 100644 --- a/pkg/cluster/service_test.go +++ b/pkg/cluster/service_test.go @@ -75,10 +75,10 @@ func TestListToolchainClusterConfigs(t *testing.T) { status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) require.NoError(t, toolchainv1alpha1.AddToScheme(scheme.Scheme)) - m1, sec1 := test.NewToolchainClusterWithEndpoint("east", test.HostOperatorNs, "secret1", "http://m1.com", status, map[string]string{"ownerClusterName": "m1ClusterName", "namespace": test.MemberOperatorNs, cluster.RoleLabel(cluster.Tenant): ""}) - m2, sec2 := test.NewToolchainClusterWithEndpoint("west", test.HostOperatorNs, "secret2", "http://m2.com", status, map[string]string{"ownerClusterName": "m2ClusterName", "namespace": test.MemberOperatorNs, cluster.RoleLabel(cluster.Tenant): ""}) - host, secHost := test.NewToolchainCluster("host", test.MemberOperatorNs, "secretHost", status, verify.Labels(test.HostOperatorNs, "hostClusterName")) - noise, secNoise := test.NewToolchainCluster("noise", "noise-namespace", "secretNoise", status, verify.Labels(test.MemberOperatorNs, "noiseClusterName")) + m1, sec1 := test.NewToolchainClusterWithEndpoint(t, "east", test.HostOperatorNs, test.MemberOperatorNs, "secret1", "http://m1.com", status, false) + m2, sec2 := test.NewToolchainClusterWithEndpoint(t, "west", test.HostOperatorNs, test.MemberOperatorNs, "secret2", "http://m2.com", status, false) + host, secHost := test.NewToolchainCluster(t, "host", test.MemberOperatorNs, test.HostOperatorNs, "secretHost", status, false) + noise, secNoise := test.NewToolchainCluster(t, "noise", "noise-namespace", "secretNoise", test.MemberOperatorNs, status, false) require.NoError(t, toolchainv1alpha1.AddToScheme(scheme.Scheme)) cl := test.NewFakeClient(t, m1, m2, host, noise, sec1, sec2, secHost, secNoise) @@ -93,16 +93,12 @@ func TestListToolchainClusterConfigs(t *testing.T) { verify.AssertClusterConfigThat(t, clusterConfigs[0]). HasName("east"). HasOperatorNamespace("toolchain-member-operator"). - HasOwnerClusterName("m1ClusterName"). HasAPIEndpoint("http://m1.com"). - ContainsLabel(cluster.RoleLabel(cluster.Tenant)). // the value is not used only the key matters RestConfigHasHost("http://m1.com") verify.AssertClusterConfigThat(t, clusterConfigs[1]). HasName("west"). HasOperatorNamespace("toolchain-member-operator"). - HasOwnerClusterName("m2ClusterName"). HasAPIEndpoint("http://m2.com"). - ContainsLabel(cluster.RoleLabel(cluster.Tenant)). // the value is not used only the key matters RestConfigHasHost("http://m2.com") }) @@ -118,9 +114,8 @@ func TestListToolchainClusterConfigs(t *testing.T) { verify.AssertClusterConfigThat(t, clusterConfigs[0]). HasName("host"). HasOperatorNamespace("toolchain-host-operator"). - HasOwnerClusterName("hostClusterName"). - HasAPIEndpoint("http://cluster.com"). - RestConfigHasHost("http://cluster.com") + HasAPIEndpoint("https://cluster.com"). + RestConfigHasHost("https://cluster.com") }) t.Run("list members when there is none present", func(t *testing.T) { @@ -167,24 +162,7 @@ func TestListToolchainClusterConfigs(t *testing.T) { } func TestNewClusterConfig(t *testing.T) { - legacyTc := func() *toolchainv1alpha1.ToolchainCluster { - return &toolchainv1alpha1.ToolchainCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tc", - Namespace: "ns", - Labels: map[string]string{ - "namespace": "operatorns", - }, - }, - Spec: toolchainv1alpha1.ToolchainClusterSpec{ - APIEndpoint: "https://over.the.rainbow", - SecretRef: toolchainv1alpha1.LocalSecretReference{ - Name: "secret", - }, - }, - } - } - newFormTc := func() *toolchainv1alpha1.ToolchainCluster { + tc := func() *toolchainv1alpha1.ToolchainCluster { return &toolchainv1alpha1.ToolchainCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "tc", @@ -198,17 +176,6 @@ func TestNewClusterConfig(t *testing.T) { } } - legacySecret := func() *corev1.Secret { - return &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "secret", - Namespace: "ns", - }, - Data: map[string][]byte{ - "token": []byte("token"), - }, - } - } kubeconfigSecret := func(t *testing.T) *corev1.Secret { t.Helper() kubeconfig := clientcmdapi.Config{ @@ -245,40 +212,9 @@ func TestNewClusterConfig(t *testing.T) { } } - t.Run("using legacy fields in ToolchainCluster and token in secret", func(t *testing.T) { - tc := legacyTc() - secret := legacySecret() - - cl := test.NewFakeClient(t, tc, secret) - - cfg, err := cluster.NewClusterConfig(cl, tc, 1*time.Second) - require.NoError(t, err) - - assert.Equal(t, "https://over.the.rainbow", cfg.APIEndpoint) - assert.Equal(t, "operatorns", cfg.OperatorNamespace) - assert.Equal(t, "token", cfg.RestConfig.BearerToken) - }) - t.Run("using kubeconfig in secret", func(t *testing.T) { - tc := newFormTc() - secret := kubeconfigSecret(t) - - cl := test.NewFakeClient(t, tc, secret) - - cfg, err := cluster.NewClusterConfig(cl, tc, 1*time.Second) - require.NoError(t, err) - - assert.Equal(t, "https://over.the.rainbow", cfg.APIEndpoint) - assert.Equal(t, "operatorns", cfg.OperatorNamespace) - assert.Equal(t, "token", cfg.RestConfig.BearerToken) - }) - - t.Run("uses kubeconfig in precedence over legacy fields", func(t *testing.T) { - tc := newFormTc() - // Combine the kubeconfig and the token in the same secret. - // We should see auth from the kubeconfig used... + tc := tc() secret := kubeconfigSecret(t) - secret.Data["token"] = []byte("not-the-token-we-want") cl := test.NewFakeClient(t, tc, secret) diff --git a/pkg/cluster/service_whitebox_test.go b/pkg/cluster/service_whitebox_test.go index 7a2c4796..3a9aa839 100644 --- a/pkg/cluster/service_whitebox_test.go +++ b/pkg/cluster/service_whitebox_test.go @@ -20,7 +20,7 @@ func TestRefreshCacheInService(t *testing.T) { // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - toolchainCluster, sec := test.NewToolchainCluster("east", test.HostOperatorNs, "secret", status, map[string]string{"ownerClusterName": test.NameMember, "namespace": test.MemberOperatorNs}) + toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.HostOperatorNs, test.MemberOperatorNs, "secret", status, false) s := scheme.Scheme err := toolchainv1alpha1.AddToScheme(s) require.NoError(t, err) @@ -75,8 +75,7 @@ func TestUpdateClientBasedOnRestConfig(t *testing.T) { // given defer gock.Off() statusTrue := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - toolchainCluster1, sec1 := test.NewToolchainCluster("east", test.HostOperatorNs, "secret1", statusTrue, - map[string]string{"namespace": test.HostOperatorNs}) + toolchainCluster1, sec1 := test.NewToolchainCluster(t, "east", test.HostOperatorNs, test.HostOperatorNs, "secret1", statusTrue, false) t.Run("don't update when RestConfig is the same", func(t *testing.T) { // given @@ -136,6 +135,5 @@ func newToolchainClusterService(cl client.Client, timeout time.Duration, tcNs st func assertMemberCluster(t *testing.T, cachedCluster *CachedToolchainCluster, status toolchainv1alpha1.ToolchainClusterStatus) { assert.Equal(t, status, *cachedCluster.ClusterStatus) - assert.Equal(t, test.NameMember, cachedCluster.OwnerClusterName) - assert.Equal(t, "http://cluster.com", cachedCluster.APIEndpoint) + assert.Equal(t, "https://cluster.com", cachedCluster.APIEndpoint) } diff --git a/pkg/test/cluster.go b/pkg/test/cluster.go index cc58f6fb..a259fa23 100644 --- a/pkg/test/cluster.go +++ b/pkg/test/cluster.go @@ -1,10 +1,15 @@ package test import ( + "testing" + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/clientcmd" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ) const ( @@ -12,16 +17,21 @@ const ( NameMember = "east" ) -func NewToolchainCluster(name, tcNs, secName string, status toolchainv1alpha1.ToolchainClusterStatus, labels map[string]string) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { - return NewToolchainClusterWithEndpoint(name, tcNs, secName, "http://cluster.com", status, labels) +func NewToolchainCluster(t *testing.T, name, tcNs, operatorNs, secName string, status toolchainv1alpha1.ToolchainClusterStatus, insecure bool) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { + t.Helper() + return NewToolchainClusterWithEndpoint(t, name, tcNs, operatorNs, secName, "https://cluster.com", status, insecure) } -func NewToolchainClusterWithEndpoint(name, tcNs, secName, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus, labels map[string]string) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { +func NewToolchainClusterWithEndpoint(t *testing.T, name, tcNs, operatorNs, secName, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus, insecureTls bool) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { + t.Helper() gock.New(apiEndpoint). Get("api"). Persist(). Reply(200). BodyString("{}") + + kubeConfig := createKubeConfigContent(t, createKubeConfig(apiEndpoint, operatorNs, "mycooltoken", insecureTls)) + secret := &corev1.Secret{ ObjectMeta: v1.ObjectMeta{ Name: secName, @@ -29,7 +39,7 @@ func NewToolchainClusterWithEndpoint(name, tcNs, secName, apiEndpoint string, st }, Type: corev1.SecretTypeOpaque, Data: map[string][]byte{ - "token": []byte("mycooltoken"), + "kubeconfig": kubeConfig, }, } @@ -45,7 +55,6 @@ func NewToolchainClusterWithEndpoint(name, tcNs, secName, apiEndpoint string, st ObjectMeta: v1.ObjectMeta{ Name: name, Namespace: tcNs, - Labels: labels, }, Status: status, }, secret @@ -59,3 +68,33 @@ func NewClusterStatus(conType toolchainv1alpha1.ConditionType, conStatus corev1. }}, } } + +func createKubeConfig(apiEndpoint, namespace, token string, insecureTls bool) *clientcmdapi.Config { + return &clientcmdapi.Config{ + Clusters: map[string]*clientcmdapi.Cluster{ + "cluster": { + Server: apiEndpoint, + InsecureSkipTLSVerify: insecureTls, + }, + }, + AuthInfos: map[string]*clientcmdapi.AuthInfo{ + "auth": { + Token: token, + }, + }, + Contexts: map[string]*clientcmdapi.Context{ + "ctx": { + AuthInfo: "auth", + Cluster: "cluster", + Namespace: namespace, + }, + }, + CurrentContext: "ctx", + } +} + +func createKubeConfigContent(t *testing.T, kubeConfig *clientcmdapi.Config) []byte { + data, err := clientcmd.Write(*kubeConfig) + require.NoError(t, err) + return data +} diff --git a/pkg/test/verify/cluster.go b/pkg/test/verify/cluster.go index 2911711c..4852154c 100644 --- a/pkg/test/verify/cluster.go +++ b/pkg/test/verify/cluster.go @@ -43,6 +43,7 @@ var testCases = map[string]struct { } func AddToolchainClusterAsMember(t *testing.T, functionToVerify FunctionToVerify) { + t.Helper() // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) @@ -50,12 +51,14 @@ func AddToolchainClusterAsMember(t *testing.T, functionToVerify FunctionToVerify t.Run("add member ToolchainCluster with namespace label set", func(t *testing.T) { for testName, data := range testCases { t.Run(testName, func(t *testing.T) { - - toolchainCluster, sec := test.NewToolchainCluster("east", test.HostOperatorNs, "secret", status, Labels("member-ns", test.NameHost)) + toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.HostOperatorNs, "member-ns", "secret", status, data.insecure) // the caBundle should be always ignored toolchainCluster.Spec.CABundle = "ZHVtbXk=" toolchainCluster.Spec.DisabledTLSValidations = data.disabledTLSValidations + if toolchainCluster.Labels == nil { + toolchainCluster.Labels = map[string]string{} + } toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] = "" cl := test.NewFakeClient(t, toolchainCluster, sec) service := newToolchainClusterService(t, cl, data.insecure) @@ -72,29 +75,7 @@ func AddToolchainClusterAsMember(t *testing.T, functionToVerify FunctionToVerify _, found := toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] require.True(t, found) assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) - assert.Equal(t, test.NameHost, cachedToolchainCluster.OwnerClusterName) - assert.Equal(t, "http://cluster.com", cachedToolchainCluster.APIEndpoint) - }) - } - }) - - t.Run("add member ToolchainCluster without namespace label set should fail", func(t *testing.T) { - for testName, data := range testCases { - t.Run(testName, func(t *testing.T) { - toolchainCluster, sec := test.NewToolchainCluster("east", test.HostOperatorNs, "secret", status, Labels("", test.NameHost)) - // the caBundle should be always ignored - toolchainCluster.Spec.CABundle = "ZHVtbXk=" - toolchainCluster.Spec.DisabledTLSValidations = data.disabledTLSValidations - toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] = "" - cl := test.NewFakeClient(t, toolchainCluster, sec) - service := newToolchainClusterService(t, cl, data.insecure) - defer service.DeleteToolchainCluster("east") - // when - err := functionToVerify(toolchainCluster, cl, service) - // then - require.Error(t, err) - _, ok := cluster.GetCachedToolchainCluster("east") - require.False(t, ok) + assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) }) } }) @@ -107,7 +88,7 @@ func AddToolchainClusterAsHost(t *testing.T, functionToVerify FunctionToVerify) t.Run("add host ToolchainCluster with namespace label set", func(t *testing.T) { for testName, data := range testCases { t.Run(testName, func(t *testing.T) { - toolchainCluster, sec := test.NewToolchainCluster("east", test.MemberOperatorNs, "secret", status, Labels("host-ns", test.NameMember)) + toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.MemberOperatorNs, "host-ns", "secret", status, data.insecure) // the caBundle should be always ignored toolchainCluster.Spec.CABundle = "ZHVtbXk=" toolchainCluster.Spec.DisabledTLSValidations = data.disabledTLSValidations @@ -131,30 +112,7 @@ func AddToolchainClusterAsHost(t *testing.T, functionToVerify FunctionToVerify) _, found := toolchainCluster.Labels[expectedToolChainClusterRoleLabel] require.False(t, found) assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) - assert.Equal(t, test.NameMember, cachedToolchainCluster.OwnerClusterName) - assert.Equal(t, "http://cluster.com", cachedToolchainCluster.APIEndpoint) - }) - } - }) - - t.Run("add host ToolchainCluster without namespace label set should fail", func(t *testing.T) { - for testName, data := range testCases { - t.Run(testName, func(t *testing.T) { - toolchainCluster, sec := test.NewToolchainCluster("east", test.MemberOperatorNs, "secret", status, Labels("", test.NameMember)) - // the caBundle should be always ignored - toolchainCluster.Spec.CABundle = "ZHVtbXk=" - toolchainCluster.Spec.DisabledTLSValidations = data.disabledTLSValidations - cl := test.NewFakeClient(t, toolchainCluster, sec) - service := newToolchainClusterService(t, cl, data.insecure) - defer service.DeleteToolchainCluster("east") - - // when - err := functionToVerify(toolchainCluster, cl, service) - - // then - require.Error(t, err) - _, ok := cluster.GetCachedToolchainCluster("east") - require.False(t, ok) + assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) }) } }) @@ -164,7 +122,7 @@ func AddToolchainClusterFailsBecauseOfMissingSecret(t *testing.T, functionToVeri // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - toolchainCluster, _ := test.NewToolchainCluster("east", test.MemberOperatorNs, "secret", status, Labels("", test.NameHost)) + toolchainCluster, _ := test.NewToolchainCluster(t, "east", test.MemberOperatorNs, "", "secret", status, false) cl := test.NewFakeClient(t, toolchainCluster) service := newToolchainClusterService(t, cl, true) @@ -182,13 +140,13 @@ func AddToolchainClusterFailsBecauseOfEmptySecret(t *testing.T, functionToVerify // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - toolchainCluster, _ := test.NewToolchainCluster("east", test.MemberOperatorNs, "secret", status, - Labels(test.MemberOperatorNs, test.NameHost)) + toolchainCluster, _ := test.NewToolchainCluster(t, "east", test.MemberOperatorNs, test.MemberOperatorNs, "secret", status, false) secret := &corev1.Secret{ ObjectMeta: v1.ObjectMeta{ Name: "secret", Namespace: test.MemberOperatorNs, - }} + }, + } cl := test.NewFakeClient(t, toolchainCluster, secret) service := newToolchainClusterService(t, cl, true) @@ -206,11 +164,9 @@ func UpdateToolchainCluster(t *testing.T, functionToVerify FunctionToVerify) { // given defer gock.Off() statusTrue := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - toolchainCluster1, sec1 := test.NewToolchainCluster("east", test.HostOperatorNs, "secret1", statusTrue, - Labels(test.HostOperatorNs, test.NameMember)) + toolchainCluster1, sec1 := test.NewToolchainCluster(t, "east", test.HostOperatorNs, test.HostOperatorNs, "secret1", statusTrue, true) statusFalse := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionFalse) - toolchainCluster2, sec2 := test.NewToolchainCluster("east", test.HostOperatorNs, "secret2", statusFalse, - Labels(test.HostOperatorNs, test.NameMember)) + toolchainCluster2, sec2 := test.NewToolchainCluster(t, "east", test.HostOperatorNs, test.HostOperatorNs, "secret2", statusFalse, true) cl := test.NewFakeClient(t, toolchainCluster2, sec1, sec2) service := newToolchainClusterService(t, cl, true) defer service.DeleteToolchainCluster("east") @@ -228,17 +184,15 @@ func UpdateToolchainCluster(t *testing.T, functionToVerify FunctionToVerify) { AssertClusterConfigThat(t, cachedToolchainCluster.Config). HasName("east"). HasOperatorNamespace("toolchain-host-operator"). - HasOwnerClusterName(test.NameMember). - HasAPIEndpoint("http://cluster.com"). - RestConfigHasHost("http://cluster.com") + HasAPIEndpoint("https://cluster.com"). + RestConfigHasHost("https://cluster.com") } func DeleteToolchainCluster(t *testing.T, functionToVerify FunctionToVerify) { // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - toolchainCluster, sec := test.NewToolchainCluster("east", test.MemberOperatorNs, "sec", status, - Labels(test.MemberOperatorNs, test.NameHost)) + toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.MemberOperatorNs, test.MemberOperatorNs, "sec", status, true) cl := test.NewFakeClient(t, sec) service := newToolchainClusterService(t, cl, true) err := service.AddOrUpdateToolchainCluster(toolchainCluster) From a718700447063d91735563f4c2d2b83ee5615e05 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 26 Aug 2024 17:46:19 +0200 Subject: [PATCH 2/6] making linter happy --- .../toolchaincluster/healthchecker_test.go | 3 +-- .../toolchaincluster_controller_test.go | 20 +++++++++---------- pkg/cluster/service.go | 7 ++++--- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/controllers/toolchaincluster/healthchecker_test.go b/controllers/toolchaincluster/healthchecker_test.go index 5cd7c48b..e34365b0 100644 --- a/controllers/toolchaincluster/healthchecker_test.go +++ b/controllers/toolchaincluster/healthchecker_test.go @@ -5,7 +5,6 @@ import ( "fmt" "testing" - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/stretchr/testify/require" @@ -58,7 +57,7 @@ func TestClusterHealthChecks(t *testing.T) { for k, tc := range tests { t.Run(k, func(t *testing.T) { // given - tcType, sec := newToolchainCluster(t, tc.tcType, tcNs, tc.apiEndPoint, toolchainv1alpha1.ToolchainClusterStatus{}, false) + tcType, sec := newToolchainCluster(t, tc.tcType, tcNs, tc.apiEndPoint) cl := test.NewFakeClient(t, tcType, sec) reset := setupCachedClusters(t, cl, tcType) defer reset() diff --git a/controllers/toolchaincluster/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go index fd344e6e..5e1db73e 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -45,7 +45,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("ToolchainCluster not found", func(t *testing.T) { // given - NotFound, sec := newToolchainCluster(t, "notfound", tcNs, "http://not-found.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + NotFound, sec := newToolchainCluster(t, "notfound", tcNs, "http://not-found.com") cl := test.NewFakeClient(t, sec) reset := setupCachedClusters(t, cl, NotFound) @@ -62,7 +62,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("Error while getting ToolchainCluster", func(t *testing.T) { // given - tc, sec := newToolchainCluster(t, "tc", tcNs, "http://tc.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + tc, sec := newToolchainCluster(t, "tc", tcNs, "http://tc.com") cl := test.NewFakeClient(t, sec) cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { @@ -83,7 +83,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("reconcile successful and requeued", func(t *testing.T) { // given - stable, sec := newToolchainCluster(t, "stable", tcNs, "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + stable, sec := newToolchainCluster(t, "stable", tcNs, "https://cluster.com") cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) @@ -102,7 +102,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("toolchain cluster cache not found", func(t *testing.T) { // given - unstable, _ := newToolchainCluster(t, "unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + unstable, _ := newToolchainCluster(t, "unstable", tcNs, "http://unstable.com") cl := test.NewFakeClient(t, unstable) controller, req := prepareReconcile(unstable, cl, requeAfter) @@ -117,7 +117,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("error while updating a toolchain cluster status on cache not found", func(t *testing.T) { // given - stable, _ := newToolchainCluster(t, "stable", tcNs, "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + stable, _ := newToolchainCluster(t, "stable", tcNs, "https://cluster.com") cl := test.NewFakeClient(t, stable) cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { @@ -137,7 +137,7 @@ func TestClusterControllerChecks(t *testing.T) { t.Run("error while updating a toolchain cluster status when health-check failed", func(t *testing.T) { // given - stable, sec := newToolchainCluster(t, "stable", tcNs, "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + stable, sec := newToolchainCluster(t, "stable", tcNs, "https://cluster.com") expectedErr := fmt.Errorf("my test error") cl := test.NewFakeClient(t, stable, sec) cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { @@ -163,7 +163,7 @@ func TestClusterControllerChecks(t *testing.T) { func TestGetClusterHealth(t *testing.T) { t.Run("Check health default", func(t *testing.T) { // given - stable, sec := newToolchainCluster(t, "stable", "test-namespace", "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + stable, sec := newToolchainCluster(t, "stable", "test-namespace", "https://cluster.com") cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) @@ -184,7 +184,7 @@ func TestGetClusterHealth(t *testing.T) { }) t.Run("get health condition when health obtained is false ", func(t *testing.T) { // given - stable, sec := newToolchainCluster(t, "stable", "test-namespace", "https://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}, false) + stable, sec := newToolchainCluster(t, "stable", "test-namespace", "https://cluster.com") cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) @@ -238,8 +238,8 @@ func setupCachedClusters(t *testing.T, cl *test.FakeClient, clusters ...*toolcha } } -func newToolchainCluster(t *testing.T, name, tcNs string, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus, insecureTls bool) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { - toolchainCluster, secret := test.NewToolchainClusterWithEndpoint(t, name, tcNs, "test-namespace", "secret", apiEndpoint, status, insecureTls) +func newToolchainCluster(t *testing.T, name, tcNs string, apiEndpoint string) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { + toolchainCluster, secret := test.NewToolchainClusterWithEndpoint(t, name, tcNs, "test-namespace", "secret", apiEndpoint, toolchainv1alpha1.ToolchainClusterStatus{}, false) return toolchainCluster, secret } diff --git a/pkg/cluster/service.go b/pkg/cluster/service.go index 4ef2c924..c1efd981 100644 --- a/pkg/cluster/service.go +++ b/pkg/cluster/service.go @@ -25,9 +25,10 @@ const ( // labelClusterRolePrefix is the prefix that defines the cluster role as label key labelClusterRolePrefix = "cluster-role" - toolchainAPIQPS = 20.0 - toolchainAPIBurst = 30 - toolchainTokenKey = "token" + // These are not used + // toolchainAPIQPS = 20.0 + // toolchainAPIBurst = 30 + // toolchainTokenKey = "token" ) // ToolchainClusterService manages cached cluster kube clients and related ToolchainCluster CRDs From 0381211878de4d16b8ed89bf66d565b3868aa608 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 3 Sep 2024 13:43:11 +0200 Subject: [PATCH 3/6] Remove unused methods and simplify the tests. --- .../toolchaincluster_controller.go | 25 ---- .../toolchaincluster_controller_test.go | 14 -- pkg/cluster/service.go | 6 - pkg/test/verify/cluster.go | 135 ++++++------------ 4 files changed, 41 insertions(+), 139 deletions(-) diff --git a/controllers/toolchaincluster/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go index df2bf830..3d08ed04 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -12,7 +12,6 @@ import ( kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" kubeclientset "k8s.io/client-go/kubernetes" - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -145,27 +144,3 @@ func clusterNotReadyCondition() toolchainv1alpha1.Condition { Message: healthzNotOk, } } - -func composeKubeConfigFromData(token []byte, apiEndpoint, operatorNamespace string, insecureTls bool) clientcmdapi.Config { - return clientcmdapi.Config{ - Contexts: map[string]*clientcmdapi.Context{ - "ctx": { - Cluster: "cluster", - Namespace: operatorNamespace, - AuthInfo: "auth", - }, - }, - CurrentContext: "ctx", - Clusters: map[string]*clientcmdapi.Cluster{ - "cluster": { - Server: apiEndpoint, - InsecureSkipTLSVerify: insecureTls, - }, - }, - AuthInfos: map[string]*clientcmdapi.AuthInfo{ - "auth": { - Token: string(token), - }, - }, - } -} diff --git a/controllers/toolchaincluster/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go index 5e1db73e..adc3dba2 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -9,7 +9,6 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/codeready-toolchain/toolchain-common/pkg/test" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" corev1 "k8s.io/api/core/v1" @@ -205,19 +204,6 @@ func TestGetClusterHealth(t *testing.T) { }) } -func TestComposeKubeConfig(t *testing.T) { - // when - kubeConfig := composeKubeConfigFromData([]byte("token"), "http://over.the.rainbow", "the-namespace", false) - - // then - context := kubeConfig.Contexts[kubeConfig.CurrentContext] - - assert.Equal(t, "token", kubeConfig.AuthInfos[context.AuthInfo].Token) - assert.Equal(t, "http://over.the.rainbow", kubeConfig.Clusters[context.Cluster].Server) - assert.Equal(t, "the-namespace", context.Namespace) - assert.False(t, kubeConfig.Clusters[context.Cluster].InsecureSkipTLSVerify) -} - func setupCachedClusters(t *testing.T, cl *test.FakeClient, clusters ...*toolchainv1alpha1.ToolchainCluster) func() { service := cluster.NewToolchainClusterServiceWithClient(cl, logf.Log, test.MemberOperatorNs, 0, func(config *rest.Config, options runtimeclient.Options) (runtimeclient.Client, error) { // make sure that insecure is false to make Gock mocking working properly diff --git a/pkg/cluster/service.go b/pkg/cluster/service.go index c1efd981..ef1241ef 100644 --- a/pkg/cluster/service.go +++ b/pkg/cluster/service.go @@ -19,16 +19,10 @@ import ( ) const ( - labelNamespace = "namespace" labelOwnerClusterName = "ownerClusterName" LabelType = "type" // labelClusterRolePrefix is the prefix that defines the cluster role as label key labelClusterRolePrefix = "cluster-role" - - // These are not used - // toolchainAPIQPS = 20.0 - // toolchainAPIBurst = 30 - // toolchainTokenKey = "token" ) // ToolchainClusterService manages cached cluster kube clients and related ToolchainCluster CRDs diff --git a/pkg/test/verify/cluster.go b/pkg/test/verify/cluster.go index 4852154c..bb49ed02 100644 --- a/pkg/test/verify/cluster.go +++ b/pkg/test/verify/cluster.go @@ -20,102 +20,58 @@ import ( type FunctionToVerify func(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, service cluster.ToolchainClusterService) error -var testCases = map[string]struct { - insecure bool - disabledTLSValidations []toolchainv1alpha1.TLSValidation -}{ - "no validations": { - insecure: false, - disabledTLSValidations: nil, - }, - "disabled all validations": { - insecure: true, - disabledTLSValidations: []toolchainv1alpha1.TLSValidation{toolchainv1alpha1.TLSAll}, - }, - "disabled other but not all validations": { - insecure: false, - disabledTLSValidations: []toolchainv1alpha1.TLSValidation{toolchainv1alpha1.TLSValidityPeriod, toolchainv1alpha1.TLSSubjectName}, - }, - "unsupported combination": { - insecure: false, - disabledTLSValidations: []toolchainv1alpha1.TLSValidation{toolchainv1alpha1.TLSAll, toolchainv1alpha1.TLSSubjectName}, - }, -} - func AddToolchainClusterAsMember(t *testing.T, functionToVerify FunctionToVerify) { - t.Helper() // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) - t.Run("add member ToolchainCluster with namespace label set", func(t *testing.T) { - for testName, data := range testCases { - t.Run(testName, func(t *testing.T) { - toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.HostOperatorNs, "member-ns", "secret", status, data.insecure) - // the caBundle should be always ignored - toolchainCluster.Spec.CABundle = "ZHVtbXk=" - toolchainCluster.Spec.DisabledTLSValidations = data.disabledTLSValidations - - if toolchainCluster.Labels == nil { - toolchainCluster.Labels = map[string]string{} - } - toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] = "" - cl := test.NewFakeClient(t, toolchainCluster, sec) - service := newToolchainClusterService(t, cl, data.insecure) - defer service.DeleteToolchainCluster("east") - // when - err := functionToVerify(toolchainCluster, cl, service) - // then - require.NoError(t, err) - cachedToolchainCluster, ok := cluster.GetCachedToolchainCluster("east") - require.True(t, ok) - assert.Equal(t, "member-ns", cachedToolchainCluster.OperatorNamespace) - // check that toolchain cluster role label tenant was set only on member cluster type - require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(toolchainCluster), toolchainCluster)) - _, found := toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] - require.True(t, found) - assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) - assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) - }) - } - }) + toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.HostOperatorNs, "member-ns", "secret", status, false) + + cl := test.NewFakeClient(t, toolchainCluster, sec) + service := newToolchainClusterService(t, cl, false) + defer service.DeleteToolchainCluster("east") + // when + err := functionToVerify(toolchainCluster, cl, service) + // then + require.NoError(t, err) + cachedToolchainCluster, ok := cluster.GetCachedToolchainCluster("east") + require.True(t, ok) + assert.Equal(t, "member-ns", cachedToolchainCluster.OperatorNamespace) + // check that toolchain cluster role label tenant was set only on member cluster type + require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(toolchainCluster), toolchainCluster)) + _, found := toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] + require.True(t, found) + assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) + assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) } func AddToolchainClusterAsHost(t *testing.T, functionToVerify FunctionToVerify) { // given defer gock.Off() status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionFalse) - t.Run("add host ToolchainCluster with namespace label set", func(t *testing.T) { - for testName, data := range testCases { - t.Run(testName, func(t *testing.T) { - toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.MemberOperatorNs, "host-ns", "secret", status, data.insecure) - // the caBundle should be always ignored - toolchainCluster.Spec.CABundle = "ZHVtbXk=" - toolchainCluster.Spec.DisabledTLSValidations = data.disabledTLSValidations - cl := test.NewFakeClient(t, toolchainCluster, sec) - service := newToolchainClusterService(t, cl, data.insecure) - defer service.DeleteToolchainCluster("east") - - // when - err := functionToVerify(toolchainCluster, cl, service) - - // then - require.NoError(t, err) - cachedToolchainCluster, ok := cluster.GetCachedToolchainCluster("east") - require.True(t, ok) - - assert.Equal(t, "host-ns", cachedToolchainCluster.OperatorNamespace) - - // check that toolchain cluster role label tenant is not set on host cluster - require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(toolchainCluster), toolchainCluster)) - expectedToolChainClusterRoleLabel := cluster.RoleLabel(cluster.Tenant) - _, found := toolchainCluster.Labels[expectedToolChainClusterRoleLabel] - require.False(t, found) - assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) - assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) - }) - } - }) + + toolchainCluster, sec := test.NewToolchainCluster(t, "east", test.MemberOperatorNs, "host-ns", "secret", status, false) + cl := test.NewFakeClient(t, toolchainCluster, sec) + service := newToolchainClusterService(t, cl, false) + defer service.DeleteToolchainCluster("east") + + // when + err := functionToVerify(toolchainCluster, cl, service) + + // then + require.NoError(t, err) + cachedToolchainCluster, ok := cluster.GetCachedToolchainCluster("east") + require.True(t, ok) + + assert.Equal(t, "host-ns", cachedToolchainCluster.OperatorNamespace) + + // check that toolchain cluster role label tenant is not set on host cluster + require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(toolchainCluster), toolchainCluster)) + expectedToolChainClusterRoleLabel := cluster.RoleLabel(cluster.Tenant) + _, found := toolchainCluster.Labels[expectedToolChainClusterRoleLabel] + require.False(t, found) + assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) + assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) } func AddToolchainClusterFailsBecauseOfMissingSecret(t *testing.T, functionToVerify FunctionToVerify) { @@ -208,15 +164,6 @@ func DeleteToolchainCluster(t *testing.T, functionToVerify FunctionToVerify) { assert.Nil(t, cachedToolchainCluster) } -func Labels(ns, ownerClusterName string) map[string]string { - labels := map[string]string{} - if ns != "" { - labels["namespace"] = ns - } - labels["ownerClusterName"] = ownerClusterName - return labels -} - func newToolchainClusterService(t *testing.T, cl client.Client, insecure bool) cluster.ToolchainClusterService { return cluster.NewToolchainClusterServiceWithClient(cl, logf.Log, "test-namespace", 3*time.Second, func(config *rest.Config, options client.Options) (client.Client, error) { if insecure { From 4775567df4119834b2bd1cbee94999c362751871 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 3 Sep 2024 15:15:24 +0200 Subject: [PATCH 4/6] Don't check for the cluster role label. It was only set by the test code anyway. --- pkg/test/verify/cluster.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pkg/test/verify/cluster.go b/pkg/test/verify/cluster.go index bb49ed02..78e7f41d 100644 --- a/pkg/test/verify/cluster.go +++ b/pkg/test/verify/cluster.go @@ -37,10 +37,7 @@ func AddToolchainClusterAsMember(t *testing.T, functionToVerify FunctionToVerify cachedToolchainCluster, ok := cluster.GetCachedToolchainCluster("east") require.True(t, ok) assert.Equal(t, "member-ns", cachedToolchainCluster.OperatorNamespace) - // check that toolchain cluster role label tenant was set only on member cluster type require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(toolchainCluster), toolchainCluster)) - _, found := toolchainCluster.Labels[cluster.RoleLabel(cluster.Tenant)] - require.True(t, found) assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) } @@ -65,11 +62,7 @@ func AddToolchainClusterAsHost(t *testing.T, functionToVerify FunctionToVerify) assert.Equal(t, "host-ns", cachedToolchainCluster.OperatorNamespace) - // check that toolchain cluster role label tenant is not set on host cluster require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(toolchainCluster), toolchainCluster)) - expectedToolChainClusterRoleLabel := cluster.RoleLabel(cluster.Tenant) - _, found := toolchainCluster.Labels[expectedToolChainClusterRoleLabel] - require.False(t, found) assert.Equal(t, status, *cachedToolchainCluster.ClusterStatus) assert.Equal(t, "https://cluster.com", cachedToolchainCluster.APIEndpoint) } From bef60b93f0817b01f6ba19c98752cdac762b0b32 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 5 Sep 2024 15:03:06 +0200 Subject: [PATCH 5/6] Use https for the cluster endpoints consistently everywhere to prevent (some of) the future surprises. --- pkg/cluster/service_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cluster/service_test.go b/pkg/cluster/service_test.go index 21c551ec..d0edaaf7 100644 --- a/pkg/cluster/service_test.go +++ b/pkg/cluster/service_test.go @@ -75,8 +75,8 @@ func TestListToolchainClusterConfigs(t *testing.T) { status := test.NewClusterStatus(toolchainv1alpha1.ConditionReady, corev1.ConditionTrue) require.NoError(t, toolchainv1alpha1.AddToScheme(scheme.Scheme)) - m1, sec1 := test.NewToolchainClusterWithEndpoint(t, "east", test.HostOperatorNs, test.MemberOperatorNs, "secret1", "http://m1.com", status, false) - m2, sec2 := test.NewToolchainClusterWithEndpoint(t, "west", test.HostOperatorNs, test.MemberOperatorNs, "secret2", "http://m2.com", status, false) + m1, sec1 := test.NewToolchainClusterWithEndpoint(t, "east", test.HostOperatorNs, test.MemberOperatorNs, "secret1", "https://m1.com", status, false) + m2, sec2 := test.NewToolchainClusterWithEndpoint(t, "west", test.HostOperatorNs, test.MemberOperatorNs, "secret2", "https://m2.com", status, false) host, secHost := test.NewToolchainCluster(t, "host", test.MemberOperatorNs, test.HostOperatorNs, "secretHost", status, false) noise, secNoise := test.NewToolchainCluster(t, "noise", "noise-namespace", "secretNoise", test.MemberOperatorNs, status, false) require.NoError(t, toolchainv1alpha1.AddToScheme(scheme.Scheme)) From 68405c1a88ff202e9445fbe5b7dbcfc6dd3c2ebb Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 5 Sep 2024 15:26:26 +0200 Subject: [PATCH 6/6] update the test condition to the changed endpoint urls, d'oh! --- pkg/cluster/service_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cluster/service_test.go b/pkg/cluster/service_test.go index d0edaaf7..7d4abf61 100644 --- a/pkg/cluster/service_test.go +++ b/pkg/cluster/service_test.go @@ -93,13 +93,13 @@ func TestListToolchainClusterConfigs(t *testing.T) { verify.AssertClusterConfigThat(t, clusterConfigs[0]). HasName("east"). HasOperatorNamespace("toolchain-member-operator"). - HasAPIEndpoint("http://m1.com"). - RestConfigHasHost("http://m1.com") + HasAPIEndpoint("https://m1.com"). + RestConfigHasHost("https://m1.com") verify.AssertClusterConfigThat(t, clusterConfigs[1]). HasName("west"). HasOperatorNamespace("toolchain-member-operator"). - HasAPIEndpoint("http://m2.com"). - RestConfigHasHost("http://m2.com") + HasAPIEndpoint("https://m2.com"). + RestConfigHasHost("https://m2.com") }) t.Run("list host", func(t *testing.T) {