Merge pull request #120 from datum-cloud/fix/mgmt-controller-fail-loud

fix(mgmt): fail loud on missing federation kubeconfig; rename federation client

Author: scotwells

cmd/main.go

@@ -66,10 +66,10 @@ var (
 	gitTreeState = "unknown"
 	buildDate    = "unknown"
 
-	// upstreamRestConfig holds the REST config for the upstream federation control
-	// plane (Karmada). It is populated from --upstream-kubeconfig when set, and
-	// is nil when the flag is omitted.
-	upstreamRestConfig *rest.Config
+	// federationRestConfig holds the REST config for the Karmada federation control
+	// plane. It is populated from --federation-kubeconfig when set, and is nil
+	// when the flag is omitted.
+	federationRestConfig *rest.Config
 )
 
 func init() {
@@ -86,14 +86,15 @@ func init() {
 	// +kubebuilder:scaffold:scheme
 }
 
+//nolint:gocyclo // main wires all controller paths; complexity is inherent to startup sequencing
 func main() {
 
 	var enableLeaderElection bool
 	var leaderElectionNamespace string
 	var probeAddr string
 	var serverConfigFile string
-	var upstreamKubeconfig string
-	var upstreamContext string
+	var federationKubeconfig string
+	var federationContext string
 	var enableManagementControllers bool
 	var enableCellControllers bool
 
@@ -102,11 +103,12 @@ func main() {
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
 	flag.StringVar(&leaderElectionNamespace, "leader-elect-namespace", "", "The namespace to use for leader election.")
-	flag.StringVar(&upstreamKubeconfig, "upstream-kubeconfig", "",
-		"Path to the kubeconfig file for the upstream federation control plane (Karmada). "+
+	flag.StringVar(&federationKubeconfig, "federation-kubeconfig", "",
+		"Path to the kubeconfig file for the Karmada federation control plane. "+
+			"Required when --enable-management-controllers is set. "+
 			"When omitted, federation features are disabled.")
-	flag.StringVar(&upstreamContext, "upstream-context", "",
-		"Context to use from the upstream kubeconfig. When omitted, the current context is used.")
+	flag.StringVar(&federationContext, "federation-context", "",
+		"Context to use from the federation kubeconfig. When omitted, the current context is used.")
 	flag.BoolVar(&enableManagementControllers, "enable-management-controllers", false,
 		"Enable management-plane controllers (WorkloadDeploymentFederator, InstanceProjector).")
 	flag.BoolVar(&enableCellControllers, "enable-cell-controllers", false,
@@ -123,21 +125,35 @@ func main() {
 
 	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
 
-	// Load the downstream REST config when --downstream-kubeconfig is provided.
-	// When the flag is omitted, upstreamRestConfig remains nil and federation
-	// features will be skipped at controller setup time.
-	if upstreamKubeconfig != "" {
+	// Load the federation (Karmada) control plane REST config when
+	// --federation-kubeconfig is provided. When the flag is omitted,
+	// federationRestConfig remains nil; management controllers will refuse to
+	// start if --enable-management-controllers is also set.
+	if federationKubeconfig != "" {
 		loader := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-			&clientcmd.ClientConfigLoadingRules{ExplicitPath: upstreamKubeconfig},
-			&clientcmd.ConfigOverrides{CurrentContext: upstreamContext},
+			&clientcmd.ClientConfigLoadingRules{ExplicitPath: federationKubeconfig},
+			&clientcmd.ConfigOverrides{CurrentContext: federationContext},
 		)
 		var err error
-		upstreamRestConfig, err = loader.ClientConfig()
+		federationRestConfig, err = loader.ClientConfig()
 		if err != nil {
-			setupLog.Error(err, "unable to load upstream kubeconfig", "path", upstreamKubeconfig)
+			setupLog.Error(err, "unable to load federation kubeconfig", "path", federationKubeconfig)
 			os.Exit(1)
 		}
-		setupLog.Info("upstream kubeconfig loaded", "path", upstreamKubeconfig)
+		setupLog.Info("federation kubeconfig loaded", "path", federationKubeconfig)
+	}
+
+	// Fail loud: management controllers require a federation kubeconfig. Silently
+	// skipping them when --enable-management-controllers is set would leave
+	// federation and instance projection broken with no visible signal — the same
+	// class of failure as the quota P1 issue. An operator who explicitly enables
+	// management controllers but omits --federation-kubeconfig has a misconfiguration
+	// that must surface immediately rather than at runtime.
+	if enableManagementControllers && federationRestConfig == nil {
+		setupLog.Error(nil,
+			"management controllers enabled but no federation kubeconfig configured",
+			"hint", "set --federation-kubeconfig")
+		os.Exit(1)
 	}
 
 	setupLog.Info("starting compute",
@@ -240,13 +256,15 @@ func main() {
 		}
 	}
 
-	// Build a single downstream client shared across all controllers that need
-	// to read or write to the downstream control plane. Nil when federation is disabled.
-	var downstreamClient client.Client
-	if upstreamRestConfig != nil {
-		downstreamClient, err = client.New(upstreamRestConfig, client.Options{Scheme: scheme})
+	// Build a single federation client shared across all controllers that need to
+	// read or write to the Karmada federation control plane. This is the hub that
+	// the management controllers federate through and that edge cells write back to.
+	// Nil when --federation-kubeconfig is not set (i.e. federation is disabled).
+	var federationClient client.Client
+	if federationRestConfig != nil {
+		federationClient, err = client.New(federationRestConfig, client.Options{Scheme: scheme})
 		if err != nil {
-			setupLog.Error(err, "unable to create downstream client")
+			setupLog.Error(err, "unable to create federation client")
 			os.Exit(1)
 		}
 	}
@@ -262,7 +280,7 @@ func main() {
 		clusterNameForProject := func(_ string) multicluster.ClusterName {
 			return multicluster.ClusterName(singleClusterName)
 		}
-		instanceReconciler := &controller.InstanceReconciler{UpstreamClient: downstreamClient}
+		instanceReconciler := &controller.InstanceReconciler{FederationClient: federationClient}
 		err = instanceReconciler.SetupWithManager(
 			mgr,
 			quotaRestConfig,
@@ -278,10 +296,11 @@ func main() {
 	}
 
 	// WorkloadDeploymentFederator and InstanceProjector are management-plane
-	// controllers that run on the control-plane cluster. They require a downstream
-	// control plane to be configured (--upstream-kubeconfig provided).
-	if enableManagementControllers && upstreamRestConfig != nil {
-		extra, err := setupManagementControllers(mgr, downstreamClient)
+	// controllers that run on the control-plane cluster. The fail-loud guard above
+	// ensures federationRestConfig is non-nil when enableManagementControllers is
+	// true; the nil check here is a defensive belt-and-suspenders guard.
+	if enableManagementControllers && federationRestConfig != nil {
+		extra, err := setupManagementControllers(mgr, federationClient)
 		if err != nil {
 			setupLog.Error(err, "unable to set up management controllers")
 			os.Exit(1)
@@ -427,33 +446,33 @@ func ignoreCanceled(err error) error {
 
 // setupManagementControllers wires the WorkloadDeploymentFederator and
 // InstanceProjector onto mgr. It returns any additional Runnable objects that
-// must be started alongside the main manager (the downstream manager used by
+// must be started alongside the main manager (the federation manager used by
 // InstanceProjector). Called only when management controllers are enabled and
-// an upstream REST config is available.
-func setupManagementControllers(mgr mcmanager.Manager, downstreamClient client.Client) ([]manager.Runnable, error) {
-	federator := &controller.WorkloadDeploymentFederator{UpstreamClient: downstreamClient}
+// a federation REST config is available.
+func setupManagementControllers(mgr mcmanager.Manager, federationClient client.Client) ([]manager.Runnable, error) {
+	federator := &controller.WorkloadDeploymentFederator{FederationClient: federationClient}
 	if err := federator.SetupWithManager(mgr); err != nil {
 		return nil, fmt.Errorf("WorkloadDeploymentFederator: %w", err)
 	}
 
-	// InstanceProjector runs in the Control Plane Cell, watches Instances
-	// written back by POP-cell operators, and projects them into the
-	// corresponding project namespaces via the multicluster manager.
-	downstreamMgr, err := manager.New(upstreamRestConfig, manager.Options{
+	// InstanceProjector runs in the management plane, watches Instances written
+	// back by POP-cell operators to the Karmada federation control plane, and
+	// projects them into the corresponding project namespaces via the multicluster manager.
+	federationMgr, err := manager.New(federationRestConfig, manager.Options{
 		Scheme:  scheme,
 		Metrics: metricsserver.Options{BindAddress: "0"},
 	})
 	if err != nil {
-		return nil, fmt.Errorf("downstream manager for InstanceProjector: %w", err)
+		return nil, fmt.Errorf("federation manager for InstanceProjector: %w", err)
 	}
 	if err = (&controller.InstanceProjector{
-		UpstreamClient: downstreamClient,
-		MCManager:      mgr,
-	}).SetupWithManager(downstreamMgr); err != nil {
+		FederationClient: federationClient,
+		MCManager:        mgr,
+	}).SetupWithManager(federationMgr); err != nil {
 		return nil, fmt.Errorf("InstanceProjector: %w", err)
 	}
 
-	return []manager.Runnable{downstreamMgr}, nil
+	return []manager.Runnable{federationMgr}, nil
 }
 
 // singleModeProjectID returns an InstanceProjectIDFunc for single-cell mode.

config/base/crd/bases/compute.datumapis.com_instances.yaml

@@ -35,6 +35,10 @@ spec:
       name: Message
       priority: 1
       type: string
+    - jsonPath: .status.conditions[?(@.type=="QuotaGranted")].reason
+      name: Quota
+      priority: 1
+      type: string
     name: v1alpha
     schema:
       openAPIV3Schema:

config/base/manager/manager.yaml

@@ -33,7 +33,7 @@ spec:
           - --leader-elect=$(LEADER_ELECT)
           - --health-probe-bind-address=$(HEALTH_PROBE_BIND_ADDRESS)
           - --server-config=$(SERVER_CONFIG)
-          - --upstream-kubeconfig=$(UPSTREAM_KUBECONFIG)
+          - --federation-kubeconfig=$(FEDERATION_KUBECONFIG)
           - --enable-management-controllers=$(ENABLE_MANAGEMENT_CONTROLLERS)
           - --enable-cell-controllers=$(ENABLE_CELL_CONTROLLERS)
         env:
@@ -43,7 +43,7 @@ spec:
             value: ":8081"
           - name: SERVER_CONFIG
             value: /config/config.yaml
-          - name: UPSTREAM_KUBECONFIG
+          - name: FEDERATION_KUBECONFIG
             value: ""
           - name: ENABLE_MANAGEMENT_CONTROLLERS
             value: "false"

config/overlays/management-plane/downstream_kubeconfig_patch.yaml

@@ -8,7 +8,7 @@ spec:
       containers:
         - name: manager
           env:
-            - name: UPSTREAM_KUBECONFIG
+            - name: FEDERATION_KUBECONFIG
               value: /etc/kubernetes/downstream/auth/downstream-kubeconfig.yaml
           volumeMounts:
             - name: downstream-kubeconfig

internal/controller/instance_controller.go

@@ -137,13 +137,14 @@ type InstanceReconciler struct {
 	// cluster is "single" regardless of project ID. When nil, falls back to
 	// multicluster.ClusterName(projectID), which is correct for Milo mode.
 	clusterNameForProject func(projectID string) multicluster.ClusterName
-	// UpstreamClient is an optional client pointing at the downstream control plane.
+	// FederationClient is an optional client pointing at the upstream
+	// Karmada/federation control plane (configured via --federation-kubeconfig).
 	// When non-nil, the reconciler writes a copy of each Instance back to the
-	// downstream control plane so that the InstanceProjector (running in the
+	// federation control plane so that the InstanceProjector (running in the
 	// management cluster) can aggregate status across all POP cells. Set to nil to
 	// disable federation write-back (e.g. in non-federation deployments).
-	UpstreamClient client.Client
-	finalizers     finalizer.Finalizers
+	FederationClient client.Client
+	finalizers       finalizer.Finalizers
 }
 
 // +kubebuilder:rbac:groups=compute.datumapis.com,resources=instances,verbs=get;list;watch;create;update;patch;delete
@@ -409,14 +410,14 @@ func (r *InstanceReconciler) removeQuotaSchedulingGate(ctx context.Context, cl c
 // Finalize removes the downstream write-back Instance when the local Instance is
 // deleted. It is a no-op when downstream federation is disabled.
 func (r *InstanceReconciler) Finalize(ctx context.Context, obj client.Object) (finalizer.Result, error) {
-	if r.UpstreamClient == nil {
+	if r.FederationClient == nil {
 		return finalizer.Result{}, nil
 	}
 
 	instance := obj.(*computev1alpha.Instance)
 
 	downstreamInstance := &computev1alpha.Instance{}
-	err := r.UpstreamClient.Get(ctx, client.ObjectKeyFromObject(instance), downstreamInstance)
+	err := r.FederationClient.Get(ctx, client.ObjectKeyFromObject(instance), downstreamInstance)
 	if apierrors.IsNotFound(err) {
 		// Already gone — nothing to do.
 		return finalizer.Result{}, nil
@@ -425,18 +426,18 @@ func (r *InstanceReconciler) Finalize(ctx context.Context, obj client.Object) (f
 		return finalizer.Result{}, fmt.Errorf("failed getting downstream instance for deletion: %w", err)
 	}
 
-	if err := r.UpstreamClient.Delete(ctx, downstreamInstance); client.IgnoreNotFound(err) != nil {
+	if err := r.FederationClient.Delete(ctx, downstreamInstance); client.IgnoreNotFound(err) != nil {
 		return finalizer.Result{}, fmt.Errorf("failed deleting downstream write-back instance: %w", err)
 	}
 
 	return finalizer.Result{}, nil
 }
 
 // writeBackToUpstream copies the Instance spec and status to the upstream
-// Karmada control plane so that the InstanceProjector can aggregate state from
-// all POP cells. It is a no-op when UpstreamClient is nil (federation disabled).
+// Karmada/federation control plane so that the InstanceProjector can aggregate
+// state from all POP cells. It is a no-op when FederationClient is nil (federation disabled).
 func (r *InstanceReconciler) writeBackToUpstream(ctx context.Context, clusterName multicluster.ClusterName, instance *computev1alpha.Instance) error {
-	if r.UpstreamClient == nil {
+	if r.FederationClient == nil {
 		return nil
 	}
 
@@ -451,7 +452,7 @@ func (r *InstanceReconciler) writeBackToUpstream(ctx context.Context, clusterNam
 	// "default"), which the InstanceProjector needs to find the right project cluster.
 	upstreamNamespace := instance.Namespace // fallback: cell namespace (ns-<uid>)
 	var downstreamNS corev1.Namespace
-	if err := r.UpstreamClient.Get(ctx, client.ObjectKey{Name: instance.Namespace}, &downstreamNS); err == nil {
+	if err := r.FederationClient.Get(ctx, client.ObjectKey{Name: instance.Namespace}, &downstreamNS); err == nil {
 		if v := downstreamNS.Labels[downstreamclient.UpstreamOwnerNamespaceLabel]; v != "" {
 			upstreamNamespace = v
 		}
@@ -473,18 +474,18 @@ func (r *InstanceReconciler) writeBackToUpstream(ctx context.Context, clusterNam
 	}
 
 	existing := &computev1alpha.Instance{}
-	err := r.UpstreamClient.Get(ctx, client.ObjectKeyFromObject(writeBack), existing)
+	err := r.FederationClient.Get(ctx, client.ObjectKeyFromObject(writeBack), existing)
 	if apierrors.IsNotFound(err) {
 		// Ensure the namespace exists in the downstream control plane before creating the Instance.
 		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: instance.Namespace}}
-		if err := r.UpstreamClient.Create(ctx, ns); err != nil && !apierrors.IsAlreadyExists(err) {
+		if err := r.FederationClient.Create(ctx, ns); err != nil && !apierrors.IsAlreadyExists(err) {
 			return fmt.Errorf("failed ensuring downstream namespace: %w", err)
 		}
-		if err := r.UpstreamClient.Create(ctx, writeBack); err != nil {
+		if err := r.FederationClient.Create(ctx, writeBack); err != nil {
 			return fmt.Errorf("failed creating downstream write-back instance: %w", err)
 		}
 		writeBack.Status = instance.Status
-		if err := r.UpstreamClient.Status().Update(ctx, writeBack); err != nil {
+		if err := r.FederationClient.Status().Update(ctx, writeBack); err != nil {
 			return fmt.Errorf("failed updating downstream write-back instance status after create: %w", err)
 		}
 		return nil
@@ -498,15 +499,15 @@ func (r *InstanceReconciler) writeBackToUpstream(ctx context.Context, clusterNam
 		!apiequality.Semantic.DeepEqual(existing.Labels, writeBack.Labels) {
 		existing.Spec = instance.Spec
 		existing.Labels = writeBack.Labels
-		if err := r.UpstreamClient.Update(ctx, existing); err != nil {
+		if err := r.FederationClient.Update(ctx, existing); err != nil {
 			return fmt.Errorf("failed updating downstream write-back instance: %w", err)
 		}
 	}
 
 	// Update status only if it differs.
 	if !apiequality.Semantic.DeepEqual(existing.Status, instance.Status) {
 		existing.Status = instance.Status
-		if err := r.UpstreamClient.Status().Update(ctx, existing); err != nil {
+		if err := r.FederationClient.Status().Update(ctx, existing); err != nil {
 			return fmt.Errorf("failed updating downstream write-back instance status: %w", err)
 		}
 	}