Repository navigation
fix(releases): use helm secret namespace for cross-namespace health - #758
Tyagiquamar wants to merge 4 commits into
Conversation
Track the namespace where each release secret is stored separately from release.Namespace so API routes and helm actions work when those differ. Fixes komodorio#683
undera
left a comment
There was a problem hiding this comment.
Thanks for picking up #683. The direction is right — the release's storage namespace and its target namespace differ — but in its current form this doesn't fix the reported case.
1. Default mode is unchanged. Without -n, Namespaces is [""] (main.go:62). List() then passes "" as the storage namespace, NewRelease falls back to orig.Namespace, and /api/helm/releases still reports projectcontour for educates-contour. The fix only works when namespaces are passed explicitly. The storage namespace can't be recovered from the release.Release object either: Helm's secrets driver drops the secret's namespace on decode (pkg/storage/driver/secrets.go, List only copies Labels). So for the all-namespaces case it has to come from somewhere else, e.g. listing the release secrets/configmaps directly, or iterating over the visible namespaces.
2. ReleaseElement.Namespace gets overloaded. That field is the release's target namespace: the UI displays it, and the resources handler falls back to the :ns URL param for manifest objects without metadata.namespace (helmHandlers.go:158). Replacing it with the storage namespace makes health lookups for those objects hit the wrong namespace. The two values need to be separate fields: the storage namespace for routing and Helm actions, the target namespace for display and resource fallback. The frontend needs a matching change to build URLs from the storage namespace.
3. Minor:
client.AllNamespaces = falseis effectively a no-op forns == "", since the driver is initialised with an empty namespace and lists everything anyway.- The tests cover the fallback helper, not the list path that actually breaks. A test for
List()in the all-namespaces mode, with storage namespace ≠ release namespace, would pin the bug down. - Please fill in the PR description and link #683.
The error messages now including the namespace, and History/Rollback/Uninstall using the storage namespace once it's known, are both correct and worth keeping.
…iscover all-namespaces
|
@undera Thanks for the clear and detailed review. Addressed all items in commit
|
undera
left a comment
There was a problem hiding this comment.
Thanks for the update. The two fields are now separated correctly, and routing by storageNamespace in the frontend fixes the URL problem from #683. A few things are still open:
1. build is failing. TestE2E panics with a nil pointer: GetReleases → Releases.List → K8s.GetNameSpaces → cfgProxyObject.ToRESTConfig (kubectl.go:46). The new namespace discovery runs in the default [""] mode, and the test's fake kube config can't serve it. This can be reproduced locally with go test ./pkg/dashboard/ -run TestE2E.
2. A failed namespace listing is swallowed silently. If GetNameSpaces() errors (for example, RBAC without cluster-wide list namespaces), List() falls back to AllNamespaces = true without logging anything. In that fallback, #683 quietly comes back. Please log the error so the degraded mode is visible.
3. Cost per refresh. In the default mode, every /api/helm/releases call now makes one namespaces call plus one Helm list call per namespace, and the UI polls this endpoint. On clusters with hundreds of namespaces this turns one API round trip into hundreds. One alternative is to keep the single AllNamespaces = true list and get the storage namespace from one cluster-wide list of the release objects' metadata (label owner=helm, using the configured driver: secrets or configmaps). That maps name+version to the namespace in a single call.
4. The new test doesn't exercise the fixed path. TestListReleases_StorageNamespace sets Namespaces: []string{"educates"}, the explicit-namespace mode that already worked after the first commit, and fakeHelmNSConfigGetter ignores the namespace anyway. The case that was broken is Namespaces: []string{""}. A test for that case would also have caught (1).
Minor: in Resources, rel can't be nil once ByName returned no error, so the rel != nil && rel.Orig != nil guard isn't needed. Falling back to rel.Orig.Namespace is the right fix.
|
Thanks for the thorough review and guidance @undera! All feedback has been addressed in commit 1be7780:
Verified with |
|
Extracted TestHReleaseToJSON_StorageNamespace into a separate test function in commit aab18ef to resolve the CI gocyclo threshold failure (gocyclo -over 19). Both gocyclo -over 19 main.go pkg and go test ./pkg/dashboard/... pass cleanly. |
Summary
Separate release storage namespace from target manifest namespace, discover release storage namespaces in all-namespaces mode, and align frontend routing with storage namespaces.
Fixes #683
Root cause
Helm allows deploying manifests into a target namespace while storing the release secret or configmap in a distinct storage namespace. Previously:
-nwas omitted (Namespaces: [""]),List()passed"", causingNewReleaseto fall back toorig.Namespaceinstead of discovering the storage namespace.ReleaseElement, using a singleNamespacefield overloaded target display and Helm storage routing: health lookups for manifests without an explicit namespace fell back to the URL parameter, querying the wrong namespace.Technical solution
ReleaseElement(pkg/dashboard/handlers/helmHandlers.go), keepNamespaceas the release target namespace (orig.Namespace), and addStorageNamespacefor where Helm stores release data.HReleaseToJSON, populate both fields.Resourceshandler, fall back torel.Orig.Namespacefor manifest objects missing an explicitmetadata.namespace.Releases.List()(pkg/dashboard/objects/releases.go), when running in all-namespaces mode (Namespaces: [""]), resolve visible cluster namespaces viaK8s.GetNameSpaces()and iterate through each namespace, settingclient.AllNamespaces = falseto capture the exact storage namespace for each release. If namespace listing is unavailable, fall back toclient.AllNamespaces = true.storageNamespacetoReleaseinfrontend/src/data/types.ts.apiService.getResourceStatusandInstalledPackageCardonClick navigation to route viastorageNamespace || namespace.TestHReleaseToJSONinpkg/dashboard/handlers/helmHandlers_test.goto assertNamespaceandStorageNamespaceare preserved separately.TestListReleases_StorageNamespaceinpkg/dashboard/objects/releases_test.goverifying that releases with differing target and storage namespaces preserve both values.Test plan
pkg/dashboard/handlersandpkg/dashboard/objectstests in Docker (golang:1.26): all passed.gofmt.