feat: sync credential templates to managed agents#878
Open
jparsai wants to merge 2 commits intoargoproj-labs:mainfrom
Open
feat: sync credential templates to managed agents#878jparsai wants to merge 2 commits intoargoproj-labs:mainfrom
jparsai wants to merge 2 commits intoargoproj-labs:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds support for syncing repository-credential secrets ( Changes
Sequence Diagram(s)sequenceDiagram
participant Principal as Principal Cluster
participant Kube as Kubernetes API
participant PrincipalSvc as Principal Service
participant Agent as Managed Agent
Principal->>Kube: Delete AppProject
Kube-->>Principal: Ack
Principal->>PrincipalSvc: deleteAppProjectCallback
activate PrincipalSvc
PrincipalSvc->>PrincipalSvc: syncRepositoriesForProject(ctx, project)
PrincipalSvc->>Kube: List secrets (label=repository)
Kube-->>PrincipalSvc: Repo secrets list
PrincipalSvc->>Kube: List secrets (label=repo-creds)
Kube-->>PrincipalSvc: Repo-creds list
PrincipalSvc->>Agent: Enqueue repository delete events for matched agents
deactivate PrincipalSvc
Agent->>Kube: Reconcile secrets/events
Kube-->>Agent: Secret created/updated/deleted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #878 +/- ##
==========================================
+ Coverage 46.31% 46.65% +0.34%
==========================================
Files 122 122
Lines 17410 17416 +6
==========================================
+ Hits 8063 8126 +63
+ Misses 8605 8537 -68
- Partials 742 753 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jparsai
requested review from
chetan-rns,
jannfis,
jgwest and
mikeshng
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
principal/callbacks.go (1)
869-881:⚠️ Potential issue | 🟠 MajorKeep
projectToReposin sync even when one project lookup returnsNotFound.
syncRepositoriesForProjectnow depends on this index for later AppProject-driven reconciliation, but the map is only rekeyed when bothGetcalls succeed. If a repo/repo-creds secret moves fromproject=Atoproject=BwhileAis already deleted orBdoes not exist yet, the entry stays underA, so later updates/deletes forBwill miss it andAcan still be treated as owning it.🛠️ Proposed fix
- if oldproject != nil && newProject != nil && oldproject.Name != newProject.Name { - // The project name has changed. Delete the repository from the old project. - s.projectToRepos.Delete(oldproject.Name, new.Name) - s.projectToRepos.Add(newProject.Name, new.Name) + if string(oldProjectName) != string(newProjectName) { + // Keep the project index aligned with the secret payload even if one side + // no longer exists yet. + s.projectToRepos.Delete(string(oldProjectName), new.Name) + s.projectToRepos.Add(string(newProjectName), new.Name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@principal/callbacks.go` around lines 869 - 881, The projectToRepos rekeying only runs when both s.projectManager.Get calls succeed; instead ensure we rekey even if one Get returned NotFound by deriving project names from whichever Project object is available or from the requested names used in the Get call (oldproject, newProject, and newProjectName/oldProjectName strings). Update the logic around s.projectManager.Get and the subsequent block that calls s.projectToRepos.Delete(oldproject.Name, new.Name) and s.projectToRepos.Add(newProject.Name, new.Name) so it computes oldName = (oldproject != nil ? oldproject.Name : string(oldProjectName)) and newName = (newProject != nil ? newProject.Name : string(newProjectName)) and then, if oldName != newName, call s.projectToRepos.Delete(oldName, new.Name) and s.projectToRepos.Add(newName, new.Name) so the index stays in sync even when a Get returns NotFound.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@principal/callbacks.go`:
- Around line 869-881: The projectToRepos rekeying only runs when both
s.projectManager.Get calls succeed; instead ensure we rekey even if one Get
returned NotFound by deriving project names from whichever Project object is
available or from the requested names used in the Get call (oldproject,
newProject, and newProjectName/oldProjectName strings). Update the logic around
s.projectManager.Get and the subsequent block that calls
s.projectToRepos.Delete(oldproject.Name, new.Name) and
s.projectToRepos.Add(newProject.Name, new.Name) so it computes oldName =
(oldproject != nil ? oldproject.Name : string(oldProjectName)) and newName =
(newProject != nil ? newProject.Name : string(newProjectName)) and then, if
oldName != newName, call s.projectToRepos.Delete(oldName, new.Name) and
s.projectToRepos.Add(newName, new.Name) so the index stays in sync even when a
Get returns NotFound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a18e6c9-8974-4963-a3bf-cc9c5bc9d4b6
📒 Files selected for processing (8)
internal/backend/kubernetes/repository/filters_test.gointernal/backend/kubernetes/repository/kubernetes.goprincipal/callbacks.goprincipal/callbacks_test.goprincipal/server.goprincipal/server_test.gotest/e2e/fixture/fixture.gotest/e2e/repocreds_test.go
Contributor
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 `@principal/callbacks.go`:
- Around line 485-486: When cleaning up repos after an AppProject deletion,
treat a NotFound error for the project as an expected state instead of an error:
in the code paths invoked by syncRepositoriesForProject (specifically in
syncRepositoryUpdatesToAgents and any function that looks up the current
AppProject), detect the NotFound error (using the kube API error checker, e.g.
errors.IsNotFound(err)) and log it at Info/Debug level and return/continue
normally rather than logging as an error; this prevents false-positive error
logs during genuine project deletions while preserving error logging for other
failure modes.
In `@principal/server_test.go`:
- Around line 448-455: The mock expectations for selector-specific List calls on
mockRepoBackend and mockProjBackend are not being asserted; add calls to
mockRepoBackend.AssertExpectations(t) and mockProjBackend.AssertExpectations(t)
after the test logic (after the code that invokes List) so the test fails if
either selector-specific path is no longer exercised; reference the mock objects
mockRepoBackend, mockProjBackend and the AssertExpectations(t) method to ensure
the selector-specific On("List", ..., backend.RepositorySelector{...})
expectations are validated.
In `@test/e2e/repocreds_test.go`:
- Around line 85-98: The helper createRepoCredsAndWaitForSync currently returns
once any Secret with the same name exists on the managed agent; update it to
assert that the synced Secret has the expected secret-type label and value
(e.g., label key "secret-type" with value "repo-creds") before returning. Inside
the requires.Eventually check (where you Get into tempRepoCreds via
ManagedAgentClient.Get), verify tempRepoCreds.Labels is non-nil and
tempRepoCreds.Labels["secret-type"] == "repo-creds" (or your repo-creds label
key/value used elsewhere) and only return true when that condition is met so the
tests wait for a real repo-creds replica.
🪄 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: 9649d764-be81-4348-9d36-a64e9c941470
📒 Files selected for processing (8)
internal/backend/kubernetes/repository/filters_test.gointernal/backend/kubernetes/repository/kubernetes.goprincipal/callbacks.goprincipal/callbacks_test.goprincipal/server.goprincipal/server_test.gotest/e2e/fixture/fixture.gotest/e2e/repocreds_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/backend/kubernetes/repository/filters_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/backend/kubernetes/repository/kubernetes.go
- test/e2e/fixture/fixture.go
Assisted by: Cursor Signed-off-by: Jayendra Parsai <jparsai@redhat.com>
Signed-off-by: Jayendra Parsai <jparsai@redhat.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
principal/server.go (1)
1009-1015: Consider centralizing the replicated secret-type allowlist.The same
repository/repo-credsset now appears in multiple call sites. Pulling it into a shared helper or const would reduce drift the next time this list changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@principal/server.go` around lines 1009 - 1015, The repeated inline allowlist of secret types (common.LabelValueSecretTypeRepository and common.LabelValueSecretTypeRepoCreds) used when calling s.repoManager.List (e.g., the loop over secretType) should be centralized; create a single exported/packaged constant or helper (e.g., RepositorySecretAllowlist or func AllowedRepositorySecretTypes()) and replace the duplicated inline slice in all call sites (places that iterate secretType, call repoManager.List with Labels using common.LabelKeySecretType) to reference that central value so future changes are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@principal/server.go`:
- Around line 1009-1015: The repeated inline allowlist of secret types
(common.LabelValueSecretTypeRepository and common.LabelValueSecretTypeRepoCreds)
used when calling s.repoManager.List (e.g., the loop over secretType) should be
centralized; create a single exported/packaged constant or helper (e.g.,
RepositorySecretAllowlist or func AllowedRepositorySecretTypes()) and replace
the duplicated inline slice in all call sites (places that iterate secretType,
call repoManager.List with Labels using common.LabelKeySecretType) to reference
that central value so future changes are made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e75e7fbe-da30-4986-9c23-bd5a64190876
📒 Files selected for processing (8)
internal/backend/kubernetes/repository/filters_test.gointernal/backend/kubernetes/repository/kubernetes.goprincipal/callbacks.goprincipal/callbacks_test.goprincipal/server.goprincipal/server_test.gotest/e2e/fixture/fixture.gotest/e2e/repocreds_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/backend/kubernetes/repository/filters_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- principal/server_test.go
- test/e2e/repocreds_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is to add support for syncing Argo CD credential templates (repo-creds) from principal to
managed agents. Repository secret's project-scoped distribution logic is reused for repo-creds as well.
It also has an fix where deleting an AppProject on the principal did not
clean up the associated repository secrets from agents. Same is followed for repo-creds as well.
Fixes #820
How to test changes / Special notes to the reviewer:
argocd.argoproj.io/secret-type: repo-credslabel and project field matching the AppProject nameAssisted by: Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor