Skip to content

Name events explicitly instead of with an invalid generateName - #58

Merged
Alphadelta14 merged 3 commits into
MaterializeInc:mainfrom
Alphadelta14:heather/fix-event-names
Oct 13, 2026
Merged

Alphadelta14 merged 3 commits into
MaterializeInc:mainfrom
Alphadelta14:heather/fix-event-names

Conversation

@Alphadelta14

@Alphadelta14 Alphadelta14 commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Problem

EventRecorder creates every event with generateName: "<regarding name>.". The API server validates generateName as a DNS subdomain on its own, tolerating only a trailing -, so every create fails with a 422 and no event is ever published:

Event "12345678-1234-1234-1234-123456789012.d9cpg" is invalid: metadata.generateName: Invalid value: "12345678-1234-1234-1234-123456789012.": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character

The unit tests didn't catch this because MockApiServer accepted any name.

Changes

  • Event names: events are now created with an explicit metadata.name of <regarding name>.<hex nanoseconds>, the scheme client-go's events/v1 recorder uses. When that isn't a valid DNS subdomain, the name falls back to event.<hex nanoseconds>. This happens for names that only meet path-segment rules (RBAC objects may be named system:foo or Foo), for names too long to fit the suffix, and for objects without a name. The event's regarding reference still identifies the object. The old fallback, the reporter's controller name, always contains a / and so was never a valid name.
  • Mock API server: MockApiServer now validates created names. name must be a DNS subdomain, and generateName must be one apart from a trailing -. Invalid names get a 422. The check also limits each label to 63 bytes, as RFC 1123 does. The API server doesn't enforce that for event names (its IsDNS1123Subdomain checks only the 253-byte total and the character pattern), but it keeps the mock at least as strict as the real server. Run against the old events.rs, 10 existing tests fail with this.

Verification

  • cargo test --all-features passes and cargo clippy --all-features --all-targets -- -D warnings is clean.
  • Real API server: I ran a local orchestratord (Materialize's operator) against a kind cluster on Kubernetes 1.35, with this branch patched in, and created a resource that fails to reconcile.
    • On 0.13.0, every publish logged the 422 above and no events were created.
    • With this branch, the ReconcileFailed event was created, retries were counted on one event (x4 in kubectl describe), and a change in the error started a new event.

Notes

  • CHANGELOG.md gets a hand-written ## Unreleased entry, following the existing hand-written entries. With release-plz now generating release PRs (Add release-plz workflow #56), that entry may need folding into whatever release-plz writes.
  • Materialize's operator needs this in a 0.13.1 release. Its nightly event tests have failed on every run since it moved to 0.13.0.

🤖 Generated with Claude Code

EventRecorder created every event with a generateName of `<name>.`. The
API server validates generateName as a DNS subdomain, tolerating only a
trailing `-`, so it rejected every create with a 422 and no event was
ever published. Name events `<name>.<hex nanoseconds>` instead, as
client-go does, truncating the regarding name so the result stays a
valid subdomain.

The mock API server accepted any name, which is how this went
unnoticed. It now validates created names the way the API server does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

RFC 1123 label-length validation is missing, allowing generated names that the real API server rejects.

2 open findings
What changed in this PR

Fixes event publication failures caused by invalid generateName values.

Changes:

  • Generates explicit timestamp-based event names.
  • Adds API-style name validation to the mock server.
  • Documents the fix in the changelog.
File Description
src/​events.rs Generates and tests explicit event names.
src/​test_util.rs Validates names in mock create requests.
CHANGELOG.md Records the event publication fix.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/events.rs Outdated
Comment thread src/test_util.rs Outdated

@alex-hunt-materialize alex-hunt-materialize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The copilot review comments seem valid to me. I'm not sure why they are marked as resolved.

This is strictly better than we had before, but I think it may still be possible to make things rejected by the api server.

I'm not sure how much value we get from the mock server. We might want to eventually make the tests run a real K8S cluster in Kind.

bosconi
bosconi previously approved these changes Oct 13, 2026

@bosconi bosconi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, modulo one possible small tweak.

Comment thread src/events.rs Outdated
Comment on lines +412 to +413
}
format!("{prefix}{suffix}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

client-go's events/v1 recorder, which the description cites, now validates this name and falls back when it is invalid (GenerateEventName, kubernetes/kubernetes#127594). Some kinds allow names that are not DNS subdomains: RBAC names only need to be path segments, so for example system:controller:foo is valid. A Controller over such a kind gets a 422 on every failure event, same as before this PR. Truncation can't fix that, but a fallback prefix can. This needs is_dns_subdomain moved out of test_util into events.rs (pub(crate), imported by test_util), plus a test case like event_name(Some("system:controller:foo"), time) == "event.123456789abcdef0".

Suggested change
}
format!("{prefix}{suffix}")
}
let name = format!("{prefix}{suffix}");
// Some kinds allow names that are not DNS subdomains (RBAC names need
// only be path segments), which no suffix can make valid.
if is_dns_subdomain(&name) {
name
} else {
format!("event{suffix}")
}

Some kinds only require names to be path segments, so an RBAC role named
`system:foo` or `Foo` produced an event name the API server rejects.
Truncating long names was a partial fix for one way the name could be
invalid. Instead, check the whole name and fall back to `event.<hex>`
for any name that isn't a valid subdomain, whatever the reason.

The subdomain check now also limits labels to 63 bytes. The API server
doesn't enforce that for event names, but enforcing it keeps the check,
which the mock API server shares, at least as strict as the API server.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cargo-deny started failing on the newly published advisory that
unsafe-libyaml is unmaintained. It comes in through kube-client 3's
serde_yaml and can't be dropped without moving to kube 4, which
replaces serde_yaml with serde-saphyr but is a breaking upgrade for
this crate's users.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The mock skips generateName validation whenever an explicit name is present, diverging from API-server behavior.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread src/test_util.rs
Comment on lines +153 to +170
if let Some(name) = metadata["name"].as_str() {
return if is_dns_subdomain(name) {
Ok(name.to_owned())
} else {
Err(format!("metadata.name: Invalid value: {name:?}"))
};
}
let Some(prefix) = metadata["generateName"].as_str() else {
return Err("metadata.name: Required value".to_owned());
};
let masked = match prefix.strip_suffix('-') {
Some(rest) => format!("{rest}a"),
None => prefix.to_owned(),
};
if !is_dns_subdomain(&masked) {
return Err(format!("metadata.generateName: Invalid value: {prefix:?}"));
}
Ok(format!("{prefix}{suffix}"))
@Alphadelta14
Alphadelta14 merged commit 991b7af into MaterializeInc:main Oct 13, 2026
2 checks passed
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.

4 participants