Skip to content

fix: improve teleport operator timing and eliminate race conditions #291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ssyno wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: improve teleport operator timing and eliminate race conditions #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
beff811
fix: improve teleport operator timing and eliminate race conditions
ssyno Jul 30, 2025
dd192be
Merge branch 'main' into fix-teleport-timing-race-conditions
ssyno Aug 20, 2025
64ce91b
Merge branch 'main' into fix-teleport-timing-race-conditions
ssyno Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions helm/teleport-operator/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ pod:

resources:
limits:
cpu: 500m
memory: 1Gi
requests:
cpu: 250m
memory: 500Mi
requests:
cpu: 100m
memory: 250Mi

# Add seccomp to pod security context
podSecurityContext:
Expand Down
126 changes: 125 additions & 1 deletion internal/controller/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,22 @@ package controller

import (
"context"
"strings"
"time"

"github.com/giantswarm/microerror"
"github.com/go-logr/logr"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
capi "sigs.k8s.io/cluster-api/api/v1beta1"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

"github.com/giantswarm/teleport-operator/internal/pkg/config"
"github.com/giantswarm/teleport-operator/internal/pkg/key"
Expand Down Expand Up @@ -63,6 +71,7 @@ type ClusterReconciler struct {
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.4/pkg/reconcile
func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
log := r.Log.WithValues("cluster", req.NamespacedName)
start := time.Now()

cluster := &capi.Cluster{}
if err := r.Client.Get(ctx, req.NamespacedName, cluster); err != nil {
Expand All @@ -72,7 +81,7 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
return ctrl.Result{}, microerror.Mask(err)
}

log.Info("Reconciling cluster", "cluster", cluster)
log.Info("Reconciling cluster", "cluster", cluster, "creation_time", cluster.CreationTimestamp, "reconcile_start", start)

@stone-z stone-z Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine to measure the reconciliation time. If we do so, it would be nice to have it as a metric

@ssyno ssyno Jul 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure, we will go with metrics


appsEnabled, err := r.Teleport.AreTeleportAppsEnabled(ctx, cluster.Name, cluster.Namespace)
if err != nil {
Expand Down Expand Up @@ -254,12 +263,127 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

// We need to requeue to check the teleport token validity
// and update secret for the cluster, if it expires
log.Info("Reconcile completed", "duration", time.Since(start))
return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil
}

// SetupWithManager sets up the controller with the Manager.
func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
return ctrl.NewControllerManagedBy(mgr).
For(&capi.Cluster{}).

@stone-z stone-z Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO this line suggests that the operator should be reacting immediately to Cluster CR creation, so the 3 minute lag time is external to the operator

@ssyno ssyno Jul 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah this should be the issue, watcher means the operator responds immediately to new clusters, the delay is happening in the tbot deployment or in the Teleport API calls.

@stone-z stone-z Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When a new Cluster CR is created, do we spin up or re-deploy tbot? If so, then I think the solution here is to immediately create the placeholder secret so that e2e can wait for it to be populated, and then fill it in once tbot is done

Watches(
&source.Kind{Type: &corev1.Secret{}},
handler.EnqueueRequestsFromMapFunc(r.findClustersForKubeconfigSecret),
builder.WithPredicates(predicate.NewPredicateFuncs(r.isKubeconfigSecret)),
).
Watches(
&source.Kind{Type: &corev1.ConfigMap{}},
handler.EnqueueRequestsFromMapFunc(r.findClustersForTbotConfigMap),
builder.WithPredicates(predicate.NewPredicateFuncs(r.isTbotConfigMap)),
).
Comment on lines +274 to +283

@stone-z stone-z Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is it necessary to watch the resources we create?

@ssyno ssyno Jul 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess we have to do so, if we want to use the watchers architecture

Complete(r)
}

// isKubeconfigSecret checks if a secret is a teleport kubeconfig secret we should watch
func (r *ClusterReconciler) isKubeconfigSecret(obj client.Object) bool {
secret, ok := obj.(*corev1.Secret)
if !ok {
return false
}

// Check if it's in the teleport bot namespace and matches our naming pattern
if secret.Namespace != key.TeleportBotNamespace {
return false
}

// Check if it matches teleport kubeconfig secret naming pattern: teleport-{cluster}-kubeconfig
return strings.HasPrefix(secret.Name, "teleport-") && strings.HasSuffix(secret.Name, "-kubeconfig")
}

// isTbotConfigMap checks if a configmap is a tbot configmap we should watch
func (r *ClusterReconciler) isTbotConfigMap(obj client.Object) bool {
cm, ok := obj.(*corev1.ConfigMap)
if !ok {
return false
}

// Check if it's in the teleport bot namespace and matches our naming pattern
if cm.Namespace != key.TeleportBotNamespace {
return false
}

// Check if it matches tbot configmap naming pattern: teleport-tbot-{cluster}-config
return strings.HasPrefix(cm.Name, "teleport-tbot-") && strings.HasSuffix(cm.Name, "-config")
}
Comment on lines +288 to +317

@stone-z stone-z Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If these are truly necessary, they should be key functions, not instance methods


// findClustersForKubeconfigSecret maps kubeconfig secret changes back to cluster reconcile requests
func (r *ClusterReconciler) findClustersForKubeconfigSecret(obj client.Object) []reconcile.Request {
secret, ok := obj.(*corev1.Secret)
if !ok {
return nil
}

// Extract cluster name from secret name: teleport-{cluster}-kubeconfig -> {cluster}
if !strings.HasPrefix(secret.Name, "teleport-") || !strings.HasSuffix(secret.Name, "-kubeconfig") {
return nil
}

clusterName := secret.Name[len("teleport-") : len(secret.Name)-len("-kubeconfig")]

// Find the cluster in all organization namespaces
clusters := &capi.ClusterList{}
if err := r.Client.List(context.TODO(), clusters); err != nil {
r.Log.Error(err, "Failed to list clusters for kubeconfig secret", "secret", secret.Name)
return nil
}

var requests []reconcile.Request
for _, cluster := range clusters.Items {
if cluster.Name == clusterName {
requests = append(requests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: cluster.Name,
Namespace: cluster.Namespace,
},
})
}
}

return requests
}

// findClustersForTbotConfigMap maps tbot configmap changes back to cluster reconcile requests
func (r *ClusterReconciler) findClustersForTbotConfigMap(obj client.Object) []reconcile.Request {
cm, ok := obj.(*corev1.ConfigMap)
if !ok {
return nil
}

// Extract cluster name from configmap name: teleport-tbot-{cluster}-config -> {cluster}
if !strings.HasPrefix(cm.Name, "teleport-tbot-") || !strings.HasSuffix(cm.Name, "-config") {
return nil
}

clusterName := cm.Name[len("teleport-tbot-") : len(cm.Name)-len("-config")]

// Find the cluster in all organization namespaces
clusters := &capi.ClusterList{}
if err := r.Client.List(context.TODO(), clusters); err != nil {
r.Log.Error(err, "Failed to list clusters for tbot configmap", "configmap", cm.Name)
return nil
}

var requests []reconcile.Request
for _, cluster := range clusters.Items {
if cluster.Name == clusterName {
requests = append(requests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: cluster.Name,
Namespace: cluster.Namespace,
},
})
}
}

return requests
}