config: preserve legacy grpc address across sparse imports#13317
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates legacy v3 [grpc] → v4 server plugin migration to avoid emitting unset (zero-value) fields during merge, preventing sparse imported configs from unintentionally overwriting existing addresses.
Changes:
- Adjust legacy gRPC migration to only populate plugin fields that were set in the legacy config.
- Add table-driven tests covering sparse legacy gRPC imports and expected merge behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/containerd/server/config/config.go | Builds migrated gRPC/grpc-tcp plugin maps sparsely to avoid overwriting fields with empty/zero values. |
| cmd/containerd/server/config/config_test.go | Adds table-driven coverage for sparse v3 gRPC imports and address preservation/override behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| grpcConfig["uid"] = grpcUID | ||
| grpcConfig["gid"] = grpcGID | ||
| } else { | ||
| if grpcUID != 0 { | ||
| grpcConfig["uid"] = grpcUID | ||
| } | ||
| if grpcGID != 0 { | ||
| grpcConfig["gid"] = grpcGID | ||
| } |
There was a problem hiding this comment.
When grpcAddress != "", this unconditionally emits uid/gid into the migrated plugin config even when they’re zero values. That contradicts the stated goal of only emitting fields that were actually set in the legacy config, and it can also overwrite non-zero defaults or previously-merged values with 0 during subsequent merges. Consider applying the same conditional logic for uid/gid regardless of whether address is set (i.e., only set them in grpcConfig when non-zero), while keeping address emission gated on grpcAddress != "".
| grpcConfig["uid"] = grpcUID | |
| grpcConfig["gid"] = grpcGID | |
| } else { | |
| if grpcUID != 0 { | |
| grpcConfig["uid"] = grpcUID | |
| } | |
| if grpcGID != 0 { | |
| grpcConfig["gid"] = grpcGID | |
| } | |
| } | |
| if grpcUID != 0 { | |
| grpcConfig["uid"] = grpcUID | |
| } | |
| if grpcGID != 0 { | |
| grpcConfig["gid"] = grpcGID |
prakleumas
left a comment
There was a problem hiding this comment.
beep boop
Hello! I am @samuelkarp's friendly code review robot 🤖! Sam asked me to take a look and post my comments directly on GitHub.
Please treat my comments as suggestions and advisory, not mandates. Since I am an LLM and my output is used here directly, you should evaluate carefully and see if my suggestions make sense. Feel free to directly resolve any you disagree with.
This is not an automated action. This bot does not read or reply to comments.
samuelkarp
left a comment
There was a problem hiding this comment.
Bot comments are reasonable suggestions, otherwise LGTM.
When migrating v3 [grpc] config to the v4 server plugin config, only emit fields that were actually set in the legacy config. This prevents a sparse imported [grpc] section, such as one only setting max_send_message_size, from generating address = "" and overwriting the address migrated from the root config. Add table-driven coverage for sparse legacy grpc imports, including address preservation, imported address override, and default address retention when no imported config specifies address. Signed-off-by: Wei Fu <fuweid89@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When migrating v3 [grpc] config to the v4 server plugin config, only emit fields that were actually set in the legacy config. This prevents a sparse imported [grpc] section, such as one only setting max_send_message_size, from generating address = "" and overwriting the address migrated from the root config.
Add table-driven coverage for sparse legacy grpc imports, including address preservation, imported address override, and default address retention when no imported config specifies address.
Fixes: #13316
@dmcgowan @berkayoz