From ebd03b421a2f7a64853999e4b510618bb38094bb Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 11:16:47 +0200 Subject: [PATCH 01/16] feat(sync): add --dry-run flag and fix authinfo deduplication (#51, #54) Implements two related improvements to the sync command: **dry-run (#51)** - Add `--dry-run` flag that runs the full merge logic but prints a structured diff instead of writing the kubeconfig file. - Diff output shows added (+), removed (-), and modified (~) entries for clusters, contexts, and auth infos (managed entries only). - CA changes are shown as SHA-256 fingerprints (first 16 hex chars). - Exec arg changes are shown as per-arg +/- lines. - New `SyncDryRunResult` output type with plain, interactive (colour), JSON, and YAML rendering. - Spinner label changes to "Simulating merge (dry-run)..." when active. **authinfo deduplication fixes (#54)** - Extract authinfo comparison/dedup logic into `cmd/authinfo.go`. - Fix `authInfoEqual` to compare `InteractiveMode` and `Env` on exec configs (previously ignored, could produce false-positive matches). - Fix `generateAuthInfoKey` for auth-provider path to include `idp-issuer-url`; clusters with different issuers but the same client-id no longer incorrectly share a single auth entry. - Fix `generateAuthInfoKey` for exec path to include sorted env vars. - Prefer existing unmanaged local auth names: when `--merge-identical- users` is enabled and a local (unmanaged) auth entry already has matching credentials, reuse its name instead of creating a new `cloudctl:auth-` entry. Contexts are updated to reference the local name. Signed-off-by: onuryilmaz --- cmd/authinfo.go | 180 +++++++++++++ cmd/kubeconfigdiff.go | 322 +++++++++++++++++++++++ cmd/output/interactive_printer.go | 62 +++++ cmd/output/output_test.go | 104 ++++++++ cmd/output/plain_printer.go | 44 ++++ cmd/output/types.go | 24 ++ cmd/sync.go | 204 ++++----------- cmd/sync_test.go | 411 ++++++++++++++++++++++++++++++ 8 files changed, 1193 insertions(+), 158 deletions(-) create mode 100644 cmd/authinfo.go create mode 100644 cmd/kubeconfigdiff.go diff --git a/cmd/authinfo.go b/cmd/authinfo.go new file mode 100644 index 0000000..74222b3 --- /dev/null +++ b/cmd/authinfo.go @@ -0,0 +1,180 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors +// SPDX-License-Identifier: Apache-2.0 + +package cmd + +import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "fmt" + "maps" + "slices" + "sort" + "strings" + + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" +) + +// authInfoEqual compares two AuthInfo objects, excluding "id-token" and "refresh-token". +func authInfoEqual(a, b *clientcmdapi.AuthInfo) bool { + // Compare ClientCertificateData + if !bytes.Equal(a.ClientCertificateData, b.ClientCertificateData) { + return false + } + + // Compare ClientKeyData + if !bytes.Equal(a.ClientKeyData, b.ClientKeyData) { + return false + } + + // Compare Exec first (new style) + if (a.Exec == nil) != (b.Exec == nil) { + return false + } + if a.Exec != nil && b.Exec != nil { + if a.Exec.Command != b.Exec.Command || a.Exec.APIVersion != b.Exec.APIVersion { + return false + } + if !slices.Equal(a.Exec.Args, b.Exec.Args) { + return false + } + if a.Exec.InteractiveMode != b.Exec.InteractiveMode { + return false + } + if !equalExecEnv(a.Exec.Env, b.Exec.Env) { + return false + } + return true + } + + // Compare AuthProvider, excluding "id-token" and "refresh-token" + if (a.AuthProvider == nil) != (b.AuthProvider == nil) { + return false + } + if a.AuthProvider != nil && b.AuthProvider != nil { + // Compare AuthProvider Name + if a.AuthProvider.Name != b.AuthProvider.Name { + return false + } + + // Compare AuthProvider Config excluding "id-token" and "refresh-token" + aConfigFiltered := filterAuthProviderConfig(a.AuthProvider.Config) + bConfigFiltered := filterAuthProviderConfig(b.AuthProvider.Config) + if !maps.Equal(aConfigFiltered, bConfigFiltered) { + return false + } + } + + return true +} + +// equalExecEnv compares two ExecEnvVar slices for equality. +func equalExecEnv(a, b []clientcmdapi.ExecEnvVar) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i].Name != b[i].Name || a[i].Value != b[i].Value { + return false + } + } + return true +} + +// filterAuthProviderConfig returns a copy of the config map excluding "id-token" and "refresh-token". +func filterAuthProviderConfig(config map[string]string) map[string]string { + filtered := make(map[string]string) + for k, v := range config { + if k != "id-token" && k != "refresh-token" { + filtered[k] = v + } + } + return filtered +} + +// generateAuthInfoKey creates a unique key for an AuthInfo based on specific AuthProvider fields, +// excluding "id-token" and "refresh-token". It uses "idp-issuer-url", "client-id", "client-secret", +// "auth-request-extra-params", and "extra-scopes" to generate the key. +func generateAuthInfoKey(authInfo *clientcmdapi.AuthInfo) string { + // Exec-based key: derive from stable subset of args to avoid including tokens + if authInfo.Exec != nil { + // Extract known kubelogin flags + var issuer, clientID, clientSecret, extraParams string + var scopes []string + var envParts []string + for _, arg := range authInfo.Exec.Args { + switch { + case strings.HasPrefix(arg, "--oidc-issuer-url="): + issuer = strings.TrimPrefix(arg, "--oidc-issuer-url=") + case strings.HasPrefix(arg, "--oidc-client-id="): + clientID = strings.TrimPrefix(arg, "--oidc-client-id=") + case strings.HasPrefix(arg, "--oidc-client-secret="): + clientSecret = strings.TrimPrefix(arg, "--oidc-client-secret=") + case strings.HasPrefix(arg, "--oidc-extra-scope="): + scopes = append(scopes, strings.TrimPrefix(arg, "--oidc-extra-scope=")) + case strings.HasPrefix(arg, "--oidc-auth-request-extra-params="): + extraParams = strings.TrimPrefix(arg, "--oidc-auth-request-extra-params=") + } + } + sort.Strings(scopes) + // Include sorted Env in the key so changes to env vars result in a different key + for _, e := range authInfo.Exec.Env { + envParts = append(envParts, e.Name+"="+e.Value) + } + sort.Strings(envParts) + data := fmt.Sprintf("exec:issuer:%s;client-id:%s;client-secret:%s;extra-params:%s;scopes:%s;env:%s", + issuer, clientID, clientSecret, extraParams, strings.Join(scopes, ","), strings.Join(envParts, ",")) + return data + } + + if authInfo.AuthProvider == nil { + // For AuthInfos without AuthProvider, use a different unique identifier + // Here, we'll use the hash of ClientCertificateData and ClientKeyData + h := sha256.New() + h.Write(authInfo.ClientCertificateData) + h.Write(authInfo.ClientKeyData) + return fmt.Sprintf("cert:%s", hex.EncodeToString(h.Sum(nil))) + } + + // Extract the required fields from AuthProvider Config, including idp-issuer-url + // to avoid incorrectly deduplicating clusters with different issuers but same client-id. + issuerURL := authInfo.AuthProvider.Config["idp-issuer-url"] + clientID := authInfo.AuthProvider.Config["client-id"] + clientSecret := authInfo.AuthProvider.Config["client-secret"] + authRequestExtraParams := authInfo.AuthProvider.Config["auth-request-extra-params"] + extraScopes := authInfo.AuthProvider.Config["extra-scopes"] + + // Concatenate the fields in a consistent order + data := fmt.Sprintf("idp-issuer-url:%s;client-id:%s;client-secret:%s;auth-request-extra-params:%s;extra-scopes:%s", + issuerURL, clientID, clientSecret, authRequestExtraParams, extraScopes) + + return data +} + +// mergeAuthInfo merges two AuthInfo objects, preserving id-token and refresh-token from localAuth. +func mergeAuthInfo(serverAuth, localAuth *clientcmdapi.AuthInfo) *clientcmdapi.AuthInfo { + if localAuth == nil { + // If there's no local AuthInfo, return the server AuthInfo as is + return serverAuth + } + + // Create a copy of the serverAuth to avoid mutating the original + mergedAuth := serverAuth.DeepCopy() + + // Preserve id-token and refresh-token from localAuth + if localAuth.AuthProvider != nil && mergedAuth.AuthProvider != nil { + // Ensure the merged config map is initialized to avoid panics on assignment + if mergedAuth.AuthProvider.Config == nil { + mergedAuth.AuthProvider.Config = make(map[string]string) + } + if idToken, exists := localAuth.AuthProvider.Config["id-token"]; exists { + mergedAuth.AuthProvider.Config["id-token"] = idToken + } + if refreshToken, exists := localAuth.AuthProvider.Config["refresh-token"]; exists { + mergedAuth.AuthProvider.Config["refresh-token"] = refreshToken + } + } + + return mergedAuth +} diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go new file mode 100644 index 0000000..68bb7db --- /dev/null +++ b/cmd/kubeconfigdiff.go @@ -0,0 +1,322 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors +// SPDX-License-Identifier: Apache-2.0 + +package cmd + +import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "fmt" + "sort" + + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + + "github.com/cloudoperators/cloudctl/cmd/output" +) + +// DiffChangeType describes the kind of change detected for a kubeconfig entry. +type DiffChangeType string + +const ( + DiffChangeAdded DiffChangeType = "added" + DiffChangeRemoved DiffChangeType = "removed" + DiffChangeModified DiffChangeType = "modified" +) + +// KubeconfigDiff holds the set of entry-level differences between two kubeconfigs. +// Only managed entries (those carrying the configured prefix) are included. +type KubeconfigDiff struct { + Clusters []EntryDiff + Contexts []EntryDiff + AuthInfos []EntryDiff +} + +// EntryDiff describes the diff for a single named kubeconfig entry. +type EntryDiff struct { + Name string + ChangeType DiffChangeType + Fields []FieldDiff // non-empty only for DiffChangeModified +} + +// FieldDiff is an internal field-level change, mapped to output.FieldChange for printing. +type FieldDiff struct { + Field string + Old string + New string +} + +// diffKubeconfig computes the diff between the old and new kubeconfig, +// restricting to entries whose names are managed (have the prefix). +func diffKubeconfig(oldCfg, newCfg *clientcmdapi.Config) KubeconfigDiff { + var d KubeconfigDiff + d.Clusters = diffClusters(oldCfg, newCfg) + d.Contexts = diffContexts(oldCfg, newCfg) + d.AuthInfos = diffAuthInfos(oldCfg, newCfg) + return d +} + +// diffClusters returns added/removed/modified cluster entries for managed names. +func diffClusters(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { + var diffs []EntryDiff + + // Added or modified in new + for name, newCluster := range newCfg.Clusters { + if !isManaged(name) { + continue + } + oldCluster, exists := oldCfg.Clusters[name] + if !exists { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeAdded}) + continue + } + var fields []FieldDiff + if oldCluster.Server != newCluster.Server { + fields = append(fields, FieldDiff{Field: "Server", Old: oldCluster.Server, New: newCluster.Server}) + } + if !bytes.Equal(oldCluster.CertificateAuthorityData, newCluster.CertificateAuthorityData) { + oldFP := caFingerprint(oldCluster.CertificateAuthorityData) + newFP := caFingerprint(newCluster.CertificateAuthorityData) + fields = append(fields, FieldDiff{Field: "CA", Old: oldFP, New: newFP}) + } + if !labelsExtensionEqual(oldCluster.Extensions, newCluster.Extensions) { + oldLbl := string(extensionRaw(oldCluster.Extensions, "labels")) + newLbl := string(extensionRaw(newCluster.Extensions, "labels")) + fields = append(fields, FieldDiff{Field: "Labels", Old: oldLbl, New: newLbl}) + } + if len(fields) > 0 { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeModified, Fields: fields}) + } + } + + // Removed from old + for name := range oldCfg.Clusters { + if !isManaged(name) { + continue + } + if _, exists := newCfg.Clusters[name]; !exists { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeRemoved}) + } + } + + sort.Slice(diffs, func(i, j int) bool { return diffs[i].Name < diffs[j].Name }) + return diffs +} + +// diffContexts returns added/removed/modified context entries for managed names. +func diffContexts(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { + var diffs []EntryDiff + + for name, newCtx := range newCfg.Contexts { + if !isManaged(name) { + continue + } + oldCtx, exists := oldCfg.Contexts[name] + if !exists { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeAdded}) + continue + } + var fields []FieldDiff + if oldCtx.Cluster != newCtx.Cluster { + fields = append(fields, FieldDiff{Field: "Cluster", Old: oldCtx.Cluster, New: newCtx.Cluster}) + } + if oldCtx.AuthInfo != newCtx.AuthInfo { + fields = append(fields, FieldDiff{Field: "AuthInfo", Old: oldCtx.AuthInfo, New: newCtx.AuthInfo}) + } + if oldCtx.Namespace != newCtx.Namespace { + fields = append(fields, FieldDiff{Field: "Namespace", Old: oldCtx.Namespace, New: newCtx.Namespace}) + } + if len(fields) > 0 { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeModified, Fields: fields}) + } + } + + for name := range oldCfg.Contexts { + if !isManaged(name) { + continue + } + if _, exists := newCfg.Contexts[name]; !exists { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeRemoved}) + } + } + + sort.Slice(diffs, func(i, j int) bool { return diffs[i].Name < diffs[j].Name }) + return diffs +} + +// diffAuthInfos returns added/removed/modified authinfo entries for managed names. +func diffAuthInfos(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { + var diffs []EntryDiff + + for name, newAuth := range newCfg.AuthInfos { + if !isManaged(name) { + continue + } + oldAuth, exists := oldCfg.AuthInfos[name] + if !exists { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeAdded}) + continue + } + if authInfoEqual(oldAuth, newAuth) { + continue + } + var fields []FieldDiff + // Exec-based diff + if newAuth.Exec != nil && oldAuth.Exec != nil { + fields = append(fields, argsDiff(oldAuth.Exec.Args, newAuth.Exec.Args)...) + } else if newAuth.Exec != nil && oldAuth.Exec == nil { + fields = append(fields, FieldDiff{Field: "Auth type", Old: "auth-provider", New: "exec-plugin"}) + } else if newAuth.Exec == nil && oldAuth.Exec != nil { + fields = append(fields, FieldDiff{Field: "Auth type", Old: "exec-plugin", New: "auth-provider"}) + } + // AuthProvider diff + if newAuth.AuthProvider != nil && oldAuth.AuthProvider != nil { + oldFiltered := filterAuthProviderConfig(oldAuth.AuthProvider.Config) + newFiltered := filterAuthProviderConfig(newAuth.AuthProvider.Config) + allKeys := make(map[string]struct{}) + for k := range oldFiltered { + allKeys[k] = struct{}{} + } + for k := range newFiltered { + allKeys[k] = struct{}{} + } + sortedKeys := make([]string, 0, len(allKeys)) + for k := range allKeys { + sortedKeys = append(sortedKeys, k) + } + sort.Strings(sortedKeys) + for _, k := range sortedKeys { + ov := oldFiltered[k] + nv := newFiltered[k] + if ov != nv { + fields = append(fields, FieldDiff{Field: fmt.Sprintf("auth-provider.%s", k), Old: ov, New: nv}) + } + } + } + if len(fields) > 0 { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeModified, Fields: fields}) + } else { + // authInfoEqual returned false but no specific fields identified — generic modified + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeModified}) + } + } + + for name := range oldCfg.AuthInfos { + if !isManaged(name) { + continue + } + if _, exists := newCfg.AuthInfos[name]; !exists { + diffs = append(diffs, EntryDiff{Name: name, ChangeType: DiffChangeRemoved}) + } + } + + sort.Slice(diffs, func(i, j int) bool { return diffs[i].Name < diffs[j].Name }) + return diffs +} + +// argsDiff computes per-argument differences between two exec arg lists, returning +// FieldDiff entries for args that appear only in old (removed) or only in new (added). +func argsDiff(oldArgs, newArgs []string) []FieldDiff { + oldSet := make(map[string]bool, len(oldArgs)) + for _, a := range oldArgs { + oldSet[a] = true + } + newSet := make(map[string]bool, len(newArgs)) + for _, a := range newArgs { + newSet[a] = true + } + + var removed, added []string + for _, a := range oldArgs { + if !newSet[a] { + removed = append(removed, a) + } + } + for _, a := range newArgs { + if !oldSet[a] { + added = append(added, a) + } + } + + var diffs []FieldDiff + for _, r := range removed { + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: r, New: ""}) + } + for _, a := range added { + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: "", New: a}) + } + return diffs +} + +// caFingerprint returns the first 16 hex characters of the SHA-256 hash of ca data. +func caFingerprint(data []byte) string { + if len(data) == 0 { + return "" + } + h := sha256.Sum256(data) + return hex.EncodeToString(h[:])[:16] +} + +// toOutputDiffEntries converts internal EntryDiff slice to output.DiffEntry slice. +func toOutputDiffEntries(diffs []EntryDiff) []output.DiffEntry { + result := make([]output.DiffEntry, 0, len(diffs)) + for _, d := range diffs { + entry := output.DiffEntry{ + Name: d.Name, + ChangeType: string(d.ChangeType), + } + for _, f := range d.Fields { + entry.Fields = append(entry.Fields, output.FieldChange{ + Field: f.Field, + Old: f.Old, + New: f.New, + }) + } + result = append(result, entry) + } + return result +} + +// buildDryRunResult converts a KubeconfigDiff to an output.SyncDryRunResult. +func buildDryRunResult(diff KubeconfigDiff) output.SyncDryRunResult { + var added, removed, modified int + countChanges := func(entries []EntryDiff) { + for _, e := range entries { + switch e.ChangeType { + case DiffChangeAdded: + added++ + case DiffChangeRemoved: + removed++ + case DiffChangeModified: + modified++ + } + } + } + countChanges(diff.Clusters) + countChanges(diff.Contexts) + countChanges(diff.AuthInfos) + + clusters := toOutputDiffEntries(diff.Clusters) + contexts := toOutputDiffEntries(diff.Contexts) + authInfos := toOutputDiffEntries(diff.AuthInfos) + + // Use empty slices instead of nil for consistent JSON/YAML output + if clusters == nil { + clusters = []output.DiffEntry{} + } + if contexts == nil { + contexts = []output.DiffEntry{} + } + if authInfos == nil { + authInfos = []output.DiffEntry{} + } + + return output.SyncDryRunResult{ + Clusters: clusters, + Contexts: contexts, + AuthInfos: authInfos, + Added: added, + Removed: removed, + Modified: modified, + } +} diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index 6978e97..c182a88 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -94,6 +94,8 @@ func (p *interactivePrinter) Print(v any) error { switch t := v.(type) { case SyncResult: writeErr = p.printSyncResult(t) + case SyncDryRunResult: + writeErr = p.printSyncDryRunResult(t) case ClusterVersionResult: w("%s %s\n", styleFaint.Render("Kubernetes version:"), styleBold.Render(t.Version)) case VersionInfo: @@ -199,3 +201,63 @@ func (p *interactivePrinter) printSyncResult(r SyncResult) error { w("%s\n", summary) return writeErr } + +func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { + var writeErr error + w := func(format string, a ...any) { + if writeErr != nil { + return + } + _, writeErr = fmt.Fprintf(p.w, format, a...) + } + + w("%s\n\n", styleFaint.Render("Dry-run: no changes will be written.")) + + total := r.Added + r.Removed + r.Modified + if total == 0 { + w("%s\n", styleFaint.Render("No changes detected.")) + return writeErr + } + + printSection := func(title string, entries []DiffEntry) { + if len(entries) == 0 { + return + } + n := len(entries) + w("%s (%d change(s))\n", styleHeader.Render(title), n) + for _, e := range entries { + switch e.ChangeType { + case "added": + w(" %s %s\n", styleGreen.Render("+"), e.Name) + case "removed": + w(" %s %s\n", styleRed.Render("-"), e.Name) + case "modified": + w(" %s %s\n", styleYellow.Render("~"), e.Name) + for _, f := range e.Fields { + if f.Old != "" { + w(" %-12s %s\n", f.Field+":", styleRed.Render("- "+f.Old)) + } + if f.New != "" { + w(" %-12s %s\n", f.Field+":", styleGreen.Render("+ "+f.New)) + } + } + } + } + w("\n") + } + + printSection("CLUSTERS", r.Clusters) + printSection("CONTEXTS", r.Contexts) + printSection("AUTH INFOS", r.AuthInfos) + + summaryParts := []string{ + styleGreen.Render(fmt.Sprintf("%d added", r.Added)), + styleRed.Render(fmt.Sprintf("%d removed", r.Removed)), + styleYellow.Render(fmt.Sprintf("%d modified", r.Modified)), + } + w("Summary: %s, %s, %s. %s\n", + summaryParts[0], summaryParts[1], summaryParts[2], + styleFaint.Render("No changes will be written.")) + + return writeErr +} diff --git a/cmd/output/output_test.go b/cmd/output/output_test.go index 3a21751..33b5026 100644 --- a/cmd/output/output_test.go +++ b/cmd/output/output_test.go @@ -209,3 +209,107 @@ func TestStartSpinner_NoOp_JSON(t *testing.T) { stop() g.Expect(buf.String()).To(BeEmpty()) } + +// --------------------------------------------------------------------------- +// SyncDryRunResult printers +// --------------------------------------------------------------------------- + +func TestPlainPrinter_SyncDryRunResult_Changes(t *testing.T) { + g := NewWithT(t) + var buf bytes.Buffer + p := output.New(output.FormatText, false, &buf) + + result := output.SyncDryRunResult{ + Clusters: []output.DiffEntry{ + {Name: "cloudctl:prod-eu-1", ChangeType: "added"}, + {Name: "cloudctl:staging-de", ChangeType: "removed"}, + {Name: "cloudctl:prod-eu-2", ChangeType: "modified", Fields: []output.FieldChange{ + {Field: "Server", Old: "https://old.example.com", New: "https://new.example.com"}, + }}, + }, + Contexts: []output.DiffEntry{}, + AuthInfos: []output.DiffEntry{}, + Added: 1, + Removed: 1, + Modified: 1, + } + g.Expect(p.Print(result)).To(Succeed()) + + out := buf.String() + g.Expect(out).To(ContainSubstring("Dry-run: no changes will be written.")) + g.Expect(out).To(ContainSubstring("+ cloudctl:prod-eu-1")) + g.Expect(out).To(ContainSubstring("- cloudctl:staging-de")) + g.Expect(out).To(ContainSubstring("~ cloudctl:prod-eu-2")) + g.Expect(out).To(ContainSubstring("https://old.example.com")) + g.Expect(out).To(ContainSubstring("https://new.example.com")) + g.Expect(out).To(ContainSubstring("Summary:")) + g.Expect(out).To(ContainSubstring("1 added")) + g.Expect(out).To(ContainSubstring("1 removed")) + g.Expect(out).To(ContainSubstring("1 modified")) + g.Expect(out).To(ContainSubstring("No changes will be written.")) +} + +func TestPlainPrinter_SyncDryRunResult_NoChanges(t *testing.T) { + g := NewWithT(t) + var buf bytes.Buffer + p := output.New(output.FormatText, false, &buf) + + result := output.SyncDryRunResult{ + Clusters: []output.DiffEntry{}, + Contexts: []output.DiffEntry{}, + AuthInfos: []output.DiffEntry{}, + } + g.Expect(p.Print(result)).To(Succeed()) + + out := buf.String() + g.Expect(out).To(ContainSubstring("No changes detected.")) +} + +func TestJSONPrinter_SyncDryRunResult(t *testing.T) { + g := NewWithT(t) + var buf bytes.Buffer + p := output.New(output.FormatJSON, false, &buf) + + result := output.SyncDryRunResult{ + Clusters: []output.DiffEntry{ + {Name: "cloudctl:prod", ChangeType: "added"}, + }, + Contexts: []output.DiffEntry{}, + AuthInfos: []output.DiffEntry{}, + Added: 1, + Removed: 0, + Modified: 0, + } + g.Expect(p.Print(result)).To(Succeed()) + + var got output.SyncDryRunResult + g.Expect(json.Unmarshal(buf.Bytes(), &got)).To(Succeed()) + g.Expect(got.Added).To(Equal(1)) + g.Expect(got.Removed).To(Equal(0)) + g.Expect(got.Clusters).To(HaveLen(1)) + g.Expect(got.Clusters[0].Name).To(Equal("cloudctl:prod")) + g.Expect(got.Clusters[0].ChangeType).To(Equal("added")) +} + +func TestYAMLPrinter_SyncDryRunResult(t *testing.T) { + g := NewWithT(t) + var buf bytes.Buffer + p := output.New(output.FormatYAML, false, &buf) + + result := output.SyncDryRunResult{ + Clusters: []output.DiffEntry{ + {Name: "cloudctl:staging", ChangeType: "removed"}, + }, + Contexts: []output.DiffEntry{}, + AuthInfos: []output.DiffEntry{}, + Added: 0, + Removed: 1, + Modified: 0, + } + g.Expect(p.Print(result)).To(Succeed()) + + var got output.SyncDryRunResult + g.Expect(yaml.Unmarshal(buf.Bytes(), &got)).To(Succeed()) + g.Expect(got.Removed).To(Equal(1)) + g.Expect(got.Clusters[0].ChangeType).To(Equal("removed")) +} diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index ad79c08..c1f054d 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -66,6 +66,50 @@ func (p *plainPrinter) Print(v any) error { } } + case SyncDryRunResult: + w("Dry-run: no changes will be written.\n\n") + total := t.Added + t.Removed + t.Modified + if total == 0 { + w("No changes detected.\n") + break + } + printDiffSection := func(title string, entries []DiffEntry) { + if len(entries) == 0 { + return + } + n := 0 + for _, e := range entries { + if e.ChangeType != "" { + n++ + } + } + w("%s (%d change(s))\n", title, n) + for _, e := range entries { + switch e.ChangeType { + case "added": + w(" + %s\n", e.Name) + case "removed": + w(" - %s\n", e.Name) + case "modified": + w(" ~ %s\n", e.Name) + for _, f := range e.Fields { + if f.Old != "" { + w(" %-12s - %s\n", f.Field+":", f.Old) + } + if f.New != "" { + w(" %-12s + %s\n", f.Field+":", f.New) + } + } + } + } + w("\n") + } + printDiffSection("CLUSTERS", t.Clusters) + printDiffSection("CONTEXTS", t.Contexts) + printDiffSection("AUTH INFOS", t.AuthInfos) + w("Summary: %d added, %d removed, %d modified. No changes will be written.\n", + t.Added, t.Removed, t.Modified) + case ClusterVersionResult: w("Kubernetes version: %s\n", t.Version) diff --git a/cmd/output/types.go b/cmd/output/types.go index cded2bf..7e6638b 100644 --- a/cmd/output/types.go +++ b/cmd/output/types.go @@ -50,3 +50,27 @@ type VersionInfo struct { type ErrorResult struct { Error string `json:"error" yaml:"error"` } + +// SyncDryRunResult is the output of `sync --dry-run`. +type SyncDryRunResult struct { + Clusters []DiffEntry `json:"clusters" yaml:"clusters"` + Contexts []DiffEntry `json:"contexts" yaml:"contexts"` + AuthInfos []DiffEntry `json:"authInfos" yaml:"authInfos"` + Added int `json:"added" yaml:"added"` + Removed int `json:"removed" yaml:"removed"` + Modified int `json:"modified" yaml:"modified"` +} + +// DiffEntry describes a single added, removed, or modified kubeconfig entry. +type DiffEntry struct { + Name string `json:"name" yaml:"name"` + ChangeType string `json:"changeType" yaml:"changeType"` + Fields []FieldChange `json:"fields,omitempty" yaml:"fields,omitempty"` +} + +// FieldChange describes a field-level change within a modified entry. +type FieldChange struct { + Field string `json:"field" yaml:"field"` + Old string `json:"old" yaml:"old"` + New string `json:"new" yaml:"new"` +} diff --git a/cmd/sync.go b/cmd/sync.go index 142417b..ddd8b2d 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -10,11 +10,9 @@ import ( "encoding/json" "fmt" "log/slog" - "maps" "os" "os/exec" "path/filepath" - "sort" "strings" greenhousemetav1alpha1 "github.com/cloudoperators/greenhouse/api/meta/v1alpha1" @@ -42,6 +40,7 @@ var ( kubeloginPath string kubeloginExtraArgs []string kubeloginTokenCacheDir string + dryRun bool ) func init() { @@ -73,6 +72,8 @@ 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") + // BindPFlags can theroretically return an error if called with `nil` as an argument // which should never happened after at least one flag was defined. That's why the output // there is ignored. @@ -102,6 +103,9 @@ Examples: # Use a dedicated Greenhouse kubeconfig and emit JSON output cloudctl sync -n my-org -k ~/.kube/greenhouse.yaml -o json + # Preview what would change without writing + cloudctl sync -n my-org --dry-run + # Debug mode — shows every cluster/authinfo/context decision on stderr cloudctl sync -n my-org --log-level debug`, RunE: runSync, @@ -120,6 +124,7 @@ func runSync(cmd *cobra.Command, args []string) error { kubeloginPath = viper.GetString("kubelogin-path") kubeloginExtraArgs = viper.GetStringSlice("kubelogin-extra-args") kubeloginTokenCacheDir = viper.GetString("kubelogin-token-cache-dir") + dryRun = viper.GetBool("dry-run") format, err := output.ParseFormat(viper.GetString("output")) if err != nil { @@ -211,7 +216,14 @@ func runSync(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create server config: %w", err) } - stopMerge := printer.StartSpinner("Merging kubeconfigs...") + // Take a snapshot before merge for dry-run diff. + localConfigBefore := localConfig.DeepCopy() + + spinnerLabel := "Merging kubeconfigs..." + if dryRun { + spinnerLabel = "Simulating merge (dry-run)..." + } + stopMerge := printer.StartSpinner(spinnerLabel) err = mergeKubeconfig(localConfig, serverConfig) stopMerge() if err != nil { @@ -219,6 +231,11 @@ func runSync(cmd *cobra.Command, args []string) error { return fmt.Errorf(`failed to merge ClusterKubeconfig: %w`, err) } + if dryRun { + diff := diffKubeconfig(localConfigBefore, localConfig) + return printer.Print(buildDryRunResult(diff)) + } + if writeErr := writeConfig(localConfig, remoteClusterKubeconfig); writeErr != nil { _ = printer.Print(buildFailedSyncResult(ready, notReady, writeErr)) return fmt.Errorf("failed to write merged kubeconfig: %w", writeErr) @@ -408,119 +425,6 @@ func isManaged(name string) bool { return strings.HasPrefix(name, prefix+":") } -// authInfoEqual compares two AuthInfo objects, excluding "id-token" and "refresh-token". -func authInfoEqual(a, b *clientcmdapi.AuthInfo) bool { - // Compare ClientCertificateData - if !bytes.Equal(a.ClientCertificateData, b.ClientCertificateData) { - return false - } - - // Compare ClientKeyData - if !bytes.Equal(a.ClientKeyData, b.ClientKeyData) { - return false - } - - // Compare Exec first (new style) - if (a.Exec == nil) != (b.Exec == nil) { - return false - } - if a.Exec != nil && b.Exec != nil { - if a.Exec.Command != b.Exec.Command || a.Exec.APIVersion != b.Exec.APIVersion { - return false - } - if len(a.Exec.Args) != len(b.Exec.Args) { - return false - } - for i := range a.Exec.Args { - if a.Exec.Args[i] != b.Exec.Args[i] { - return false - } - } - return true - } - - // Compare AuthProvider, excluding "id-token" and "refresh-token" - if (a.AuthProvider == nil) != (b.AuthProvider == nil) { - return false - } - if a.AuthProvider != nil && b.AuthProvider != nil { - // Compare AuthProvider Name - if a.AuthProvider.Name != b.AuthProvider.Name { - return false - } - - // Compare AuthProvider Config excluding "id-token" and "refresh-token" - aConfigFiltered := filterAuthProviderConfig(a.AuthProvider.Config) - bConfigFiltered := filterAuthProviderConfig(b.AuthProvider.Config) - if !maps.Equal(aConfigFiltered, bConfigFiltered) { - return false - } - } - - return true -} - -// filterAuthProviderConfig returns a copy of the config map excluding "id-token" and "refresh-token". -func filterAuthProviderConfig(config map[string]string) map[string]string { - filtered := make(map[string]string) - for k, v := range config { - if k != "id-token" && k != "refresh-token" { - filtered[k] = v - } - } - return filtered -} - -// generateAuthInfoKey creates a unique key for an AuthInfo based on specific AuthProvider fields, -// excluding "id-token" and "refresh-token". It uses "client-id", "client-secret", -// "auth-request-extra-params", and "extra-scopes" to generate the key. -func generateAuthInfoKey(authInfo *clientcmdapi.AuthInfo) string { - // Exec-based key: derive from stable subset of args to avoid including tokens - if authInfo.Exec != nil { - // Extract known kubelogin flags - var issuer, clientID, clientSecret, extraParams string - var scopes []string - for _, arg := range authInfo.Exec.Args { - switch { - case strings.HasPrefix(arg, "--oidc-issuer-url="): - issuer = strings.TrimPrefix(arg, "--oidc-issuer-url=") - case strings.HasPrefix(arg, "--oidc-client-id="): - clientID = strings.TrimPrefix(arg, "--oidc-client-id=") - case strings.HasPrefix(arg, "--oidc-client-secret="): - clientSecret = strings.TrimPrefix(arg, "--oidc-client-secret=") - case strings.HasPrefix(arg, "--oidc-extra-scope="): - scopes = append(scopes, strings.TrimPrefix(arg, "--oidc-extra-scope=")) - case strings.HasPrefix(arg, "--oidc-auth-request-extra-params="): - extraParams = strings.TrimPrefix(arg, "--oidc-auth-request-extra-params=") - } - } - sort.Strings(scopes) - data := fmt.Sprintf("exec:issuer:%s;client-id:%s;client-secret:%s;extra-params:%s;scopes:%s", issuer, clientID, clientSecret, extraParams, strings.Join(scopes, ",")) - return data - } - - if authInfo.AuthProvider == nil { - // For AuthInfos without AuthProvider, use a different unique identifier - // Here, we'll use the hash of ClientCertificateData and ClientKeyData - h := sha256.New() - h.Write(authInfo.ClientCertificateData) - h.Write(authInfo.ClientKeyData) - return fmt.Sprintf("cert:%s", hex.EncodeToString(h.Sum(nil))) - } - - // Extract the required fields from AuthProvider Config - clientID := authInfo.AuthProvider.Config["client-id"] - clientSecret := authInfo.AuthProvider.Config["client-secret"] - authRequestExtraParams := authInfo.AuthProvider.Config["auth-request-extra-params"] - extraScopes := authInfo.AuthProvider.Config["extra-scopes"] - - // Concatenate the fields in a consistent order - data := fmt.Sprintf("client-id:%s;client-secret:%s;auth-request-extra-params:%s;extra-scopes:%s", - clientID, clientSecret, authRequestExtraParams, extraScopes) - - return data -} - // buildKubeloginArgs constructs kubelogin arguments from an oidc auth-provider config and extra args func buildKubeloginArgs(cfg map[string]string, extra []string, tokenCacheDir string) []string { args := []string{"get-token"} @@ -590,20 +494,32 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap var authInfoMap map[string]string // key: unique identifier, value: managed AuthInfo name if mergeIdenticalUsers { authInfoMap = make(map[string]string) - } - // Merge AuthInfos - for serverName, serverAuth := range serverConfig.AuthInfos { - var managedAuthName string + // Build a reverse lookup of unmanaged local auth entries so we can reuse their names + // instead of creating new cloudctl:auth- entries. + existingLocalKeys := make(map[string]string) // authInfoKey → local name + for localName, localAuth := range localConfig.AuthInfos { + if !isManaged(localName) { + existingLocalKeys[generateAuthInfoKey(localAuth)] = localName + } + } - if mergeIdenticalUsers { - // Generate a unique key based on AuthInfo excluding id-token and refresh-token + // Merge AuthInfos + for serverName, serverAuth := range serverConfig.AuthInfos { uniqueKey := generateAuthInfoKey(serverAuth) + + // If an unmanaged local entry has the same credentials, reuse its name. + if localName, found := existingLocalKeys[uniqueKey]; found { + slog.Debug("reusing existing local authinfo", "name", localName, "server", serverName) + authInfoMap[uniqueKey] = localName + continue + } + hash := sha256.Sum256([]byte(uniqueKey)) hashString := hex.EncodeToString(hash[:])[:16] // Using the first 16 chars for brevity - managedAuthName = fmt.Sprintf("%s:auth-%s", prefix, hashString) + managedAuthName := fmt.Sprintf("%s:auth-%s", prefix, hashString) - // **Merge AuthInfo to preserve id-token and refresh-token** + // Merge AuthInfo to preserve id-token and refresh-token if existingAuth, exists := localConfig.AuthInfos[managedAuthName]; exists { slog.Debug("merging authinfo tokens", "name", managedAuthName, "server", serverName) mergedAuth := mergeAuthInfo(serverAuth, existingAuth) @@ -614,16 +530,18 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap } authInfoMap[uniqueKey] = managedAuthName - } else { - // Without merging, manage AuthInfos normally - managedAuthName = managedNameFunc(serverName) + } + } else { + // Without merging, manage AuthInfos normally + for serverName, serverAuth := range serverConfig.AuthInfos { + managedAuthName := managedNameFunc(serverName) localAuth, exists := localConfig.AuthInfos[managedAuthName] if !exists { slog.Debug("adding authinfo", "name", managedAuthName) localConfig.AuthInfos[managedAuthName] = serverAuth } else { if !authInfoEqual(localAuth, serverAuth) { - // **Merge AuthInfo to preserve id-token and refresh-token** + // Merge AuthInfo to preserve id-token and refresh-token slog.Debug("updating authinfo", "name", managedAuthName) mergedAuth := mergeAuthInfo(serverAuth, localAuth) localConfig.AuthInfos[managedAuthName] = mergedAuth @@ -766,36 +684,6 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap return nil } -// Helper function to merge AuthInfo objects while preserving id-token and refresh-token -func mergeAuthInfo(serverAuth, localAuth *clientcmdapi.AuthInfo) *clientcmdapi.AuthInfo { - if localAuth == nil { - // If there's no local AuthInfo, return the server AuthInfo as is - return serverAuth - } - - // Create a copy of the serverAuth to avoid mutating the original - mergedAuth := serverAuth.DeepCopy() - - // Preserve id-token and refresh-token from localAuth - if localAuth.AuthProvider != nil && mergedAuth.AuthProvider != nil { - // Ensure the merged config map is initialized to avoid panics on assignment - if mergedAuth.AuthProvider.Config == nil { - mergedAuth.AuthProvider.Config = make(map[string]string) - } - if idToken, exists := localAuth.AuthProvider.Config["id-token"]; exists { - mergedAuth.AuthProvider.Config["id-token"] = idToken - } - if refreshToken, exists := localAuth.AuthProvider.Config["refresh-token"]; exists { - mergedAuth.AuthProvider.Config["refresh-token"] = refreshToken - } - } - - // Additionally, preserve other fields if necessary. - // For example, ClientCertificateData and ClientKeyData are already handled - - return mergedAuth -} - // labelsExtensionEqual returns true if the "labels" named extension is equal in both maps. func labelsExtensionEqual(a, b map[string]runtime.Object) bool { ar := extensionRaw(a, "labels") diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 2d61a31..c078d80 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -467,3 +467,414 @@ func TestGenerateAuthInfoKey_ExecStableIgnoresOrder(t *testing.T) { kb := generateAuthInfoKey(b) g.Expect(ka).To(Equal(kb)) } + +// --------------------------------------------------------------------------- +// AuthInfo refactor (#54) — new tests +// --------------------------------------------------------------------------- + +func TestAuthInfoEqual_ExecEnvConsidered(t *testing.T) { + g := NewWithT(t) + + base := &clientcmdapi.AuthInfo{Exec: &clientcmdapi.ExecConfig{ + APIVersion: "client.authentication.k8s.io/v1", + Command: "kubelogin", + Args: []string{"get-token"}, + Env: []clientcmdapi.ExecEnvVar{{Name: "HTTP_PROXY", Value: "http://proxy.example.com"}}, + }} + diff := &clientcmdapi.AuthInfo{Exec: &clientcmdapi.ExecConfig{ + APIVersion: "client.authentication.k8s.io/v1", + Command: "kubelogin", + Args: []string{"get-token"}, + Env: []clientcmdapi.ExecEnvVar{{Name: "HTTP_PROXY", Value: "http://other.example.com"}}, + }} + + g.Expect(authInfoEqual(base, diff)).To(BeFalse(), "different Env values must not be equal") + + same := base.DeepCopy() + g.Expect(authInfoEqual(base, same)).To(BeTrue(), "identical Env must be equal") +} + +func TestAuthInfoEqual_ExecInteractiveModeConsidered(t *testing.T) { + g := NewWithT(t) + + a := &clientcmdapi.AuthInfo{Exec: &clientcmdapi.ExecConfig{ + APIVersion: "client.authentication.k8s.io/v1", + Command: "kubelogin", + Args: []string{"get-token"}, + InteractiveMode: clientcmdapi.IfAvailableExecInteractiveMode, + }} + b := a.DeepCopy() + b.Exec.InteractiveMode = clientcmdapi.NeverExecInteractiveMode + + g.Expect(authInfoEqual(a, b)).To(BeFalse(), "different InteractiveMode must not be equal") + g.Expect(authInfoEqual(a, a.DeepCopy())).To(BeTrue()) +} + +func TestGenerateAuthInfoKey_AuthProviderIncludesIssuer(t *testing.T) { + g := NewWithT(t) + + // Same client-id but different issuers — must produce different keys + a := &clientcmdapi.AuthInfo{ + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://issuer-a.example.com", + "client-id": "same-client-id", + "client-secret": "same-secret", + }, + }, + } + b := &clientcmdapi.AuthInfo{ + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://issuer-b.example.com", + "client-id": "same-client-id", + "client-secret": "same-secret", + }, + }, + } + + ka := generateAuthInfoKey(a) + kb := generateAuthInfoKey(b) + g.Expect(ka).ToNot(Equal(kb), "different idp-issuer-url must produce different keys") +} + +func TestGenerateAuthInfoKey_ExecEnvAffectsKey(t *testing.T) { + g := NewWithT(t) + + a := &clientcmdapi.AuthInfo{Exec: &clientcmdapi.ExecConfig{ + APIVersion: "client.authentication.k8s.io/v1", + Command: "kubelogin", + Args: []string{"get-token", "--oidc-issuer-url=https://issuer", "--oidc-client-id=cid"}, + Env: []clientcmdapi.ExecEnvVar{{Name: "HTTP_PROXY", Value: "http://proxy.example.com"}}, + }} + b := &clientcmdapi.AuthInfo{Exec: &clientcmdapi.ExecConfig{ + APIVersion: "client.authentication.k8s.io/v1", + Command: "kubelogin", + Args: []string{"get-token", "--oidc-issuer-url=https://issuer", "--oidc-client-id=cid"}, + // no Env + }} + + ka := generateAuthInfoKey(a) + kb := generateAuthInfoKey(b) + g.Expect(ka).ToNot(Equal(kb), "exec env change must produce different key") +} + +func TestMergeKubeconfig_PrefersExistingLocalAuthName(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + mergeIdenticalUsers = true + t.Cleanup(func() { + prefix = orig + mergeIdenticalUsers = true + }) + + // The local kubeconfig has an unmanaged user entry with the same OIDC creds + // as what the server would produce. + localAuth := &clientcmdapi.AuthInfo{ + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://issuer.example.com", + "client-id": "cid", + "client-secret": "csec", + }, + }, + } + localConfig := clientcmdapi.NewConfig() + localConfig.AuthInfos["my-existing-user"] = localAuth + + // Server config has the same OIDC creds + serverAuth := &clientcmdapi.AuthInfo{ + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://issuer.example.com", + "client-id": "cid", + "client-secret": "csec", + }, + }, + } + serverConfig := clientcmdapi.NewConfig() + serverConfig.AuthInfos["server-user"] = serverAuth + serverConfig.Clusters["prod"] = &clientcmdapi.Cluster{Server: "https://prod.example.com"} + serverConfig.Contexts["prod"] = &clientcmdapi.Context{ + Cluster: "prod", + AuthInfo: "server-user", + } + + err := mergeKubeconfig(localConfig, serverConfig) + g.Expect(err).ToNot(HaveOccurred()) + + // The unmanaged "my-existing-user" should be reused — no cloudctl:auth-* should be created + for name := range localConfig.AuthInfos { + g.Expect(name).ToNot(HavePrefix("cloudctl:auth-"), "should reuse existing local user, not create managed auth entry") + } + // The context should reference the existing local user + ctx, ok := localConfig.Contexts["prod"] + g.Expect(ok).To(BeTrue()) + g.Expect(ctx.AuthInfo).To(Equal("my-existing-user")) +} + +func TestMergeKubeconfig_DeduplicatesSameOIDCUsers(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + mergeIdenticalUsers = true + t.Cleanup(func() { + prefix = orig + mergeIdenticalUsers = true + }) + + // Two clusters with the same OIDC config should share one auth entry + sharedAuth := clientcmdapi.AuthInfo{ + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://issuer.example.com", + "client-id": "shared-client-id", + "client-secret": "shared-secret", + }, + }, + } + serverConfig := clientcmdapi.NewConfig() + serverConfig.AuthInfos["cluster-a-user"] = sharedAuth.DeepCopy() + serverConfig.AuthInfos["cluster-b-user"] = sharedAuth.DeepCopy() + serverConfig.Clusters["cluster-a"] = &clientcmdapi.Cluster{Server: "https://a.example.com"} + serverConfig.Clusters["cluster-b"] = &clientcmdapi.Cluster{Server: "https://b.example.com"} + serverConfig.Contexts["cluster-a"] = &clientcmdapi.Context{Cluster: "cluster-a", AuthInfo: "cluster-a-user"} + serverConfig.Contexts["cluster-b"] = &clientcmdapi.Context{Cluster: "cluster-b", AuthInfo: "cluster-b-user"} + + localConfig := clientcmdapi.NewConfig() + err := mergeKubeconfig(localConfig, serverConfig) + g.Expect(err).ToNot(HaveOccurred()) + + // Count managed auth entries — should be exactly 1 + managedAuthCount := 0 + var sharedAuthName string + for name := range localConfig.AuthInfos { + if isManaged(name) { + managedAuthCount++ + sharedAuthName = name + } + } + g.Expect(managedAuthCount).To(Equal(1), "two clusters with same OIDC config should share one auth entry") + + // Both contexts must reference the same auth entry + ctxA := localConfig.Contexts["cluster-a"] + ctxB := localConfig.Contexts["cluster-b"] + g.Expect(ctxA).ToNot(BeNil()) + g.Expect(ctxB).ToNot(BeNil()) + g.Expect(ctxA.AuthInfo).To(Equal(sharedAuthName)) + g.Expect(ctxB.AuthInfo).To(Equal(sharedAuthName)) +} + +// --------------------------------------------------------------------------- +// Dry-run / diffKubeconfig tests (#51) +// --------------------------------------------------------------------------- + +func newCfg() *clientcmdapi.Config { + return clientcmdapi.NewConfig() +} + +func TestDiffKubeconfig_AddedCluster(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + newCfg2 := newCfg() + newCfg2.Clusters["cloudctl:prod-eu-1"] = &clientcmdapi.Cluster{Server: "https://prod-eu-1.example.com"} + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Clusters).To(HaveLen(1)) + g.Expect(diff.Clusters[0].Name).To(Equal("cloudctl:prod-eu-1")) + g.Expect(diff.Clusters[0].ChangeType).To(Equal(DiffChangeAdded)) +} + +func TestDiffKubeconfig_RemovedCluster(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Clusters["cloudctl:staging-de"] = &clientcmdapi.Cluster{Server: "https://staging.example.com"} + newCfg2 := newCfg() + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Clusters).To(HaveLen(1)) + g.Expect(diff.Clusters[0].Name).To(Equal("cloudctl:staging-de")) + g.Expect(diff.Clusters[0].ChangeType).To(Equal(DiffChangeRemoved)) +} + +func TestDiffKubeconfig_ModifiedClusterServerURL(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Clusters["cloudctl:prod-eu-2"] = &clientcmdapi.Cluster{Server: "https://old.example.com"} + newCfg2 := newCfg() + newCfg2.Clusters["cloudctl:prod-eu-2"] = &clientcmdapi.Cluster{Server: "https://new.example.com"} + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Clusters).To(HaveLen(1)) + g.Expect(diff.Clusters[0].ChangeType).To(Equal(DiffChangeModified)) + g.Expect(diff.Clusters[0].Fields).To(HaveLen(1)) + g.Expect(diff.Clusters[0].Fields[0].Field).To(Equal("Server")) + g.Expect(diff.Clusters[0].Fields[0].Old).To(Equal("https://old.example.com")) + g.Expect(diff.Clusters[0].Fields[0].New).To(Equal("https://new.example.com")) +} + +func TestDiffKubeconfig_ModifiedClusterCA(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Clusters["cloudctl:prod"] = &clientcmdapi.Cluster{CertificateAuthorityData: []byte("old-ca-data")} + newCfg2 := newCfg() + newCfg2.Clusters["cloudctl:prod"] = &clientcmdapi.Cluster{CertificateAuthorityData: []byte("new-ca-data")} + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Clusters).To(HaveLen(1)) + g.Expect(diff.Clusters[0].ChangeType).To(Equal(DiffChangeModified)) + caField := diff.Clusters[0].Fields[0] + g.Expect(caField.Field).To(Equal("CA")) + // Value should be a hex fingerprint, not raw bytes + g.Expect(caField.Old).To(HaveLen(16)) + g.Expect(caField.New).To(HaveLen(16)) + g.Expect(caField.Old).ToNot(Equal(caField.New)) +} + +func TestDiffKubeconfig_AddedContext(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + newCfg2 := newCfg() + newCfg2.Contexts["cloudctl:prod"] = &clientcmdapi.Context{Cluster: "cloudctl:prod", AuthInfo: "cloudctl:auth-abc"} + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Contexts).To(HaveLen(1)) + g.Expect(diff.Contexts[0].ChangeType).To(Equal(DiffChangeAdded)) +} + +func TestDiffKubeconfig_RemovedContext(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Contexts["cloudctl:staging"] = &clientcmdapi.Context{Cluster: "cloudctl:staging"} + newCfg2 := newCfg() + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Contexts).To(HaveLen(1)) + g.Expect(diff.Contexts[0].ChangeType).To(Equal(DiffChangeRemoved)) +} + +func TestDiffKubeconfig_ModifiedAuthInfoExecArgs(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.AuthInfos["cloudctl:auth-abc123"] = &clientcmdapi.AuthInfo{ + Exec: &clientcmdapi.ExecConfig{ + Command: "kubelogin", + Args: []string{"get-token", "--oidc-client-secret=old-secret"}, + }, + } + newCfg2 := newCfg() + newCfg2.AuthInfos["cloudctl:auth-abc123"] = &clientcmdapi.AuthInfo{ + Exec: &clientcmdapi.ExecConfig{ + Command: "kubelogin", + Args: []string{"get-token", "--oidc-client-secret=new-secret"}, + }, + } + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.AuthInfos).To(HaveLen(1)) + g.Expect(diff.AuthInfos[0].ChangeType).To(Equal(DiffChangeModified)) + g.Expect(diff.AuthInfos[0].Fields).ToNot(BeEmpty()) +} + +func TestDiffKubeconfig_UnchangedEntriesExcluded(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + cfg := newCfg() + cfg.Clusters["cloudctl:unchanged"] = &clientcmdapi.Cluster{Server: "https://same.example.com"} + + diff := diffKubeconfig(cfg, cfg) + g.Expect(diff.Clusters).To(BeEmpty(), "unchanged entries must not appear in diff") +} + +func TestDiffKubeconfig_UnmanagedEntriesIgnored(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Clusters["my-own-cluster"] = &clientcmdapi.Cluster{Server: "https://personal.example.com"} + newCfg2 := newCfg() + // unmanaged entry removed from new — must not appear in diff + + diff := diffKubeconfig(oldCfg, newCfg2) + g.Expect(diff.Clusters).To(BeEmpty()) + g.Expect(diff.Contexts).To(BeEmpty()) + g.Expect(diff.AuthInfos).To(BeEmpty()) +} + +func TestRunSync_DryRun_NoWrite(t *testing.T) { + g := NewWithT(t) + + orig := prefix + prefix = "cloudctl" + mergeIdenticalUsers = true + t.Cleanup(func() { + prefix = orig + mergeIdenticalUsers = true + }) + + // Build a "before" and "after" config to simulate what dry-run does + localConfigBefore := clientcmdapi.NewConfig() + localConfigBefore.Clusters["cloudctl:existing"] = &clientcmdapi.Cluster{Server: "https://existing.example.com"} + + // Simulate an incoming server config that adds a new cluster + serverConfig := clientcmdapi.NewConfig() + serverConfig.Clusters["existing"] = &clientcmdapi.Cluster{Server: "https://existing.example.com"} + serverConfig.Clusters["new-cluster"] = &clientcmdapi.Cluster{Server: "https://new.example.com"} + + localConfig := localConfigBefore.DeepCopy() + err := mergeKubeconfig(localConfig, serverConfig) + g.Expect(err).ToNot(HaveOccurred()) + + diff := diffKubeconfig(localConfigBefore, localConfig) + result := buildDryRunResult(diff) + + // The new cluster should appear as added + g.Expect(result.Added).To(BeNumerically(">=", 1)) + g.Expect(result.Clusters).To(ContainElement( + HaveField("ChangeType", "added"), + )) + + // Original localConfigBefore must not have been modified + g.Expect(localConfigBefore.Clusters).ToNot(HaveKey("cloudctl:new-cluster")) +} From d44d806de4e92fb99aab58f9a42139ce021960f1 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 11:34:57 +0200 Subject: [PATCH 02/16] fix(sync): context-centric dry-run output with server URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the three-section CLUSTERS/CONTEXTS/AUTH INFOS dry-run output with a single CLUSTER ACCESSES section, one entry per context. Changes: - Add AccessDiff type to output/types.go and Accesses field to SyncDryRunResult (legacy fields kept for JSON/YAML compat) - Fix diffContexts: filter by managed cluster reference instead of managed context name — context names are stored without prefix so the old isManaged(name) check always returned false - Add isManagedContext helper that inspects the context's Cluster field - Add buildAccessDiffs: derives []AccessDiff from context-level diffs plus cluster-level server URL changes (for contexts whose cluster changed but whose own refs did not) - Update buildDryRunResult to accept old/new configs and count changes from Accesses rather than raw cluster/context/authinfo counts - Update plain and interactive printers to render CLUSTER ACCESSES with server URL shown inline for added entries - Fix and extend tests accordingly Closes #51, #54 Signed-off-by: onuryilmaz --- cmd/kubeconfigdiff.go | 170 +++++++++++++++++++++++++++--- cmd/output/interactive_printer.go | 42 ++++---- cmd/output/output_test.go | 17 +-- cmd/output/plain_printer.go | 45 +++----- cmd/output/types.go | 21 ++-- cmd/sync.go | 2 +- cmd/sync_test.go | 114 +++++++++++++++++++- 7 files changed, 323 insertions(+), 88 deletions(-) diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index 68bb7db..0dda98e 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -103,12 +103,26 @@ func diffClusters(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { return diffs } -// diffContexts returns added/removed/modified context entries for managed names. +// isManagedContext returns true if the context at name references a managed cluster +// (i.e. a cluster whose name has the managed prefix). Context names themselves are +// stored without the prefix, so we check the cluster reference instead. +func isManagedContext(name string, oldCfg, newCfg *clientcmdapi.Config) bool { + if ctx, ok := newCfg.Contexts[name]; ok { + return isManaged(ctx.Cluster) + } + if ctx, ok := oldCfg.Contexts[name]; ok { + return isManaged(ctx.Cluster) + } + return false +} + +// diffContexts returns added/removed/modified context entries for managed contexts. +// A context is considered managed if its cluster reference carries the managed prefix. func diffContexts(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { var diffs []EntryDiff for name, newCtx := range newCfg.Contexts { - if !isManaged(name) { + if !isManagedContext(name, oldCfg, newCfg) { continue } oldCtx, exists := oldCfg.Contexts[name] @@ -132,7 +146,7 @@ func diffContexts(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { } for name := range oldCfg.Contexts { - if !isManaged(name) { + if !isManagedContext(name, oldCfg, newCfg) { continue } if _, exists := newCfg.Contexts[name]; !exists { @@ -277,30 +291,151 @@ func toOutputDiffEntries(diffs []EntryDiff) []output.DiffEntry { return result } +// buildAccessDiffs derives a []output.AccessDiff from the context-level diff, +// enriching each entry with the server URL (looked up from the cluster the +// context references) and credential-change information. +// It also synthesises modified access entries for contexts whose underlying +// cluster changed server URL or CA (tracked in diff.Clusters) even when the +// context itself was not structurally modified. +func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) []output.AccessDiff { + // accesses keyed by context name to allow merging + type accessEntry struct { + access output.AccessDiff + fromCtx bool // originated from a context-level diff + } + byName := make(map[string]*accessEntry) + + // 1. Process explicit context-level diffs (added / removed / modified context refs) + for _, ctxDiff := range diff.Contexts { + name := ctxDiff.Name + entry := &accessEntry{ + access: output.AccessDiff{ + Name: name, + ChangeType: string(ctxDiff.ChangeType), + }, + fromCtx: true, + } + + switch ctxDiff.ChangeType { + case DiffChangeAdded: + if ctx, ok := newCfg.Contexts[name]; ok { + if cluster, ok := newCfg.Clusters[ctx.Cluster]; ok { + entry.access.Server = cluster.Server + } + } + case DiffChangeRemoved: + if ctx, ok := oldCfg.Contexts[name]; ok { + if cluster, ok := oldCfg.Clusters[ctx.Cluster]; ok { + entry.access.Server = cluster.Server + } + } + case DiffChangeModified: + oldCtx := oldCfg.Contexts[name] + newCtx := newCfg.Contexts[name] + if oldCtx != nil && newCtx != nil { + oldCluster := oldCfg.Clusters[oldCtx.Cluster] + newCluster := newCfg.Clusters[newCtx.Cluster] + oldServer, newServer := "", "" + if oldCluster != nil { + oldServer = oldCluster.Server + } + if newCluster != nil { + newServer = newCluster.Server + } + if oldServer != newServer { + entry.access.Fields = append(entry.access.Fields, output.FieldChange{ + Field: "Server", + Old: oldServer, + New: newServer, + }) + } + // Check if credentials changed + oldAuth := oldCfg.AuthInfos[oldCtx.AuthInfo] + newAuth := newCfg.AuthInfos[newCtx.AuthInfo] + if (oldAuth != nil && newAuth != nil && !authInfoEqual(oldAuth, newAuth)) || + (oldAuth == nil) != (newAuth == nil) { + entry.access.Fields = append(entry.access.Fields, output.FieldChange{Field: "Credentials", Old: "changed", New: ""}) + } + } + } + byName[name] = entry + } + + // 2. For each modified cluster, find contexts in newCfg that reference it. + // If those contexts were not already captured via context-level diffs, add + // a "modified" access entry for the server URL change. + modifiedClusters := make(map[string]EntryDiff, len(diff.Clusters)) + for _, cd := range diff.Clusters { + if cd.ChangeType == DiffChangeModified { + modifiedClusters[cd.Name] = cd + } + } + if len(modifiedClusters) > 0 { + for ctxName, ctx := range newCfg.Contexts { + if _, alreadyHandled := byName[ctxName]; alreadyHandled { + continue + } + clusterDiff, affected := modifiedClusters[ctx.Cluster] + if !affected { + continue + } + var fields []output.FieldChange + for _, f := range clusterDiff.Fields { + if f.Field == "Server" { + fields = append(fields, output.FieldChange{ + Field: "Server", + Old: f.Old, + New: f.New, + }) + } + } + if len(fields) == 0 { + continue + } + byName[ctxName] = &accessEntry{ + access: output.AccessDiff{ + Name: ctxName, + ChangeType: string(DiffChangeModified), + Fields: fields, + }, + } + } + } + + // Flatten and sort + accesses := make([]output.AccessDiff, 0, len(byName)) + for _, e := range byName { + accesses = append(accesses, e.access) + } + sort.Slice(accesses, func(i, j int) bool { return accesses[i].Name < accesses[j].Name }) + return accesses +} + // buildDryRunResult converts a KubeconfigDiff to an output.SyncDryRunResult. -func buildDryRunResult(diff KubeconfigDiff) output.SyncDryRunResult { +// oldCfg and newCfg are needed to build the context-centric AccessDiff entries. +func buildDryRunResult(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) output.SyncDryRunResult { + accesses := buildAccessDiffs(diff, oldCfg, newCfg) + var added, removed, modified int - countChanges := func(entries []EntryDiff) { - for _, e := range entries { - switch e.ChangeType { - case DiffChangeAdded: - added++ - case DiffChangeRemoved: - removed++ - case DiffChangeModified: - modified++ - } + for _, a := range accesses { + switch DiffChangeType(a.ChangeType) { + case DiffChangeAdded: + added++ + case DiffChangeRemoved: + removed++ + case DiffChangeModified: + modified++ } } - countChanges(diff.Clusters) - countChanges(diff.Contexts) - countChanges(diff.AuthInfos) clusters := toOutputDiffEntries(diff.Clusters) contexts := toOutputDiffEntries(diff.Contexts) authInfos := toOutputDiffEntries(diff.AuthInfos) // Use empty slices instead of nil for consistent JSON/YAML output + if accesses == nil { + accesses = []output.AccessDiff{} + } if clusters == nil { clusters = []output.DiffEntry{} } @@ -312,6 +447,7 @@ func buildDryRunResult(diff KubeconfigDiff) output.SyncDryRunResult { } return output.SyncDryRunResult{ + Accesses: accesses, Clusters: clusters, Contexts: contexts, AuthInfos: authInfos, diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index c182a88..c54e436 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -211,45 +211,39 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { _, writeErr = fmt.Fprintf(p.w, format, a...) } - w("%s\n\n", styleFaint.Render("Dry-run: no changes will be written.")) - total := r.Added + r.Removed + r.Modified if total == 0 { w("%s\n", styleFaint.Render("No changes detected.")) return writeErr } - printSection := func(title string, entries []DiffEntry) { - if len(entries) == 0 { - return - } - n := len(entries) - w("%s (%d change(s))\n", styleHeader.Render(title), n) - for _, e := range entries { - switch e.ChangeType { - case "added": - w(" %s %s\n", styleGreen.Render("+"), e.Name) - case "removed": - w(" %s %s\n", styleRed.Render("-"), e.Name) - case "modified": - w(" %s %s\n", styleYellow.Render("~"), e.Name) - for _, f := range e.Fields { + w("%s\n\n", styleFaint.Render("Dry-run: no changes will be written.")) + w("%s (%d change(s))\n", styleHeader.Render("CLUSTER ACCESSES"), total) + + for _, a := range r.Accesses { + switch a.ChangeType { + case "added": + w(" %s %-20s %s\n", styleGreen.Render("+"), a.Name, styleFaint.Render(a.Server)) + case "removed": + w(" %s %-20s %s\n", styleRed.Render("-"), a.Name, styleFaint.Render("(removed)")) + case "modified": + w(" %s %s\n", styleYellow.Render("~"), a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" %-16s %s\n", "Credentials:", styleYellow.Render("changed")) + } else { if f.Old != "" { - w(" %-12s %s\n", f.Field+":", styleRed.Render("- "+f.Old)) + w(" %-16s %s\n", f.Field+":", styleRed.Render("- "+f.Old)) } if f.New != "" { - w(" %-12s %s\n", f.Field+":", styleGreen.Render("+ "+f.New)) + w(" %-16s %s\n", f.Field+":", styleGreen.Render("+ "+f.New)) } } } } - w("\n") } - printSection("CLUSTERS", r.Clusters) - printSection("CONTEXTS", r.Contexts) - printSection("AUTH INFOS", r.AuthInfos) - + w("\n") summaryParts := []string{ styleGreen.Render(fmt.Sprintf("%d added", r.Added)), styleRed.Render(fmt.Sprintf("%d removed", r.Removed)), diff --git a/cmd/output/output_test.go b/cmd/output/output_test.go index 33b5026..bad09aa 100644 --- a/cmd/output/output_test.go +++ b/cmd/output/output_test.go @@ -220,13 +220,14 @@ func TestPlainPrinter_SyncDryRunResult_Changes(t *testing.T) { p := output.New(output.FormatText, false, &buf) result := output.SyncDryRunResult{ - Clusters: []output.DiffEntry{ - {Name: "cloudctl:prod-eu-1", ChangeType: "added"}, - {Name: "cloudctl:staging-de", ChangeType: "removed"}, - {Name: "cloudctl:prod-eu-2", ChangeType: "modified", Fields: []output.FieldChange{ + Accesses: []output.AccessDiff{ + {Name: "prod-eu-1", ChangeType: "added", Server: "https://prod-eu-1.example.com"}, + {Name: "staging-de", ChangeType: "removed", Server: "https://staging.example.com"}, + {Name: "prod-eu-2", ChangeType: "modified", Fields: []output.FieldChange{ {Field: "Server", Old: "https://old.example.com", New: "https://new.example.com"}, }}, }, + Clusters: []output.DiffEntry{}, Contexts: []output.DiffEntry{}, AuthInfos: []output.DiffEntry{}, Added: 1, @@ -237,9 +238,11 @@ func TestPlainPrinter_SyncDryRunResult_Changes(t *testing.T) { out := buf.String() g.Expect(out).To(ContainSubstring("Dry-run: no changes will be written.")) - g.Expect(out).To(ContainSubstring("+ cloudctl:prod-eu-1")) - g.Expect(out).To(ContainSubstring("- cloudctl:staging-de")) - g.Expect(out).To(ContainSubstring("~ cloudctl:prod-eu-2")) + g.Expect(out).To(ContainSubstring("CLUSTER ACCESSES")) + g.Expect(out).To(ContainSubstring("+ prod-eu-1")) + g.Expect(out).To(ContainSubstring("https://prod-eu-1.example.com")) + g.Expect(out).To(ContainSubstring("- staging-de")) + g.Expect(out).To(ContainSubstring("~ prod-eu-2")) g.Expect(out).To(ContainSubstring("https://old.example.com")) g.Expect(out).To(ContainSubstring("https://new.example.com")) g.Expect(out).To(ContainSubstring("Summary:")) diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index c1f054d..e083260 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -67,47 +67,36 @@ func (p *plainPrinter) Print(v any) error { } case SyncDryRunResult: - w("Dry-run: no changes will be written.\n\n") total := t.Added + t.Removed + t.Modified if total == 0 { w("No changes detected.\n") break } - printDiffSection := func(title string, entries []DiffEntry) { - if len(entries) == 0 { - return - } - n := 0 - for _, e := range entries { - if e.ChangeType != "" { - n++ - } - } - w("%s (%d change(s))\n", title, n) - for _, e := range entries { - switch e.ChangeType { - case "added": - w(" + %s\n", e.Name) - case "removed": - w(" - %s\n", e.Name) - case "modified": - w(" ~ %s\n", e.Name) - for _, f := range e.Fields { + w("Dry-run: no changes will be written.\n\n") + w("CLUSTER ACCESSES (%d change(s))\n", total) + for _, a := range t.Accesses { + switch a.ChangeType { + case "added": + w(" + %-20s %s\n", a.Name, a.Server) + case "removed": + w(" - %-20s (removed)\n", a.Name) + case "modified": + w(" ~ %s\n", a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" %-14s changed\n", "Credentials:") + } else { if f.Old != "" { - w(" %-12s - %s\n", f.Field+":", f.Old) + w(" %-14s - %s\n", f.Field+":", f.Old) } if f.New != "" { - w(" %-12s + %s\n", f.Field+":", f.New) + w(" %-14s + %s\n", f.Field+":", f.New) } } } } - w("\n") } - printDiffSection("CLUSTERS", t.Clusters) - printDiffSection("CONTEXTS", t.Contexts) - printDiffSection("AUTH INFOS", t.AuthInfos) - w("Summary: %d added, %d removed, %d modified. No changes will be written.\n", + w("\nSummary: %d added, %d removed, %d modified. No changes will be written.\n", t.Added, t.Removed, t.Modified) case ClusterVersionResult: diff --git a/cmd/output/types.go b/cmd/output/types.go index 7e6638b..11427af 100644 --- a/cmd/output/types.go +++ b/cmd/output/types.go @@ -51,14 +51,23 @@ type ErrorResult struct { Error string `json:"error" yaml:"error"` } +// AccessDiff describes one cluster access (context) that is changing. +type AccessDiff struct { + Name string `json:"name" yaml:"name"` + ChangeType string `json:"changeType" yaml:"changeType"` + Server string `json:"server,omitempty" yaml:"server,omitempty"` + Fields []FieldChange `json:"fields,omitempty" yaml:"fields,omitempty"` +} + // SyncDryRunResult is the output of `sync --dry-run`. type SyncDryRunResult struct { - Clusters []DiffEntry `json:"clusters" yaml:"clusters"` - Contexts []DiffEntry `json:"contexts" yaml:"contexts"` - AuthInfos []DiffEntry `json:"authInfos" yaml:"authInfos"` - Added int `json:"added" yaml:"added"` - Removed int `json:"removed" yaml:"removed"` - Modified int `json:"modified" yaml:"modified"` + Accesses []AccessDiff `json:"accesses" yaml:"accesses"` + Clusters []DiffEntry `json:"clusters" yaml:"clusters"` + Contexts []DiffEntry `json:"contexts" yaml:"contexts"` + AuthInfos []DiffEntry `json:"authInfos" yaml:"authInfos"` + Added int `json:"added" yaml:"added"` + Removed int `json:"removed" yaml:"removed"` + Modified int `json:"modified" yaml:"modified"` } // DiffEntry describes a single added, removed, or modified kubeconfig entry. diff --git a/cmd/sync.go b/cmd/sync.go index ddd8b2d..873abc9 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -233,7 +233,7 @@ func runSync(cmd *cobra.Command, args []string) error { if dryRun { diff := diffKubeconfig(localConfigBefore, localConfig) - return printer.Print(buildDryRunResult(diff)) + return printer.Print(buildDryRunResult(diff, localConfigBefore, localConfig)) } if writeErr := writeConfig(localConfig, remoteClusterKubeconfig); writeErr != nil { diff --git a/cmd/sync_test.go b/cmd/sync_test.go index c078d80..a052300 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -763,7 +763,8 @@ func TestDiffKubeconfig_AddedContext(t *testing.T) { oldCfg := newCfg() newCfg2 := newCfg() - newCfg2.Contexts["cloudctl:prod"] = &clientcmdapi.Context{Cluster: "cloudctl:prod", AuthInfo: "cloudctl:auth-abc"} + // Context name has no prefix; cluster reference is prefixed (managed) + newCfg2.Contexts["prod"] = &clientcmdapi.Context{Cluster: "cloudctl:prod", AuthInfo: "cloudctl:auth-abc"} diff := diffKubeconfig(oldCfg, newCfg2) g.Expect(diff.Contexts).To(HaveLen(1)) @@ -777,7 +778,8 @@ func TestDiffKubeconfig_RemovedContext(t *testing.T) { t.Cleanup(func() { prefix = orig }) oldCfg := newCfg() - oldCfg.Contexts["cloudctl:staging"] = &clientcmdapi.Context{Cluster: "cloudctl:staging"} + // Context name has no prefix; cluster reference is prefixed (managed) + oldCfg.Contexts["staging"] = &clientcmdapi.Context{Cluster: "cloudctl:staging"} newCfg2 := newCfg() diff := diffKubeconfig(oldCfg, newCfg2) @@ -856,25 +858,127 @@ func TestRunSync_DryRun_NoWrite(t *testing.T) { // Build a "before" and "after" config to simulate what dry-run does localConfigBefore := clientcmdapi.NewConfig() localConfigBefore.Clusters["cloudctl:existing"] = &clientcmdapi.Cluster{Server: "https://existing.example.com"} + localConfigBefore.Contexts["existing"] = &clientcmdapi.Context{Cluster: "cloudctl:existing", AuthInfo: "cloudctl:auth-abc"} // Simulate an incoming server config that adds a new cluster serverConfig := clientcmdapi.NewConfig() serverConfig.Clusters["existing"] = &clientcmdapi.Cluster{Server: "https://existing.example.com"} serverConfig.Clusters["new-cluster"] = &clientcmdapi.Cluster{Server: "https://new.example.com"} + sharedAuth := &clientcmdapi.AuthInfo{ + Exec: &clientcmdapi.ExecConfig{ + APIVersion: "client.authentication.k8s.io/v1", + Command: "kubelogin", + Args: []string{"get-token", "--oidc-issuer-url=https://issuer.example.com"}, + }, + } + serverConfig.AuthInfos["shared-user"] = sharedAuth + serverConfig.Contexts["existing"] = &clientcmdapi.Context{Cluster: "existing", AuthInfo: "shared-user"} + serverConfig.Contexts["new-cluster"] = &clientcmdapi.Context{Cluster: "new-cluster", AuthInfo: "shared-user"} localConfig := localConfigBefore.DeepCopy() err := mergeKubeconfig(localConfig, serverConfig) g.Expect(err).ToNot(HaveOccurred()) diff := diffKubeconfig(localConfigBefore, localConfig) - result := buildDryRunResult(diff) + result := buildDryRunResult(diff, localConfigBefore, localConfig) - // The new cluster should appear as added + // The new cluster access should appear as added g.Expect(result.Added).To(BeNumerically(">=", 1)) - g.Expect(result.Clusters).To(ContainElement( + g.Expect(result.Accesses).To(ContainElement( HaveField("ChangeType", "added"), )) // Original localConfigBefore must not have been modified g.Expect(localConfigBefore.Clusters).ToNot(HaveKey("cloudctl:new-cluster")) } + +// --------------------------------------------------------------------------- +// buildAccessDiffs tests +// --------------------------------------------------------------------------- + +func TestBuildAccessDiffs_Added(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + newCfg2 := newCfg() + newCfg2.Clusters["cloudctl:prod-eu-1"] = &clientcmdapi.Cluster{Server: "https://prod-eu-1.example.com"} + newCfg2.Contexts["prod-eu-1"] = &clientcmdapi.Context{Cluster: "cloudctl:prod-eu-1", AuthInfo: "cloudctl:auth-abc"} + + diff := diffKubeconfig(oldCfg, newCfg2) + accesses := buildAccessDiffs(diff, oldCfg, newCfg2) + + g.Expect(accesses).To(HaveLen(1)) + g.Expect(accesses[0].Name).To(Equal("prod-eu-1")) + g.Expect(accesses[0].ChangeType).To(Equal("added")) + g.Expect(accesses[0].Server).To(Equal("https://prod-eu-1.example.com")) +} + +func TestBuildAccessDiffs_Removed(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Clusters["cloudctl:staging"] = &clientcmdapi.Cluster{Server: "https://staging.example.com"} + oldCfg.Contexts["staging"] = &clientcmdapi.Context{Cluster: "cloudctl:staging", AuthInfo: "cloudctl:auth-abc"} + newCfg2 := newCfg() + + diff := diffKubeconfig(oldCfg, newCfg2) + accesses := buildAccessDiffs(diff, oldCfg, newCfg2) + + g.Expect(accesses).To(HaveLen(1)) + g.Expect(accesses[0].Name).To(Equal("staging")) + g.Expect(accesses[0].ChangeType).To(Equal("removed")) + g.Expect(accesses[0].Server).To(Equal("https://staging.example.com")) +} + +func TestBuildAccessDiffs_ModifiedServer(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + oldCfg := newCfg() + oldCfg.Clusters["cloudctl:prod-eu-2"] = &clientcmdapi.Cluster{Server: "https://old.example.com"} + oldCfg.Contexts["prod-eu-2"] = &clientcmdapi.Context{Cluster: "cloudctl:prod-eu-2", AuthInfo: "cloudctl:auth-abc"} + oldCfg.AuthInfos["cloudctl:auth-abc"] = &clientcmdapi.AuthInfo{ + Exec: &clientcmdapi.ExecConfig{Command: "kubelogin", Args: []string{"get-token"}}, + } + + newCfg2 := newCfg() + newCfg2.Clusters["cloudctl:prod-eu-2"] = &clientcmdapi.Cluster{Server: "https://new.example.com"} + newCfg2.Contexts["prod-eu-2"] = &clientcmdapi.Context{Cluster: "cloudctl:prod-eu-2", AuthInfo: "cloudctl:auth-abc"} + newCfg2.AuthInfos["cloudctl:auth-abc"] = &clientcmdapi.AuthInfo{ + Exec: &clientcmdapi.ExecConfig{Command: "kubelogin", Args: []string{"get-token"}}, + } + + diff := diffKubeconfig(oldCfg, newCfg2) + accesses := buildAccessDiffs(diff, oldCfg, newCfg2) + + g.Expect(accesses).To(HaveLen(1)) + g.Expect(accesses[0].Name).To(Equal("prod-eu-2")) + g.Expect(accesses[0].ChangeType).To(Equal("modified")) + g.Expect(accesses[0].Fields).To(ContainElement( + And(HaveField("Field", "Server"), HaveField("Old", "https://old.example.com"), HaveField("New", "https://new.example.com")), + )) +} + +func TestBuildAccessDiffs_NoChanges(t *testing.T) { + g := NewWithT(t) + orig := prefix + prefix = "cloudctl" + t.Cleanup(func() { prefix = orig }) + + cfg := newCfg() + cfg.Clusters["cloudctl:unchanged"] = &clientcmdapi.Cluster{Server: "https://same.example.com"} + cfg.Contexts["unchanged"] = &clientcmdapi.Context{Cluster: "cloudctl:unchanged", AuthInfo: "cloudctl:auth-abc"} + + diff := diffKubeconfig(cfg, cfg) + accesses := buildAccessDiffs(diff, cfg, cfg) + + g.Expect(accesses).To(BeEmpty()) +} From 524582ad341474383684d5f167d55ed3352efcb3 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 12:25:44 +0200 Subject: [PATCH 03/16] =?UTF-8?q?fix(sync):=20address=20Copilot=20review?= =?UTF-8?q?=20=E2=80=94=20security,=20correctness,=20test=20isolation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Redact sensitive values in dry-run diff output: - auth-provider config keys in sensitiveAuthProviderKeys (client-secret) are replaced with in FieldDiff Old/New - exec args matching sensitiveArgPrefixes (--oidc-client-secret=) are replaced with --oidc-client-secret= via redactArg() - Forward all cluster field changes (CA, Labels, Server) to access diffs in buildAccessDiffs step 2, not just Server - Synthesise access-level "Credentials: changed" entries in step 3 for contexts whose managed authinfo changed but whose context refs did not, preventing false "No changes detected" reports on credential-only syncs - Make equalExecEnv order-independent using a frequency map so env var reordering does not cause spurious authinfo churn - Fix test isolation: capture original mergeIdenticalUsers value before mutation and restore it in t.Cleanup (was always resetting to true) Signed-off-by: onuryilmaz --- cmd/authinfo.go | 12 +++++-- cmd/kubeconfigdiff.go | 73 +++++++++++++++++++++++++++++++++++++------ cmd/sync_test.go | 9 ++++-- 3 files changed, 78 insertions(+), 16 deletions(-) diff --git a/cmd/authinfo.go b/cmd/authinfo.go index 74222b3..b6c9cb9 100644 --- a/cmd/authinfo.go +++ b/cmd/authinfo.go @@ -69,13 +69,19 @@ func authInfoEqual(a, b *clientcmdapi.AuthInfo) bool { return true } -// equalExecEnv compares two ExecEnvVar slices for equality. +// equalExecEnv compares two ExecEnvVar slices for equality, independent of ordering. func equalExecEnv(a, b []clientcmdapi.ExecEnvVar) bool { if len(a) != len(b) { return false } - for i := range a { - if a[i].Name != b[i].Name || a[i].Value != b[i].Value { + // Build a frequency map so order differences are not treated as changes. + counts := make(map[string]int, len(a)) + for _, e := range a { + counts[e.Name+"="+e.Value]++ + } + for _, e := range b { + counts[e.Name+"="+e.Value]-- + if counts[e.Name+"="+e.Value] < 0 { return false } } diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index 0dda98e..d4b4a17 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -9,12 +9,25 @@ import ( "encoding/hex" "fmt" "sort" + "strings" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "github.com/cloudoperators/cloudctl/cmd/output" ) +// sensitiveAuthProviderKeys are auth-provider config keys whose values must +// not appear in plain or structured diff output. +var sensitiveAuthProviderKeys = map[string]bool{ + "client-secret": true, +} + +// sensitiveArgPrefixes are kubelogin (and similar) flag prefixes whose values +// must not appear verbatim in diff output. +var sensitiveArgPrefixes = []string{ + "--oidc-client-secret=", +} + // DiffChangeType describes the kind of change detected for a kubeconfig entry. type DiffChangeType string @@ -203,6 +216,9 @@ func diffAuthInfos(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { ov := oldFiltered[k] nv := newFiltered[k] if ov != nv { + if sensitiveAuthProviderKeys[k] { + ov, nv = "", "" + } fields = append(fields, FieldDiff{Field: fmt.Sprintf("auth-provider.%s", k), Old: ov, New: nv}) } } @@ -228,8 +244,20 @@ func diffAuthInfos(oldCfg, newCfg *clientcmdapi.Config) []EntryDiff { return diffs } +// redactArg replaces the value portion of a sensitive flag with , +// leaving the flag name intact for readability (e.g. "--oidc-client-secret="). +func redactArg(arg string) string { + for _, pfx := range sensitiveArgPrefixes { + if strings.HasPrefix(arg, pfx) { + return pfx + "" + } + } + return arg +} + // argsDiff computes per-argument differences between two exec arg lists, returning // FieldDiff entries for args that appear only in old (removed) or only in new (added). +// Values of sensitive flags (e.g. --oidc-client-secret=) are redacted. func argsDiff(oldArgs, newArgs []string) []FieldDiff { oldSet := make(map[string]bool, len(oldArgs)) for _, a := range oldArgs { @@ -254,10 +282,10 @@ func argsDiff(oldArgs, newArgs []string) []FieldDiff { var diffs []FieldDiff for _, r := range removed { - diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: r, New: ""}) + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: redactArg(r), New: ""}) } for _, a := range added { - diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: "", New: a}) + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: "", New: redactArg(a)}) } return diffs } @@ -363,7 +391,7 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) // 2. For each modified cluster, find contexts in newCfg that reference it. // If those contexts were not already captured via context-level diffs, add - // a "modified" access entry for the server URL change. + // a "modified" access entry reflecting the cluster field changes (Server, CA, Labels). modifiedClusters := make(map[string]EntryDiff, len(diff.Clusters)) for _, cd := range diff.Clusters { if cd.ChangeType == DiffChangeModified { @@ -381,13 +409,11 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } var fields []output.FieldChange for _, f := range clusterDiff.Fields { - if f.Field == "Server" { - fields = append(fields, output.FieldChange{ - Field: "Server", - Old: f.Old, - New: f.New, - }) - } + fields = append(fields, output.FieldChange{ + Field: f.Field, + Old: f.Old, + New: f.New, + }) } if len(fields) == 0 { continue @@ -402,6 +428,33 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } } + // 3. For each modified authinfo, find contexts in newCfg that reference it. + // If those contexts were not already captured above, emit a "modified" access + // entry with Credentials: changed so credential-only syncs are not silent. + modifiedAuthInfos := make(map[string]struct{}, len(diff.AuthInfos)) + for _, ad := range diff.AuthInfos { + if ad.ChangeType == DiffChangeModified { + modifiedAuthInfos[ad.Name] = struct{}{} + } + } + if len(modifiedAuthInfos) > 0 { + for ctxName, ctx := range newCfg.Contexts { + if _, alreadyHandled := byName[ctxName]; alreadyHandled { + continue + } + if _, affected := modifiedAuthInfos[ctx.AuthInfo]; !affected { + continue + } + byName[ctxName] = &accessEntry{ + access: output.AccessDiff{ + Name: ctxName, + ChangeType: string(DiffChangeModified), + Fields: []output.FieldChange{{Field: "Credentials", Old: "changed", New: ""}}, + }, + } + } + } + // Flatten and sort accesses := make([]output.AccessDiff, 0, len(byName)) for _, e := range byName { diff --git a/cmd/sync_test.go b/cmd/sync_test.go index a052300..eeafc5c 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -565,11 +565,12 @@ func TestMergeKubeconfig_PrefersExistingLocalAuthName(t *testing.T) { g := NewWithT(t) orig := prefix + origMerge := mergeIdenticalUsers prefix = "cloudctl" mergeIdenticalUsers = true t.Cleanup(func() { prefix = orig - mergeIdenticalUsers = true + mergeIdenticalUsers = origMerge }) // The local kubeconfig has an unmanaged user entry with the same OIDC creds @@ -623,11 +624,12 @@ func TestMergeKubeconfig_DeduplicatesSameOIDCUsers(t *testing.T) { g := NewWithT(t) orig := prefix + origMerge := mergeIdenticalUsers prefix = "cloudctl" mergeIdenticalUsers = true t.Cleanup(func() { prefix = orig - mergeIdenticalUsers = true + mergeIdenticalUsers = origMerge }) // Two clusters with the same OIDC config should share one auth entry @@ -848,11 +850,12 @@ func TestRunSync_DryRun_NoWrite(t *testing.T) { g := NewWithT(t) orig := prefix + origMerge := mergeIdenticalUsers prefix = "cloudctl" mergeIdenticalUsers = true t.Cleanup(func() { prefix = orig - mergeIdenticalUsers = true + mergeIdenticalUsers = origMerge }) // Build a "before" and "after" config to simulate what dry-run does From eba6ce60681d12600ac419ad02cf5ded942e2c2e Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 12:39:27 +0200 Subject: [PATCH 04/16] fix(output): table layout for dry-run modified entries with change summary Replace the per-entry indented field listing with a compact two-column table (NAME | CHANGES) for modified cluster accesses. The CHANGES column shows a single word or comma-separated list (credentials, server, ca, labels, config) so 174 modified entries are scannable at a glance. The summary line now includes a per-change-type breakdown: Summary: 0 added, 0 removed, 174 modified (45 credentials, 129 config). Added/removed entries retain their original single-line format with the server URL inline. Added accessChangeSummary and modifiedBreakdown helpers (shared by both plain and interactive printers). Signed-off-by: onuryilmaz --- cmd/output/interactive_printer.go | 68 ++++++++++++----- cmd/output/output_test.go | 3 +- cmd/output/plain_printer.go | 119 +++++++++++++++++++++++++----- 3 files changed, 149 insertions(+), 41 deletions(-) diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index c54e436..030ce75 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -6,6 +6,7 @@ package output import ( "fmt" "io" + "strings" "sync" "time" @@ -220,34 +221,63 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { w("%s\n\n", styleFaint.Render("Dry-run: no changes will be written.")) w("%s (%d change(s))\n", styleHeader.Render("CLUSTER ACCESSES"), total) + // Determine column width for the name column. + nameWidth := len("NAME") for _, a := range r.Accesses { - switch a.ChangeType { - case "added": - w(" %s %-20s %s\n", styleGreen.Render("+"), a.Name, styleFaint.Render(a.Server)) - case "removed": - w(" %s %-20s %s\n", styleRed.Render("-"), a.Name, styleFaint.Render("(removed)")) - case "modified": - w(" %s %s\n", styleYellow.Render("~"), a.Name) - for _, f := range a.Fields { - if f.Field == "Credentials" { - w(" %-16s %s\n", "Credentials:", styleYellow.Render("changed")) - } else { - if f.Old != "" { - w(" %-16s %s\n", f.Field+":", styleRed.Render("- "+f.Old)) - } - if f.New != "" { - w(" %-16s %s\n", f.Field+":", styleGreen.Render("+ "+f.New)) - } - } + if len(a.Name) > nameWidth { + nameWidth = len(a.Name) + } + } + nameWidth += 2 + + // Print added/removed entries with server URL inline. + hasAddedOrRemoved := r.Added > 0 || r.Removed > 0 + if hasAddedOrRemoved { + w(" %-*s %s\n", nameWidth, styleBold.Render("NAME"), styleBold.Render("SERVER")) + for _, a := range r.Accesses { + switch a.ChangeType { + case "added": + w(" %s %-*s %s\n", styleGreen.Render("+"), nameWidth, a.Name, styleFaint.Render(a.Server)) + case "removed": + w(" %s %-*s %s\n", styleRed.Render("-"), nameWidth, a.Name, styleFaint.Render("(removed)")) } } } + // Print modified entries as a table with a CHANGES column. + if r.Modified > 0 { + if hasAddedOrRemoved { + w("\n") + } + w(" %-*s %s\n", nameWidth, styleBold.Render("NAME"), styleBold.Render("CHANGES")) + for _, a := range r.Accesses { + if a.ChangeType != "modified" { + continue + } + summary := accessChangeSummary(a) + var styledSummary string + switch summary { + case "credentials": + styledSummary = styleYellow.Render(summary) + case "server", "ca": + styledSummary = styleRed.Render(summary) + default: + styledSummary = styleFaint.Render(summary) + } + w(" %s %-*s %s\n", styleYellow.Render("~"), nameWidth, a.Name, styledSummary) + } + } + w("\n") + modBreakdown := modifiedBreakdown(r.Accesses) + modSuffix := "" + if r.Modified > 0 && len(modBreakdown) > 0 { + modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" + } summaryParts := []string{ styleGreen.Render(fmt.Sprintf("%d added", r.Added)), styleRed.Render(fmt.Sprintf("%d removed", r.Removed)), - styleYellow.Render(fmt.Sprintf("%d modified", r.Modified)), + styleYellow.Render(fmt.Sprintf("%d modified%s", r.Modified, modSuffix)), } w("Summary: %s, %s, %s. %s\n", summaryParts[0], summaryParts[1], summaryParts[2], diff --git a/cmd/output/output_test.go b/cmd/output/output_test.go index bad09aa..1393bf4 100644 --- a/cmd/output/output_test.go +++ b/cmd/output/output_test.go @@ -243,8 +243,7 @@ func TestPlainPrinter_SyncDryRunResult_Changes(t *testing.T) { g.Expect(out).To(ContainSubstring("https://prod-eu-1.example.com")) g.Expect(out).To(ContainSubstring("- staging-de")) g.Expect(out).To(ContainSubstring("~ prod-eu-2")) - g.Expect(out).To(ContainSubstring("https://old.example.com")) - g.Expect(out).To(ContainSubstring("https://new.example.com")) + g.Expect(out).To(ContainSubstring("server")) // change summary column g.Expect(out).To(ContainSubstring("Summary:")) g.Expect(out).To(ContainSubstring("1 added")) g.Expect(out).To(ContainSubstring("1 removed")) diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index e083260..127b968 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -6,6 +6,8 @@ package output import ( "fmt" "io" + "sort" + "strings" ) type plainPrinter struct { @@ -74,30 +76,52 @@ func (p *plainPrinter) Print(v any) error { } w("Dry-run: no changes will be written.\n\n") w("CLUSTER ACCESSES (%d change(s))\n", total) + + // Determine column width for the name column. + nameWidth := len("NAME") for _, a := range t.Accesses { - switch a.ChangeType { - case "added": - w(" + %-20s %s\n", a.Name, a.Server) - case "removed": - w(" - %-20s (removed)\n", a.Name) - case "modified": - w(" ~ %s\n", a.Name) - for _, f := range a.Fields { - if f.Field == "Credentials" { - w(" %-14s changed\n", "Credentials:") - } else { - if f.Old != "" { - w(" %-14s - %s\n", f.Field+":", f.Old) - } - if f.New != "" { - w(" %-14s + %s\n", f.Field+":", f.New) - } - } + if len(a.Name) > nameWidth { + nameWidth = len(a.Name) + } + } + nameWidth += 2 + + // Print added/removed entries with server URL inline. + hasAddedOrRemoved := t.Added > 0 || t.Removed > 0 + if hasAddedOrRemoved { + w(" %-*s %s\n", nameWidth, "NAME", "SERVER") + for _, a := range t.Accesses { + switch a.ChangeType { + case "added": + w(" + %-*s %s\n", nameWidth, a.Name, a.Server) + case "removed": + w(" - %-*s (removed)\n", nameWidth, a.Name) } } } - w("\nSummary: %d added, %d removed, %d modified. No changes will be written.\n", - t.Added, t.Removed, t.Modified) + + // Print modified entries as a table with a CHANGES column. + if t.Modified > 0 { + if hasAddedOrRemoved { + w("\n") + } + w(" %-*s %s\n", nameWidth, "NAME", "CHANGES") + for _, a := range t.Accesses { + if a.ChangeType != "modified" { + continue + } + w(" ~ %-*s %s\n", nameWidth, a.Name, accessChangeSummary(a)) + } + } + + // Build summary with per-change-type breakdown for modified entries. + modBreakdown := modifiedBreakdown(t.Accesses) + modSuffix := "" + if t.Modified > 0 && len(modBreakdown) > 0 { + modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" + } + w("\nSummary: %d added, %d removed, %d modified%s. No changes will be written.\n", + t.Added, t.Removed, t.Modified, modSuffix) case ClusterVersionResult: w("Kubernetes version: %s\n", t.Version) @@ -121,3 +145,58 @@ func (p *plainPrinter) PrintError(err error) { func (p *plainPrinter) StartSpinner(_ string) func() { return func() {} } + +// accessChangeSummary returns a compact, human-readable string describing what +// changed in a modified AccessDiff entry (e.g. "credentials", "server", "config"). +func accessChangeSummary(a AccessDiff) string { + if len(a.Fields) == 0 { + return "config" + } + seen := make(map[string]struct{}, len(a.Fields)) + for _, f := range a.Fields { + switch f.Field { + case "Credentials": + seen["credentials"] = struct{}{} + case "Server": + seen["server"] = struct{}{} + case "CA": + seen["ca"] = struct{}{} + case "Labels": + seen["labels"] = struct{}{} + default: + seen["config"] = struct{}{} + } + } + parts := make([]string, 0, len(seen)) + for k := range seen { + parts = append(parts, k) + } + sort.Strings(parts) + return strings.Join(parts, ", ") +} + +// modifiedBreakdown counts distinct change reasons across all modified accesses +// and returns them as sorted "N reason" strings (e.g. ["45 credentials", "129 config"]). +func modifiedBreakdown(accesses []AccessDiff) []string { + counts := make(map[string]int) + for _, a := range accesses { + if a.ChangeType != "modified" { + continue + } + counts[accessChangeSummary(a)]++ + } + if len(counts) == 0 { + return nil + } + // Collect keys and sort for deterministic output. + keys := make([]string, 0, len(counts)) + for k := range counts { + keys = append(keys, k) + } + sort.Strings(keys) + parts := make([]string, 0, len(keys)) + for _, k := range keys { + parts = append(parts, fmt.Sprintf("%d %s", counts[k], k)) + } + return parts +} From 4f2d2f90c484ade1405db2e88929e64e086c8586 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 12:52:20 +0200 Subject: [PATCH 05/16] =?UTF-8?q?fix(output):=20show=20old=20=E2=86=92=20n?= =?UTF-8?q?ew=20field=20values=20for=20structural=20changes=20in=20dry-run?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Entries with server/CA/label changes now expand with indented detail lines: ~ eu-de-3 server: https://old.k8s.example.com → https://new.k8s.example.com ca: CN=old expires=2025-01-01 → CN=new expires=2026-01-01 Credential-only entries remain as compact single-line table rows. Approach mirrors the compare_crd_vs_registry hack tool's field diff output, collapsed onto one line with an arrow instead of two separate label rows. Signed-off-by: onuryilmaz --- cmd/output/interactive_printer.go | 38 ++++++++++++++++++++++--------- cmd/output/plain_printer.go | 37 ++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index 030ce75..8af3e73 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -244,7 +244,9 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { } } - // Print modified entries as a table with a CHANGES column. + // Print modified entries. Entries with only credential changes are + // shown as compact single-line table rows. Entries with field-level + // values (server, ca, etc.) expand with old → new detail lines. if r.Modified > 0 { if hasAddedOrRemoved { w("\n") @@ -254,17 +256,31 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { if a.ChangeType != "modified" { continue } - summary := accessChangeSummary(a) - var styledSummary string - switch summary { - case "credentials": - styledSummary = styleYellow.Render(summary) - case "server", "ca": - styledSummary = styleRed.Render(summary) - default: - styledSummary = styleFaint.Render(summary) + if hasDetailFields(a) { + w(" %s %s\n", styleYellow.Render("~"), a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" %-14s %s\n", "credentials:", styleYellow.Render("changed")) + } else if f.Old != "" || f.New != "" { + label := strings.ToLower(f.Field) + ":" + w(" %-14s %s → %s\n", label, + styleRed.Render(orElse(f.Old, "(none)")), + styleGreen.Render(orElse(f.New, "(none)"))) + } + } + } else { + summary := accessChangeSummary(a) + var styledSummary string + switch summary { + case "credentials": + styledSummary = styleYellow.Render(summary) + case "server", "ca": + styledSummary = styleRed.Render(summary) + default: + styledSummary = styleFaint.Render(summary) + } + w(" %s %-*s %s\n", styleYellow.Render("~"), nameWidth, a.Name, styledSummary) } - w(" %s %-*s %s\n", styleYellow.Render("~"), nameWidth, a.Name, styledSummary) } } diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index 127b968..b0045e7 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -100,7 +100,9 @@ func (p *plainPrinter) Print(v any) error { } } - // Print modified entries as a table with a CHANGES column. + // Print modified entries. Entries with only credential changes are + // shown as compact single-line table rows. Entries with field-level + // values (server, ca, etc.) expand with old → new detail lines. if t.Modified > 0 { if hasAddedOrRemoved { w("\n") @@ -110,7 +112,19 @@ func (p *plainPrinter) Print(v any) error { if a.ChangeType != "modified" { continue } - w(" ~ %-*s %s\n", nameWidth, a.Name, accessChangeSummary(a)) + if hasDetailFields(a) { + w(" ~ %s\n", a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" %-14s changed\n", "credentials:") + } else if f.Old != "" || f.New != "" { + label := strings.ToLower(f.Field) + ":" + w(" %-14s %s → %s\n", label, orElse(f.Old, "(none)"), orElse(f.New, "(none)")) + } + } + } else { + w(" ~ %-*s %s\n", nameWidth, a.Name, accessChangeSummary(a)) + } } } @@ -146,6 +160,25 @@ func (p *plainPrinter) StartSpinner(_ string) func() { return func() {} } +// hasDetailFields returns true when the access diff contains at least one +// non-Credentials field that has old/new values worth printing inline. +func hasDetailFields(a AccessDiff) bool { + for _, f := range a.Fields { + if f.Field != "Credentials" && (f.Old != "" || f.New != "") { + return true + } + } + return false +} + +// orElse returns s if non-empty, otherwise fallback. +func orElse(s, fallback string) string { + if s != "" { + return s + } + return fallback +} + // accessChangeSummary returns a compact, human-readable string describing what // changed in a modified AccessDiff entry (e.g. "credentials", "server", "config"). func accessChangeSummary(a AccessDiff) string { From 2d3cb0c4ed68edc418d64f1c0f7b4e209cb0e8a5 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 13:33:51 +0200 Subject: [PATCH 06/16] feat(sync): add --dry-run-format=table|diff flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a --dry-run-format flag (default: table) to control how dry-run output is rendered: table (default): compact NAME | CHANGES table, identical to previous behaviour — credential-only rows are single-line, structural changes (server, ca) expand with old → new inline. diff: git-style output showing each changed field as separate - old / + new lines, matching the style of compare_crd_vs_registry. cloudctl sync -n sci --dry-run --dry-run-format=diff The format is carried on SyncDryRunResult.Format (json:"-" yaml:"-") so JSON/YAML structured output is unaffected. Signed-off-by: onuryilmaz --- cmd/output/interactive_printer.go | 57 +++++++++-- cmd/output/plain_printer.go | 161 +++++++++++++++++++----------- cmd/output/types.go | 3 + cmd/sync.go | 7 +- 4 files changed, 161 insertions(+), 67 deletions(-) diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index 8af3e73..c995112 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -221,7 +221,16 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { w("%s\n\n", styleFaint.Render("Dry-run: no changes will be written.")) w("%s (%d change(s))\n", styleHeader.Render("CLUSTER ACCESSES"), total) - // Determine column width for the name column. + if r.Format == "diff" { + p.printDryRunDiff(w, r) + } else { + p.printDryRunTable(w, r) + } + + return writeErr +} + +func (p *interactivePrinter) printDryRunTable(w func(string, ...any), r SyncDryRunResult) { nameWidth := len("NAME") for _, a := range r.Accesses { if len(a.Name) > nameWidth { @@ -230,7 +239,6 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { } nameWidth += 2 - // Print added/removed entries with server URL inline. hasAddedOrRemoved := r.Added > 0 || r.Removed > 0 if hasAddedOrRemoved { w(" %-*s %s\n", nameWidth, styleBold.Render("NAME"), styleBold.Render("SERVER")) @@ -244,9 +252,6 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { } } - // Print modified entries. Entries with only credential changes are - // shown as compact single-line table rows. Entries with field-level - // values (server, ca, etc.) expand with old → new detail lines. if r.Modified > 0 { if hasAddedOrRemoved { w("\n") @@ -284,12 +289,50 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { } } - w("\n") + p.printDryRunSummary(w, r) +} + +func (p *interactivePrinter) printDryRunDiff(w func(string, ...any), r SyncDryRunResult) { + for _, a := range r.Accesses { + switch a.ChangeType { + case "added": + w("%s %s\n", styleGreen.Render("+"), a.Name) + if a.Server != "" { + w(" %s %-12s %s\n", styleGreen.Render("+"), "server:", a.Server) + } + case "removed": + w("%s %s\n", styleRed.Render("-"), a.Name) + if a.Server != "" { + w(" %s %-12s %s\n", styleRed.Render("-"), "server:", a.Server) + } + case "modified": + w("%s %s\n", styleYellow.Render("~"), a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" %s %-12s %s\n", styleYellow.Render("~"), "credentials:", styleYellow.Render("changed")) + } else { + label := strings.ToLower(f.Field) + ":" + if f.Old != "" { + w(" %s %-12s %s\n", styleRed.Render("-"), label, styleRed.Render(f.Old)) + } + if f.New != "" { + w(" %s %-12s %s\n", styleGreen.Render("+"), label, styleGreen.Render(f.New)) + } + } + } + } + } + + p.printDryRunSummary(w, r) +} + +func (p *interactivePrinter) printDryRunSummary(w func(string, ...any), r SyncDryRunResult) { modBreakdown := modifiedBreakdown(r.Accesses) modSuffix := "" if r.Modified > 0 && len(modBreakdown) > 0 { modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" } + w("\n") summaryParts := []string{ styleGreen.Render(fmt.Sprintf("%d added", r.Added)), styleRed.Render(fmt.Sprintf("%d removed", r.Removed)), @@ -298,6 +341,4 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { w("Summary: %s, %s, %s. %s\n", summaryParts[0], summaryParts[1], summaryParts[2], styleFaint.Render("No changes will be written.")) - - return writeErr } diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index b0045e7..9cf744e 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -77,65 +77,11 @@ func (p *plainPrinter) Print(v any) error { w("Dry-run: no changes will be written.\n\n") w("CLUSTER ACCESSES (%d change(s))\n", total) - // Determine column width for the name column. - nameWidth := len("NAME") - for _, a := range t.Accesses { - if len(a.Name) > nameWidth { - nameWidth = len(a.Name) - } - } - nameWidth += 2 - - // Print added/removed entries with server URL inline. - hasAddedOrRemoved := t.Added > 0 || t.Removed > 0 - if hasAddedOrRemoved { - w(" %-*s %s\n", nameWidth, "NAME", "SERVER") - for _, a := range t.Accesses { - switch a.ChangeType { - case "added": - w(" + %-*s %s\n", nameWidth, a.Name, a.Server) - case "removed": - w(" - %-*s (removed)\n", nameWidth, a.Name) - } - } - } - - // Print modified entries. Entries with only credential changes are - // shown as compact single-line table rows. Entries with field-level - // values (server, ca, etc.) expand with old → new detail lines. - if t.Modified > 0 { - if hasAddedOrRemoved { - w("\n") - } - w(" %-*s %s\n", nameWidth, "NAME", "CHANGES") - for _, a := range t.Accesses { - if a.ChangeType != "modified" { - continue - } - if hasDetailFields(a) { - w(" ~ %s\n", a.Name) - for _, f := range a.Fields { - if f.Field == "Credentials" { - w(" %-14s changed\n", "credentials:") - } else if f.Old != "" || f.New != "" { - label := strings.ToLower(f.Field) + ":" - w(" %-14s %s → %s\n", label, orElse(f.Old, "(none)"), orElse(f.New, "(none)")) - } - } - } else { - w(" ~ %-*s %s\n", nameWidth, a.Name, accessChangeSummary(a)) - } - } - } - - // Build summary with per-change-type breakdown for modified entries. - modBreakdown := modifiedBreakdown(t.Accesses) - modSuffix := "" - if t.Modified > 0 && len(modBreakdown) > 0 { - modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" + if t.Format == "diff" { + p.printDryRunDiff(w, t) + } else { + p.printDryRunTable(w, t) } - w("\nSummary: %d added, %d removed, %d modified%s. No changes will be written.\n", - t.Added, t.Removed, t.Modified, modSuffix) case ClusterVersionResult: w("Kubernetes version: %s\n", t.Version) @@ -160,6 +106,105 @@ func (p *plainPrinter) StartSpinner(_ string) func() { return func() {} } +// printDryRunTable renders dry-run output as a compact NAME | CHANGES table. +func (p *plainPrinter) printDryRunTable(w func(string, ...any), t SyncDryRunResult) { + nameWidth := len("NAME") + for _, a := range t.Accesses { + if len(a.Name) > nameWidth { + nameWidth = len(a.Name) + } + } + nameWidth += 2 + + hasAddedOrRemoved := t.Added > 0 || t.Removed > 0 + if hasAddedOrRemoved { + w(" %-*s %s\n", nameWidth, "NAME", "SERVER") + for _, a := range t.Accesses { + switch a.ChangeType { + case "added": + w(" + %-*s %s\n", nameWidth, a.Name, a.Server) + case "removed": + w(" - %-*s (removed)\n", nameWidth, a.Name) + } + } + } + + if t.Modified > 0 { + if hasAddedOrRemoved { + w("\n") + } + w(" %-*s %s\n", nameWidth, "NAME", "CHANGES") + for _, a := range t.Accesses { + if a.ChangeType != "modified" { + continue + } + if hasDetailFields(a) { + w(" ~ %s\n", a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" %-14s changed\n", "credentials:") + } else if f.Old != "" || f.New != "" { + label := strings.ToLower(f.Field) + ":" + w(" %-14s %s → %s\n", label, orElse(f.Old, "(none)"), orElse(f.New, "(none)")) + } + } + } else { + w(" ~ %-*s %s\n", nameWidth, a.Name, accessChangeSummary(a)) + } + } + } + + modBreakdown := modifiedBreakdown(t.Accesses) + modSuffix := "" + if t.Modified > 0 && len(modBreakdown) > 0 { + modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" + } + w("\nSummary: %d added, %d removed, %d modified%s. No changes will be written.\n", + t.Added, t.Removed, t.Modified, modSuffix) +} + +// printDryRunDiff renders dry-run output in git-style unified diff format: +// each changed field is shown as a - (old) and + (new) line. +func (p *plainPrinter) printDryRunDiff(w func(string, ...any), t SyncDryRunResult) { + for _, a := range t.Accesses { + switch a.ChangeType { + case "added": + w("+ %s\n", a.Name) + if a.Server != "" { + w(" + server: %s\n", a.Server) + } + case "removed": + w("- %s\n", a.Name) + if a.Server != "" { + w(" - server: %s\n", a.Server) + } + case "modified": + w("~ %s\n", a.Name) + for _, f := range a.Fields { + if f.Field == "Credentials" { + w(" ~ credentials: changed\n") + } else { + label := strings.ToLower(f.Field) + ":" + if f.Old != "" { + w(" - %-12s %s\n", label, f.Old) + } + if f.New != "" { + w(" + %-12s %s\n", label, f.New) + } + } + } + } + } + + modBreakdown := modifiedBreakdown(t.Accesses) + modSuffix := "" + if t.Modified > 0 && len(modBreakdown) > 0 { + modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" + } + w("\nSummary: %d added, %d removed, %d modified%s. No changes will be written.\n", + t.Added, t.Removed, t.Modified, modSuffix) +} + // hasDetailFields returns true when the access diff contains at least one // non-Credentials field that has old/new values worth printing inline. func hasDetailFields(a AccessDiff) bool { diff --git a/cmd/output/types.go b/cmd/output/types.go index 11427af..ba6d968 100644 --- a/cmd/output/types.go +++ b/cmd/output/types.go @@ -68,6 +68,9 @@ type SyncDryRunResult struct { Added int `json:"added" yaml:"added"` Removed int `json:"removed" yaml:"removed"` Modified int `json:"modified" yaml:"modified"` + // Format controls how plain/interactive printers render this result. + // Accepted values: "table" (default), "diff". Ignored by JSON/YAML output. + Format string `json:"-" yaml:"-"` } // DiffEntry describes a single added, removed, or modified kubeconfig entry. diff --git a/cmd/sync.go b/cmd/sync.go index 873abc9..6df13dd 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -41,6 +41,7 @@ var ( kubeloginExtraArgs []string kubeloginTokenCacheDir string dryRun bool + dryRunFormat string ) func init() { @@ -73,6 +74,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().StringVar(&dryRunFormat, "dry-run-format", "table", "Dry-run display style: table (default) or diff (git-style old/new lines)") // BindPFlags can theroretically return an error if called with `nil` as an argument // which should never happened after at least one flag was defined. That's why the output @@ -125,6 +127,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") + dryRunFormat = viper.GetString("dry-run-format") format, err := output.ParseFormat(viper.GetString("output")) if err != nil { @@ -233,7 +236,9 @@ func runSync(cmd *cobra.Command, args []string) error { if dryRun { diff := diffKubeconfig(localConfigBefore, localConfig) - return printer.Print(buildDryRunResult(diff, localConfigBefore, localConfig)) + result := buildDryRunResult(diff, localConfigBefore, localConfig) + result.Format = dryRunFormat + return printer.Print(result) } if writeErr := writeConfig(localConfig, remoteClusterKubeconfig); writeErr != nil { From 4488e574166b4f4b086212aab36036fff1eaccfa Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 13:46:01 +0200 Subject: [PATCH 07/16] fix(sync): suppress no-op context diffs from authinfo hash reassignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When mergeIdenticalUsers is enabled, authinfo deduplication can change a context's AuthInfo pointer from one cloudctl:auth- name to another while the actual credentials are identical. diffContexts flags this as a modified context, and buildAccessDiffs step 1 finds no user-visible field changes (authInfoEqual returns true for the same underlying auth). Previously these were emitted as "config" modified entries with no detail — pure noise. Now they are silently dropped from the access diff so only genuine changes (server URL, CA, actual credential changes) are reported. Signed-off-by: onuryilmaz --- cmd/kubeconfigdiff.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index d4b4a17..28b5fea 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -386,6 +386,12 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } } } + // Skip modified entries that have no user-visible field changes — + // these arise from internal authinfo hash reassignment during + // deduplication where the effective credentials are unchanged. + if ctxDiff.ChangeType == DiffChangeModified && len(entry.access.Fields) == 0 { + continue + } byName[name] = entry } From 8d84ceafe992205f17e55f855468d81cf07423df Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 14:04:43 +0200 Subject: [PATCH 08/16] feat(sync): show actual field diffs for credential changes in dry-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of the generic "Credentials: changed" sentinel, compare exec args and auth-provider config directly when building access diffs so both table and diff formats can display real old → new values per field. Also carry field diffs through step 3 (authinfo-centric path) so contexts updated via authinfo changes also show specific field changes. Update accessChangeSummary to map Exec Args, Auth type, and auth-provider.* fields to the "credentials" category (was incorrectly falling through to "config"). Signed-off-by: onuryilmaz --- cmd/kubeconfigdiff.go | 132 +++++++++++++++++++++++++++++------- cmd/output/plain_printer.go | 17 +++-- 2 files changed, 117 insertions(+), 32 deletions(-) diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index 28b5fea..5d45d62 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -360,30 +360,79 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) case DiffChangeModified: oldCtx := oldCfg.Contexts[name] newCtx := newCfg.Contexts[name] - if oldCtx != nil && newCtx != nil { - oldCluster := oldCfg.Clusters[oldCtx.Cluster] - newCluster := newCfg.Clusters[newCtx.Cluster] - oldServer, newServer := "", "" - if oldCluster != nil { - oldServer = oldCluster.Server - } - if newCluster != nil { - newServer = newCluster.Server + if oldCtx == nil || newCtx == nil { + break + } + // Server URL change (resolve through cluster refs) + oldCluster := oldCfg.Clusters[oldCtx.Cluster] + newCluster := newCfg.Clusters[newCtx.Cluster] + oldServer, newServer := "", "" + if oldCluster != nil { + oldServer = oldCluster.Server + } + if newCluster != nil { + newServer = newCluster.Server + } + if oldServer != newServer { + entry.access.Fields = append(entry.access.Fields, output.FieldChange{ + Field: "Server", + Old: oldServer, + New: newServer, + }) + } + // Carry through any credential field diffs from the authinfo diff + // (exec args, auth-provider config). If the auth objects are simply + // absent or unequal with no specific fields, fall back to the + // generic sentinel. + oldAuth := oldCfg.AuthInfos[oldCtx.AuthInfo] + newAuth := newCfg.AuthInfos[newCtx.AuthInfo] + credChanged := (oldAuth != nil && newAuth != nil && !authInfoEqual(oldAuth, newAuth)) || + (oldAuth == nil) != (newAuth == nil) + if credChanged { + // Try to produce specific field-level diffs by comparing the + // auth objects directly, the same way diffAuthInfos does. + var authFields []output.FieldChange + if oldAuth != nil && newAuth != nil { + if newAuth.Exec != nil && oldAuth.Exec != nil { + for _, fd := range argsDiff(oldAuth.Exec.Args, newAuth.Exec.Args) { + authFields = append(authFields, output.FieldChange{ + Field: fd.Field, + Old: fd.Old, + New: fd.New, + }) + } + } else if newAuth.Exec != nil && oldAuth.Exec == nil { + authFields = append(authFields, output.FieldChange{Field: "Auth type", Old: "auth-provider", New: "exec-plugin"}) + } else if newAuth.Exec == nil && oldAuth.Exec != nil { + authFields = append(authFields, output.FieldChange{Field: "Auth type", Old: "exec-plugin", New: "auth-provider"}) + } + if newAuth.AuthProvider != nil && oldAuth.AuthProvider != nil { + oldFiltered := filterAuthProviderConfig(oldAuth.AuthProvider.Config) + newFiltered := filterAuthProviderConfig(newAuth.AuthProvider.Config) + for _, k := range sortedKeys(oldFiltered, newFiltered) { + ov, nv := oldFiltered[k], newFiltered[k] + if ov != nv { + if sensitiveAuthProviderKeys[k] { + ov, nv = "", "" + } + authFields = append(authFields, output.FieldChange{ + Field: fmt.Sprintf("auth-provider.%s", k), + Old: ov, + New: nv, + }) + } + } + } } - if oldServer != newServer { + if len(authFields) > 0 { + entry.access.Fields = append(entry.access.Fields, authFields...) + } else { entry.access.Fields = append(entry.access.Fields, output.FieldChange{ - Field: "Server", - Old: oldServer, - New: newServer, + Field: "Credentials", + Old: "changed", + New: "", }) } - // Check if credentials changed - oldAuth := oldCfg.AuthInfos[oldCtx.AuthInfo] - newAuth := newCfg.AuthInfos[newCtx.AuthInfo] - if (oldAuth != nil && newAuth != nil && !authInfoEqual(oldAuth, newAuth)) || - (oldAuth == nil) != (newAuth == nil) { - entry.access.Fields = append(entry.access.Fields, output.FieldChange{Field: "Credentials", Old: "changed", New: ""}) - } } } // Skip modified entries that have no user-visible field changes — @@ -435,12 +484,13 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } // 3. For each modified authinfo, find contexts in newCfg that reference it. - // If those contexts were not already captured above, emit a "modified" access - // entry with Credentials: changed so credential-only syncs are not silent. - modifiedAuthInfos := make(map[string]struct{}, len(diff.AuthInfos)) + // Carry the actual field diffs (exec args, auth-provider config) through so + // diff format can show real old/new values. Fall back to a generic + // "Credentials: changed" entry only when no specific fields were identified. + modifiedAuthInfos := make(map[string]EntryDiff, len(diff.AuthInfos)) for _, ad := range diff.AuthInfos { if ad.ChangeType == DiffChangeModified { - modifiedAuthInfos[ad.Name] = struct{}{} + modifiedAuthInfos[ad.Name] = ad } } if len(modifiedAuthInfos) > 0 { @@ -448,14 +498,27 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) if _, alreadyHandled := byName[ctxName]; alreadyHandled { continue } - if _, affected := modifiedAuthInfos[ctx.AuthInfo]; !affected { + ad, affected := modifiedAuthInfos[ctx.AuthInfo] + if !affected { continue } + var fields []output.FieldChange + for _, f := range ad.Fields { + fields = append(fields, output.FieldChange{ + Field: f.Field, + Old: f.Old, + New: f.New, + }) + } + if len(fields) == 0 { + // authInfoEqual returned false but no specific fields were identified. + fields = []output.FieldChange{{Field: "Credentials", Old: "changed", New: ""}} + } byName[ctxName] = &accessEntry{ access: output.AccessDiff{ Name: ctxName, ChangeType: string(DiffChangeModified), - Fields: []output.FieldChange{{Field: "Credentials", Old: "changed", New: ""}}, + Fields: fields, }, } } @@ -515,3 +578,20 @@ func buildDryRunResult(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) Modified: modified, } } + +// sortedKeys returns the union of keys from two string maps, sorted. +func sortedKeys(a, b map[string]string) []string { + seen := make(map[string]struct{}, len(a)+len(b)) + for k := range a { + seen[k] = struct{}{} + } + for k := range b { + seen[k] = struct{}{} + } + keys := make([]string, 0, len(seen)) + for k := range seen { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index 9cf744e..ada27eb 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -206,7 +206,8 @@ func (p *plainPrinter) printDryRunDiff(w func(string, ...any), t SyncDryRunResul } // hasDetailFields returns true when the access diff contains at least one -// non-Credentials field that has old/new values worth printing inline. +// field that has old/new values worth printing inline (excludes the synthetic +// Credentials sentinel which carries no real values). func hasDetailFields(a AccessDiff) bool { for _, f := range a.Fields { if f.Field != "Credentials" && (f.Old != "" || f.New != "") { @@ -232,15 +233,19 @@ func accessChangeSummary(a AccessDiff) string { } seen := make(map[string]struct{}, len(a.Fields)) for _, f := range a.Fields { - switch f.Field { - case "Credentials": + switch { + case f.Field == "Credentials": seen["credentials"] = struct{}{} - case "Server": + case f.Field == "Server": seen["server"] = struct{}{} - case "CA": + case f.Field == "CA": seen["ca"] = struct{}{} - case "Labels": + case f.Field == "Labels": seen["labels"] = struct{}{} + case f.Field == "Exec Args" || f.Field == "Auth type": + seen["credentials"] = struct{}{} + case strings.HasPrefix(f.Field, "auth-provider."): + seen["credentials"] = struct{}{} default: seen["config"] = struct{}{} } From e5c5a244dab514ec8a63990e245da959d3d9b50e Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 14:12:45 +0200 Subject: [PATCH 09/16] refactor(sync): drop table view, use diff-style output exclusively for dry-run The table format was redundant and less readable than the git-style diff output. Remove --dry-run-format flag, Format field from SyncDryRunResult, and all table rendering code. The diff view (- old / + new lines per field) is now the only dry-run output style. Signed-off-by: onuryilmaz --- cmd/output/interactive_printer.go | 68 +------------------------ cmd/output/plain_printer.go | 84 +------------------------------ cmd/output/types.go | 3 -- cmd/sync.go | 7 +-- 4 files changed, 3 insertions(+), 159 deletions(-) diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index c995112..122e253 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -221,77 +221,11 @@ func (p *interactivePrinter) printSyncDryRunResult(r SyncDryRunResult) error { w("%s\n\n", styleFaint.Render("Dry-run: no changes will be written.")) w("%s (%d change(s))\n", styleHeader.Render("CLUSTER ACCESSES"), total) - if r.Format == "diff" { - p.printDryRunDiff(w, r) - } else { - p.printDryRunTable(w, r) - } + p.printDryRunDiff(w, r) return writeErr } -func (p *interactivePrinter) printDryRunTable(w func(string, ...any), r SyncDryRunResult) { - nameWidth := len("NAME") - for _, a := range r.Accesses { - if len(a.Name) > nameWidth { - nameWidth = len(a.Name) - } - } - nameWidth += 2 - - hasAddedOrRemoved := r.Added > 0 || r.Removed > 0 - if hasAddedOrRemoved { - w(" %-*s %s\n", nameWidth, styleBold.Render("NAME"), styleBold.Render("SERVER")) - for _, a := range r.Accesses { - switch a.ChangeType { - case "added": - w(" %s %-*s %s\n", styleGreen.Render("+"), nameWidth, a.Name, styleFaint.Render(a.Server)) - case "removed": - w(" %s %-*s %s\n", styleRed.Render("-"), nameWidth, a.Name, styleFaint.Render("(removed)")) - } - } - } - - if r.Modified > 0 { - if hasAddedOrRemoved { - w("\n") - } - w(" %-*s %s\n", nameWidth, styleBold.Render("NAME"), styleBold.Render("CHANGES")) - for _, a := range r.Accesses { - if a.ChangeType != "modified" { - continue - } - if hasDetailFields(a) { - w(" %s %s\n", styleYellow.Render("~"), a.Name) - for _, f := range a.Fields { - if f.Field == "Credentials" { - w(" %-14s %s\n", "credentials:", styleYellow.Render("changed")) - } else if f.Old != "" || f.New != "" { - label := strings.ToLower(f.Field) + ":" - w(" %-14s %s → %s\n", label, - styleRed.Render(orElse(f.Old, "(none)")), - styleGreen.Render(orElse(f.New, "(none)"))) - } - } - } else { - summary := accessChangeSummary(a) - var styledSummary string - switch summary { - case "credentials": - styledSummary = styleYellow.Render(summary) - case "server", "ca": - styledSummary = styleRed.Render(summary) - default: - styledSummary = styleFaint.Render(summary) - } - w(" %s %-*s %s\n", styleYellow.Render("~"), nameWidth, a.Name, styledSummary) - } - } - } - - p.printDryRunSummary(w, r) -} - func (p *interactivePrinter) printDryRunDiff(w func(string, ...any), r SyncDryRunResult) { for _, a := range r.Accesses { switch a.ChangeType { diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index ada27eb..f61702e 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -76,12 +76,7 @@ func (p *plainPrinter) Print(v any) error { } w("Dry-run: no changes will be written.\n\n") w("CLUSTER ACCESSES (%d change(s))\n", total) - - if t.Format == "diff" { - p.printDryRunDiff(w, t) - } else { - p.printDryRunTable(w, t) - } + p.printDryRunDiff(w, t) case ClusterVersionResult: w("Kubernetes version: %s\n", t.Version) @@ -106,63 +101,6 @@ func (p *plainPrinter) StartSpinner(_ string) func() { return func() {} } -// printDryRunTable renders dry-run output as a compact NAME | CHANGES table. -func (p *plainPrinter) printDryRunTable(w func(string, ...any), t SyncDryRunResult) { - nameWidth := len("NAME") - for _, a := range t.Accesses { - if len(a.Name) > nameWidth { - nameWidth = len(a.Name) - } - } - nameWidth += 2 - - hasAddedOrRemoved := t.Added > 0 || t.Removed > 0 - if hasAddedOrRemoved { - w(" %-*s %s\n", nameWidth, "NAME", "SERVER") - for _, a := range t.Accesses { - switch a.ChangeType { - case "added": - w(" + %-*s %s\n", nameWidth, a.Name, a.Server) - case "removed": - w(" - %-*s (removed)\n", nameWidth, a.Name) - } - } - } - - if t.Modified > 0 { - if hasAddedOrRemoved { - w("\n") - } - w(" %-*s %s\n", nameWidth, "NAME", "CHANGES") - for _, a := range t.Accesses { - if a.ChangeType != "modified" { - continue - } - if hasDetailFields(a) { - w(" ~ %s\n", a.Name) - for _, f := range a.Fields { - if f.Field == "Credentials" { - w(" %-14s changed\n", "credentials:") - } else if f.Old != "" || f.New != "" { - label := strings.ToLower(f.Field) + ":" - w(" %-14s %s → %s\n", label, orElse(f.Old, "(none)"), orElse(f.New, "(none)")) - } - } - } else { - w(" ~ %-*s %s\n", nameWidth, a.Name, accessChangeSummary(a)) - } - } - } - - modBreakdown := modifiedBreakdown(t.Accesses) - modSuffix := "" - if t.Modified > 0 && len(modBreakdown) > 0 { - modSuffix = " (" + strings.Join(modBreakdown, ", ") + ")" - } - w("\nSummary: %d added, %d removed, %d modified%s. No changes will be written.\n", - t.Added, t.Removed, t.Modified, modSuffix) -} - // printDryRunDiff renders dry-run output in git-style unified diff format: // each changed field is shown as a - (old) and + (new) line. func (p *plainPrinter) printDryRunDiff(w func(string, ...any), t SyncDryRunResult) { @@ -205,26 +143,6 @@ func (p *plainPrinter) printDryRunDiff(w func(string, ...any), t SyncDryRunResul t.Added, t.Removed, t.Modified, modSuffix) } -// hasDetailFields returns true when the access diff contains at least one -// field that has old/new values worth printing inline (excludes the synthetic -// Credentials sentinel which carries no real values). -func hasDetailFields(a AccessDiff) bool { - for _, f := range a.Fields { - if f.Field != "Credentials" && (f.Old != "" || f.New != "") { - return true - } - } - return false -} - -// orElse returns s if non-empty, otherwise fallback. -func orElse(s, fallback string) string { - if s != "" { - return s - } - return fallback -} - // accessChangeSummary returns a compact, human-readable string describing what // changed in a modified AccessDiff entry (e.g. "credentials", "server", "config"). func accessChangeSummary(a AccessDiff) string { diff --git a/cmd/output/types.go b/cmd/output/types.go index ba6d968..11427af 100644 --- a/cmd/output/types.go +++ b/cmd/output/types.go @@ -68,9 +68,6 @@ type SyncDryRunResult struct { Added int `json:"added" yaml:"added"` Removed int `json:"removed" yaml:"removed"` Modified int `json:"modified" yaml:"modified"` - // Format controls how plain/interactive printers render this result. - // Accepted values: "table" (default), "diff". Ignored by JSON/YAML output. - Format string `json:"-" yaml:"-"` } // DiffEntry describes a single added, removed, or modified kubeconfig entry. diff --git a/cmd/sync.go b/cmd/sync.go index 6df13dd..873abc9 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -41,7 +41,6 @@ var ( kubeloginExtraArgs []string kubeloginTokenCacheDir string dryRun bool - dryRunFormat string ) func init() { @@ -74,7 +73,6 @@ 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().StringVar(&dryRunFormat, "dry-run-format", "table", "Dry-run display style: table (default) or diff (git-style old/new lines)") // BindPFlags can theroretically return an error if called with `nil` as an argument // which should never happened after at least one flag was defined. That's why the output @@ -127,7 +125,6 @@ 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") - dryRunFormat = viper.GetString("dry-run-format") format, err := output.ParseFormat(viper.GetString("output")) if err != nil { @@ -236,9 +233,7 @@ func runSync(cmd *cobra.Command, args []string) error { if dryRun { diff := diffKubeconfig(localConfigBefore, localConfig) - result := buildDryRunResult(diff, localConfigBefore, localConfig) - result.Format = dryRunFormat - return printer.Print(result) + return printer.Print(buildDryRunResult(diff, localConfigBefore, localConfig)) } if writeErr := writeConfig(localConfig, remoteClusterKubeconfig); writeErr != nil { From 00292309001455cd17f724ae2dae06734ece9bd6 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 14:16:29 +0200 Subject: [PATCH 10/16] fix(sync): fix dry-run summary breakdown and collapse redacted-equal fields Two fixes: - modifiedBreakdown now counts each change category (credentials, server, etc.) independently per access entry instead of using the compound summary string as a map key. This prevents "1 credentials, server" appearing as a separate bucket alongside "173 credentials". - Fields where old == new after redaction (e.g. both sides show ) are now rendered as "~ field: changed" instead of a meaningless "- / + " pair. Signed-off-by: onuryilmaz --- cmd/output/interactive_printer.go | 2 ++ cmd/output/plain_printer.go | 39 +++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/cmd/output/interactive_printer.go b/cmd/output/interactive_printer.go index 122e253..e5e7ffb 100644 --- a/cmd/output/interactive_printer.go +++ b/cmd/output/interactive_printer.go @@ -244,6 +244,8 @@ func (p *interactivePrinter) printDryRunDiff(w func(string, ...any), r SyncDryRu for _, f := range a.Fields { if f.Field == "Credentials" { w(" %s %-12s %s\n", styleYellow.Render("~"), "credentials:", styleYellow.Render("changed")) + } else if f.Old == f.New { + w(" %s %-12s %s\n", styleYellow.Render("~"), strings.ToLower(f.Field)+":", styleYellow.Render("changed")) } else { label := strings.ToLower(f.Field) + ":" if f.Old != "" { diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index f61702e..9817746 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -121,6 +121,9 @@ func (p *plainPrinter) printDryRunDiff(w func(string, ...any), t SyncDryRunResul for _, f := range a.Fields { if f.Field == "Credentials" { w(" ~ credentials: changed\n") + } else if f.Old == f.New { + // Both sides redacted to the same string — show as a generic change. + w(" ~ %-12s changed\n", strings.ToLower(f.Field)+":") } else { label := strings.ToLower(f.Field) + ":" if f.Old != "" { @@ -176,20 +179,48 @@ func accessChangeSummary(a AccessDiff) string { return strings.Join(parts, ", ") } -// modifiedBreakdown counts distinct change reasons across all modified accesses -// and returns them as sorted "N reason" strings (e.g. ["45 credentials", "129 config"]). +// modifiedBreakdown counts individual change categories across all modified +// accesses and returns them as sorted "N reason" strings +// (e.g. ["45 credentials", "1 server"]). +// A single access entry can contribute to multiple categories. func modifiedBreakdown(accesses []AccessDiff) []string { counts := make(map[string]int) for _, a := range accesses { if a.ChangeType != "modified" { continue } - counts[accessChangeSummary(a)]++ + // Count each category at most once per access entry. + seen := make(map[string]struct{}) + for _, f := range a.Fields { + var cat string + switch { + case f.Field == "Credentials": + cat = "credentials" + case f.Field == "Server": + cat = "server" + case f.Field == "CA": + cat = "ca" + case f.Field == "Labels": + cat = "labels" + case f.Field == "Exec Args" || f.Field == "Auth type": + cat = "credentials" + case strings.HasPrefix(f.Field, "auth-provider."): + cat = "credentials" + default: + cat = "config" + } + if _, already := seen[cat]; !already { + seen[cat] = struct{}{} + counts[cat]++ + } + } + if len(a.Fields) == 0 { + counts["config"]++ + } } if len(counts) == 0 { return nil } - // Collect keys and sort for deterministic output. keys := make([]string, 0, len(counts)) for k := range counts { keys = append(keys, k) From 7416b4d198c2be06d70259c54738cead49055084 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 17:36:41 +0200 Subject: [PATCH 11/16] =?UTF-8?q?fix(sync):=20address=20Copilot=20review?= =?UTF-8?q?=20=E2=80=94=20key=20correctness=20and=20diff=20completeness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - generateAuthInfoKey: include Exec.Command, APIVersion, InteractiveMode in exec-based key so entries with different plugins are not deduplicated - generateAuthInfoKey: include AuthProvider.Name in auth-provider key to prevent collisions between different provider types with identical config - argsDiff: pair up sensitive flag changes (same prefix in both removed and added) into a single modified entry instead of separate - / + lines that misleadingly look identical - buildAccessDiffs steps 2 & 3: merge cluster and credential field diffs into existing access entries instead of skipping already-handled contexts, so CA/Labels changes and credential changes are never silently dropped Signed-off-by: onuryilmaz --- cmd/authinfo.go | 10 +++-- cmd/kubeconfigdiff.go | 101 ++++++++++++++++++++++++++++++++---------- 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/cmd/authinfo.go b/cmd/authinfo.go index b6c9cb9..7404221 100644 --- a/cmd/authinfo.go +++ b/cmd/authinfo.go @@ -129,7 +129,8 @@ func generateAuthInfoKey(authInfo *clientcmdapi.AuthInfo) string { envParts = append(envParts, e.Name+"="+e.Value) } sort.Strings(envParts) - data := fmt.Sprintf("exec:issuer:%s;client-id:%s;client-secret:%s;extra-params:%s;scopes:%s;env:%s", + data := fmt.Sprintf("exec:cmd:%s;api:%s;mode:%s;issuer:%s;client-id:%s;client-secret:%s;extra-params:%s;scopes:%s;env:%s", + authInfo.Exec.Command, authInfo.Exec.APIVersion, authInfo.Exec.InteractiveMode, issuer, clientID, clientSecret, extraParams, strings.Join(scopes, ","), strings.Join(envParts, ",")) return data } @@ -151,9 +152,10 @@ func generateAuthInfoKey(authInfo *clientcmdapi.AuthInfo) string { authRequestExtraParams := authInfo.AuthProvider.Config["auth-request-extra-params"] extraScopes := authInfo.AuthProvider.Config["extra-scopes"] - // Concatenate the fields in a consistent order - data := fmt.Sprintf("idp-issuer-url:%s;client-id:%s;client-secret:%s;auth-request-extra-params:%s;extra-scopes:%s", - issuerURL, clientID, clientSecret, authRequestExtraParams, extraScopes) + // Concatenate the fields in a consistent order, including the provider name + // to prevent collisions between different auth-provider types. + data := fmt.Sprintf("name:%s;idp-issuer-url:%s;client-id:%s;client-secret:%s;auth-request-extra-params:%s;extra-scopes:%s", + authInfo.AuthProvider.Name, issuerURL, clientID, clientSecret, authRequestExtraParams, extraScopes) return data } diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index 5d45d62..5c0ecb2 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -258,6 +258,8 @@ func redactArg(arg string) string { // argsDiff computes per-argument differences between two exec arg lists, returning // FieldDiff entries for args that appear only in old (removed) or only in new (added). // Values of sensitive flags (e.g. --oidc-client-secret=) are redacted. +// When a sensitive flag appears in both removed and added (value changed), a single +// entry with Old and New both set to "" is emitted rather than separate lines. func argsDiff(oldArgs, newArgs []string) []FieldDiff { oldSet := make(map[string]bool, len(oldArgs)) for _, a := range oldArgs { @@ -280,12 +282,57 @@ func argsDiff(oldArgs, newArgs []string) []FieldDiff { } } + // Pair up sensitive flag changes: if the same prefix appears in both removed + // and added, the value changed — emit a single modified entry instead of + // separate remove+add lines that both redact to the same visible string. + pairedPrefixes := make(map[string]bool) + for _, pfx := range sensitiveArgPrefixes { + var inRemoved, inAdded bool + for _, r := range removed { + if strings.HasPrefix(r, pfx) { + inRemoved = true + break + } + } + for _, a := range added { + if strings.HasPrefix(a, pfx) { + inAdded = true + break + } + } + if inRemoved && inAdded { + pairedPrefixes[pfx] = true + } + } + var diffs []FieldDiff + // Emit paired sensitive changes as a single modified entry. + for pfx := range pairedPrefixes { + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: pfx + "", New: pfx + ""}) + } for _, r := range removed { - diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: redactArg(r), New: ""}) + isPaired := false + for pfx := range pairedPrefixes { + if strings.HasPrefix(r, pfx) { + isPaired = true + break + } + } + if !isPaired { + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: redactArg(r), New: ""}) + } } for _, a := range added { - diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: "", New: redactArg(a)}) + isPaired := false + for pfx := range pairedPrefixes { + if strings.HasPrefix(a, pfx) { + isPaired = true + break + } + } + if !isPaired { + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: "", New: redactArg(a)}) + } } return diffs } @@ -445,8 +492,8 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } // 2. For each modified cluster, find contexts in newCfg that reference it. - // If those contexts were not already captured via context-level diffs, add - // a "modified" access entry reflecting the cluster field changes (Server, CA, Labels). + // Merge cluster field changes (Server, CA, Labels) into the access entry, + // creating a new entry if the context was not already captured in step 1. modifiedClusters := make(map[string]EntryDiff, len(diff.Clusters)) for _, cd := range diff.Clusters { if cd.ChangeType == DiffChangeModified { @@ -455,9 +502,6 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } if len(modifiedClusters) > 0 { for ctxName, ctx := range newCfg.Contexts { - if _, alreadyHandled := byName[ctxName]; alreadyHandled { - continue - } clusterDiff, affected := modifiedClusters[ctx.Cluster] if !affected { continue @@ -473,20 +517,27 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) if len(fields) == 0 { continue } - byName[ctxName] = &accessEntry{ - access: output.AccessDiff{ - Name: ctxName, - ChangeType: string(DiffChangeModified), - Fields: fields, - }, + if existing, ok := byName[ctxName]; ok { + // Merge cluster fields into the existing entry. + existing.access.Fields = append(fields, existing.access.Fields...) + } else { + byName[ctxName] = &accessEntry{ + access: output.AccessDiff{ + Name: ctxName, + ChangeType: string(DiffChangeModified), + Fields: fields, + }, + } } } } // 3. For each modified authinfo, find contexts in newCfg that reference it. // Carry the actual field diffs (exec args, auth-provider config) through so - // diff format can show real old/new values. Fall back to a generic - // "Credentials: changed" entry only when no specific fields were identified. + // diff format can show real old/new values. Merge into an existing entry when + // one was already created by step 1 or 2, so credential changes are never + // silently dropped. Fall back to a generic "Credentials: changed" entry only + // when no specific fields were identified. modifiedAuthInfos := make(map[string]EntryDiff, len(diff.AuthInfos)) for _, ad := range diff.AuthInfos { if ad.ChangeType == DiffChangeModified { @@ -495,9 +546,6 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } if len(modifiedAuthInfos) > 0 { for ctxName, ctx := range newCfg.Contexts { - if _, alreadyHandled := byName[ctxName]; alreadyHandled { - continue - } ad, affected := modifiedAuthInfos[ctx.AuthInfo] if !affected { continue @@ -514,12 +562,17 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) // authInfoEqual returned false but no specific fields were identified. fields = []output.FieldChange{{Field: "Credentials", Old: "changed", New: ""}} } - byName[ctxName] = &accessEntry{ - access: output.AccessDiff{ - Name: ctxName, - ChangeType: string(DiffChangeModified), - Fields: fields, - }, + if existing, ok := byName[ctxName]; ok { + // Merge credential fields into the existing entry. + existing.access.Fields = append(existing.access.Fields, fields...) + } else { + byName[ctxName] = &accessEntry{ + access: output.AccessDiff{ + Name: ctxName, + ChangeType: string(DiffChangeModified), + Fields: fields, + }, + } } } } From 758809bf8489f482e0c3c770290455987eae5960 Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 18:19:46 +0200 Subject: [PATCH 12/16] fix(sync): gate authinfo reuse on authInfoEqual and deduplicate field diffs - Guard unmanaged local authinfo reuse with authInfoEqual() in addition to the key match, preventing incorrect rewiring when non-OIDC fields differ. Also call mergeAuthInfo() on reuse so server-side changes are applied while local tokens are preserved. - Add deduplicateFieldChanges() to buildAccessDiffs() so that field diffs contributed by multiple sources (context, cluster, authinfo) for the same access entry are deduplicated before printing, avoiding repeated lines and inflated modifiedBreakdown counts. Signed-off-by: onuryilmaz --- cmd/kubeconfigdiff.go | 23 ++++++++++++++++++++++- cmd/sync.go | 14 +++++++++++--- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index 5c0ecb2..5546748 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -577,9 +577,10 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) } } - // Flatten and sort + // Flatten, deduplicate fields, and sort accesses := make([]output.AccessDiff, 0, len(byName)) for _, e := range byName { + e.access.Fields = deduplicateFieldChanges(e.access.Fields) accesses = append(accesses, e.access) } sort.Slice(accesses, func(i, j int) bool { return accesses[i].Name < accesses[j].Name }) @@ -648,3 +649,23 @@ func sortedKeys(a, b map[string]string) []string { sort.Strings(keys) return keys } + +// deduplicateFieldChanges removes duplicate FieldChange entries, preserving order. +// Duplicates can arise when multiple diff sources (context, cluster, authinfo) all +// contribute the same field change to the same access entry. +func deduplicateFieldChanges(fields []output.FieldChange) []output.FieldChange { + if len(fields) <= 1 { + return fields + } + type key struct{ field, old, new string } + seen := make(map[key]struct{}, len(fields)) + out := fields[:0:0] + for _, f := range fields { + k := key{f.Field, f.Old, f.New} + if _, dup := seen[k]; !dup { + seen[k] = struct{}{} + out = append(out, f) + } + } + return out +} diff --git a/cmd/sync.go b/cmd/sync.go index 873abc9..9a2b0f2 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -509,10 +509,18 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap uniqueKey := generateAuthInfoKey(serverAuth) // If an unmanaged local entry has the same credentials, reuse its name. + // Gate on authInfoEqual (excluding tokens) so we don't rewire a context + // to an unmanaged entry whose non-OIDC fields differ from the server config. if localName, found := existingLocalKeys[uniqueKey]; found { - slog.Debug("reusing existing local authinfo", "name", localName, "server", serverName) - authInfoMap[uniqueKey] = localName - continue + localAuth := localConfig.AuthInfos[localName] + if authInfoEqual(localAuth, serverAuth) { + slog.Debug("reusing existing local authinfo", "name", localName, "server", serverName) + // Merge server config into the local entry to pick up any server-side + // changes while preserving local tokens. + localConfig.AuthInfos[localName] = mergeAuthInfo(serverAuth, localAuth) + authInfoMap[uniqueKey] = localName + continue + } } hash := sha256.Sum256([]byte(uniqueKey)) From dd0014b9984fbc1cf6e52a1ab619c610ac35edcc Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 19:25:19 +0200 Subject: [PATCH 13/16] fix(output): remove unused accessChangeSummary function Signed-off-by: onuryilmaz --- cmd/output/plain_printer.go | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/cmd/output/plain_printer.go b/cmd/output/plain_printer.go index 9817746..e482bf6 100644 --- a/cmd/output/plain_printer.go +++ b/cmd/output/plain_printer.go @@ -146,39 +146,6 @@ func (p *plainPrinter) printDryRunDiff(w func(string, ...any), t SyncDryRunResul t.Added, t.Removed, t.Modified, modSuffix) } -// accessChangeSummary returns a compact, human-readable string describing what -// changed in a modified AccessDiff entry (e.g. "credentials", "server", "config"). -func accessChangeSummary(a AccessDiff) string { - if len(a.Fields) == 0 { - return "config" - } - seen := make(map[string]struct{}, len(a.Fields)) - for _, f := range a.Fields { - switch { - case f.Field == "Credentials": - seen["credentials"] = struct{}{} - case f.Field == "Server": - seen["server"] = struct{}{} - case f.Field == "CA": - seen["ca"] = struct{}{} - case f.Field == "Labels": - seen["labels"] = struct{}{} - case f.Field == "Exec Args" || f.Field == "Auth type": - seen["credentials"] = struct{}{} - case strings.HasPrefix(f.Field, "auth-provider."): - seen["credentials"] = struct{}{} - default: - seen["config"] = struct{}{} - } - } - parts := make([]string, 0, len(seen)) - for k := range seen { - parts = append(parts, k) - } - sort.Strings(parts) - return strings.Join(parts, ", ") -} - // modifiedBreakdown counts individual change categories across all modified // accesses and returns them as sorted "N reason" strings // (e.g. ["45 credentials", "1 server"]). From 6ce1e4f6945a2681743f3e56ae075c3b2be725fc Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 19:40:33 +0200 Subject: [PATCH 14/16] fix(sync): pick lexicographically smallest name for deterministic authinfo reuse Signed-off-by: onuryilmaz --- cmd/sync.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 9a2b0f2..47af49f 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -13,6 +13,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" greenhousemetav1alpha1 "github.com/cloudoperators/greenhouse/api/meta/v1alpha1" @@ -497,12 +498,19 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap // Build a reverse lookup of unmanaged local auth entries so we can reuse their names // instead of creating new cloudctl:auth- entries. - existingLocalKeys := make(map[string]string) // authInfoKey → local name + // Collect all names per key first, then pick the lexicographically smallest to ensure + // deterministic reuse when multiple unmanaged entries share the same key. + keyToNames := make(map[string][]string) for localName, localAuth := range localConfig.AuthInfos { if !isManaged(localName) { - existingLocalKeys[generateAuthInfoKey(localAuth)] = localName + key := generateAuthInfoKey(localAuth) + keyToNames[key] = append(keyToNames[key], localName) } } + existingLocalKeys := make(map[string]string, len(keyToNames)) // authInfoKey → local name + for key, names := range keyToNames { + existingLocalKeys[key] = slices.Min(names) + } // Merge AuthInfos for serverName, serverAuth := range serverConfig.AuthInfos { From 5f019cdceab65e3fb9e7f58631e3dee57e1ccb3d Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 19:49:14 +0200 Subject: [PATCH 15/16] fix(sync): surface namespace changes in dry-run access diff output Signed-off-by: onuryilmaz --- cmd/kubeconfigdiff.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index 5546748..c701b63 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -410,6 +410,16 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) if oldCtx == nil || newCtx == nil { break } + // Namespace change (carried directly from the context-level diff fields) + for _, fd := range ctxDiff.Fields { + if fd.Field == "Namespace" { + entry.access.Fields = append(entry.access.Fields, output.FieldChange{ + Field: "Namespace", + Old: fd.Old, + New: fd.New, + }) + } + } // Server URL change (resolve through cluster refs) oldCluster := oldCfg.Clusters[oldCtx.Cluster] newCluster := newCfg.Clusters[newCtx.Cluster] From 2a114ec1d8e5f3ee14d7f6448efdbac12d2f076e Mon Sep 17 00:00:00 2001 From: onuryilmaz Date: Fri, 10 Jul 2026 22:15:03 +0200 Subject: [PATCH 16/16] fix(sync): don't mutate unmanaged authinfo on reuse; fix non-deterministic argsDiff order; drop unused fromCtx field; rename misleading test Signed-off-by: onuryilmaz --- cmd/kubeconfigdiff.go | 12 +++++++----- cmd/sync.go | 6 +++--- cmd/sync_test.go | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cmd/kubeconfigdiff.go b/cmd/kubeconfigdiff.go index c701b63..ef1a4db 100644 --- a/cmd/kubeconfigdiff.go +++ b/cmd/kubeconfigdiff.go @@ -307,8 +307,12 @@ func argsDiff(oldArgs, newArgs []string) []FieldDiff { var diffs []FieldDiff // Emit paired sensitive changes as a single modified entry. - for pfx := range pairedPrefixes { - diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: pfx + "", New: pfx + ""}) + // Iterate over sensitiveArgPrefixes (ordered slice) rather than the map to + // keep output order deterministic regardless of how many prefixes match. + for _, pfx := range sensitiveArgPrefixes { + if pairedPrefixes[pfx] { + diffs = append(diffs, FieldDiff{Field: "Exec Args", Old: pfx + "", New: pfx + ""}) + } } for _, r := range removed { isPaired := false @@ -375,8 +379,7 @@ func toOutputDiffEntries(diffs []EntryDiff) []output.DiffEntry { func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) []output.AccessDiff { // accesses keyed by context name to allow merging type accessEntry struct { - access output.AccessDiff - fromCtx bool // originated from a context-level diff + access output.AccessDiff } byName := make(map[string]*accessEntry) @@ -388,7 +391,6 @@ func buildAccessDiffs(diff KubeconfigDiff, oldCfg, newCfg *clientcmdapi.Config) Name: name, ChangeType: string(ctxDiff.ChangeType), }, - fromCtx: true, } switch ctxDiff.ChangeType { diff --git a/cmd/sync.go b/cmd/sync.go index 47af49f..1927657 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -523,9 +523,9 @@ func mergeKubeconfig(localConfig *clientcmdapi.Config, serverConfig *clientcmdap localAuth := localConfig.AuthInfos[localName] if authInfoEqual(localAuth, serverAuth) { slog.Debug("reusing existing local authinfo", "name", localName, "server", serverName) - // Merge server config into the local entry to pick up any server-side - // changes while preserving local tokens. - localConfig.AuthInfos[localName] = mergeAuthInfo(serverAuth, localAuth) + // Credentials are identical — leave the unmanaged local entry untouched + // to preserve any local-only fields (Token, TokenFile, Impersonate, etc.) + // that are outside authInfoEqual's comparison scope. authInfoMap[uniqueKey] = localName continue } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index eeafc5c..cc44136 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -846,7 +846,7 @@ func TestDiffKubeconfig_UnmanagedEntriesIgnored(t *testing.T) { g.Expect(diff.AuthInfos).To(BeEmpty()) } -func TestRunSync_DryRun_NoWrite(t *testing.T) { +func TestDryRun_MergeAndDiff_NoWrite(t *testing.T) { g := NewWithT(t) orig := prefix