-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
cmd/k8s-operator,k8s-operator,go.{mod,sum}: publish proxy status condition for annotated services #12463
Conversation
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.
With the change to use metav1.Condition
, the serialisation of the CRDs hasn't changed at all, they've just got some updated validation rules and descriptions. I manually checked all usages of setting conditions in the operator and didn't find any that would violate the new validation rules, so I think this should be safe.
I also tested:
- Applying the old operator + CRDs
- Creating a Service-based proxy and Connector subnet router
- Rollout an operator from this PR
- Observe successful condition changes
- Rollout CRDs from this PR
- Observe successful condition changes
Adds a new TailscaleProxyReady condition type for use in corev1.Service conditions. Also switches our CRDs to use metav1.Condition instead of ConnectorCondition. The Go structs are seralized identically, but it updates some descriptions and validation rules. Updates #12216 Co-authored-by: Irbe Krumina <[email protected]> Signed-off-by: Tom Proctor <[email protected]>
587b7ab
to
9517ff4
Compare
@@ -144,6 +194,8 @@ func TestLoadBalancerClass(t *testing.T) { | |||
expectMissing[appsv1.StatefulSet](t, fc, "operator-ns", shortName) | |||
expectMissing[corev1.Service](t, fc, "operator-ns", shortName) | |||
expectMissing[corev1.Secret](t, fc, "operator-ns", fullName) | |||
|
|||
// Note that the Tailscale-specific condition status should be gone now. |
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.
Nice
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.
Thanks @tomhjp , this looks great, I've played around with the new conditions for a bit and all seems to work as expected 🥳
I've left a couple nits, mostly optional.
We'll have to decide what to do with the redundant comment lines before merging. Generally, I am not as concerned about these as I was if this was in spec- I don't think anyone will really run kubectl explain connector.status.conditions
(or, I never do) they will likely just run kubectl describe
on the created resources at which point the condition messages should be self explanatory.
We could:
- see if we can get rid of the generated comments via codegen (we already do some hackery)
- revert using upstream conditions for our own types
- keep things as is in this PR (for now)
Maybe let's take a look if we can get rid of the comments and if not, then decide which of the other two?
Co-authored-by: Irbe Krumina <[email protected]> Signed-off-by: Tom Proctor <[email protected]>
Signed-off-by: Tom Proctor <[email protected]>
if _, ok := svc.Annotations[AnnotationHostname]; ok { | ||
violations = append(violations, fmt.Sprintf("invalid Tailscale hostname specified %q: %s", svcName, err)) | ||
} else { | ||
violations = append(violations, fmt.Sprintf("invalid Tailscale hostname %q, use %q annotation to override: %s", svcName, AnnotationHostname, err)) |
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.
Nice!
0b04a2d
to
dfd93a3
Compare
Updates controller-tools to a pseudo version from master and controller-runtime to latest tagged. Latest from controller-tools brings in the fix that excludes comments and TODOs from generated CRD documentation. Updates #12216 Signed-off-by: Tom Proctor <[email protected]>
dfd93a3
to
f987c3b
Compare
See kubernetes-sigs/controller-runtime#2633 for details. Updates #12216 Signed-off-by: Tom Proctor <[email protected]>
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.
Looks great, thanks for working on this!
Glad about 121178c cleans up our test code a fair bit
We should also create another issue to do the same with Ingress resources |
Thanks! |
…ition for annotated services (tailscale#12463) Adds a new TailscaleProxyReady condition type for use in corev1.Service conditions. Also switch our CRDs to use metav1.Condition instead of ConnectorCondition. The Go structs are seralized identically, but it updates some descriptions and validation rules. Update k8s controller-tools and controller-runtime deps to fix the documentation generation for metav1.Condition so that it excludes comments and TODOs. Stop expecting the fake client to populate TypeMeta in tests. See kubernetes-sigs/controller-runtime#2633 for details of the change. Finally, make some minor improvements to validation for service hostnames. Fixes tailscale#12216 Co-authored-by: Irbe Krumina <[email protected]> Signed-off-by: Tom Proctor <[email protected]>
…ition for annotated services (tailscale#12463) Adds a new TailscaleProxyReady condition type for use in corev1.Service conditions. Also switch our CRDs to use metav1.Condition instead of ConnectorCondition. The Go structs are seralized identically, but it updates some descriptions and validation rules. Update k8s controller-tools and controller-runtime deps to fix the documentation generation for metav1.Condition so that it excludes comments and TODOs. Stop expecting the fake client to populate TypeMeta in tests. See kubernetes-sigs/controller-runtime#2633 for details of the change. Finally, make some minor improvements to validation for service hostnames. Fixes tailscale#12216 Co-authored-by: Irbe Krumina <[email protected]> Signed-off-by: Tom Proctor <[email protected]>
Adds a new
TailscaleProxyReady
condition type to anycorev1.Service
that has been set up as a Tailscale proxy. This makes it easier to monitor the status of the proxy including any errors, and also enables users to run e.g.kubectl wait --for=condition=TailscaleProxyReady service nginx
to wait until the proxy is ready to serve traffic.The workflow to exercise that (assuming the operator is already deployed), would be something like:
Fixes #12216