From c047b4043f25aa7ce172ab2bda4ec8c34464f76d Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 13 Jun 2024 13:01:48 +0200 Subject: [PATCH 1/3] Add migration step to move ToolchainCluster.Spec.APIEndpoint status and read the connection configuration from the kubeconfig stored in the secret. --- .../toolchaincluster_controller.go | 12 ++ go.mod | 2 +- go.sum | 4 +- pkg/cluster/service.go | 63 ++++++-- pkg/cluster/service_test.go | 135 +++++++++++++++++- 5 files changed, 199 insertions(+), 17 deletions(-) diff --git a/controllers/toolchaincluster/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go index 13ed77e7..cebbde60 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -69,6 +69,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } + // TODO: merge this into the "updateStatus" function coming in https://github.com/codeready-toolchain/toolchain-common/pull/401 + if err = r.setApiEndpointAndOperatorNamespace(ctx, toolchainCluster, cachedCluster); err != nil { + return reconcile.Result{}, err + } + clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) if err != nil { reqLogger.Error(err, "cannot create ClientSet for the ToolchainCluster") @@ -89,6 +94,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{RequeueAfter: r.RequeAfter}, nil } +func (r *Reconciler) setApiEndpointAndOperatorNamespace(ctx context.Context, tc *toolchainv1alpha1.ToolchainCluster, cached *cluster.CachedToolchainCluster) error { + tc.Status.APIEndpoint = cached.APIEndpoint + tc.Status.OperatorNamespace = cached.OperatorNamespace + + return r.Client.Status().Update(ctx, tc) +} + func (r *Reconciler) migrateSecretToKubeConfig(ctx context.Context, tc *toolchainv1alpha1.ToolchainCluster) error { if len(tc.Spec.SecretRef.Name) == 0 { return nil diff --git a/go.mod b/go.mod index fefab31d..69296b89 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/codeready-toolchain/toolchain-common go 1.20 require ( - github.com/codeready-toolchain/api v0.0.0-20240620205422-a29b6c658d03 + github.com/codeready-toolchain/api v0.0.0-20240708122235-0af5a9a178bb github.com/go-logr/logr v1.2.3 github.com/golang-jwt/jwt/v5 v5.2.0 github.com/lestrrat-go/jwx v1.2.29 diff --git a/go.sum b/go.sum index 8e0a7979..5bc1fb18 100644 --- a/go.sum +++ b/go.sum @@ -115,8 +115,8 @@ github.com/cncf/xds/go v0.0.0-20210312221358-fbca930ec8ed/go.mod h1:eXthEFrGJvWH github.com/cockroachdb/datadriven v0.0.0-20200714090401-bf6692d28da5/go.mod h1:h6jFvWxBdQXxjopDMZyH2UVceIRfR84bdzbkoKrsWNo= github.com/cockroachdb/errors v1.2.4/go.mod h1:rQD95gz6FARkaKkQXUksEje/d9a6wBJoCr5oaCLELYA= github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f/go.mod h1:i/u985jwjWRlyHXQbwatDASoW0RMlZ/3i9yJHE2xLkI= -github.com/codeready-toolchain/api v0.0.0-20240620205422-a29b6c658d03 h1:uqApdceOkNQI1wy2gwA6pEUzhLlqqbbc+ChTvEshIW0= -github.com/codeready-toolchain/api v0.0.0-20240620205422-a29b6c658d03/go.mod h1:ie9p4LenCCS0LsnbWp6/xwpFDdCWYE0KWzUO6Sk1g0E= +github.com/codeready-toolchain/api v0.0.0-20240708122235-0af5a9a178bb h1:Wc9CMsv0ODZv9dM5qF3OI0mFDO95YNIXV/8oRvoz8aE= +github.com/codeready-toolchain/api v0.0.0-20240708122235-0af5a9a178bb/go.mod h1:ie9p4LenCCS0LsnbWp6/xwpFDdCWYE0KWzUO6Sk1g0E= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/etcd v3.3.13+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= diff --git a/pkg/cluster/service.go b/pkg/cluster/service.go index 94f3c195..f410f93d 100644 --- a/pkg/cluster/service.go +++ b/pkg/cluster/service.go @@ -160,16 +160,9 @@ func (s *ToolchainClusterService) enrichLogger(cluster *toolchainv1alpha1.Toolch // NewClusterConfig generate a new cluster config by fetching the necessary info the given ToolchainCluster's associated Secret and taking all data from ToolchainCluster CR func NewClusterConfig(cl client.Client, toolchainCluster *toolchainv1alpha1.ToolchainCluster, 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) - } - secretName := toolchainCluster.Spec.SecretRef.Name if secretName == "" { - return nil, errors.Errorf("cluster %s does not have a secret name", clusterName) + return nil, errors.Errorf("cluster %s does not have a secret name", toolchainCluster.Name) } secret := &v1.Secret{} name := types.NamespacedName{ @@ -178,7 +171,22 @@ func NewClusterConfig(cl client.Client, toolchainCluster *toolchainv1alpha1.Tool } err := cl.Get(context.TODO(), name, secret) if err != nil { - return nil, errors.Wrapf(err, "unable to get secret %s for cluster %s", name, clusterName) + 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) + } 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] @@ -201,8 +209,8 @@ func NewClusterConfig(cl client.Client, toolchainCluster *toolchainv1alpha1.Tool restConfig.Timeout = timeout return &Config{ - Name: toolchainCluster.Name, - APIEndpoint: toolchainCluster.Spec.APIEndpoint, + Name: clusterName, + APIEndpoint: apiEndpoint, RestConfig: restConfig, OperatorNamespace: toolchainCluster.Labels[labelNamespace], OwnerClusterName: toolchainCluster.Labels[labelOwnerClusterName], @@ -210,6 +218,39 @@ func NewClusterConfig(cl client.Client, toolchainCluster *toolchainv1alpha1.Tool }, nil } +func loadConfigFromKubeConfig(toolchainCluster *toolchainv1alpha1.ToolchainCluster, secret *v1.Secret) (*Config, error) { + cfg, err := clientcmd.Load(secret.Data["kubeconfig"]) + if err != nil { + return nil, err + } + clientCfg := clientcmd.NewDefaultClientConfig(*cfg, &clientcmd.ConfigOverrides{}) + restCfg, err := clientCfg.ClientConfig() + if err != nil { + return nil, err + } + + context := cfg.Contexts[cfg.CurrentContext] + if context == nil { + // this should theoretically never happen because we should not be able to get the rest config from an invalid kube config. + return nil, errors.New("There is no current context in the provided kubeconfig. Cannot pick a context unambiguously.") + } + + cluster := cfg.Clusters[context.Cluster] + if cluster == nil { + // this should theoretically never happen because we should not be able to get the rest config from an invalid kube config. + return nil, errors.New("Could not unambiguously identify the cluster configuration to use in the kubeconfig.") + } + + return &Config{ + Name: toolchainCluster.Name, + APIEndpoint: cluster.Server, + RestConfig: restCfg, + OperatorNamespace: context.Namespace, + OwnerClusterName: toolchainCluster.Labels[labelOwnerClusterName], + Labels: toolchainCluster.Labels, + }, nil +} + func IsReady(clusterStatus *toolchainv1alpha1.ToolchainClusterStatus) bool { for _, condition := range clusterStatus.Conditions { if condition.Type == toolchainv1alpha1.ConditionReady { diff --git a/pkg/cluster/service_test.go b/pkg/cluster/service_test.go index 67c393d1..b8630bdf 100644 --- a/pkg/cluster/service_test.go +++ b/pkg/cluster/service_test.go @@ -6,13 +6,18 @@ import ( "testing" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + 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/codeready-toolchain/toolchain-common/pkg/test/verify" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/tools/clientcmd" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -22,7 +27,6 @@ func TestAddToolchainClusterAsMember(t *testing.T) { // when return service.AddOrUpdateToolchainCluster(toolchainCluster) }) - } func TestAddToolchainClusterAsHost(t *testing.T) { @@ -132,7 +136,7 @@ func TestListToolchainClusterConfigs(t *testing.T) { }) t.Run("when list fails", func(t *testing.T) { - //given + // given cl := test.NewFakeClient(t, m1, m2, host, noise, sec1, sec2, secHost, secNoise) cl.MockList = func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { return fmt.Errorf("some error") @@ -147,7 +151,7 @@ func TestListToolchainClusterConfigs(t *testing.T) { }) t.Run("when get secret fails", func(t *testing.T) { - //given + // given cl := test.NewFakeClient(t, m1, m2, host, noise, sec1, sec2, secHost, secNoise) cl.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { return fmt.Errorf("some error") @@ -161,3 +165,128 @@ func TestListToolchainClusterConfigs(t *testing.T) { require.Empty(t, clusterConfigs) }) } + +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 { + return &toolchainv1alpha1.ToolchainCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tc", + Namespace: "ns", + }, + Spec: toolchainv1alpha1.ToolchainClusterSpec{ + SecretRef: toolchainv1alpha1.LocalSecretReference{ + Name: "secret", + }, + }, + } + } + + 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{ + Clusters: map[string]*clientcmdapi.Cluster{ + "cluster": { + Server: "https://over.the.rainbow", + }, + }, + Contexts: map[string]*clientcmdapi.Context{ + "ctx": { + Cluster: "cluster", + AuthInfo: "auth", + Namespace: "operatorns", + }, + }, + AuthInfos: map[string]*clientcmdapi.AuthInfo{ + "auth": { + Token: "token", + }, + }, + CurrentContext: "ctx", + } + kubeconfigContents, err := clientcmd.Write(kubeconfig) + require.NoError(t, err) + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: "ns", + }, + Data: map[string][]byte{ + "kubeconfig": kubeconfigContents, + }, + } + } + + 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... + secret := kubeconfigSecret(t) + secret.Data["token"] = []byte("not-the-token-we-want") + + 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) + }) +} From 21c208434123af7314426eee1c2f04144012bc4c Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 10 Jul 2024 16:46:07 +0200 Subject: [PATCH 2/3] Make sure to only update the status after the conditions have been initialized and don't clear out other fields than the conditions in the healtchcheck from the status. --- controllers/toolchaincluster/healthchecker.go | 3 +-- .../toolchaincluster/toolchaincluster_controller.go | 8 ++------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/controllers/toolchaincluster/healthchecker.go b/controllers/toolchaincluster/healthchecker.go index 04fcd3c1..3fab8840 100644 --- a/controllers/toolchaincluster/healthchecker.go +++ b/controllers/toolchaincluster/healthchecker.go @@ -28,7 +28,6 @@ type HealthChecker struct { } func (hc *HealthChecker) updateIndividualClusterStatus(ctx context.Context, toolchainCluster *toolchainv1alpha1.ToolchainCluster) error { - currentClusterStatus := hc.getClusterHealthStatus(ctx) for index, currentCond := range currentClusterStatus.Conditions { @@ -39,7 +38,7 @@ func (hc *HealthChecker) updateIndividualClusterStatus(ctx context.Context, tool } } - toolchainCluster.Status = *currentClusterStatus + toolchainCluster.Status.Conditions = currentClusterStatus.Conditions if err := hc.localClusterClient.Status().Update(ctx, toolchainCluster); err != nil { return errors.Wrapf(err, "Failed to update the status of cluster %s", toolchainCluster.Name) } diff --git a/controllers/toolchaincluster/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go index cebbde60..f462dfc2 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -70,9 +70,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. } // TODO: merge this into the "updateStatus" function coming in https://github.com/codeready-toolchain/toolchain-common/pull/401 - if err = r.setApiEndpointAndOperatorNamespace(ctx, toolchainCluster, cachedCluster); err != nil { - return reconcile.Result{}, err - } + r.setApiEndpointAndOperatorNamespace(ctx, toolchainCluster, cachedCluster) clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) if err != nil { @@ -94,11 +92,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{RequeueAfter: r.RequeAfter}, nil } -func (r *Reconciler) setApiEndpointAndOperatorNamespace(ctx context.Context, tc *toolchainv1alpha1.ToolchainCluster, cached *cluster.CachedToolchainCluster) error { +func (r *Reconciler) setApiEndpointAndOperatorNamespace(ctx context.Context, tc *toolchainv1alpha1.ToolchainCluster, cached *cluster.CachedToolchainCluster) { tc.Status.APIEndpoint = cached.APIEndpoint tc.Status.OperatorNamespace = cached.OperatorNamespace - - return r.Client.Status().Update(ctx, tc) } func (r *Reconciler) migrateSecretToKubeConfig(ctx context.Context, tc *toolchainv1alpha1.ToolchainCluster) error { From de603e703e53496677f41388ff77c49966bfdc8d Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 9 Aug 2024 12:26:21 +0200 Subject: [PATCH 3/3] Simplify the way we obtain the info to compose the configuration for creating the client for the remote clusters. --- pkg/cluster/service.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/pkg/cluster/service.go b/pkg/cluster/service.go index 2f9618d4..31ba528e 100644 --- a/pkg/cluster/service.go +++ b/pkg/cluster/service.go @@ -175,7 +175,7 @@ func NewClusterConfig(cl client.Client, toolchainCluster *toolchainv1alpha1.Tool } if _, ok := secret.Data["kubeconfig"]; ok { - return loadConfigFromKubeConfig(toolchainCluster, secret) + return loadConfigFromKubeConfig(toolchainCluster, secret, timeout) } else { return loadConfigFromLegacyToolchainCluster(toolchainCluster, secret, timeout) } @@ -218,7 +218,7 @@ func loadConfigFromLegacyToolchainCluster(toolchainCluster *toolchainv1alpha1.To }, nil } -func loadConfigFromKubeConfig(toolchainCluster *toolchainv1alpha1.ToolchainCluster, secret *v1.Secret) (*Config, error) { +func loadConfigFromKubeConfig(toolchainCluster *toolchainv1alpha1.ToolchainCluster, secret *v1.Secret, timeout time.Duration) (*Config, error) { cfg, err := clientcmd.Load(secret.Data["kubeconfig"]) if err != nil { return nil, err @@ -229,23 +229,19 @@ func loadConfigFromKubeConfig(toolchainCluster *toolchainv1alpha1.ToolchainClust return nil, err } - context := cfg.Contexts[cfg.CurrentContext] - if context == nil { - // this should theoretically never happen because we should not be able to get the rest config from an invalid kube config. - return nil, errors.New("There is no current context in the provided kubeconfig. Cannot pick a context unambiguously.") - } + // This is questionable, but the timeout is currently configurable in the member configuration so let's keep it here... + restCfg.Timeout = timeout - cluster := cfg.Clusters[context.Cluster] - if cluster == nil { - // this should theoretically never happen because we should not be able to get the rest config from an invalid kube config. - return nil, errors.New("Could not unambiguously identify the cluster configuration to use in the kubeconfig.") + operatorNamespace, _, err := clientCfg.Namespace() + if err != nil { + return nil, fmt.Errorf("Could not determine the operator namespace from the current context in the provided kubeconfig because of: %w", err) } return &Config{ Name: toolchainCluster.Name, - APIEndpoint: cluster.Server, + APIEndpoint: restCfg.Host, RestConfig: restCfg, - OperatorNamespace: context.Namespace, + OperatorNamespace: operatorNamespace, OwnerClusterName: toolchainCluster.Labels[labelOwnerClusterName], Labels: toolchainCluster.Labels, }, nil