Add validation to ateapi ExternalVolume

cmd/ateapi/internal/controlapi/actor.go

@@ -103,7 +103,7 @@ func (s *ServiceImpl) CreateActor(ctx context.Context, inActor *ateapipb.Actor)
 		LatestSnapshot: sourceSnapshotStatus.GetSnapshot(),
 		SourceSnapshot: sourceSnapshotStatus,
 	}
-	if errs := validateActorUpdate(ctx, field.NewPath("actor"), outActor, inActor, true); len(errs) > 0 {
+	if errs := validateActorCreate(ctx, field.NewPath("actor"), outActor); len(errs) > 0 {
 		return nil, toGRPCInternalError(errs)
 	}
 
@@ -481,6 +481,17 @@ func validateSuspendActorRequest(req *ateapipb.SuspendActorRequest) field.ErrorL
 	return errs
 }
 
+func validateActorCreate(ctx context.Context, fldPath *field.Path, val *ateapipb.Actor) field.ErrorList {
+	// Call the generated validation.
+	op := operation.Operation{Type: operation.Create}
+	errs := Validate_Actor(ctx, op, fldPath, val, nil)
+	// Status is optional in the schema, but is actually required to be set
+	// by the server. If it was specified, it was already validated above,
+	// but if it was not specified we need to flag that as an error.
+	errs = append(errs, validate.RequiredPointer(ctx, op, fldPath.Child("status"), val.GetStatus(), nil)...)
+	return errs
+}
+
 func validateActorUpdate(ctx context.Context, fldPath *field.Path, newVal, oldVal *ateapipb.Actor, requireStatus bool) field.ErrorList {
 	// Call the generated validation.
 	op := operation.Operation{Type: operation.Update}

cmd/ateapi/internal/controlapi/validate.go

@@ -17,6 +17,7 @@ package controlapi
 import (
 	"context"
 	"reflect"
+	"strings"
 
 	"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
 	"google.golang.org/grpc/codes"
@@ -97,3 +98,35 @@ func ValidateCustom_UpdateActorRequest_Actor(ctx context.Context, op operation.O
 func ValidateCustom_WorkerAssignment_WorkerPodIp(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *string) field.ErrorList {
 	return validation.IsValidIP(fldPath, *value)
 }
+
+// ValidateCustom_ExternalVolume_VolumeType checks that a volume type string is well-formed.
+// It allows an optional "substrate.io/" prefix, followed by a valid DNS-1123 subdomain.
+func ValidateCustom_ExternalVolume_VolumeType(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *string) field.ErrorList {
+	if value == nil || *value == "" {
+		return nil
+	}
+	var errs field.ErrorList
+	valToValidate := strings.TrimPrefix(*value, "substrate.io/")
+	for _, msg := range validation.IsDNS1123Subdomain(valToValidate) {
+		errs = append(errs, field.Invalid(fldPath, *value, msg))
+	}
+	return errs
+}
+
+// ValidateCustom_ExternalVolume_StorageVolumeId checks that an external volume's storage ID does not
+// contain control characters (U+0000-U+0008, U+000B, U+000C, U+000E-U+001F, U+007F-U+009F).
+func ValidateCustom_ExternalVolume_StorageVolumeId(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ *string) field.ErrorList {
+	if value == nil || *value == "" {
+		return nil
+	}
+	for _, r := range *value {
+		if (r >= 0x0000 && r <= 0x0008) ||
+			r == 0x000B ||
+			r == 0x000C ||
+			(r >= 0x000E && r <= 0x001F) ||
+			(r >= 0x007F && r <= 0x009F) {
+			return field.ErrorList{field.Invalid(fldPath, *value, "must not contain control characters (U+0000-U+0008, U+000B, U+000C, U+000E-U+001F, U+007F-U+009F)")}
+		}
+	}
+	return nil
+}

cmd/ateapi/internal/controlapi/validation_test.go

@@ -776,3 +776,194 @@ func TestValidateTrustBundleDataSource(t *testing.T) {
 		})
 	}
 }
+
+func validExternalVolume(mutate ...func(*ateapipb.ExternalVolume)) *ateapipb.ExternalVolume {
+	v := &ateapipb.ExternalVolume{
+		VolumeName:      "my-vol",
+		StorageVolumeId: "valid-storage-id",
+		VolumeType:      "mock",
+		Status:          ateapipb.ExternalVolume_STATUS_CREATED,
+	}
+	for _, m := range mutate {
+		m(v)
+	}
+	return v
+}
+
+func TestValidateExternalVolume(t *testing.T) {
+	valid := validExternalVolume
+
+	tests := []struct {
+		name string
+		obj  *ateapipb.ExternalVolume
+		want field.ErrorList
+	}{
+		{
+			name: "valid external volume",
+			obj:  valid(),
+		},
+		{
+			name: "valid external volume with empty storage volume id",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "" }),
+		},
+		{
+			name: "invalid storage volume id with null U+0000",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol\x00id" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "")},
+		},
+		{
+			name: "invalid storage volume id with unit separator U+001F",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol\x1fid" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "")},
+		},
+		{
+			name: "invalid storage volume id with DEL U+007F",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol\x7fid" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "")},
+		},
+		{
+			name: "invalid storage volume id with C1 control U+0080",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol\u0080id" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "")},
+		},
+		{
+			name: "invalid storage volume id with C1 control U+009F",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol\u009fid" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "")},
+		},
+		{
+			name: "valid volume type",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "mock" }),
+		},
+		{
+			name: "valid csi volume type",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "pd.csi.storage.gke.io" }),
+		},
+		{
+			name: "missing volume type",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "" }),
+			want: field.ErrorList{field.Required(field.NewPath("volume_type"), "")},
+		},
+		{
+			name: "invalid volume type with uppercase",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "MockPlugin" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("volume_type"), nil, "")},
+		},
+		{
+			name: "valid volume type with 253 characters",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = strings.Repeat("a", 253) }),
+		},
+		{
+			name: "invalid volume type exceeding 253 characters",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = strings.Repeat("a", 254) }),
+			want: field.ErrorList{
+				field.Invalid(field.NewPath("volume_type"), nil, ""),
+				field.TooLong(field.NewPath("volume_type"), nil, 253).WithOrigin("maxLength"),
+			},
+		},
+		{
+			name: "valid volume with substrate.io prefixed volume type",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "substrate.io/mock" }),
+		},
+		{
+			name: "invalid volume type with empty plugin after substrate.io prefix",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "substrate.io/" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("volume_type"), nil, "")},
+		},
+		{
+			name: "invalid volume type with invalid plugin name after substrate.io prefix",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "substrate.io/Mock_Plugin" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("volume_type"), nil, "")},
+		},
+		{
+			name: "missing volume name",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeName = "" }),
+			want: field.ErrorList{field.Required(field.NewPath("volume_name"), "")},
+		},
+		{
+			name: "invalid volume name exceeding 63 characters",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeName = strings.Repeat("a", 64) }),
+			want: field.ErrorList{field.TooLong(field.NewPath("volume_name"), nil, 63).WithOrigin("maxLength")},
+		},
+		{
+			name: "invalid volume type with non-substrate prefix",
+			obj:  valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "other.io/mock" }),
+			want: field.ErrorList{field.Invalid(field.NewPath("volume_type"), nil, "")},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			obj := proto.CloneOf(tt.obj)
+			op := operation.Operation{Type: operation.Create}
+			matcher := field.ErrorMatcher{}.ByType().ByField()
+			matcher.Test(t, tt.want, Validate_ExternalVolume(context.Background(), op, nil, obj, nil))
+		})
+	}
+}
+
+func TestValidateExternalVolume_Update(t *testing.T) {
+	valid := validExternalVolume
+
+	tests := []struct {
+		name   string
+		oldObj *ateapipb.ExternalVolume
+		newObj *ateapipb.ExternalVolume
+		want   field.ErrorList
+	}{
+		{
+			name:   "unchanged volume is valid",
+			oldObj: valid(),
+			newObj: valid(),
+			want:   nil,
+		},
+		{
+			name:   "volume_name changed is invalid",
+			oldObj: valid(func(v *ateapipb.ExternalVolume) { v.VolumeName = "vol1" }),
+			newObj: valid(func(v *ateapipb.ExternalVolume) { v.VolumeName = "vol2" }),
+			want:   field.ErrorList{field.Invalid(field.NewPath("volume_name"), nil, "").WithOrigin("immutable")},
+		},
+		{
+			name:   "storage_volume_id transition from empty to non-empty is valid",
+			oldObj: valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "" }),
+			newObj: valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol-id-1" }),
+			want:   nil,
+		},
+		{
+			name:   "storage_volume_id changed once set is invalid",
+			oldObj: valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol-id-1" }),
+			newObj: valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol-id-2" }),
+			want:   field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "").WithOrigin("update")},
+		},
+		{
+			name:   "storage_volume_id unset once set is invalid",
+			oldObj: valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "vol-id-1" }),
+			newObj: valid(func(v *ateapipb.ExternalVolume) { v.StorageVolumeId = "" }),
+			want:   field.ErrorList{field.Invalid(field.NewPath("storage_volume_id"), nil, "").WithOrigin("update")},
+		},
+		{
+			name:   "volume_type changed is invalid",
+			oldObj: valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "mock" }),
+			newObj: valid(func(v *ateapipb.ExternalVolume) { v.VolumeType = "pd.csi.storage.gke.io" }),
+			want:   field.ErrorList{field.Invalid(field.NewPath("volume_type"), nil, "").WithOrigin("immutable")},
+		},
+		{
+			name: "status and volume_context changed is valid",
+			oldObj: valid(func(v *ateapipb.ExternalVolume) {
+				v.Status = ateapipb.ExternalVolume_STATUS_PENDING
+				v.VolumeContext = nil
+			}),
+			newObj: valid(func(v *ateapipb.ExternalVolume) {
+				v.Status = ateapipb.ExternalVolume_STATUS_CREATED
+				v.VolumeContext = map[string]string{"foo": "bar"}
+			}),
+			want: nil,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			op := operation.Operation{Type: operation.Update}
+			matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
+			matcher.Test(t, tt.want, Validate_ExternalVolume(context.Background(), op, nil, tt.newObj, tt.oldObj))
+		})
+	}
+}

cmd/ateapi/internal/controlapi/volumes.go

@@ -30,6 +30,9 @@ import (
 
 // initialActorVolumes constructs initial volume objects in PENDING state before volume creation.
 func initialActorVolumes(ctx context.Context, scLister storagev1listers.StorageClassLister, template *ateapipb.ActorTemplate) ([]*ateapipb.ExternalVolume, error) {
+	if template == nil {
+		return nil, status.Error(codes.InvalidArgument, "template is required")
+	}
 	var volumes []*ateapipb.ExternalVolume
 	for _, vol := range template.GetVolumes() {
 		if vol.GetExternalVolumeTemplate() != nil {

cmd/ateapi/internal/controlapi/volumes_test.go

@@ -117,6 +117,13 @@ func TestInitialActorVolumes_PendingState(t *testing.T) {
 	}
 }
 
+func TestInitialActorVolumes_NilTemplate(t *testing.T) {
+	_, err := initialActorVolumes(context.Background(), nil, nil)
+	if err == nil {
+		t.Fatal("expected error for nil template, got nil")
+	}
+}
+
 func TestCreateActorVolumes(t *testing.T) {
 	ctx := context.Background()