MGMT-22151: Fix multipart form file cleanup on V2UploadLogs validation errors

[maorfr]

Problem

When uploading logs via POST /v2/clusters/{cluster_id}/logs, if file size validation fails (e.g., file exceeds the 250MB limit), the error response is returned before the handler function executes. This means the defer cleanup block in v2uploadLogs never runs, leaving multipart temporary files on disk and causing disk space issues over time.

Solution

Added cleanup logic in the generated ServeHTTP method's error path to ensure multipart form files are removed when BindValidRequest fails.

Why This Location?

The cleanup is placed in restapi/operations/installer/v2_upload_logs.go at the error path of BindValidRequest because Validation happens before handler execution: The BindValidRequest call (line 61) performs all parameter validation, including file size checks, before the handler is invoked. If validation fails, the handler's defer cleanup never executes.

[openshift-ci-robot]

@maorfr: This pull request references MGMT-22151 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

In response to this:

Problem

When uploading logs via POST /v2/clusters/{cluster_id}/logs, if file size validation fails (e.g., file exceeds the 250MB limit), the error response is returned before the handler function executes. This means the defer cleanup block in v2uploadLogs never runs, leaving multipart temporary files on disk and causing disk space issues over time.

Solution

Added cleanup logic in the generated ServeHTTP method's error path to ensure multipart form files are removed when BindValidRequest fails.

Why This Location?

The cleanup is placed in restapi/operations/installer/v2_upload_logs.go at the error path of BindValidRequest because Validation happens before handler execution: The BindValidRequest call (line 61) performs all parameter validation, including file size checks, before the handler is invoked. If validation fails, the handler's defer cleanup never executes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai[bot]]

Walkthrough

Adds resource cleanup logic to the V2UploadLogs handler to release multipart form files and request body when parameter binding or validation fails, preventing resource leaks during error handling.

Changes

Cohort / File(s)	Summary
Error Path Cleanup restapi/operations/installer/v2_upload_logs.go	Adds multipart form cleanup and request body closure on BindValidRequest failure in ServeHTTP

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify cleanup logic executes only on validation failure and not on success path
• Confirm multipart form removal and body closure do not interfere with subsequent error response handling
• Check that cleanup operations handle edge cases where form or body may be absent

✨ Finishing touches

• [ ] 📝 Generate docstrings

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment

[!WARNING] There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maorfr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [maorfr]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[eranco74]

Hmm, see the first line in the file: // Code generated by go-swagger; DO NOT EDIT. We need to change the template that generates this

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 43.23%. Comparing base (a77e580) to head (12c0b01). :warning: Report is 249 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8294      +/-   ##
==========================================
- Coverage   43.24%   43.23%   -0.01%     
==========================================
  Files         405      405              
  Lines       70495    70495              
==========================================
- Hits        30484    30481       -3     
- Misses      37279    37281       +2     
- Partials     2732     2733       +1     

see 2 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[openshift-ci[bot]]

@maorfr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/edge-verify-generated-code	12c0b0106de4fa9160a0d4e3e10c19d7d79246cc	link	true	/test edge-verify-generated-code

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[rccrdpccl]

A possible workaround could be setting a middleware api.AddMiddlewareFor("POST", "/v2/clusters/{cluster_id}/logs", myMultipartCleanupFunction), while we wait for upstream contirbution to swagger to trickle down.

Haven't really looked into this, but it could be a path forward

[carbonin]

Also this is a thing that should be handled by go's http server request handling logic. Specifically here. So the question I want answered is why is that not working?

I wrote a test program and the only way I could get the files to stick around was with a panic, but we didn't see any in the logs.

[eranco74]

As discussed f2f, this issue might be related to: https://github.com/openshift/assisted-service/pull/8355

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale