Validate profile config before gateway startup #519
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| package gateway | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/google/jsonschema-go/jsonschema" | ||
|
|
||
| "github.com/docker/mcp-gateway/pkg/catalog" | ||
| "github.com/docker/mcp-gateway/pkg/workingset" | ||
| ) | ||
|
|
||
| // serverValidation holds validation results for a single server. | ||
| type serverValidation struct { | ||
| serverName string | ||
| missingSecrets []string | ||
| missingConfig []string | ||
| imagePullError error | ||
| } | ||
|
|
||
| func validateWorkingSetServerConfigs(ws workingset.WorkingSet) []serverValidation { | ||
| var validationErrors []serverValidation | ||
|
|
||
| for _, server := range ws.Servers { | ||
| if server.Snapshot == nil { | ||
| continue | ||
| } | ||
|
|
||
| serverName := server.Snapshot.Server.Name | ||
| missingConfig := validateServerConfig(server.Snapshot.Server, server.Config) | ||
| if len(missingConfig) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| validationErrors = append(validationErrors, serverValidation{ | ||
| serverName: serverName, | ||
| missingConfig: missingConfig, | ||
| }) | ||
| } | ||
|
|
||
| return validationErrors | ||
| } | ||
|
|
||
| func validateServerConfig(server catalog.Server, serverConfigMap map[string]any) []string { | ||
| var missingConfig []string | ||
|
|
||
| for _, configItem := range server.Config { | ||
| schemaMap, ok := configItem.(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| properties, ok := schemaMap["properties"].(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| requiredProps := requiredConfigProperties(schemaMap) | ||
|
|
||
| for propName, propSchema := range properties { | ||
| propSchemaMap, ok := propSchema.(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| configValue, exists := serverConfigMap[propName] | ||
| if !exists { | ||
| if _, hasDefault := propSchemaMap["default"]; hasDefault { | ||
| continue | ||
| } | ||
| if requiredProps[propName] { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (missing)", propName)) | ||
| } | ||
| continue | ||
| } | ||
| if isEmptyConfigValue(configValue) { | ||
| if requiredProps[propName] { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (missing)", propName)) | ||
| } | ||
| continue | ||
|
Comment on lines
+77
to
+81
There was a problem hiding this comment.
For a profile with a supported object-valued config property, an empty object is still a present value and may satisfy the JSON schema unless that schema adds nested Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Addressed in |
||
| } | ||
|
Comment on lines
+77
to
+82
Contributor
There was a problem hiding this comment.
A negative test would pin it: Minor, separate: an empty optional string now skips
Contributor
Author
There was a problem hiding this comment. Addressed in |
||
|
|
||
| schemaBytes, err := json.Marshal(propSchemaMap) | ||
| if err != nil { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (invalid schema)", propName)) | ||
| continue | ||
| } | ||
|
|
||
| var propSchemaObj jsonschema.Schema | ||
| if err := json.Unmarshal(schemaBytes, &propSchemaObj); err != nil { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (invalid schema)", propName)) | ||
| continue | ||
| } | ||
|
|
||
| resolved, err := propSchemaObj.Resolve(nil) | ||
| if err != nil { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (schema resolution failed)", propName)) | ||
| continue | ||
| } | ||
|
|
||
| if err := resolved.Validate(configValue); err != nil { | ||
| errMsg := err.Error() | ||
| if len(errMsg) > 100 { | ||
| errMsg = errMsg[:97] + "..." | ||
| } | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (%s)", propName, errMsg)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return missingConfig | ||
| } | ||
|
|
||
| func requiredConfigProperties(schemaMap map[string]any) map[string]bool { | ||
| requiredProps := make(map[string]bool) | ||
| switch requiredList := schemaMap["required"].(type) { | ||
| case []any: | ||
| for _, r := range requiredList { | ||
| if s, ok := r.(string); ok { | ||
| requiredProps[s] = true | ||
| } | ||
| } | ||
| case []string: | ||
| for _, s := range requiredList { | ||
| requiredProps[s] = true | ||
| } | ||
| } | ||
| return requiredProps | ||
| } | ||
|
|
||
| func isEmptyConfigValue(v any) bool { | ||
| if v == nil { | ||
| return true | ||
| } | ||
| if s, ok := v.(string); ok { | ||
| return s == "" | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func formatProfileValidationError(profileName string, validationErrors []serverValidation) error { | ||
| var errorMessages []string | ||
| errorMessages = append(errorMessages, fmt.Sprintf("Cannot activate profile '%s'. Validation failed for %d server(s):", profileName, len(validationErrors))) | ||
|
|
||
| for _, validation := range validationErrors { | ||
| errorMessages = append(errorMessages, fmt.Sprintf("\nServer '%s':", validation.serverName)) | ||
|
|
||
| if len(validation.missingSecrets) > 0 { | ||
| errorMessages = append(errorMessages, fmt.Sprintf(" Missing secrets: %s", strings.Join(validation.missingSecrets, ", "))) | ||
| } | ||
|
|
||
| if len(validation.missingConfig) > 0 { | ||
| errorMessages = append(errorMessages, fmt.Sprintf(" Missing/invalid config: %s", strings.Join(validation.missingConfig, ", "))) | ||
| } | ||
|
|
||
| if validation.imagePullError != nil { | ||
| errorMessages = append(errorMessages, fmt.Sprintf(" Image pull failed: %v", validation.imagePullError)) | ||
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("%s", strings.Join(errorMessages, "\n")) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ActivateProfileis the other caller of the sharedvalidateServerConfig, and this refactor moved its inline validation + error formatting out into the new helpers (validateServerConfig+formatProfileValidationError). But there's no test exercising this path — only the startup/readOncepath is covered (TestWorkingSetConfigurationReadRejectsMissingRequiredConfig); agrepforActivateProfileacross*_test.gofinds nothing.Worth adding a test that calls
ActivateProfilewith a server missing a required config value (and ideally also a missing secret) and asserts the aggregated message —Cannot activate profile '…'/Server '…'/Missing/invalid config: … (missing)— so the wiring (does the handler still collectmissingConfigand aggregate it withmissingSecrets/imagePullError?) is locked, not just the helper in isolation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
60153709. AddedTestActivateProfileRejectsMissingRequiredConfig, which exercisesActivateProfilewith a remote server snapshot, missing required config, and a missing secret against an empty secrets-engine response. It asserts the aggregatedCannot activate profile,Server,Missing secrets, andMissing/invalid configmessage pieces.