feat: multi cluster support for the listener #240

vertex451 · 2025-05-30T15:37:51Z

Resolves #149

Changes

Gateway

Refactored gateway/manager and took out its parts into separate packages:
At some point I realized that we have map for GraphQL schemas and map for resolvers/runtime-clients and they are coupled due to relation to a single cluster.
And I migrated to a domain driven design - introduced targetcluster package that represents a single k8s cluster. So now, instead of having several maps, we have one map of clusters, where each single cluster holds its related schema, resolvers and runtime clients.

Listener

Refactored code to divide between direct KCP kubeconfig and clusteraccess using Strategy pattern
Used golang-commons lifecycle package for Reconcilers

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

…into feat/mutli-cluster

gateway/manager/targetcluster/connection.go

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

aaronschweig

Left a few comments in the PR.

In general I think the PR is already in quite a good state. What I can see though is a diversion from the usual pattern in which we write operators.

A few things to note:

to adhere to best practices the usage of our lifecycle package in the golang-commons library is encouraged
Artem: Done
Keep the Reconcilers straight. I am not sold completely on the introduction of the strategies pattern. It feels a bit too much, for something that is determined once on startup. I could understand the introduction of such a pattern a bit more, if we would have on reconciler potentially working on multiple different patterns/inputs. But since we have a KCP reconciler, which is dedicated to performing specific KCP tasks, a ClusterAccess resource reconciler which is able to reconcile ClusterAccesses and non of them are managed by a single reconciler (which would not be ideal anyway) the strategies feel a bit like an overengineered solution for a simple configuration switch in the startup period of the operator
Artem: Replaced strategy patter with conditional approach
I do think it is a good idea, to abstract a cluster layer, that handles a bit the logic of getting a client and the necessary information to gateway to a cluster
I didn't look into the tests yet, since you mentioned there were still WIP
A small wish from mine: Don't code it like Java. Whilst I sometimes see the benefits of introducing design patterns, I want to avoid to have too many indirections or abstractions where not absolutely necessary. Within the framework of controller-runtime and the combination with the golang-commons library there already are a lot of useful abstractions that help us build mantainable operators, so maybe looking into them, also the pattern of subroutines (term used to encapsulate a specific portion of reconcilation logic) could help when trying to find the right way to abstract/implement the logic needed.
Artem: Appreciate your feedback, will keep in mind the idea of following the idiomatic Golang way

aaronschweig · 2025-06-12T11:37:13Z

gateway/manager/targetcluster/connection.go

+	tmpFile, err := os.CreateTemp("", "kubeconfig-*.yaml")
+	if err != nil {
+		return nil, fmt.Errorf("failed to create temporary kubeconfig file: %w", err)
+	}
+	defer os.Remove(tmpFile.Name())
+
+	if _, err := tmpFile.Write(kubeconfigData); err != nil {
+		tmpFile.Close()
+		return nil, fmt.Errorf("failed to write kubeconfig to temporary file: %w", err)
+	}
+	tmpFile.Close()
+
+	// Build config from kubeconfig
+	config, err := clientcmd.BuildConfigFromFlags("", tmpFile.Name())
+	if err != nil {
+		return nil, fmt.Errorf("failed to build config from kubeconfig: %w", err)


there should be another option to parse the necessary information from the auth metadata. If it is structured, there probably is the correct struct already hidden somewhere, without the need to use a temporary file

Removed temporary file usage.

aaronschweig · 2025-06-12T11:38:19Z

gateway/manager/targetcluster/cluster.go

+	if tc.metadata != nil && tc.metadata.Path != "" {
+		path = tc.metadata.Path
+	}
+	return fmt.Sprintf("http://localhost:%s/%s/graphql", appCfg.Gateway.Port, path)


this is only valid for locally running services. If this GetEndpoint is needed for something else than logging, this is not the correct approach

now it logs new ednpoint with localhost only in case of local_development=true.
Otherwise it logs /workspace/graphql

gateway/manager/targetcluster/connection.go

aaronschweig · 2025-06-12T11:39:28Z

gateway/manager/targetcluster/connection.go

+	}
+
+	if metadata.Auth.Kubeconfig == "" {
+		return nil, fmt.Errorf("kubeconfig data is empty")


no interpolation made, so errors.New is sufficient

aaronschweig · 2025-06-12T11:40:44Z

gateway/manager/targetcluster/metadata.go

+// ClusterMetadata represents cluster connection information embedded in schema files
+// Simplified for standard Kubernetes clusters with kubeconfig authentication
+type ClusterMetadata struct {
+	Host string        `json:"host"`
+	Path string        `json:"path"`
+	Auth *AuthMetadata `json:"auth,omitempty"`
+}
+
+// AuthMetadata contains kubeconfig authentication for standard Kubernetes clusters
+type AuthMetadata struct {
+	Type       string `json:"type"`       // Only "kubeconfig" is supported
+	Kubeconfig string `json:"kubeconfig"` // base64 encoded kubeconfig
+}
+
+// FileData represents the data extracted from a schema file
+type FileData struct {
+	Definitions map[string]interface{} `json:"definitions"`
+	Metadata    *ClusterMetadata       `json:"x-cluster-metadata"`
+}


not the biggest fan of type only files - it makes most sense to colocate them to their usage if possible

listener/reconciler/clusteraccess/strategy.go

aaronschweig · 2025-06-12T11:44:20Z

listener/reconciler/types.go

Same as above, I don't like the separate type files - please colocate to their usage if possible

aaronschweig · 2025-06-12T11:46:04Z

listener/kcp/reconciler_factory.go

+	case CRDRegisteredNoObjects:
+		log.Info().Msg("Using ClusterAccess reconciler - waiting for ClusterAccess objects to be created")
+		return newClusterAccessReconciler(opts, discoveryInterface, preReconcileFunc, log)
+	case CRDRegisteredWithObjects:
+		log.Info().Msg("Using ClusterAccess reconciler")
+		return newClusterAccessReconciler(opts, discoveryInterface, preReconcileFunc, log)


this does seem a bit unnessesary to differentiate between no cluster access resources available and if there are some present, especially since there is no special handling of these cases.

aaronschweig · 2025-06-12T11:46:49Z

listener/kcp/reconciler_factory.go

+	if err := k8sClient.List(ctx, clusterAccessList); err != nil {
+		log.Info().Err(err).Msg("ClusterAccess CRD not registered")
+		return CRDNotRegistered
+	}


there could be a multitude of failures leading to this error messages, all but one indicating a maybe totally different failure than a missing CRD.

made logging and error's return value more specific

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

…into feat/mutli-cluster

aaronschweig · 2025-06-18T13:00:00Z

common/apis/v1alpha1/clusteraccess_types.go

+	if in.SecretRef != nil {
+		in, out := &in.SecretRef, &out.SecretRef
+		*out = new(SecretRef)
+		**out = **in
+	}
+	if in.ConfigMapRef != nil {
+		in, out := &in.ConfigMapRef, &out.ConfigMapRef
+		*out = new(ConfigMapRef)
+		**out = **in
+	}
+}
+
+func (in *CAConfig) DeepCopy() *CAConfig {
+	if in == nil {
+		return nil
+	}
+	out := new(CAConfig)
+	in.DeepCopyInto(out)
+	return out
+}
+
+func (in *AuthConfig) DeepCopyInto(out *AuthConfig) {
+	*out = *in
+	if in.SecretRef != nil {
+		in, out := &in.SecretRef, &out.SecretRef
+		*out = new(SecretRef)
+		**out = **in
+	}
+	if in.KubeconfigSecretRef != nil {
+		in, out := &in.KubeconfigSecretRef, &out.KubeconfigSecretRef
+		*out = new(KubeconfigSecretRef)
+		**out = **in
+	}
+	if in.ClientCertificateRef != nil {
+		in, out := &in.ClientCertificateRef, &out.ClientCertificateRef
+		*out = new(ClientCertificateRef)
+		**out = **in
+	}
+}
+
+func (in *AuthConfig) DeepCopy() *AuthConfig {
+	if in == nil {
+		return nil
+	}
+	out := new(AuthConfig)
+	in.DeepCopyInto(out)
+	return out
+}
+
+func (in *SecretRef) DeepCopyInto(out *SecretRef) {
+	*out = *in
+}
+
+func (in *SecretRef) DeepCopy() *SecretRef {
+	if in == nil {
+		return nil
+	}
+	out := new(SecretRef)
+	in.DeepCopyInto(out)
+	return out
+}
+
+func (in *ConfigMapRef) DeepCopyInto(out *ConfigMapRef) {
+	*out = *in
+}
+
+func (in *ConfigMapRef) DeepCopy() *ConfigMapRef {
+	if in == nil {
+		return nil
+	}
+	out := new(ConfigMapRef)
+	in.DeepCopyInto(out)
+	return out
+}
+
+func (in *KubeconfigSecretRef) DeepCopyInto(out *KubeconfigSecretRef) {
+	*out = *in
+}
+
+func (in *KubeconfigSecretRef) DeepCopy() *KubeconfigSecretRef {
+	if in == nil {
+		return nil
+	}
+	out := new(KubeconfigSecretRef)
+	in.DeepCopyInto(out)
+	return out
+}
+
+func (in *ClientCertificateRef) DeepCopyInto(out *ClientCertificateRef) {
+	*out = *in
+}
+
+func (in *ClientCertificateRef) DeepCopy() *ClientCertificateRef {
+	if in == nil {
+		return nil
+	}
+	out := new(ClientCertificateRef)
+	in.DeepCopyInto(out)
+	return out
+}


These normally get generated in a separate file

aaronschweig · 2025-06-18T13:00:56Z

listener/reconciler/kcp/reconciler.go

+	apiBinding := &kcpapis.APIBinding{}
+	if err := r.opts.Client.Get(ctx, req.NamespacedName, apiBinding); err != nil {
+		// If the resource is not found, it might have been deleted
+		if client.IgnoreNotFound(err) == nil {
+			r.log.Info().Str("apiBinding", req.Name).Msg("APIBinding resource not found, might have been deleted")
+			return ctrl.Result{}, nil
+		}
+		r.log.Error().Err(err).Str("apiBinding", req.Name).Msg("failed to fetch APIBinding resource")
+		return ctrl.Result{}, err
+	}


this is done by the shared library

aaronschweig · 2025-06-18T13:01:17Z

listener/reconciler/clusteraccess/reconciler.go

+	clusterAccess := &gatewayv1alpha1.ClusterAccess{}
+	if err := r.opts.Client.Get(ctx, req.NamespacedName, clusterAccess); err != nil {
+		if client.IgnoreNotFound(err) == nil {
+			r.log.Info().Str("clusterAccess", req.Name).Msg("ClusterAccess resource not found, might have been deleted")
+			return ctrl.Result{}, nil
+		}
+		r.log.Error().Err(err).Str("clusterAccess", req.Name).Msg("failed to fetch ClusterAccess resource")
+		return ctrl.Result{}, err
+	}
+


done by the shared library

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

pteich

The current structure works for me. I can understand what you want to achieve and can follow along. But the reconciler factory for me feels like an unnecessary abstraction. Besides of that I left some additional comments here and there.

pteich · 2025-06-18T16:17:21Z

gateway/manager/manager.go

 	"github.com/openmfp/golang-commons/logger"
+	"github.com/pkg/errors"


yet another error package? what is the specific use for it?

pteich · 2025-06-18T16:26:20Z

cmd/gateway.go

@@ -48,21 +47,14 @@ var gatewayCmd = &cobra.Command{

 		ctrl.SetLogger(zap.New(zap.UseDevMode(true)))


It's not part of the PR but as we had this discussion before this could be:

ctrl.SetLogger(log.Logr())

pteich · 2025-06-18T16:44:49Z

listener/reconciler/factory.go

+)
+
+// CreateReconciler creates a reconciler based on the configuration
+func CreateReconciler(


tbh I am not really sure about the factory. Especially as the only logic here is the switch based on the app config. For me it feels like an unnecessary abstraction as the user of the factory already knows what he want get back based on the config. So the different create functions could be public and the switch could be part of the caller code. This way at least for me it would be way more obvious what type of reconciler you got back.

pteich · 2025-06-18T20:51:07Z

listener/reconciler/factory.go

+		return nil, err
+	}
+
+	return standard.NewReconciler(opts, restCfg, ioHandler, schemaResolver, discoveryInterface, restMapper, log)


I would name the package singlecluster instead of "standard" to align with the purpose and create function name.

pteich · 2025-06-18T20:54:45Z

gateway/manager/targetcluster/registry.go

+func NewClusterRegistry(
+	log *logger.Logger,
+	appCfg appConfig.Config,
+	roundTripperFactory func(*rest.Config) http.RoundTripper,


You should use your already defined RoundTripperFactory type.

pteich · 2025-06-18T20:54:55Z

gateway/manager/targetcluster/registry.go

+	clusters            map[string]*TargetCluster
+	log                 *logger.Logger
+	appCfg              appConfig.Config
+	roundTripperFactory func(*rest.Config) http.RoundTripper


You should use your already defined RoundTripperFactory type.

pteich · 2025-06-18T20:55:42Z

gateway/manager/interfaces.go

+}
+
+// RoundTripperFactory creates HTTP round trippers for authentication
+type RoundTripperFactory func(*rest.Config) http.RoundTripper


Looks like you defined it but forgot to use it :)

feat: multi cluster support for the listener

5bfcce5

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

github-actions bot added the feature label May 30, 2025

vertex451 added 5 commits May 30, 2025 17:39

cleanup

dbade00

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

merged main

5ef479d

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

feat: gateway

208d253

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

listener and gateway

679681f

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

Merge branch 'main' of github.com:openmfp/kubernetes-graphql-gateway …

65a4195

…into feat/mutli-cluster

github-advanced-security bot found potential problems Jun 9, 2025

View reviewed changes

gateway/manager/targetcluster/connection.go Fixed Show fixed Hide fixed

vertex451 added 19 commits June 9, 2025 19:38

fix config storing

04d95c2

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

fixed certificate issue

cbe76e4

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

returned kcp direct approach to the listener

95cbd65

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

testing

757ffa0

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

script

afdef55

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

cleanup

a766071

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

cleanup

77f8e9e

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

generate

d6268cb

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

genereted fresh ClusterAccess

27e566c

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

cleanup

bc874c1

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

strategy pattern in reconciling

05f2b10

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

strategy pattern in reconciling

eb67fcd

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

fix conenction target

6765e2e

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

moved handler to router package to cluster

8caffa0

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

merged clusterRegistry and clusterRouter

b8c411b

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

removed redundant code

5147ca1

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

cleanup

df79f7f

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

simplify kcp part

4c53899

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

fixed unit tests

8506a97

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

aaronschweig requested changes Jun 12, 2025

View reviewed changes

vertex451 added 2 commits June 18, 2025 14:09

feat: use golang-commons lifecycle

5d8fc12

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

Merge branch 'main' of github.com:openmfp/kubernetes-graphql-gateway …

7c43577

…into feat/mutli-cluster

aaronschweig requested changes Jun 18, 2025

View reviewed changes

cleanup

96b3c03

On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>

pteich reviewed Jun 18, 2025

View reviewed changes

		"github.com/openmfp/golang-commons/logger"
		"github.com/pkg/errors"

		@@ -48,21 +47,14 @@ var gatewayCmd = &cobra.Command{

		ctrl.SetLogger(zap.New(zap.UseDevMode(true)))

feat: multi cluster support for the listener #240

Are you sure you want to change the base?

feat: multi cluster support for the listener #240

Uh oh!

Conversation

vertex451 commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Gateway

Listener

Uh oh!

Uh oh!

aaronschweig left a comment • edited by vertex451 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pteich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vertex451 commented May 30, 2025 •

edited

Loading

aaronschweig left a comment •

edited by vertex451

Loading