Skip to content

feat(x): status message field#517

Merged
joe0BAB merged 2 commits into
mainfrom
feat/statusmessage-field
May 13, 2026
Merged

feat(x): status message field#517
joe0BAB merged 2 commits into
mainfrom
feat/statusmessage-field

Conversation

@joe0BAB
Copy link
Copy Markdown
Collaborator

@joe0BAB joe0BAB commented May 13, 2026

No description provided.

joe0BAB added 2 commits May 13, 2026 15:48
Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
@joe0BAB joe0BAB marked this pull request as ready for review May 13, 2026 13:54
@joe0BAB joe0BAB merged commit f4fb5ef into main May 13, 2026
21 of 22 checks passed
@joe0BAB joe0BAB deleted the feat/statusmessage-field branch May 13, 2026 13:58
Copy link
Copy Markdown

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🔴 CRITICAL

This PR introduces a breaking protobuf wire-format incompatibility: the existing secrets_provider field is renumbered from 7 → 8 to make room for status_message at field 7. Since protobuf field numbers are immutable identifiers, any mixed-version deployment will silently corrupt or drop SecretsProvider data with no error. See inline comments for details and the recommended fix.

// Type-specific fields
oneof metadata {
SecretsProvider secrets_provider = 7;
SecretsProvider secrets_provider = 8;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIGH: Breaking protobuf field renumbering — secrets_provider moved from field 7 to field 8

This PR inserts status_message at field number 7 and renumbers the pre-existing secrets_provider oneof field from 7 → 8. Protobuf field numbers are the sole wire-format identifiers and must never change after first deployment.

Impact in mixed-version deployments:

  • An old server (field 7 = SecretsProvider) sending to a new client: the new client decodes tag-7 bytes as status_message (a string), producing garbage text and silently dropping the SecretsProvider value.
  • A new server (field 7 = status_message, field 8 = SecretsProvider) sending to an old client: the old client ignores tag-8 entirely (unknown field), losing secrets routing with no error.

No parse error is raised because both fields share wire-type 2 (length-delimited), making this a silent, undetectable corruption.

Fix: Leave secrets_provider at its original field number 7. Assign status_message a new, never-previously-used field number (e.g. 9):

string status_message = 9;
// ...
SecretsProvider secrets_provider = 7;  // unchanged


type plugin_SecretsProvider struct {
SecretsProvider *SecretsProvider `protobuf:"bytes,7,opt,name=secrets_provider,json=secretsProvider,oneof"`
SecretsProvider *SecretsProvider `protobuf:"bytes,8,opt,name=secrets_provider,json=secretsProvider,oneof"`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIGH: Generated struct tag confirms secrets_provider encoded at field number 8 (was 7)

The generated protobuf:"bytes,8,..." struct tag directly reflects the breaking field renumber in the .proto source. This is where the actual encoding/decoding occurs at runtime:

  • GetSecretsProvider() will return nil when decoding messages written by old code (which encodes it at tag 7).
  • Tag 7 will instead be decoded as status_message, producing a garbled string from the binary-encoded SecretsProvider protobuf bytes.

The raw file descriptor in the same diff confirms: \x18\a (field 7) now maps to status_message and \x18\b (field 8) maps to secrets_provider.

This is a generated-file consequence of the root cause in api.proto. Fixing the .proto field numbers and regenerating will resolve this automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants