Repository navigation
fix(konnectivity): render the server in the Deployment first revision - #1372
Conversation
✅ Deploy Preview for kamaji-documentation canceled.
|
prometherion
left a comment
prometherion
left a comment
There was a problem hiding this comment.
Shouldn't we then remove the handler that wraps the Konnectivity builder? It's going to be a no-op since it doesn't ass anything else.
| if tenantControlPlane.Spec.Addons.Konnectivity != nil { | ||
| builder.Konnectivity{Scheme: *r.Client.Scheme()}.Build(r.resource, *tenantControlPlane) | ||
| } | ||
|
|
There was a problem hiding this comment.
If I understand correctly, we're adding this builder only to avoid the second Replica Set, and keeping the Konnectivity Server handler in place.
kamaji/internal/resources/konnectivity/deployment_resource.go
Lines 23 to 28 in 314e3b5
I think it's not entirely elegant, since we're ending up with multiple handlers doing the same thing: do you think we should consolidate the handlers (Deployment + Konnectivity) all together?
There was a problem hiding this comment.
Yes, agreed. I'll push a follow-up commit soon
There was a problem hiding this comment.
The Deployment builder now adds and removes the Konnectivity server itself, and the Konnectivity Deployment handler is gone. Could you please check?
There was a problem hiding this comment.
Should I do the same for the Service in a separate PR? konnectivity.ServiceResource adds its port to the Service created by KubernetesServiceResource. What do you think?
There was a problem hiding this comment.
It's safe for the Service, it doesn't rollout a new Pod so it's fine keeping it as a separate handler.
| k.buildVolumeMounts(&deployment.Spec.Template.Spec) | ||
| k.buildVolumes(tenantControlPlane.Status.Addons.Konnectivity, &deployment.Spec.Template.Spec) | ||
|
|
||
| k.Scheme.Default(deployment) |
There was a problem hiding this comment.
Why has this been removed? IIRC, it's required to avoid an infinite loop while reconciling due to default values applied by the API Server, causing the reflection package to return a false positive of non-identical structs.
There was a problem hiding this comment.
We had two calls, and one of them is still here, in Deployment.Build:
kamaji/internal/builders/controlplane/deployment.go
Lines 153 to 154 in 088ff08
Konnectivity{}.Build now runs right before it, so the Konnectivity container, volumes and mounts get the defaults too.
…clastix#1372) * fix(konnectivity): render the server in the Deployment first revision * refactor(konnectivity): render the server from the Deployment builder
With konnectivity enabled, a new TCP gets two ReplicaSets at once. The first one has no konnectivity-server, so its kube-apiserver crash-loops on the missing egress selector config until the second one is ready and replaces it.
The Deployment builder now adds and removes the Konnectivity server itself, and the separate Konnectivity Deployment handler is gone.
Discussed in #663.
Before
After