feat: Kuma Discovery Service (KDS) server #839
Conversation
There was a problem hiding this comment.
I am impressed by the simplicity of the solution and the huge work you did to provide through testing. A few minor notes from me.
I'll leave it for @jakubdyszkiewicz to approve.
| "github.com/Kong/kuma/pkg/core/runtime/component" | ||
| ) | ||
|
|
||
| const grpcMaxConcurrentStreams = 1000000 |
There was a problem hiding this comment.
I guess an arbitrary number, but still - do we have a proof this can be supported in practice?
nit: I also prefer these numbers to be in the form 1*1000*1000 so one can read it easier.
There was a problem hiding this comment.
This number is taken from another xDS implementations:
pkg/sds/server/grpc.gopkg/mads/server/grpc.gopkg/xds/server/grpc.go
All of them have the same constant with the same value. @jakubdyszkiewicz do you have a notion where is that number taken from?
There was a problem hiding this comment.
Out of the thin air ;)
You have to pick a number, measure with metrics it and adjust if needed. This was our pick.
| grpcOptions = append(grpcOptions, grpc.MaxConcurrentStreams(grpcMaxConcurrentStreams)) | ||
| grpcServer := grpc.NewServer(grpcOptions...) | ||
|
|
||
| lis, err := net.Listen("tcp", fmt.Sprintf(":%d", s.config.GrpcPort)) |
There was a problem hiding this comment.
I guess that is not a problem for K8s, but how safe is to listen on "all" interfaces in Universal? Shall we have also IP configuration to be able to denote the specific IP:port where the service is running?
There was a problem hiding this comment.
That is probably a nit though, as we have most of our servers behaving the same way. We might want to handle this in a separate task and to go and fix all the servers. Don't bother with it here.
| <-tc.stop() | ||
|
|
||
| }) | ||
| }) |
There was a problem hiding this comment.
That is an impressive set of tests. Great job!
| Conf: &mesh_proto.ProxyTemplate_Conf{ | ||
| Imports: []string{"default-kuma-profile"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The painful bit here is that we don't automagically get this tested once we add a new type. All the tests will just pas without even noticing the new resource type. I guess that's the best we can do today though.
pkg/kds/server/server_test.go
Outdated
| func kumaResources(response *v2.DiscoveryResponse) (resources []*mesh_proto.KumaResource, _ error) { | ||
| for _, r := range response.Resources { | ||
| kr := &mesh_proto.KumaResource{} | ||
| if err := ptypes.UnmarshalAny(r, kr); err != nil { |
There was a problem hiding this comment.
Hmm is this consistent with Maps?
There was a problem hiding this comment.
I replaced MarshalAny to MarshalAnyDeterministic, but I'm not sure I understand what is the problem with UnmarshalAny and maps, could you please give more details.
| if 65535 < c.GrpcPort { | ||
| errs = multierr.Append(errs, errors.Errorf(".GrpcPort must be in the range [0, 65535]")) | ||
| } | ||
| if c.RefreshInterval <= 0 { |
There was a problem hiding this comment.
can this be negative though?
There was a problem hiding this comment.
Duration is int64 so yes it can
# Conflicts: # app/kuma-cp/cmd/run.go # pkg/config/app/kuma-cp/config.go # pkg/config/app/kuma-cp/kuma-cp.defaults.yaml
Summary
This PR introduces a new API between Kuma CP Local and Global. That API is needed for resource synchronization. API based on Envoy xDS.
Limitations:
Full changelog
Issues resolved
Fix https://github.com/Kong/kuma/issues/803
Documentation