backend/helm: fix double-write HTTP response and missing headers#5056
backend/helm: fix double-write HTTP response and missing headers#5056NAME-ASHWANIYADAV wants to merge 7 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
size/M
|
@illume hii ! |
|
/assign @illume |
There was a problem hiding this comment.
Pull request overview
Fixes HTTP response handling issues in Helm backend handlers to prevent double-writing responses and ensure JSON Content-Type headers are actually sent.
Changes:
- Add missing
returnafterhttp.ErrorinListChartsand add structured logging forListChartserror paths. - Set
Content-Type: application/jsonbeforeWriteHeader()inListChartsandListReposo clients receive the header. - Add/extend tests to validate
Content-Typeand theListChartserror status code.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/pkg/helm/charts.go | Fix ListCharts error fallthrough, correct header ordering, add structured logs. |
| backend/pkg/helm/repository.go | Correct Content-Type header ordering in ListRepo. |
| backend/pkg/helm/charts_test.go | Add tests for ListCharts error status and Content-Type header. |
| backend/pkg/helm/repository_test.go | Add test ensuring ListRepo sets Content-Type correctly. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: NAME-ASHWANIYADAV 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 |
|
Fixed: changed the indentation of the space to match the other files in the package. |
| func TestListChartError(t *testing.T) { | ||
| // Create a handler with invalid settings that will cause listCharts to fail. | ||
| // Using a non-existent repository config path forces a repo.LoadFile error. | ||
| invalidSettings := cli.New() | ||
| invalidSettings.RepositoryConfig = "/non-existent-path/repositories.yaml" | ||
|
|
There was a problem hiding this comment.
TestListChartError hard-codes an absolute path ("/non-existent-path/repositories.yaml") to force repo.LoadFile to fail. This can be non-deterministic if that path exists in some environments and is less portable across OSes. Prefer using t.TempDir() + filepath.Join(...) to construct a guaranteed-nonexistent file path under a temp directory (or create an invalid YAML file there) so the failure is deterministic.
backend/pkg/helm/repository.go
Outdated
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
|
|
||
| err = json.NewEncoder(w).Encode(response) | ||
| if err != nil { | ||
| logger.Log(logger.LevelError, nil, err, "encoding response") | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
ListRepo writes the 200 status and starts the response before calling json.Encoder.Encode. If encoding fails (e.g., client disconnect / write error), the handler can only log and cannot reliably communicate failure to the client because headers are already sent. Consider encoding into an intermediate buffer first and only then setting headers + status and writing the body, so true encoding failures can return a 5xx cleanly without partially-written JSON.
|
@illume i have implemented the changes and updated the code based on copilot |
Fixes #5055
What this PR does
This PR fixes three related HTTP response handling defects in the Helm backend handlers.
1. Fix missing return after
http.Errorin ListChartsFile: backend/pkg/helm/charts.go
When listCharts() failed, the handler called
http.Error()but did not return. Execution fell through tow.WriteHeader(http.StatusOK), producing:http: superfluous response.WriteHeader call2. Fix
Content-Typeheader ordering in ListCharts and ListRepoFiles: backend/pkg/helm/charts.go, backend/pkg/helm/repository.go
Both handlers set
Content-TypeafterWriteHeader(). In Go'snet/http,WriteHeadersends the response headers immediately — any header mutations afterward are silently ignored. This meantContent-Type: application/jsonwas never sent to clients.3. Add structured logging to charts.go
Unlike repository.go and release.go, charts.go had no
logger.Logcalls. Added structured logging for both error paths in ListCharts for consistent observability.Testing
New tests added
Content-Type: application/jsonheader is correctly set on success responsesContent-Type: application/jsonheader is correctly set for ListRepoVerification results
go vet ./pkg/helm/— Clean (exit code 0)go fmt ./pkg/helm/— No changes neededgo test ./pkg/helm/ -v— All 8 tests pass (1.712s)Changes
logger.Log