Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/api/internal/handlers/sandbox_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ var errVolumesNotSupported = errors.New("volumes are not supported")

var errNoEnvdVersion = errors.New("no envd version provided")

const minEnvdVersionForVolumes = "0.5.8"
const minEnvdVersionForVolumes = "0.5.12"
Comment on lines 316 to +319
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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:

  1. User calls POST /sandboxes with volumeMounts: [{name: "my-vol", path: "/data"}] using a very old template where build.EnvdVersion == nil.
  2. PostSandboxes calls convertAPIVolumesToOrchestratorVolumes(..., build).
  3. Inside that function: envdVersion := sharedUtils.DerefOrDefault(env.EnvdVersion, "") // returns "".
  4. The condition envdVersion == "" is true → function returns nil, errNoEnvdVersion.
  5. Back in PostSandboxes: errors.Is(err, errVolumesNotSupported) → false. errors.Is(err, ErrVolumeMountsDisabled) → false. errors.As(err, &vne) for InvalidVolumeMountsError → false.
  6. Execution falls to telemetry.ReportError(...) and a.sendAPIStoreError(c, http.StatusInternalServerError, "failed to convert volume mounts").
  7. User receives a 500 with no actionable information.


func convertAPIVolumesToOrchestratorVolumes(ctx context.Context, sqlClient *sqlcdb.Client, featureFlags featureFlagsClient, teamID uuid.UUID, volumeMounts []api.SandboxVolumeMount, env *queries.EnvBuild) ([]*orchestrator.SandboxVolumeMount, error) {
// are any volumes configured?
Expand Down
4 changes: 2 additions & 2 deletions packages/api/internal/handlers/sandbox_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ func TestOrchestrator_convertVolumeMounts(t *testing.T) {

actual, err := convertAPIVolumesToOrchestratorVolumes(
t.Context(), db.SqlcClient, ffClient,
teamID, tc.input, &queries.EnvBuild{EnvdVersion: utils.ToPtr("0.5.8")},
teamID, tc.input, &queries.EnvBuild{EnvdVersion: utils.ToPtr(minEnvdVersionForVolumes)},
)
assert.Equal(t, tc.err, err)
assert.Equal(t, tc.expected, actual)
Expand Down Expand Up @@ -484,7 +484,7 @@ func TestOrchestrator_convertVolumeMounts(t *testing.T) {
t.Context(), db.SqlcClient, ffClient,
teamID, []api.SandboxVolumeMount{
{Name: "vol1", Path: "/vol1"},
}, &queries.EnvBuild{EnvdVersion: utils.ToPtr("0.5.8")},
}, &queries.EnvBuild{EnvdVersion: utils.ToPtr(minEnvdVersionForVolumes)},
)
require.NoError(t, err)
assert.Equal(t, []*orchestrator.SandboxVolumeMount{
Expand Down
2 changes: 1 addition & 1 deletion packages/envd/pkg/version.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
package pkg

const Version = "0.5.11"
const Version = "0.5.12"
Loading