Repository navigation
fix: preserve TelemetryIngest spec in v1beta5 ConvertFrom - #6536
Conversation
There was a problem hiding this comment.
Generally agree with the changes, but there's some additional work to be done.
The v1beta4 convert_from code is effectively unreachable since it's no longer served and we have some pending tasks to strip most code from deprecated API versions in general to reduce the maintenance burden. So to keep it focused, please remove that part.
Given that we missed the v1beta5 I think we need to add an integration test to catch these things earlier next time. In the validation webhook tests we only exercise the path forward. Please adapt it to also do the path backwards. Of course excluding unserved versions.
ConvertTo (v1beta5 -> latest) copies Spec.TelemetryIngest via toTelemetryIngestSpec, but the matching ConvertFrom never had a fromTelemetryIngestSpec - so reading a DynaKube back as v1beta5 returned spec.telemetryIngest=nil. A subsequent PUT then clobbered the latest storage version with nil, silently dropping the field from etcd. Added fromTelemetryIngestSpec mirroring the to-side helper, wired it into the ConvertFrom dispatcher and added round-trip unit tests. The webhook integration test only exercised the forward conversion path, which is how the missing ConvertFrom went unnoticed. Extended it to also read served versions back in their own version (the Hub -> spoke path) and added telemetryIngest to the testdata so this class of regression is caught.
8f97ced to
28002fe
Compare
knQzx
changed the title
|
done - removed the v1beta4 part, so it's v1beta5 only now extended the webhook integration test to also exercise the backward path (reading a served version back, i.e. hub -> spoke / ConvertFrom) for served versions only, and added telemetryIngest to the testdata so the dropped field is actually covered - fails on the old code, passes with the fix |
description
Spec.TelemetryIngestis copied byConvertTo(v1beta5 -> latest) viatoTelemetryIngestSpec, but the matchingConvertFrom(latest -> v1beta5) has nofromTelemetryIngestSpecand silently drops it. the field exists on v1beta5, so anyone GETting a DynaKube as v1beta5 seesspec.telemetryIngest = null, and a following PUT clobbers the latest storage version (v1beta6) with null, dropping the field from etcdroot cause
PR #4903 added the converter only in the convert_to direction. the reverse was overlooked and got inherited when v1beta5 came in
fix
fromTelemetryIngestSpecto v1beta5convert_from.go, mirroringtoTelemetryIngestSpec, and wire it into theConvertFromdispatcherthis was the only asymmetric field, the other
to*/from*helpers are all symmetric (checked the full diff of helper invocations)how can this be tested?
pkg/api/v1beta5/dynakube/...tests passConvertTothenConvertFrom, asserts every subfield is preserved) plus amigrate telemetryIngesttest for the latest -> v1beta5 directionpkg/api/validation/dynakube) to also exercise the backward path: served versions are now read back in their own version (hub -> spoke /ConvertFrom), unserved versions stay excluded (v1beta4 only asserts a NoMatchError), andtelemetryIngestwas added to the testdata so the dropped field is actually covered