Update the secret sync - #68
Conversation
- Skip vaults containing 'fournos_skip_sync' key - Add destructive sync to delete orphaned managed secrets - Improve logging for skipped entries and deletions Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Script as Sync Script
participant Vault as Vault Server
participant K8s as Kubernetes API
Script->>K8s: List managed Secrets (by label)
K8s-->>Script: Return current managed Secrets
Script->>Vault: Fetch all Vault entries
Vault-->>Script: Return Vault secrets
Script->>Script: Filter out skipped entries<br/>(fournos_skip_sync=true)
Script->>Script: Track processed Secrets<br/>from Vault
alt Dry-run Mode
Script->>Script: Identify unprocessed Secrets<br/>to be deleted
Script-->>Script: Report would-be deletions
else Normal Mode
Script->>K8s: Delete unprocessed<br/>managed Secrets
K8s-->>Script: Deletion confirmed
end
Script-->>Script: Complete sync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hacks/sync_vault_secrets.py`:
- Around line 183-193: The current destructive helpers swallow Kubernetes
ApiException errors which hides failures from the overall sync process; change
_list_managed_secrets and _delete_secret so they do not convert API errors into
silent successes: in _list_managed_secrets (the function that builds
label_selector using LABEL_MANAGED_BY, MANAGER_VALUE and LABEL_VAULT_ENTRY and
calls v1.list_namespaced_secret) stop returning an empty list on
ApiException—log the error and re-raise the exception (or let it propagate) so
sync() can detect failure; in _delete_secret ensure you only swallow
404/NotFound but for any other ApiException log and re-raise (do not suppress
non-404 failures) so the destructive pass reports errors to the caller.
- Around line 252-260: The preview mode never sees deletions because
existing_managed_secrets is only populated when dry_run is False; fix by always
loading the existing managed secrets for the destructive diff so --dry-run can
report deletions: call _list_managed_secrets(namespace) (or
_list_managed_secrets(v1, namespace)) unconditionally and ensure a v1 client is
created when needed (move or duplicate the v1 = _k8s_core_api() call so v1 is
available for listing even in dry_run), leaving actual mutation logic gated by
the dry_run flag.
- Around line 290-296: The skip branch currently continues before marking the
vault's secret as processed, so SKIP_SYNC_KEY vaults (checked via kv_data and
vault_name) still end up in existing_managed_secrets - processed_secrets and get
purged; fix by adding the corresponding secret identifier to processed_secrets
(e.g., processed_secrets.add(secret_name) or the appropriate key used elsewhere)
before the continue and then increment skipped_count as now so skipped vaults
are treated as processed and won't be deleted later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffbeacdd-2f01-49d6-981f-99f4c8208b8c
📒 Files selected for processing (1)
hacks/sync_vault_secrets.py
|
/lgtm |
2 participants
Fixes: #53 #63
Summary by CodeRabbit