Skip to content

feat(sync): dry-run mode with diff output and authinfo deduplication#66

Open
onuryilmaz wants to merge 16 commits into
mainfrom
feat/dry-run-authinfo-dedup
Open

feat(sync): dry-run mode with diff output and authinfo deduplication#66
onuryilmaz wants to merge 16 commits into
mainfrom
feat/dry-run-authinfo-dedup

Conversation

@onuryilmaz

@onuryilmaz onuryilmaz commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds --dry-run flag to sync — previews what would change without writing to the kubeconfig file
  • Fixes authinfo deduplication: identical OIDC credentials across clusters are merged into a single shared authinfo entry (one login for all)
  • Output is context-centric: one entry per cluster access, not per internal kubeconfig entry
  • Diff-style output shows actual old/new field values per changed argument

Dry-run output

No changes:

No changes detected.

Additions and removals:

Dry-run: no changes will be written.

CLUSTER ACCESSES (3 change(s))
+ prod-eu-1
  + server:       https://prod-eu-1.k8s.example.com
+ prod-eu-2
  + server:       https://prod-eu-2.k8s.example.com
- old-cluster
  - server:       https://old.k8s.example.com

Summary: 2 added, 1 removed, 0 modified. No changes will be written.

Credential and server changes:

Dry-run: no changes will be written.

CLUSTER ACCESSES (3 change(s))
~ prod-eu-1
  - exec args:    --oidc-client-id=old-id
  + exec args:    --oidc-client-id=new-id
~ prod-eu-2
  - server:       https://old.k8s.example.com
  + server:       https://new.k8s.example.com
  ~ exec args:    changed
~ prod-eu-3
  ~ credentials:  changed

Summary: 0 added, 0 removed, 3 modified (2 credentials, 1 server). No changes will be written.

JSON/YAML output (-o json / -o yaml) retains the full structured diff including raw clusters, contexts, and authInfos sections for tooling.

Key design decisions

  • Context-centric: each line = one cluster access (context), hiding internal authinfo hash names
  • Redacted fields: sensitive values (--oidc-client-secret) that are identical after redaction show as ~ field: changed instead of a misleading - <redacted> / + <redacted> pair
  • No-op suppression: authinfo hash reassignment during deduplication (same credentials, new hash name) is detected and suppressed — only real changes are shown
  • Summary breakdown: counts each change category (credentials, server, ca) independently per entry

Test plan

  • cloudctl sync -n <org> --dry-run — shows diff, no kubeconfig written
  • cloudctl sync -n <org> --dry-run -o json — structured JSON output with accesses/clusters/contexts/authInfos
  • After running sync normally, re-run --dry-run — should show No changes detected.
  • make test passes

onuryilmaz added 10 commits July 10, 2026 11:16
@onuryilmaz
feat(sync): add --dry-run flag and fix authinfo deduplication (#51, #54)
ebd03b4 
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-<hash>` entry. Contexts are updated to reference the
  local name.

Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>
@onuryilmaz
fix(sync): context-centric dry-run output with server URLs
d44d806 
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 <onur.yilmaz@sap.com>
@onuryilmaz
fix(sync): address Copilot review — security, correctness, test isola…
524582a 
…tion

- Redact sensitive values in dry-run diff output:
  - auth-provider config keys in sensitiveAuthProviderKeys (client-secret)
    are replaced with <redacted> in FieldDiff Old/New
  - exec args matching sensitiveArgPrefixes (--oidc-client-secret=) are
    replaced with --oidc-client-secret=<redacted> 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 <onur.yilmaz@sap.com>
@onuryilmaz
fix(output): table layout for dry-run modified entries with change su…
eba6ce6 
…mmary

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 <onur.yilmaz@sap.com>
@onuryilmaz
fix(output): show old → new field values for structural changes in dr…
4f2d2f9 
…y-run

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 <onur.yilmaz@sap.com>
@onuryilmaz
feat(sync): add --dry-run-format=table|diff flag
2d3cb0c 
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 <onur.yilmaz@sap.com>
@onuryilmaz
fix(sync): suppress no-op context diffs from authinfo hash reassignment
4488e57 
When mergeIdenticalUsers is enabled, authinfo deduplication can change
a context's AuthInfo pointer from one cloudctl:auth-<hash> 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 <onur.yilmaz@sap.com>
@onuryilmaz
feat(sync): show actual field diffs for credential changes in dry-run
8d84cea 
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 <onur.yilmaz@sap.com>
@onuryilmaz
refactor(sync): drop table view, use diff-style output exclusively fo…
e5c5a24 
…r 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 <onur.yilmaz@sap.com>
@onuryilmaz
fix(sync): fix dry-run summary breakdown and collapse redacted-equal …
0029230 
…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
  <redacted>) are now rendered as "~ field: changed" instead of
  a meaningless "- <redacted> / + <redacted>" pair.

Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>
Copilot AI review requested due to automatic review settings July 10, 2026 14:46
@onuryilmaz onuryilmaz requested review from a team as code owners July 10, 2026 14:46
This was linked to issues Jul 10, 2026
[FEAT cloudctl] - Support Dry-Run mode for sync command #51
Open
[FEAT cloudctl] - Improved AuthInfo merging and deduplication logic #54
Open
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends cloudctl sync with a --dry-run mode that computes and prints a kubeconfig diff without writing changes, while also refactoring auth info handling to improve deduplication (including reusing existing local authinfo names when credentials match).

Changes:

  • Add sync --dry-run to simulate merges and emit context-centric diff output (text) or structured diffs (json/yaml).
  • Introduce kubeconfig diffing utilities to compute managed entry diffs and derive “cluster access” (context) level changes.
  • Refactor AuthInfo comparison/keying/merge logic into a dedicated module and expand test coverage around dedup + dry-run output.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
cmd/sync.go Adds --dry-run flag plumbing; snapshots kubeconfig pre-merge and prints diff results instead of writing when enabled; adjusts authinfo merge flow.
cmd/sync_test.go Adds tests for authinfo equality/keying, dedup behavior, and dry-run diff/access derivation.
cmd/output/types.go Introduces structured output types for dry-run results (access diffs + entry diffs).
cmd/output/plain_printer.go Adds text rendering for dry-run results, including summary breakdown.
cmd/output/interactive_printer.go Adds interactive/TUI rendering for dry-run results with colored diff output.
cmd/output/output_test.go Adds printer tests for SyncDryRunResult across text/json/yaml formats.
cmd/kubeconfigdiff.go New diff engine for managed kubeconfig entries + access-centric aggregation and redaction support.
cmd/authinfo.go New module for authinfo equality/key generation and token-preserving merge behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/authinfo.go Outdated
Comment thread cmd/authinfo.go Outdated
Comment thread cmd/kubeconfigdiff.go
Comment thread cmd/kubeconfigdiff.go Outdated
Comment thread cmd/kubeconfigdiff.go Outdated
@onuryilmaz
fix(sync): address Copilot review — key correctness and diff complete…
7416b4d 
…ness

- 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 - <redacted> /
  + <redacted> 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 <onur.yilmaz@sap.com>
@onuryilmaz onuryilmaz requested a review from Copilot July 10, 2026 15:42
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comment thread cmd/sync.go Outdated
Comment thread cmd/kubeconfigdiff.go Outdated
@onuryilmaz
fix(sync): gate authinfo reuse on authInfoEqual and deduplicate field…
758809b 
… 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 <onur.yilmaz@sap.com>
@onuryilmaz
fix(output): remove unused accessChangeSummary function
dd0014b 
Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>
@onuryilmaz onuryilmaz requested a review from Copilot July 10, 2026 17:31
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread cmd/sync.go Outdated
@onuryilmaz
fix(sync): pick lexicographically smallest name for deterministic aut…
6ce1e4f 
…hinfo reuse

Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>
@onuryilmaz onuryilmaz requested a review from Copilot July 10, 2026 17:42
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread cmd/kubeconfigdiff.go
@onuryilmaz
fix(sync): surface namespace changes in dry-run access diff output
5f019cd 
Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>
@onuryilmaz onuryilmaz requested a review from Copilot July 10, 2026 17:52
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Comment thread cmd/sync.go
Comment thread cmd/kubeconfigdiff.go
Comment thread cmd/kubeconfigdiff.go
Comment thread cmd/sync_test.go Outdated
@onuryilmaz
fix(sync): don't mutate unmanaged authinfo on reuse; fix non-determin…
2a114ec 
…istic argsDiff order; drop unused fromCtx field; rename misleading test

Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEAT cloudctl] - Improved AuthInfo merging and deduplication logic [FEAT cloudctl] - Support Dry-Run mode for sync command

2 participants

@onuryilmaz