Conversation
jason-lynch
force-pushed
the
feat/PLAT-86/additional-request-validation
branch
from
eae7d95 to
ad9c9be
Compare
jason-lynch
force-pushed
the
feat/PLAT-86/censor-sensitive-fields
branch
from
04ace88 to
5d5c2b2
Compare
Adds more validation logic that could not be be enforced in the OpenAPI spec to the `server/internal/api/v1` package. PLAT-86
Builds on the existing volume validation to surface more detailed error information, including the host, node, and underlying error message. Also makes some minor changes that make it possible to distinguish between invalid volumes and other types of errors. If an unexpected error occurs, such as an image pull error, the user will get a 500 instead of a 400 error. PLAT-86
Removes the identifier pattern from the OpenAPI spec so that users get the better validation error message that we produce in the API code. PLAT-86
Refactors `ValidateVolumes` to `ValidateInstanceSpec` and adds some simple port validation. Note that this only validates that the port is unoccupied on the host. It would not catch issues within the spec, for example allocating the same port across multiple hosts. PLAT-86
jason-lynch
force-pushed
the
feat/PLAT-86/additional-request-validation
branch
from
ad9c9be to
b949a1e
Compare
Adapts the `ValidateInstanceSpec` activity to take and validate multiple specs in order to detect when a port has been allocated multiple times on a single host. Note that this will not consistently detect when a port has been allocated multiple times on a machine when there are multiple control-plane instances with different host IDs on the same machine - as in our development configuration. End users should not run the control plane this way, so we donn't need to handle that case explicitly. PLAT-86
jason-lynch
force-pushed
the
feat/PLAT-86/additional-request-validation
branch
from
b949a1e to
4ac0465
Compare
tsivaprasad
left a comment
tsivaprasad
left a comment
There was a problem hiding this comment.
Awesome work!
The verification and failure logs are very detailed and informative. For example:
HTTP/1.1 400 Bad Request
Content-Length: 150
Content-Type: application/json
Date: Wed, 18 Jun 2025 10:04:50 GMT
Goa-Error: invalid_input
{
message: "validation error for node n1, host host-1: bind source path does not exist: /host_mnt/Users/sivat/backups/host1"
name: "invalid_input"
}
The PR looks good overall—just a couple of comments just see if that make sense.
| func validateBackupRepository(cfg *api.BackupRepositorySpec, path []string) []error { | ||
| props := repoProperties{ | ||
| id: cfg.ID, | ||
| repoType: cfg.Type, | ||
| azureAccount: cfg.AzureAccount, | ||
| azureContainer: cfg.AzureContainer, | ||
| azureKey: cfg.AzureKey, | ||
| basePath: cfg.BasePath, | ||
| gcsBucket: cfg.GcsBucket, | ||
| s3Bucket: cfg.S3Bucket, | ||
| s3Region: cfg.S3Region, | ||
| customOptions: cfg.CustomOptions, | ||
| } | ||
|
|
||
| return validateRepoProperties(props, path) | ||
| } | ||
|
|
||
| func validateRestoreRepository(cfg *api.RestoreRepositorySpec, path []string) []error { | ||
| props := repoProperties{ | ||
| id: cfg.ID, | ||
| repoType: cfg.Type, | ||
| azureAccount: cfg.AzureAccount, | ||
| azureContainer: cfg.AzureContainer, | ||
| azureKey: cfg.AzureKey, | ||
| basePath: cfg.BasePath, | ||
| gcsBucket: cfg.GcsBucket, | ||
| s3Bucket: cfg.S3Bucket, | ||
| s3Region: cfg.S3Region, | ||
| customOptions: cfg.CustomOptions, | ||
| } | ||
|
|
||
| return validateRepoProperties(props, path) | ||
| } |
There was a problem hiding this comment.
These two functions perform the same logic. We can refactor them by introducing a common helper function (e.g., buildRepoProperties). What do you think?
There was a problem hiding this comment.
In my opinion, it's better to use structs when there are many options - especially when they share the same type. It's easier to use and less error-prone when you can easily see that the property and its value match up.
| cpusPath := appendPath(path, "cpus") | ||
| errs = append(errs, validateCPUs(node.Cpus, cpusPath)...) |
There was a problem hiding this comment.
In many places, we’re assigning the result of appendPath(...) to a variable and using it just once — for example this code. If we’re reusing the result multiple times this is good, otherwise better to call directly like below
errs = append(errs, validateCPUs(node.Cpus, appendPath(path, "cpus"))...)
What do you think?
There was a problem hiding this comment.
This was an intentional choice that I made because I felt it enhanced readability. I find it a little easier to read when these are on separate lines, so I'd like to keep this as-is.
|
|
||
| var futures []workflow.Future[*activities.ValidateInstanceSpecsOutput] | ||
| for _, instances := range instancesByHost { | ||
| if len(instances) < 1 { |
There was a problem hiding this comment.
Is there a specific reason for using len(instances) < 1 instead of len(instances) == 0?
There was a problem hiding this comment.
This was a common idiom when I first learned to program, and most people that I've worked with have used it as well. But, there's not a good reason for it in Go, so I'm willing to switch to == 0 going forward.
2 participants
This PR adds validation that was not possible (or not as user-friendly) to implement in our API spec. That validation includes:
It also improves the error messages for some existing validations, like CPUs, memory, and volume validation so that they include more details about what failed and where it is in the spec.
PLAT-86