feat: add default CA bundle for clusters (#29170) - #29303
Open
KHARSHAVARDHAN-eng wants to merge 1 commit into
Open
feat: add default CA bundle for clusters (#29170)#29303KHARSHAVARDHAN-eng wants to merge 1 commit into
KHARSHAVARDHAN-eng wants to merge 1 commit into
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
PR Summary by Qodofeat: add default CA bundle fallback for cluster connections (#29170)
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
Bundle ReportBundle size has no change ✅ |
Code Review by Qodo
1. Repeated CA lookup
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+110
to
+112
| for i := range clusterList.Items { | ||
| db.attachDefaultCABundle(&clusterList.Items[i]) | ||
| } |
There was a problem hiding this comment.
1. Repeated ca lookup 🐞 Bug ➹ Performance
ListClusters calls attachDefaultCABundle once per returned cluster, and attachDefaultCABundle re-reads the default CA ConfigMap each time; this adds avoidable O(n) cache lookups/locks and increases latency as cluster count grows.
Agent Prompt
### Issue description
`ListClusters()` loops over all clusters and calls `attachDefaultCABundle()` for each item, but `attachDefaultCABundle()` reads the default CA bundle from the settings manager on every call. This creates unnecessary repeated cache lookups/locking proportional to the number of clusters.
### Issue Context
- `attachDefaultCABundle()` always calls `db.settingsMgr.GetDefaultCABundle()`.
- `ListClusters()` currently calls `attachDefaultCABundle(&clusterList.Items[i])` inside a loop.
### Fix Focus Areas
- util/db/cluster.go[67-76]
- util/db/cluster.go[110-112]
### Suggested approach
- Refactor so the default CA bundle is fetched once per `ListClusters()` call (or once per request) and then applied to all returned clusters.
- Option A: make `attachDefaultCABundle` accept a `defaultCA []byte` argument (already resolved) and only do assignment.
- Option B: in `ListClusters`, call `GetDefaultCABundle()` once, then assign to each cluster.
- If you keep `attachDefaultCABundle(clusters ...*Cluster)`, call it once with all cluster pointers rather than once per cluster.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+68
to
+70
| defaultCA, err := db.settingsMgr.GetDefaultCABundle() | ||
| if err != nil || len(defaultCA) == 0 { | ||
| return |
There was a problem hiding this comment.
2. Swallowed ca read errors 🐞 Bug ◔ Observability
attachDefaultCABundle drops any non-nil error from SettingsManager.GetDefaultCABundle, so failures to read the default CA ConfigMap (e.g., cache sync/RBAC/API errors) become silent and later TLS connection failures lose their root cause.
Agent Prompt
### Issue description
`attachDefaultCABundle()` returns early on any error from `GetDefaultCABundle()` without logging or surfacing the error. This can mask real operational problems (RBAC, API failures, informer sync issues) and makes subsequent TLS errors harder to diagnose.
### Issue Context
- `SettingsManager.GetDefaultCABundle()` returns an error for non-NotFound failures.
- `attachDefaultCABundle()` currently treats *all* errors as “ignore and proceed”.
### Fix Focus Areas
- util/db/cluster.go[67-70]
- util/settings/settings.go[1983-1990]
### Suggested approach
- At minimum: log a warning when `GetDefaultCABundle()` returns a non-NotFound error.
- Preferably: return `(bool, error)` or `error` from `attachDefaultCABundle()` so callers can decide whether to fail the operation (e.g., `GetCluster`) or continue with a warning (e.g., `ListClusters`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1981
to
+1983
| // GetDefaultCABundle returns the default CA bundle byte slice stored in argocd-default-ca-cm (key: ca.crt). | ||
| // If the ConfigMap or key does not exist, an empty byte slice is returned without error. | ||
| func (mgr *SettingsManager) GetDefaultCABundle() ([]byte, error) { |
There was a problem hiding this comment.
3. Misleading ca comment 🐞 Bug ⚙ Maintainability
The GetDefaultCABundle comment says it returns an empty byte slice when missing, but the implementation returns nil; this contradicts the documented behavior and can mislead callers about nil-handling.
Agent Prompt
### Issue description
The doc comment for `GetDefaultCABundle()` says the function returns an “empty byte slice” when missing, but the implementation returns `nil` in those cases.
### Issue Context
Tests also assert `nil`, so either the comment or the behavior needs to change to match.
### Fix Focus Areas
- util/settings/settings.go[1981-1994]
### Suggested approach
- Update the comment to say it returns `nil, nil` when the ConfigMap/key is missing or empty (or change the implementation to return `[]byte{}` consistently, but then update callers/tests accordingly).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
The new configmap name |
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.
Resolves #29170
Summary
argocd-default-ca-cmConfigMap with theca.crtkey for a shared default CA bundle.tlsClientConfig.caDatawhen provided.caData.Testing
go test ./pkg/apis/application/v1alpha1/...— PASSgo test ./util/settings/...— PASSgo test -v ./util/db -run "TestCluster|TestDB_GetCluster"— PASSgo test ./cmd/argocd/commands/admin/...— PASSgofmt— PASSgit diff --check— PASSgo test -v ./util/db -run Test_ListConfiguredGPGPublicKeys— could not run locally becausegpgis not installed