From b1acad32e7a78609513972f45ae978a5cfe8983c Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Sat, 7 Nov 2026 14:50:58 +0100 Subject: [PATCH 1/5] feat(sync): preserve not-ready clusters in local kubeconfig Previously, clusters whose Ready condition was False were silently removed from the local kubeconfig on the next sync. This meant a transient controller failure (e.g. missing secret key during a Greenhouse upgrade) would cut off access to the cluster, making it impossible to reach and investigate the issue. This change keeps not-ready entries intact in the local kubeconfig: - notReady clusters that already exist locally are preserved via stub entries injected into the server-side config before the stale-cleanup pass in mergeKubeconfig. The stubs prevent deletion without touching the local entry's content. - A new --include-not-ready flag opts into fully merging not-ready cluster spec data, useful when the controller is temporarily degraded but the CRD spec is known-good (e.g. during a Greenhouse rollout). Signed-off-by: onuryilmaz --- cmd/sync.go | 70 +++++++++++++++++++++++++++++++++--- cmd/sync_test.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 4 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 29d2392..4d4f19e 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -42,6 +42,7 @@ var ( kubeloginExtraArgs []string kubeloginTokenCacheDir string dryRun bool + includeNotReady bool ) func init() { @@ -74,6 +75,7 @@ func init() { syncCmd.Flags().StringVar(&kubeloginTokenCacheDir, "kubelogin-token-cache-dir", defaultTokenCacheDir, "Directory for OIDC token cache files") syncCmd.Flags().BoolVar(&dryRun, "dry-run", false, "Preview changes without writing to the kubeconfig file") + syncCmd.Flags().BoolVar(&includeNotReady, "include-not-ready", false, "Also merge clusters whose Ready condition is False (spec data must be present)") // BindPFlags can theoretically return an error if called with `nil` as an argument // which should never happen after at least one flag was defined. That's why the output @@ -87,9 +89,13 @@ var syncCmd = &cobra.Command{ Long: `Fetches ClusterKubeconfig resources from a Greenhouse cluster and merges them into your local kubeconfig file. -Only clusters whose Ready condition is True are merged. Clusters that have -been removed from Greenhouse are cleaned up from your local config. Existing -non-managed entries are never touched. +Only clusters whose Ready condition is True are merged by default. Clusters +whose Ready condition is False are preserved in the local kubeconfig so they +remain reachable (e.g. to investigate or recover the cluster). Use +--include-not-ready to also merge their spec data. + +Clusters that have been removed from Greenhouse are cleaned up from your +local config. Existing non-managed entries are never touched. OIDC credentials are preserved across syncs: id-token and refresh-token are carried forward so you do not need to re-authenticate after every sync. @@ -223,11 +229,27 @@ func runSync(cmd *cobra.Command, args []string) error { localConfig = clientcmdapi.NewConfig() } - serverConfig, err := buildIncomingKubeconfig(ready) + toMerge := ready + if includeNotReady { + toMerge = append(toMerge, notReady...) + } + + serverConfig, err := buildIncomingKubeconfig(toMerge) if err != nil { return fmt.Errorf("failed to create server config: %w", err) } + // Preserve notReady cluster entries that already exist in the local kubeconfig. + // A cluster whose Ready condition is transiently false (e.g. controller bug, rolling + // upgrade) must not be wiped from the local kubeconfig — the user may need it to + // reach and fix the cluster. We inject stub entries into serverConfig so the stale- + // cleanup pass in mergeKubeconfig treats them as still-present on the server side. + // The stubs only affect deletion logic; they are never written to disk. + // When --include-not-ready is set, notReady entries are fully merged instead. + if !includeNotReady { + injectNotReadyStubs(serverConfig, notReady, localConfig) + } + // Take a snapshot before merge for dry-run diff — only needed when --dry-run is set. var localConfigBefore *clientcmdapi.Config if dryRun { @@ -285,6 +307,46 @@ func filterReady(items []v1alpha1.ClusterKubeconfig) []v1alpha1.ClusterKubeconfi return ready } +// injectNotReadyStubs adds stub entries into serverConfig for each notReady ClusterKubeconfig +// that already has a corresponding managed entry in localConfig. This prevents mergeKubeconfig's +// stale-cleanup pass from deleting local entries for clusters that are temporarily not-ready — +// the user needs those entries to reach and recover the cluster. +// Stubs are derived from the existing local entries (no spec data required) and are never written to disk. +func injectNotReadyStubs(serverConfig *clientcmdapi.Config, notReady []v1alpha1.ClusterKubeconfig, localConfig *clientcmdapi.Config) { + for _, ckc := range notReady { + // Look for any local managed context whose name matches this CKC, then copy its + // referenced cluster/authinfo names into serverConfig so cleanup skips them. + for ctxName, localCtx := range localConfig.Contexts { + if localCtx == nil { + continue + } + // Context name matches the CKC name (contexts are not prefixed). + if ctxName != ckc.Name { + continue + } + // Inject stub context. + if _, exists := serverConfig.Contexts[ctxName]; !exists { + serverConfig.Contexts[ctxName] = localCtx.DeepCopy() + } + // Inject stub cluster (keyed by the unprefixed name that mergeKubeconfig looks up). + clusterName := unmanagedNameFunc(localCtx.Cluster) + if _, exists := serverConfig.Clusters[clusterName]; !exists { + if src, ok := localConfig.Clusters[localCtx.Cluster]; ok { + serverConfig.Clusters[clusterName] = src.DeepCopy() + } + } + // Inject stub authinfo. + authName := unmanagedNameFunc(localCtx.AuthInfo) + if _, exists := serverConfig.AuthInfos[authName]; !exists { + if src, ok := localConfig.AuthInfos[localCtx.AuthInfo]; ok { + serverConfig.AuthInfos[authName] = src.DeepCopy() + } + } + break + } + } +} + // buildSyncResult constructs an output.SyncResult from ready and notReady cluster lists. func buildSyncResult(ready, notReady []v1alpha1.ClusterKubeconfig) output.SyncResult { result := output.SyncResult{} diff --git a/cmd/sync_test.go b/cmd/sync_test.go index fc89fb8..4a410b8 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1055,3 +1055,97 @@ func TestWriteTargetFromKUBECONFIG(t *testing.T) { }) } } + +func TestInjectNotReadyStubs_PreservesLocalEntries(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + defer func() { prefix = orig }() + + notReadyCkc := greenhousev1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{Name: "broken-cluster"}, + } + + // Local kubeconfig already has a managed entry for broken-cluster. + localConfig := clientcmdapi.NewConfig() + localConfig.Clusters["cloudctl:broken-cluster"] = &clientcmdapi.Cluster{Server: "https://broken.example.com"} + localConfig.AuthInfos["cloudctl:oidc@broken-cluster"] = &clientcmdapi.AuthInfo{} + localConfig.Contexts["broken-cluster"] = &clientcmdapi.Context{ + Cluster: "cloudctl:broken-cluster", + AuthInfo: "cloudctl:oidc@broken-cluster", + } + + serverConfig := clientcmdapi.NewConfig() + + injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + + // Stubs must be present so mergeKubeconfig's stale-cleanup does not delete them. + g.Expect(serverConfig.Contexts).To(HaveKey("broken-cluster")) + g.Expect(serverConfig.Clusters).To(HaveKey("broken-cluster")) + g.Expect(serverConfig.AuthInfos).To(HaveKey("oidc@broken-cluster")) +} + +func TestInjectNotReadyStubs_SkipsUnknownClusters(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + defer func() { prefix = orig }() + + // notReady cluster has no corresponding local entry. + notReadyCkc := greenhousev1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{Name: "new-cluster"}, + } + + serverConfig := clientcmdapi.NewConfig() + localConfig := clientcmdapi.NewConfig() + + injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + + // Nothing added — no local entry to preserve. + g.Expect(serverConfig.Contexts).To(BeEmpty()) + g.Expect(serverConfig.Clusters).To(BeEmpty()) + g.Expect(serverConfig.AuthInfos).To(BeEmpty()) +} + +func TestIncludeNotReady_MergesSpecData(t *testing.T) { + g := NewWithT(t) + + orig := includeNotReady + includeNotReady = true + defer func() { includeNotReady = orig }() + + notReadyCkc := greenhousev1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{Name: "notready-cluster"}, + } + notReadyCkc.Status.Conditions.SetConditions( + greenhousemetav1alpha1.FalseCondition( + greenhousemetav1alpha1.ReadyCondition, + "KubeconfigMissing", + "secret data key greenhousekubeconfig missing or empty", + ), + ) + notReadyCkc.Spec.Kubeconfig.Clusters = append(notReadyCkc.Spec.Kubeconfig.Clusters, + greenhousev1alpha1.ClusterKubeconfigClusterItem{ + Name: "notready-cluster", + Cluster: greenhousev1alpha1.ClusterKubeconfigCluster{ + Server: "https://notready.example.com", + }, + }, + ) + + ready, notReady := partitionReady([]greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}) + g.Expect(ready).To(BeEmpty()) + g.Expect(notReady).To(HaveLen(1)) + + // With includeNotReady=true, notReady entries are appended to the merge set. + toMerge := ready + if includeNotReady { + toMerge = append(toMerge, notReady...) + } + serverConfig, err := buildIncomingKubeconfig(toMerge) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(serverConfig.Clusters).To(HaveKey("notready-cluster")) + g.Expect(serverConfig.Clusters["notready-cluster"].Server).To(Equal("https://notready.example.com")) +} From a6a3b7e1a22c8b1c5574211fa8feb2cc90827496 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Tue, 10 Nov 2026 09:44:39 +0100 Subject: [PATCH 2/5] fix(sync): address Copilot review findings on not-ready preservation Six issues identified in PR review: - Match stubs by spec context names (not CKC resource name) so clusters whose context name differs from the resource name (e.g. e2e "demo"/ "ctx1") are correctly protected from stale cleanup. - Strip managed prefix from stub context references to prevent double-prefixed names (cloudctl:cloudctl:...) when mergeIdenticalUsers is disabled or when the merger re-applies the prefix. - Read includeNotReady from Viper so CLOUDCTL_INCLUDE_NOT_READY env var and config-file values are honoured, consistent with all other flags. - Allow --include-not-ready to bypass the len(ready)==0 early return; previously the flag had no effect when every cluster was not-ready, which is the primary motivation for the flag. - Pass the actual merge/skip sets to result builders so clusters merged via --include-not-ready are reported as synced, not skipped. - Skip unmanaged contexts in injectNotReadyStubs to avoid accidentally converting a manually-created entry into a managed one. Signed-off-by: onuryilmaz --- cmd/sync.go | 62 ++++++++++++++++++++++---------- cmd/sync_test.go | 93 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 19 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 4d4f19e..57fc78a 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -141,6 +141,7 @@ func runSync(cmd *cobra.Command, args []string) error { kubeloginExtraArgs = viper.GetStringSlice("kubelogin-extra-args") kubeloginTokenCacheDir = viper.GetString("kubelogin-token-cache-dir") dryRun = viper.GetBool("dry-run") + includeNotReady = viper.GetBool("include-not-ready") format, err := output.ParseFormat(viper.GetString("output")) if err != nil { @@ -211,10 +212,9 @@ func runSync(cmd *cobra.Command, args []string) error { ready, notReady := partitionReady(allKubeconfigs) - if len(ready) == 0 { + if len(ready) == 0 && !(includeNotReady && len(notReady) > 0) { return printer.Print(buildSyncResult(nil, notReady)) } - var localConfig *clientcmdapi.Config if remoteClusterKubeconfig != "" { localConfig, err = clientcmd.LoadFromFile(remoteClusterKubeconfig) @@ -229,12 +229,17 @@ func runSync(cmd *cobra.Command, args []string) error { localConfig = clientcmdapi.NewConfig() } - toMerge := ready + // merged = ready + notReady entries that were opted into full merge. + // skipped = notReady entries that were NOT merged (preserved in place or absent). + merged := ready + var skipped []v1alpha1.ClusterKubeconfig if includeNotReady { - toMerge = append(toMerge, notReady...) + merged = append(merged, notReady...) + } else { + skipped = notReady } - serverConfig, err := buildIncomingKubeconfig(toMerge) + serverConfig, err := buildIncomingKubeconfig(merged) if err != nil { return fmt.Errorf("failed to create server config: %w", err) } @@ -264,7 +269,7 @@ func runSync(cmd *cobra.Command, args []string) error { err = mergeKubeconfig(localConfig, serverConfig) stopMerge() if err != nil { - _ = printer.Print(buildFailedSyncResult(ready, notReady, err)) + _ = printer.Print(buildFailedSyncResult(merged, skipped, err)) return fmt.Errorf(`failed to merge ClusterKubeconfig: %w`, err) } @@ -279,11 +284,11 @@ func runSync(cmd *cobra.Command, args []string) error { } if writeErr := writeConfig(localConfig, writeTarget); writeErr != nil { - _ = printer.Print(buildFailedSyncResult(ready, notReady, writeErr)) + _ = printer.Print(buildFailedSyncResult(merged, skipped, writeErr)) return fmt.Errorf("failed to write merged kubeconfig: %w", writeErr) } - return printer.Print(buildSyncResult(ready, notReady)) + return printer.Print(buildSyncResult(merged, skipped)) } // partitionReady splits ClusterKubeconfigs into ready and notReady slices. @@ -314,35 +319,54 @@ func filterReady(items []v1alpha1.ClusterKubeconfig) []v1alpha1.ClusterKubeconfi // Stubs are derived from the existing local entries (no spec data required) and are never written to disk. func injectNotReadyStubs(serverConfig *clientcmdapi.Config, notReady []v1alpha1.ClusterKubeconfig, localConfig *clientcmdapi.Config) { for _, ckc := range notReady { - // Look for any local managed context whose name matches this CKC, then copy its - // referenced cluster/authinfo names into serverConfig so cleanup skips them. - for ctxName, localCtx := range localConfig.Contexts { - if localCtx == nil { + // Collect the set of context names this CKC owns: prefer spec context names when + // available (spec may be populated even if Ready=False), fall back to the resource + // name so we still protect entries written by an older sync. + ctxNames := make([]string, 0, len(ckc.Spec.Kubeconfig.Contexts)+1) + for _, ctxItem := range ckc.Spec.Kubeconfig.Contexts { + ctxNames = append(ctxNames, ctxItem.Name) + } + if len(ctxNames) == 0 { + ctxNames = append(ctxNames, ckc.Name) + } + + for _, ctxName := range ctxNames { + localCtx, ok := localConfig.Contexts[ctxName] + if !ok || localCtx == nil { continue } - // Context name matches the CKC name (contexts are not prefixed). - if ctxName != ckc.Name { + // Fix 6: skip unmanaged contexts — only protect entries cloudctl wrote. + if !isManaged(localCtx.Cluster) { continue } - // Inject stub context. + + // Fix 2: stub context must carry unprefixed cluster/authinfo references so + // mergeKubeconfig's lookup (which strips the prefix before checking serverConfig) + // resolves correctly and does not produce double-prefixed names. + stubCtx := &clientcmdapi.Context{ + Cluster: unmanagedNameFunc(localCtx.Cluster), + AuthInfo: unmanagedNameFunc(localCtx.AuthInfo), + Namespace: localCtx.Namespace, + } if _, exists := serverConfig.Contexts[ctxName]; !exists { - serverConfig.Contexts[ctxName] = localCtx.DeepCopy() + serverConfig.Contexts[ctxName] = stubCtx } - // Inject stub cluster (keyed by the unprefixed name that mergeKubeconfig looks up). + + // Inject stub cluster keyed by the unprefixed name. clusterName := unmanagedNameFunc(localCtx.Cluster) if _, exists := serverConfig.Clusters[clusterName]; !exists { if src, ok := localConfig.Clusters[localCtx.Cluster]; ok { serverConfig.Clusters[clusterName] = src.DeepCopy() } } - // Inject stub authinfo. + + // Inject stub authinfo keyed by the unprefixed name. authName := unmanagedNameFunc(localCtx.AuthInfo) if _, exists := serverConfig.AuthInfos[authName]; !exists { if src, ok := localConfig.AuthInfos[localCtx.AuthInfo]; ok { serverConfig.AuthInfos[authName] = src.DeepCopy() } } - break } } } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 4a410b8..f52fa34 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1084,6 +1084,71 @@ func TestInjectNotReadyStubs_PreservesLocalEntries(t *testing.T) { g.Expect(serverConfig.Contexts).To(HaveKey("broken-cluster")) g.Expect(serverConfig.Clusters).To(HaveKey("broken-cluster")) g.Expect(serverConfig.AuthInfos).To(HaveKey("oidc@broken-cluster")) + + // Fix 2: stub context references must be unprefixed to avoid double-prefix on re-merge. + g.Expect(serverConfig.Contexts["broken-cluster"].Cluster).To(Equal("broken-cluster")) + g.Expect(serverConfig.Contexts["broken-cluster"].AuthInfo).To(Equal("oidc@broken-cluster")) +} + +func TestInjectNotReadyStubs_UsesSpecContextNames(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + defer func() { prefix = orig }() + + // CKC resource name is "demo" but the context in spec is "ctx1" (as in e2e tests). + notReadyCkc := greenhousev1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{Name: "demo"}, + } + notReadyCkc.Spec.Kubeconfig.Contexts = append(notReadyCkc.Spec.Kubeconfig.Contexts, + greenhousev1alpha1.ClusterKubeconfigContextItem{ + Name: "ctx1", + }, + ) + + localConfig := clientcmdapi.NewConfig() + localConfig.Clusters["cloudctl:ctx1-cluster"] = &clientcmdapi.Cluster{Server: "https://ctx1.example.com"} + localConfig.AuthInfos["cloudctl:oidc@ctx1"] = &clientcmdapi.AuthInfo{} + localConfig.Contexts["ctx1"] = &clientcmdapi.Context{ + Cluster: "cloudctl:ctx1-cluster", + AuthInfo: "cloudctl:oidc@ctx1", + } + + serverConfig := clientcmdapi.NewConfig() + + injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + + // Stub must be keyed by spec context name "ctx1", not resource name "demo". + g.Expect(serverConfig.Contexts).To(HaveKey("ctx1")) + g.Expect(serverConfig.Contexts).NotTo(HaveKey("demo")) +} + +func TestInjectNotReadyStubs_SkipsUnmanagedContexts(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + defer func() { prefix = orig }() + + notReadyCkc := greenhousev1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{Name: "manual-cluster"}, + } + + // Context exists in local but cluster ref is NOT prefixed — it is an unmanaged entry. + localConfig := clientcmdapi.NewConfig() + localConfig.Clusters["manual-cluster"] = &clientcmdapi.Cluster{Server: "https://manual.example.com"} + localConfig.Contexts["manual-cluster"] = &clientcmdapi.Context{ + Cluster: "manual-cluster", // no cloudctl: prefix + } + + serverConfig := clientcmdapi.NewConfig() + + injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + + // Unmanaged context must not be touched. + g.Expect(serverConfig.Contexts).To(BeEmpty()) + g.Expect(serverConfig.Clusters).To(BeEmpty()) } func TestInjectNotReadyStubs_SkipsUnknownClusters(t *testing.T) { @@ -1149,3 +1214,31 @@ func TestIncludeNotReady_MergesSpecData(t *testing.T) { g.Expect(serverConfig.Clusters).To(HaveKey("notready-cluster")) g.Expect(serverConfig.Clusters["notready-cluster"].Server).To(Equal("https://notready.example.com")) } + +func TestIncludeNotReady_BypassesEarlyReturnWhenAllNotReady(t *testing.T) { + g := NewWithT(t) + + notReadyCkc := greenhousev1alpha1.ClusterKubeconfig{ + ObjectMeta: metav1.ObjectMeta{Name: "all-broken"}, + } + notReadyCkc.Status.Conditions.SetConditions( + greenhousemetav1alpha1.FalseCondition( + greenhousemetav1alpha1.ReadyCondition, + "KubeconfigMissing", + "secret data key greenhousekubeconfig missing or empty", + ), + ) + + ready, notReady := partitionReady([]greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}) + g.Expect(ready).To(BeEmpty()) + g.Expect(notReady).To(HaveLen(1)) + + // Simulate the early-return condition in runSync: + // without --include-not-ready, len(ready)==0 returns early. + // with --include-not-ready and len(notReady)>0, execution continues. + shouldContinue := func(inclNotReady bool) bool { + return !(len(ready) == 0 && !(inclNotReady && len(notReady) > 0)) + } + g.Expect(shouldContinue(false)).To(BeFalse(), "without flag: must return early") + g.Expect(shouldContinue(true)).To(BeTrue(), "with flag: must continue past early return") +} From 005b1e89d7748e4d911ec05bcd084e1c9958b32e Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Tue, 10 Nov 2026 10:28:22 +0100 Subject: [PATCH 3/5] fix(sync): simplify early-return condition to satisfy staticcheck QF1001 Signed-off-by: onuryilmaz --- cmd/sync.go | 2 +- cmd/sync_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 57fc78a..1aeebfe 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -212,7 +212,7 @@ func runSync(cmd *cobra.Command, args []string) error { ready, notReady := partitionReady(allKubeconfigs) - if len(ready) == 0 && !(includeNotReady && len(notReady) > 0) { + if len(ready) == 0 && (!includeNotReady || len(notReady) == 0) { return printer.Print(buildSyncResult(nil, notReady)) } var localConfig *clientcmdapi.Config diff --git a/cmd/sync_test.go b/cmd/sync_test.go index f52fa34..e80ab78 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1236,9 +1236,9 @@ func TestIncludeNotReady_BypassesEarlyReturnWhenAllNotReady(t *testing.T) { // Simulate the early-return condition in runSync: // without --include-not-ready, len(ready)==0 returns early. // with --include-not-ready and len(notReady)>0, execution continues. - shouldContinue := func(inclNotReady bool) bool { - return !(len(ready) == 0 && !(inclNotReady && len(notReady) > 0)) + earlyReturn := func(inclNotReady bool) bool { + return len(ready) == 0 && (!inclNotReady || len(notReady) == 0) } - g.Expect(shouldContinue(false)).To(BeFalse(), "without flag: must return early") - g.Expect(shouldContinue(true)).To(BeTrue(), "with flag: must continue past early return") + g.Expect(earlyReturn(false)).To(BeTrue(), "without flag: must return early") + g.Expect(earlyReturn(true)).To(BeFalse(), "with flag: must continue past early return") } From 262626a63b28af0dde4a6d2e0d1d3f516ef8d2a6 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Tue, 10 Nov 2026 10:45:03 +0100 Subject: [PATCH 4/5] fix(sync): address second Copilot review round findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bulgu 1: notReady entries with empty spec are kept on the preservation path even with --include-not-ready; previously they would be merged with no incoming data, causing their existing local entries to be deleted. Introduces hasSpecData() helper and filters the merged/skipped split. - Bulgu 2: stub authinfo entries are no longer added to serverConfig to avoid mergeKubeconfig's deduplication pass assigning a new hash-derived managed name and redirecting the preserved context to a different identity. injectNotReadyStubs now returns a preSeed map (server-side name → local managed name) that is pre-populated into authInfoMap before merge. The no-merge mode cleanup path also honours the pre-seed map so it does not delete protected managed authinfos. - Bulgu 3: TestIncludeNotReady_BypassesEarlyReturnWhenAllNotReady now calls the production shouldSkipMerge() helper (extracted from runSync) instead of evaluating a copied expression, so it would catch a revert. Signed-off-by: onuryilmaz --- cmd/sync.go | 79 +++++++++++++++++++++++++++++++++++++++--------- cmd/sync_test.go | 34 ++++++++++----------- 2 files changed, 82 insertions(+), 31 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 1aeebfe..ad08755 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -212,7 +212,7 @@ func runSync(cmd *cobra.Command, args []string) error { ready, notReady := partitionReady(allKubeconfigs) - if len(ready) == 0 && (!includeNotReady || len(notReady) == 0) { + if shouldSkipMerge(ready, notReady, includeNotReady) { return printer.Print(buildSyncResult(nil, notReady)) } var localConfig *clientcmdapi.Config @@ -231,10 +231,19 @@ func runSync(cmd *cobra.Command, args []string) error { // merged = ready + notReady entries that were opted into full merge. // skipped = notReady entries that were NOT merged (preserved in place or absent). + // notReady entries with an empty spec are always kept on the stub/preservation path + // regardless of --include-not-ready: they contribute no incoming data and merging + // them would delete any existing local entry without adding anything in return. merged := ready var skipped []v1alpha1.ClusterKubeconfig if includeNotReady { - merged = append(merged, notReady...) + for _, ckc := range notReady { + if hasSpecData(ckc) { + merged = append(merged, ckc) + } else { + skipped = append(skipped, ckc) + } + } } else { skipped = notReady } @@ -251,8 +260,12 @@ func runSync(cmd *cobra.Command, args []string) error { // cleanup pass in mergeKubeconfig treats them as still-present on the server side. // The stubs only affect deletion logic; they are never written to disk. // When --include-not-ready is set, notReady entries are fully merged instead. + // stubsToPreserve is only populated when stubs are injected (i.e. !includeNotReady). + // It maps server-side authinfo names to the local managed name so mergeKubeconfig + // can pre-seed authInfoMap and avoid redirecting stub contexts to a wrong authinfo. + var stubsToPreserve map[string]string if !includeNotReady { - injectNotReadyStubs(serverConfig, notReady, localConfig) + stubsToPreserve = injectNotReadyStubs(serverConfig, skipped, localConfig) } // Take a snapshot before merge for dry-run diff — only needed when --dry-run is set. @@ -266,7 +279,7 @@ func runSync(cmd *cobra.Command, args []string) error { spinnerLabel = "Simulating merge (dry-run)..." } stopMerge := printer.StartSpinner(spinnerLabel) - err = mergeKubeconfig(localConfig, serverConfig) + err = mergeKubeconfig(localConfig, serverConfig, stubsToPreserve) stopMerge() if err != nil { _ = printer.Print(buildFailedSyncResult(merged, skipped, err)) @@ -305,6 +318,23 @@ func partitionReady(items []v1alpha1.ClusterKubeconfig) (ready, notReady []v1alp return ready, notReady } +// hasSpecData reports whether the ClusterKubeconfig has at least one cluster, context, or +// authinfo entry in its spec. Resources with an entirely empty spec contribute no entries to +// the merged kubeconfig and should remain on the preservation path even when --include-not-ready +// is set, to avoid their existing local entries being deleted by the stale-cleanup pass. +func hasSpecData(ckc v1alpha1.ClusterKubeconfig) bool { + return len(ckc.Spec.Kubeconfig.Clusters) > 0 || + len(ckc.Spec.Kubeconfig.Contexts) > 0 || + len(ckc.Spec.Kubeconfig.AuthInfo) > 0 +} + +// shouldSkipMerge returns true when there is nothing useful to merge: +// no ready clusters and either the --include-not-ready flag is not set or there are no +// not-ready clusters to merge either. +func shouldSkipMerge(ready, notReady []v1alpha1.ClusterKubeconfig, inclNotReady bool) bool { + return len(ready) == 0 && (!inclNotReady || len(notReady) == 0) +} + // filterReady returns only ClusterKubeconfigs that have Ready condition set to True. // Deprecated: use partitionReady instead. func filterReady(items []v1alpha1.ClusterKubeconfig) []v1alpha1.ClusterKubeconfig { @@ -317,7 +347,10 @@ func filterReady(items []v1alpha1.ClusterKubeconfig) []v1alpha1.ClusterKubeconfi // stale-cleanup pass from deleting local entries for clusters that are temporarily not-ready — // the user needs those entries to reach and recover the cluster. // Stubs are derived from the existing local entries (no spec data required) and are never written to disk. -func injectNotReadyStubs(serverConfig *clientcmdapi.Config, notReady []v1alpha1.ClusterKubeconfig, localConfig *clientcmdapi.Config) { +// The returned map (server-side unprefixed authinfo name → local managed name) is passed to +// mergeKubeconfig as a pre-seed so stub context authinfo references are not redirected by deduplication. +func injectNotReadyStubs(serverConfig *clientcmdapi.Config, notReady []v1alpha1.ClusterKubeconfig, localConfig *clientcmdapi.Config) map[string]string { + authPreSeed := make(map[string]string) for _, ckc := range notReady { // Collect the set of context names this CKC owns: prefer spec context names when // available (spec may be populated even if Ready=False), fall back to the resource @@ -335,12 +368,12 @@ func injectNotReadyStubs(serverConfig *clientcmdapi.Config, notReady []v1alpha1. if !ok || localCtx == nil { continue } - // Fix 6: skip unmanaged contexts — only protect entries cloudctl wrote. + // skip unmanaged contexts — only protect entries cloudctl wrote. if !isManaged(localCtx.Cluster) { continue } - // Fix 2: stub context must carry unprefixed cluster/authinfo references so + // stub context must carry unprefixed cluster/authinfo references so // mergeKubeconfig's lookup (which strips the prefix before checking serverConfig) // resolves correctly and does not produce double-prefixed names. stubCtx := &clientcmdapi.Context{ @@ -360,15 +393,21 @@ func injectNotReadyStubs(serverConfig *clientcmdapi.Config, notReady []v1alpha1. } } - // Inject stub authinfo keyed by the unprefixed name. + // Record the authinfo pre-seed mapping instead of adding the stub to serverConfig. + // Putting the stub authinfo into serverConfig would cause mergeKubeconfig's + // deduplication pass to assign a new hash-derived managed name, redirecting the + // preserved context to a different authinfo entry and altering its effective identity. + // The pre-seed tells mergeKubeconfig to map the stub authinfo name directly to the + // existing local managed name, keeping the context unchanged. authName := unmanagedNameFunc(localCtx.AuthInfo) - if _, exists := serverConfig.AuthInfos[authName]; !exists { - if src, ok := localConfig.AuthInfos[localCtx.AuthInfo]; ok { - serverConfig.AuthInfos[authName] = src.DeepCopy() + if authName != "" && localCtx.AuthInfo != "" { + if _, exists := serverConfig.AuthInfos[authName]; !exists { + authPreSeed[authName] = localCtx.AuthInfo } } } } + return authPreSeed } // buildSyncResult constructs an output.SyncResult from ready and notReady cluster lists. @@ -574,7 +613,7 @@ func buildKubeloginArgs(cfg map[string]string, extra []string, tokenCacheDir str return args } -func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdapi.Config) error { +func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdapi.Config, preSeedAuthInfoMap map[string]string) error { // Merge Clusters for serverName, serverCluster := range serverConfig.Clusters { managedName := managedNameFunc(serverName) @@ -604,6 +643,13 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap if mergeIdenticalUsers { authInfoMap = make(map[string]string) + // Pre-seed stub authinfo mappings from notReady preservation. These entries were + // not added to serverConfig (to avoid dedup side effects) but their context + // references must still resolve to the correct local managed name. + for serverName, localManagedName := range preSeedAuthInfoMap { + authInfoMap[serverName] = localManagedName + } + // Build a reverse lookup of unmanaged local auth entries so we can reuse their names // instead of creating new cloudctl:auth- entries. // Store all candidate names per key; at reuse time we pick the lexicographically @@ -782,9 +828,14 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap delete(localConfig.AuthInfos, localName) } } else { - // Derive the server-side name by stripping the prefix + // Derive the server-side name by stripping the prefix. + // Also protect authinfos pre-seeded from notReady stubs — those were not + // added to serverConfig, so the server-side name lookup would otherwise + // cause them to be deleted. serverName := unmanagedNameFunc(localName) - if _, exists := serverConfig.AuthInfos[serverName]; !exists { + _, inServer := serverConfig.AuthInfos[serverName] + _, inPreSeed := preSeedAuthInfoMap[serverName] + if !inServer && !inPreSeed { slog.Debug("removing stale authinfo", "name", localName) delete(localConfig.AuthInfos, localName) } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index e80ab78..07f7446 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -608,7 +608,7 @@ func TestMergeKubeconfig_PrefersExistingLocalAuthName(t *testing.T) { AuthInfo: "server-user", } - err := mergeKubeconfig(localConfig, serverConfig) + err := mergeKubeconfig(localConfig, serverConfig, nil) g.Expect(err).ToNot(HaveOccurred()) // The unmanaged "my-existing-user" should be reused — no cloudctl:auth-* should be created @@ -653,7 +653,7 @@ func TestMergeKubeconfig_DeduplicatesSameOIDCUsers(t *testing.T) { serverConfig.Contexts["cluster-b"] = &clientcmdapi.Context{Cluster: "cluster-b", AuthInfo: "cluster-b-user"} localConfig := clientcmdapi.NewConfig() - err := mergeKubeconfig(localConfig, serverConfig) + err := mergeKubeconfig(localConfig, serverConfig, nil) g.Expect(err).ToNot(HaveOccurred()) // Count managed auth entries — should be exactly 1 @@ -880,7 +880,7 @@ func TestDryRun_MergeAndDiff_NoWrite(t *testing.T) { serverConfig.Contexts["new-cluster"] = &clientcmdapi.Context{Cluster: "new-cluster", AuthInfo: "shared-user"} localConfig := localConfigBefore.DeepCopy() - err := mergeKubeconfig(localConfig, serverConfig) + err := mergeKubeconfig(localConfig, serverConfig, nil) g.Expect(err).ToNot(HaveOccurred()) diff := diffKubeconfig(localConfigBefore, localConfig) @@ -1078,12 +1078,15 @@ func TestInjectNotReadyStubs_PreservesLocalEntries(t *testing.T) { serverConfig := clientcmdapi.NewConfig() - injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + preSeed := injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) - // Stubs must be present so mergeKubeconfig's stale-cleanup does not delete them. + // Context and cluster stubs must be present so mergeKubeconfig's stale-cleanup does not delete them. g.Expect(serverConfig.Contexts).To(HaveKey("broken-cluster")) g.Expect(serverConfig.Clusters).To(HaveKey("broken-cluster")) - g.Expect(serverConfig.AuthInfos).To(HaveKey("oidc@broken-cluster")) + // AuthInfo is no longer added to serverConfig (to avoid dedup side-effects); + // instead it is returned via the pre-seed map for authInfoMap population. + g.Expect(serverConfig.AuthInfos).NotTo(HaveKey("oidc@broken-cluster")) + g.Expect(preSeed).To(HaveKeyWithValue("oidc@broken-cluster", "cloudctl:oidc@broken-cluster")) // Fix 2: stub context references must be unprefixed to avoid double-prefix on re-merge. g.Expect(serverConfig.Contexts["broken-cluster"].Cluster).To(Equal("broken-cluster")) @@ -1117,7 +1120,7 @@ func TestInjectNotReadyStubs_UsesSpecContextNames(t *testing.T) { serverConfig := clientcmdapi.NewConfig() - injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + _ = injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) // Stub must be keyed by spec context name "ctx1", not resource name "demo". g.Expect(serverConfig.Contexts).To(HaveKey("ctx1")) @@ -1144,7 +1147,7 @@ func TestInjectNotReadyStubs_SkipsUnmanagedContexts(t *testing.T) { serverConfig := clientcmdapi.NewConfig() - injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + _ = injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) // Unmanaged context must not be touched. g.Expect(serverConfig.Contexts).To(BeEmpty()) @@ -1166,12 +1169,13 @@ func TestInjectNotReadyStubs_SkipsUnknownClusters(t *testing.T) { serverConfig := clientcmdapi.NewConfig() localConfig := clientcmdapi.NewConfig() - injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) + preSeed := injectNotReadyStubs(serverConfig, []greenhousev1alpha1.ClusterKubeconfig{notReadyCkc}, localConfig) // Nothing added — no local entry to preserve. g.Expect(serverConfig.Contexts).To(BeEmpty()) g.Expect(serverConfig.Clusters).To(BeEmpty()) g.Expect(serverConfig.AuthInfos).To(BeEmpty()) + g.Expect(preSeed).To(BeEmpty()) } func TestIncludeNotReady_MergesSpecData(t *testing.T) { @@ -1233,12 +1237,8 @@ func TestIncludeNotReady_BypassesEarlyReturnWhenAllNotReady(t *testing.T) { g.Expect(ready).To(BeEmpty()) g.Expect(notReady).To(HaveLen(1)) - // Simulate the early-return condition in runSync: - // without --include-not-ready, len(ready)==0 returns early. - // with --include-not-ready and len(notReady)>0, execution continues. - earlyReturn := func(inclNotReady bool) bool { - return len(ready) == 0 && (!inclNotReady || len(notReady) == 0) - } - g.Expect(earlyReturn(false)).To(BeTrue(), "without flag: must return early") - g.Expect(earlyReturn(true)).To(BeFalse(), "with flag: must continue past early return") + // Exercise the production shouldSkipMerge helper directly so that if runSync's + // early-return logic reverts (e.g. drops the inclNotReady guard), this test fails. + g.Expect(shouldSkipMerge(ready, notReady, false)).To(BeTrue(), "without flag: must skip merge when all clusters not-ready") + g.Expect(shouldSkipMerge(ready, notReady, true)).To(BeFalse(), "with flag: must not skip merge when all clusters not-ready") } From af2c26d9cd0f097e3072e0ab3c3f676f69489e9d Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Tue, 10 Nov 2026 14:07:11 +0100 Subject: [PATCH 5/5] fix(sync): honour pre-seed authinfo map in no-merge context resolution When --merge-identical-users=false, context merging and stale-context validation were ignoring preSeedAuthInfoMap and falling back to managedNameFunc, which would rewrite a stub context's authinfo reference to cloudctl: even though that entry does not exist in localConfig, breaking cluster access. Both the context-merge path (lines 775-776) and the stale-context validation path (lines 872-873) now check preSeedAuthInfoMap first and use the pre-seeded local managed name when present. Signed-off-by: onuryilmaz --- cmd/sync.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index ad08755..3e4971f 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -773,7 +773,14 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap localConfig.AuthInfos[managedAuthInfoName] = serverAuth } } else { - managedAuthInfoName = managedNameFunc(serverCtx.AuthInfo) + // Without deduplication, derive the managed name from the server-side name. + // For stub contexts injected by injectNotReadyStubs the authinfo was not added + // to serverConfig; its correct local managed name is in preSeedAuthInfoMap. + if preSeedName, ok := preSeedAuthInfoMap[serverCtx.AuthInfo]; ok { + managedAuthInfoName = preSeedName + } else { + managedAuthInfoName = managedNameFunc(serverCtx.AuthInfo) + } } // Update Cluster name @@ -870,7 +877,12 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap } expectedAuthInfo = mappedName } else { - expectedAuthInfo = managedNameFunc(serverCtx.AuthInfo) + // Same pre-seed lookup as in context merging above. + if preSeedName, ok := preSeedAuthInfoMap[serverCtx.AuthInfo]; ok { + expectedAuthInfo = preSeedName + } else { + expectedAuthInfo = managedNameFunc(serverCtx.AuthInfo) + } } if localCtx.Cluster != expectedCluster || localCtx.AuthInfo != expectedAuthInfo {