-
Notifications
You must be signed in to change notification settings - Fork 314
chore: Upgrade k8s to 1.35 #5562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
chore: Upgrade k8s to 1.35 #5562
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // "master" is still the default branch name for a new repository unless | ||
| // you configure it otherwise. | ||
| &AddWorkTreeOptions{Ref: "master"}, | ||
| // Don't assume a default branch name ("main" vs "master"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept running into issues with the test here, so I made this more flexible. Shouldn't impact the tests themselves, but let me know if I missed something
|
Please note that the backend is ready to go and I've spot checked functionality, but obviously UI stuff is failing and I need to have a more thorough manual test done. However, it is pretty much ready for review as is otherwise |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5562 +/- ##
==========================================
- Coverage 55.95% 55.75% -0.20%
==========================================
Files 423 424 +1
Lines 32088 32203 +115
==========================================
+ Hits 17954 17955 +1
- Misses 13084 13198 +114
Partials 1050 1050 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
97090b2 to
55a2e3a
Compare
|
✅ Build flags ❓ Is the hack of defining our own knock-off corev1 types worth it if we plan to transition away from protobufs? I'm not against it, but want to understand the risks. |
|
Putting this context here from an offline convo: turning on just the build flags isn’t sufficient because anything involving k8s objects with maps panics |
a77356a to
4ca2fd8
Compare
This updates all k8s libraries to 1.35 and enables the back compat build flag for generated protobuf files. We plan on adding a REST-ish API and deprecating our use of Connect RPC in 1.10 Signed-off-by: Taylor Thomas <[email protected]>
Signed-off-by: Taylor Thomas <[email protected]>
Signed-off-by: Taylor Thomas <[email protected]>
4ca2fd8 to
d0a0380
Compare
Ok, so this one is a doozy, and full disclaimer, I hate it. But it is what is going to have to work.
Background
Due to the upcoming removal of the gogo proto dependency, we have to enable a build flag to reenable the V1 proto message marker method
ProtoMessageso things can work with the connect API. However, because types no longer have aDescriptormethod, we would get a panic when Marshaling out any message that had a kubernetes type using baremaptypes because there is no registered type for them. We work around this already in our code with specialprotobuf_keyandprotobuf_valtags for our maps. Even worse, the k8sObjectMetatype works just fine mostly because of a quirk of message handling (detailed later)What this actually does
I tried messing around with the underlying
Codecused by ConnectRPC, but the main problem is you can't update the descriptor at runtime. So, as far as I could tell, I couldn't useprotoreflectto try to get around this and theproto.Marshalmethod doesn't work likeMarshalJSONwhere it recurses down each type in a struct and calls itsMarshalJSONmethod. So the easiest (and very very hacky) solution was to copy the types that we had problems with (Secret and ConfigMap, as well as ObjectMeta) from the k8s proto files and add them to our service. We do haveServiceAccountbut this isn't really used by the UI and I don't think it has any top level maps other than labels (which may work in this case?)Please note that our types work purely because of the quirk described below where labels and annotations are just skipped and assumed empty when returning them as a protobuf type. This doesn't affect our UI because we use the
RawFormatoption to get the yaml bytes back, which returns the full objectThe gnarly technical quirk, here there be dragons
I asked Claude to help me summarize it and its breakdown was perfect, so I present it here in its entirety:
Summary: Why ObjectMeta Works but ConfigMap Doesn't
The Key Discovery
The root cause is in how aberrantLoadMessageDescReentrant handles different Go types:
For pointer types (*struct): The function processes all struct fields, including map fields that need protobuf_key/protobuf_val tags.
For value types (struct, not pointer): The function returns an empty descriptor without processing any fields.
How This Affects ConfigMap vs Stage
ConfigMap (*v1.ConfigMap):
Stage (*v1alpha1.Stage):
Visual Comparison
The Irony
ObjectMeta works not because it's special, but because:
ConfigMap fails because it has direct map fields at the top level of its struct. These maps don't have protobuf_key/protobuf_val tags, and since ConfigMap is processed as a pointer type, these fields are encountered and their malformed descriptors cause the panic.