Repository navigation
feat(sync): preserve not-ready clusters in local kubeconfig - #87
Open
onuryilmaz wants to merge 1 commit into
Open
onuryilmaz wants to merge 1 commit into
onuryilmaz wants to merge 1 commit into
Conversation
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 <onur.yilmaz@sap.com>
There was a problem hiding this comment.
🟡 Changes recommended
The sync logic can abort on prefixed references, create dangling references, and delete managed entries when resource and context names differ.
6 open issues
What changed in this PR
This PR aims to preserve local Kubernetes access during transient Greenhouse readiness failures by keeping existing kubeconfig entries for not-ready clusters.
Changes:
- Adds local-entry stubs intended to prevent stale cleanup from deleting not-ready clusters.
- Introduces
--include-not-readyto opt into merging their spec data. - Adds tests for stub creation, unknown-cluster handling, and not-ready spec conversion.
| File | Description |
|---|---|
cmd/sync.go |
Adds preservation logic, the opt-in flag, and updated command help. |
cmd/sync_test.go |
Adds tests for preservation stubs and not-ready spec merging. |
Review effort: Balanced
| continue | ||
| } | ||
| // Context name matches the CKC name (contexts are not prefixed). | ||
| if ctxName != ckc.Name { |
| } | ||
| // Inject stub context. | ||
| if _, exists := serverConfig.Contexts[ctxName]; !exists { | ||
| serverConfig.Contexts[ctxName] = localCtx.DeepCopy() |
| 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)") |
|
|
||
| serverConfig, err := buildIncomingKubeconfig(ready) | ||
| toMerge := ready | ||
| if includeNotReady { |
| serverConfig, err := buildIncomingKubeconfig(ready) | ||
| toMerge := ready | ||
| if includeNotReady { | ||
| toMerge = append(toMerge, 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 { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ready=Falsecondition (e.g. during a Greenhouse controller upgrade or a missing secret key) previously caused cloudctl to remove the local entry, making the cluster unreachable.--include-not-readyflag fully merges not-ready cluster spec data, for cases where the CRD spec is known-good but the controller is temporarily degraded.Motivation
During the Greenhouse
v0.18.0 → v0.19.0rollout (PR cloudoperators/greenhouse#2245), all 208ClusterKubeconfigresources in thescinamespace reportReady=Falsewith reasonKubeconfigMissing. Theirspec.kubeconfigis fully populated. Runningcloudctl syncin this window would delete all 208 entries from users' local kubeconfigs, cutting off cluster access at the worst possible time.Behaviour change
Ready=False, exists in local kubeconfigReady=False, not yet in local kubeconfig--include-not-readyflagReady=FalseTest plan
TestInjectNotReadyStubs_PreservesLocalEntries— stub injection keeps existing local entriesTestInjectNotReadyStubs_SkipsUnknownClusters— no stubs for clusters not yet in local configTestIncludeNotReady_MergesSpecData—--include-not-readymerges spec data from not-ready CRDscmdtests pass