node: refuse a config schema this build cannot read - #300
Merged
Conversation
NodeConfig carried a version field that nothing ever read, and LoadNodeConfig used a non-strict unmarshal. A node handed a future config would therefore ignore the version, silently drop every key it did not recognise, and carry on. Under attenuation that means quietly running without rules the operator wrote. Forward compatibility has to be installed before it is needed, because binaries already in the field cannot be retrofitted: the version's value today is entirely in the rejection path. Gate on SupportedNodeConfigVersions and parse strictly, so an unreadable schema fails loudly either by declaring a version we do not know or by carrying a key we do not know. An absent version still reads as v1alpha1, since files predating the check omit it. Adding a schema later means listing it, decoding it into NodeConfigComplete, and leaving the rest of the node version-agnostic; that internal type already exists, only the gate was missing. Verified against every node config in the repo: the example configs, the seven .github/k8s ConfigMaps and the documentation snippets all parse clean under the strict loader.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces strict YAML unmarshaling and version validation for node configurations to prevent silent misinterpretations of attenuation rules. It defines supported config versions and adds corresponding unit tests for validation scenarios. The review feedback suggests replacing the mutable package-level map SupportedNodeConfigVersions with a read-only function to avoid potential concurrent access issues and external modifications.
Comment on lines
+53
to
+61
| // SupportedNodeConfigVersions gates LoadNodeConfig. A node must refuse a schema | ||
| // it does not know rather than parse it as this one: silently reinterpreting a | ||
| // future config would silently reinterpret its attenuation rules. Adding a | ||
| // version means adding it here and decoding it into the same internal type, so | ||
| // the rest of the node stays version-agnostic. | ||
| var SupportedNodeConfigVersions = map[string]bool{ | ||
| "": true, | ||
| NodeConfigVersionV1Alpha1: true, | ||
| } |
Contributor
There was a problem hiding this comment.
Exposing a mutable package-level map (SupportedNodeConfigVersions) is discouraged in Go. It can be modified by other packages or lead to concurrent map read/write panics if accessed concurrently. It is safer and more idiomatic to expose a read-only function instead.
// IsSupportedNodeConfigVersion returns whether the provided node config version is supported.
// A node must refuse a schema it does not know rather than parse it as this one:
// silently reinterpreting a future config would silently reinterpret its attenuation rules.
func IsSupportedNodeConfigVersion(version string) bool {
return version == "" || version == NodeConfigVersionV1Alpha1
}| return nil, fmt.Errorf("invalid node config %s: %w", path, err) | ||
| } | ||
|
|
||
| if !api.SupportedNodeConfigVersions[config.Version] { |
Contributor
There was a problem hiding this comment.
Use the safer IsSupportedNodeConfigVersion function instead of accessing the mutable map directly.
Suggested change
| if !api.SupportedNodeConfigVersions[config.Version] { | |
| if !api.IsSupportedNodeConfigVersion(config.Version) { |
setup_suite gave a cold kind cluster 60s to roll out a 2-replica control plane, moments after loading four images onto three nodes. A busy runner misses that and the whole suite fails before a single test runs, which is what happened on PR google#300: "1 of 2 updated replicas are available", 36 tests reported as 1. Poll instead. mesh_wait_for_rollout keeps calling rollout status in short slices, so it still returns the instant the workload is ready and the happy path costs exactly what it did before, but a slow machine is no longer a failure. Measured at 0.13s against an already-rolled-out deployment. A generous ceiling would normally mean slow failures, so pods that cannot recover abort immediately: a wait for a genuinely broken workload ends in about a second, not at the ceiling. CrashLoopBackOff is deliberately not in that set, since the control plane restarts a few times while the database comes up and treating it as terminal would trade one flake for another. The check is scoped to the workload's own selector, which matters: sam-console sits in ImagePullBackOff forever in this cluster, so an unscoped check aborted every single wait. That console pod was never usable anyway, because setup loads four images and the chart deploys five. Nothing in the suite uses it, so disable it rather than pay to build and load it. Failures now print pod state, recent events and pod logs. Diagnosing the PR google#300 failure meant downloading the raw CI log and re-running the suite locally three times, because teardown discarded everything.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NodeConfig carried a version field that nothing ever read, and LoadNodeConfig used a non-strict unmarshal. A node handed a future config would therefore ignore the version, silently drop every key it did not recognise, and carry on. Under attenuation that means quietly running without rules the operator wrote.
Forward compatibility has to be installed before it is needed, because binaries already in the field cannot be retrofitted: the version's value today is entirely in the rejection path. Gate on
SupportedNodeConfigVersions and parse strictly, so an unreadable schema fails loudly either by declaring a version we do not know or by carrying a key we do not know.
An absent version still reads as v1alpha1, since files predating the check omit it. Adding a schema later means listing it, decoding it into NodeConfigComplete, and leaving the rest of the node version-agnostic; that internal type already exists, only the gate was missing.
Verified against every node config in the repo: the example configs, the seven .github/k8s ConfigMaps and the documentation snippets all parse clean under the strict loader.