Validate profile config before gateway startup#519
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f04c88f1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isEmptyConfigValue(configValue) { | ||
| if requiredProps[propName] { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (missing)", propName)) | ||
| } | ||
| continue |
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 required or minProperties. This branch treats every empty map[string]any as missing and returns before resolved.Validate can decide, so gateway run --profile and profile activation now reject valid configs such as a required headers: {} object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e8bbf861. isEmptyConfigValue now only treats nil and empty strings as missing, so an empty object like headers: {} is passed through to JSON Schema validation. I also added TestValidateServerConfigAllowsEmptyObjectValue to cover the required empty-object case.
saucow
left a comment
saucow
left a comment
There was a problem hiding this comment.
Review — validate profile config before startup
Nice change. The key win is net-new: readOnce (the profile-backed startup path) had no config validation on main, and this adds it fail-closed at configuration_workingset.go:77-78, before any volume eval — so the cryptic Docker bind error (invalid docker volume ":/home/appuser/.kube/config") is replaced by the same actionable message activation already produced. The extraction into validateServerConfig reads faithful to the deleted inline logic, and the two behavioral changes both tighten (the new []string required arm, which the old []any-only cast silently skipped; and rejecting present-but-empty required values). It doesn't touch the bind or URL hardening seams.
Recommendation: approve-with-nits — two test-coverage gaps and one optional edge case, all inline; none blocking.
| } | ||
| } | ||
| } | ||
| validation.missingConfig = append(validation.missingConfig, validateServerConfig(serverConfig, profileConfig.config[serverName])...) |
There was a problem hiding this comment.
ActivateProfile is the other caller of the shared validateServerConfig, 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/readOnce path is covered (TestWorkingSetConfigurationReadRejectsMissingRequiredConfig); a grep for ActivateProfile across *_test.go finds nothing.
Worth adding a test that calls ActivateProfile with 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 collect missingConfig and aggregate it with missingSecrets/imagePullError?) is locked, not just the helper in isolation.
There was a problem hiding this comment.
Addressed in 60153709. Added TestActivateProfileRejectsMissingRequiredConfig, which exercises ActivateProfile with a remote server snapshot, missing required config, and a missing secret against an empty secrets-engine response. It asserts the aggregated Cannot activate profile, Server, Missing secrets, and Missing/invalid config message pieces.
| if isEmptyConfigValue(configValue) { | ||
| if requiredProps[propName] { | ||
| missingConfig = append(missingConfig, fmt.Sprintf("%s (missing)", propName)) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
isEmptyConfigValue returns before resolved.Validate, so the schema-validation branch below (the resolved.Validate(configValue) block) is the only thing keeping the empty-{} relaxation narrow: a required string-typed field set to {} or [] is caught only there, not by isEmptyConfigValue. That backstop currently has no test — a one-line future change broadening isEmptyConfigValue to also treat empty maps/slices as "skip" would silently drop the type-mismatch check with nothing failing.
A negative test would pin it: validateServerConfig should reject config_path: {} (and config_path: []) for a string-typed required field. (TestValidateServerConfigAllowsEmptyObjectValue covers the allow side; this covers the reject side that makes the allow safe.)
Minor, separate: an empty optional string now skips minLength/pattern validation it previously hit. Almost certainly fine — the registry import schema generator doesn't emit those constraints — just flagging the behavior change for hand-authored catalog/profile schemas.
There was a problem hiding this comment.
Addressed in 60153709. Added TestValidateServerConfigRejectsWrongTypeForRequiredString covering config_path: {} and config_path: [] against a required string schema, so those values are rejected by JSON Schema validation rather than treated as missing or skipped.
saucow
left a comment
saucow
left a comment
There was a problem hiding this comment.
Approving — this adds net-new fail-closed config validation on the profile-backed startup path (which had none), the extraction is faithful to the deleted inline logic, and both behavioral changes tighten. The items in my earlier review are test-coverage gaps (the untested ActivateProfile path and the schema-reject backstop) plus one optional edge case — none are blocking.
saucow
left a comment
saucow
left a comment
There was a problem hiding this comment.
Re-reviewed — the test additions resolve both of my earlier comments:
TestActivateProfileRejectsMissingRequiredConfigdrives theActivateProfile→validateServerConfigwiring end to end and asserts the aggregated error (including the missing-secret case).TestValidateServerConfigRejectsWrongTypeForRequiredStringlocks the schema-reject backstop forconfig_path: {}/[], so the empty-object relaxation stays narrow.
Verified the package tests pass locally. Approving — thanks for the quick turnaround.
2 participants
Summary
config_pathWhy This Is Needed
docker mcp gateway run --profile <id>usesWorkingSetConfigurationto load profile-backed servers at startup. That path was not running the same required config validation thatmcp-activate-profileruns for dynamically activated profiles.In v0.43.0, Docker volume handling was hardened to reject malformed bind mounts before invoking Docker. That is correct, but it exposed missing profile config as a low-level Docker volume error. For the Kubernetes catalog entry, the volume template is:
When
config_pathis missing from the profile, the template evaluates to:The gateway then reports:
That message is technically true, but it points users at Docker volume syntax instead of the real problem: the profile is missing required Kubernetes config.
With this change, profile startup fails before container arg construction with the same style of validation error used by profile activation, for example:
A user can then fix the profile directly:
Validation
go test ./pkg/gatewaygo test ./pkg/gateway ./pkg/workingset