-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Simple discovery server #17818
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
Simple discovery server #17818
Conversation
|
Skipping CI for Draft Pull Request. |
8396a06 to
eead354
Compare
| MinVersion: tls.VersionTLS12, | ||
| } | ||
|
|
||
| server := &http.Server{ |
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.
http3 ? I had white wine before
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.
Sorry, I don't get it ... would you like to use http3? Have I opted in to http3 automatically? Do you want us to be the "guinea pig" for http3 in kube (if so, I'm game!)
I don't think there's anything special we need from http3. We do need the client certificate information. I was thinking we would probably end up deploying this directly behind an L4 load balancer, or (failing that) using ingress or gateway with SNI routing.
In terms of backends, right now I have this with a simple in-memory implementation. Honestly that's probably good enough to get started, as we will not be offering any guarantee as to retention of these objects.
But ... if we wanted to do better, I think we should put them into etcd because (1) we should be able to run etcd pretty cheaply and we don't have to worry about wracking up a huge GCS bill if someone figures out how to make us send queries to GCS etc and (2) it means that we can use etcd-operator, which would be good from the "all the wood behind one arrow" perspective
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 was half-joking about using http3 for the discovery server but looks like the OIDC protocol is only compatible with HTTP 1.1.
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 think we can support http3, but let's start with whatever go gives us out of the box (which I think is still http1 or http2)
I do think a controversial one would be to support DNS over HTTP, if you're feeling spicy :-)
discovery/docs/kubeconfig.md
Outdated
| @@ -0,0 +1,82 @@ | |||
| # Using kubectl with Discovery Service | |||
|
|
|||
| Since the Discovery Service now emulates the Kubernetes API for `DiscoveryEndpoint` resources, you can use `kubectl` to interact with it. | |||
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'm going to ask gemini to clean up these instructions / demo scripts.
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.
Cleaned up!
discovery/pkg/discovery/k8s_types.go
Outdated
| } | ||
|
|
||
| // DiscoveryEndpoint represents a registered client in the discovery service. | ||
| type DiscoveryEndpoint struct { |
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 should move this to apis/discovery.kops.k8s.io/v1alpha1 for consistency (and probably change the version to v1alpha1)
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.
Done!
| { | ||
| Name: "discoveryendpoints", | ||
| SingularName: "discoveryendpoint", | ||
| Namespaced: true, |
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 think this probably should be cluster scoped, although I guess we could use the namespace to indicate the cluster if we we wanted to allow multiple clusters to share the same CA (which isn't a terrible idea if someone is doing multicluster).
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.
Do we consider RBAC in the equation ?
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.
So this isn't technically kube-apiserver, and I haven't implemented RBAC.
Right now we have this: anyone that has any cert signed by a CA can read the objects for that CA's universe (defined by the hash of the CA public key). You can write an object that matches your own CN only.
I probably need to build out the client side here to better understand what we actually need, whether it's acceptable to have the same CA certificate etc. (e.g. maybe we should only let kubelet certificates register, or maybe we should only let control plane nodes register, or maybe we should create a dedicated CA only for discovery)
discovery/go.mod
Outdated
|
|
||
| require ( | ||
| k8s.io/apimachinery v0.34.3 | ||
| k8s.io/client-go v0.34.3 |
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.
Technically client-go / apimachinery is only used by the clients / tests, so it might be nice to split them out. But to do that would require a separate go.mod, which is a bit of a pain.
2136f14 to
294dd35
Compare
| return s | ||
| } | ||
|
|
||
| func (s *Server) registerRoutes() { |
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.
so we never delete endpoints ?
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.
Not currently, no. You're right - I should add a TTL. (Maybe 2 hours, and then we can have nodes register every hour?). It won't be a "hard" TTL - we reserve the rights to remove objects at any time (and I should add that to the README.md / GEMINI.md)
I should probably also add explicit deletion support.
6360fc0 to
f7ff09b
Compare
1d369ae to
8ee0bca
Compare
Big TODOs, which I propose we do in follow on:
|
|
Removing WIP; still work to be done but it's safely behind a feature flag |
|
/test pull-kops-kubernetes-e2e-ubuntu-gce-build I think when we make this periodically re-register we also want to make a failure non-blocking (currently nodeup will fail if discovery.kubedisco.com is offline, which is obviously not what we want), but I don't think that is the problem here |
hakman
left a comment
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.
LGTM, let's ship it! 😁
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@justinsb: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-kops-e2e-cni-cilium-etcd |
No description provided.