Bump version for envd, require it for nfs#2385
Conversation
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit a9febb3. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c1920c3. Configure here.
|
|
||
| var errNoEnvdVersion = errors.New("no envd version provided") | ||
|
|
||
| const minEnvdVersionForVolumes = "0.5.8" | ||
| const minEnvdVersionForVolumes = "0.5.12" |
There was a problem hiding this comment.
🟣 Pre-existing bug: When a template has no recorded envd version (nil/empty EnvdVersion), convertAPIVolumesToOrchestratorVolumes returns errNoEnvdVersion, but PostSandboxes does not check for this error — it falls through to the generic handler and returns HTTP 500 with an opaque "failed to convert volume mounts" message instead of a helpful HTTP 400 asking the user to rebuild their template. The fix is to add errors.Is(err, errNoEnvdVersion) alongside the existing errVolumesNotSupported check and return a 400 with a clear rebuild message.
Extended reasoning...
Bug Analysis
What the bug is: In convertAPIVolumesToOrchestratorVolumes, when env.EnvdVersion is nil or an empty string, the function returns the sentinel error errNoEnvdVersion (line 336). The caller, PostSandboxes, has a dedicated error-handling block for the result of this function (lines ~197–218), but it only checks for three specific errors: errVolumesNotSupported, ErrVolumeMountsDisabled, and InvalidVolumeMountsError. errNoEnvdVersion is not among them.
The specific code path: When a user submits a sandbox creation request with volume mounts and their template's EnvdVersion field is nil or empty, execution reaches convertAPIVolumesToOrchestratorVolumes → hits the envdVersion == "" branch → returns (nil, errNoEnvdVersion) → back in PostSandboxes, none of the three if branches match → falls to the final else branch → telemetry.ReportError is called (treating it as an unexpected internal error) → http.StatusInternalServerError is returned with the message "failed to convert volume mounts".
Why existing code doesn't prevent it: The three error checks are each exact sentinel comparisons or type assertions. errNoEnvdVersion is a distinct sentinel defined on line 317, and it is never wrapped when returned (line 336 is a bare return nil, errNoEnvdVersion), so errors.Is would match it — but no such check exists in the handler.
Impact: Users whose templates were built without a recorded envd version receive an opaque HTTP 500 that gives no hint about the root cause or what to do. The correct behavior — returning HTTP 400 with a message like "template must be rebuilt" — already exists for the analogous errVolumesNotSupported case; errNoEnvdVersion simply needs the same treatment.
How to fix it: Add a check immediately after (or before) the errVolumesNotSupported check:
if errors.Is(err, errNoEnvdVersion) {
a.sendAPIStoreError(c, http.StatusBadRequest, "Template must be rebuilt to support volume mounts (no envd version recorded).")
return
}Step-by-step proof:
- User calls
POST /sandboxeswithvolumeMounts: [{name: "my-vol", path: "/data"}]using a very old template wherebuild.EnvdVersion == nil. PostSandboxescallsconvertAPIVolumesToOrchestratorVolumes(..., build).- Inside that function:
envdVersion := sharedUtils.DerefOrDefault(env.EnvdVersion, "") // returns "". - The condition
envdVersion == ""is true → function returnsnil, errNoEnvdVersion. - Back in
PostSandboxes:errors.Is(err, errVolumesNotSupported)→ false.errors.Is(err, ErrVolumeMountsDisabled)→ false.errors.As(err, &vne)forInvalidVolumeMountsError→ false. - Execution falls to
telemetry.ReportError(...)anda.sendAPIStoreError(c, http.StatusInternalServerError, "failed to convert volume mounts"). - User receives a 500 with no actionable information.

No description provided.