Allow custom cluster domains in remaining backends#3278
Merged
ihcsim merged 4 commits intolinkerd:masterfrom Aug 27, 2019
Merged
Allow custom cluster domains in remaining backends#3278ihcsim merged 4 commits intolinkerd:masterfrom
ihcsim merged 4 commits intolinkerd:masterfrom
Conversation
0c1ec64 to
c9b78c3
Compare
Contributor
|
@arminbuerkle I'm gonna take a look at this today. Meanwhile, can you rebase your branch with diff --cc controller/tap/server.go
index 66479264,36343fe2..00000000
--- a/controller/tap/server.go
+++ b/controller/tap/server.go
@@@ -29,12 -31,14 +30,13 @@@ const requireIDHeader = "l5d-require-id
const podIPIndex = "ip"
const defaultMaxRps = 100.0
-type (
- server struct {
- tapPort uint
- k8sAPI *k8s.API
- controllerNamespace string
- clusterDomain string
- }
-)
+// GRPCTapServer describes the gRPC server implementing pb.TapServer
+type GRPCTapServer struct {
+ tapPort uint
+ k8sAPI *k8s.API
+ controllerNamespace string
+ clusterDomain string
+}
var (
tapInterval = 1 * time.Second
@@@ -459,18 -462,33 +461,26 @@@ func NewGrpcTapServer
tapPort uint,
controllerNamespace string,
k8sAPI *k8s.API,
-) (*grpc.Server, net.Listener, error) {
+) *GRPCTapServer {
k8sAPI.Pod().Informer().AddIndexers(cache.Indexers{podIPIndex: indexPodByIP})
- return newGRPCTapServer(tapPort, controllerNamespace, k8sAPI)
- lis, err := net.Listen("tcp", addr)
- if err != nil {
- return nil, nil, err
- }
-
+ globalConfig, err := config.Global(pkgK8s.MountPathGlobalConfig)
+ clusterDomain := globalConfig.GetClusterDomain()
+ if err != nil || clusterDomain == "" {
+ clusterDomain = "cluster.local"
+ log.Warnf("failed to load cluster domain from global config: [%s] (falling back to %s)", err, clusterDomain)
+ }
+
- s, _ := newGRPCTapServer(tapPort, controllerNamespace, clusterDomain, k8sAPI)
-
- return s, lis, nil
+ return newGRPCTapServer(tapPort, controllerNamespace, clusterDomain, k8sAPI)
}
func newGRPCTapServer(
tapPort uint,
controllerNamespace string,
+ clusterDomain string,
k8sAPI *k8s.API,
-) (*grpc.Server, *server) {
- srv := &server{
+) *GRPCTapServer {
+ srv := &GRPCTapServer{
tapPort: tapPort,
k8sAPI: k8sAPI,
controllerNamespace: controllerNamespace,
diff --cc controller/tap/server_test.go
index de9d2934,1d3a30f3..00000000
--- a/controller/tap/server_test.go
+++ b/controller/tap/server_test.go
@@@ -371,7 -371,7 +371,7 @@@ status
t.Fatalf("Invalid port: %s", port)
}
- fakeGrpcServer := newGRPCTapServer(uint(tapPort), "controller-ns", k8sAPI)
- _, fakeGrpcServer := newGRPCTapServer(uint(tapPort), "controller-ns", "cluster.local", k8sAPI)
+ fakeGrpcServer := newGRPCTapServer(uint(tapPort), "controller-ns", "cluster.local", k8sAPI)
k8sAPI.Sync()
diff --git a/controller/tap/apiserver_test.go b/controller/tap/apiserver_test.go
index 1aae1af4..8216cb7e 100644
--- a/controller/tap/apiserver_test.go
+++ b/controller/tap/apiserver_test.go
@@ -47,7 +47,7 @@ data:
t.Fatalf("NewFakeAPI returned an error: %s", err)
}
- fakeGrpcServer := newGRPCTapServer(4190, "controller-ns", k8sAPI)
+ fakeGrpcServer := newGRPCTapServer(4190, "controller-ns", "cluster.local", k8sAPI)
_, _, err = NewAPIServer("localhost:0", tls.Certificate{}, k8sAPI, fakeGrpcServer, false)
if !reflect.DeepEqual(err, exp.err) {
|
ihcsim
reviewed
Aug 21, 2019
c9b78c3 to
4c25d9c
Compare
Contributor
Author
|
@ihcsim thanks for your review and your suggestions. I added all of them and rebased the branch on master. Unfortunately i found another occurrence of a hard coded diff --git a/controller/api/destination/server.go b/controller/api/destination/server.go
index ba0b56a0..673cbb6b 100644
--- a/controller/api/destination/server.go
+++ b/controller/api/destination/server.go
@@ -174,7 +174,7 @@ func (s *server) GetProfile(dest *pb.GetDestination, stream pb.Destination_GetPr
// The adaptor merges profile updates with traffic split updates and
// publishes the result to the translator.
- tsAdaptor := newTrafficSplitAdaptor(translator, service, port)
+ tsAdaptor := newTrafficSplitAdaptor(translator, service, port, s.clusterDomain)
// Subscribe the adaptor to traffic split updates.
err = s.trafficSplits.Subscribe(service, tsAdaptor)
diff --git a/controller/api/destination/traffic_split_adaptor.go b/controller/api/destination/traffic_split_adaptor.go
index 8ca814f6..64587284 100644
--- a/controller/api/destination/traffic_split_adaptor.go
+++ b/controller/api/destination/traffic_split_adaptor.go
@@ -17,18 +17,20 @@ import (
// a source of profile updates (such as a ProfileWatcher) and a source of
// traffic split updates (such as a TrafficSplitWatcher).
type trafficSplitAdaptor struct {
- listener watcher.ProfileUpdateListener
- id watcher.ServiceID
- port watcher.Port
- profile *sp.ServiceProfile
- split *ts.TrafficSplit
+ listener watcher.ProfileUpdateListener
+ id watcher.ServiceID
+ port watcher.Port
+ profile *sp.ServiceProfile
+ split *ts.TrafficSplit
+ clusterDomain string
}
-func newTrafficSplitAdaptor(listener watcher.ProfileUpdateListener, id watcher.ServiceID, port watcher.Port) *trafficSplitAdaptor {
+func newTrafficSplitAdaptor(listener watcher.ProfileUpdateListener, id watcher.ServiceID, port watcher.Port, clusterDomain string) *trafficSplitAdaptor {
return &trafficSplitAdaptor{
- listener: listener,
- id: id,
- port: port,
+ listener: listener,
+ id: id,
+ port: port,
+ clusterDomain: clusterDomain,
}
}
@@ -56,7 +58,7 @@ func (tsa *trafficSplitAdaptor) publish() {
dst := &sp.WeightedDst{
// The proxy expects authorities to be absolute and have the
// host part end with a trailing dot.
- Authority: fmt.Sprintf("%s.%s.svc.cluster.local.:%d", backend.Service, tsa.id.Namespace, tsa.port),
+ Authority: fmt.Sprintf("%s.%s.svc.%s.:%d", backend.Service, tsa.id.Namespace, tsa.clusterDomain, tsa.port),
Weight: backend.Weight,
}
overrides = append(overrides, dst)
diff --git a/controller/api/destination/traffic_split_adaptor_test.go b/controller/api/destination/traffic_split_adaptor_test.go
index cb7b8fc0..882b08c4 100644
--- a/controller/api/destination/traffic_split_adaptor_test.go
+++ b/controller/api/destination/traffic_split_adaptor_test.go
@@ -42,7 +42,7 @@ func TestTrafficSplitAdaptor(t *testing.T) {
t.Run("Profile update", func(t *testing.T) {
listener := watcher.NewBufferingProfileListener()
- adaptor := newTrafficSplitAdaptor(listener, watcher.ServiceID{Name: "foo", Namespace: "ns"}, watcher.Port(80))
+ adaptor := newTrafficSplitAdaptor(listener, watcher.ServiceID{Name: "foo", Namespace: "ns"}, watcher.Port(80), "cluster.local")
adaptor.Update(profile)
@@ -54,7 +54,7 @@ func TestTrafficSplitAdaptor(t *testing.T) {
t.Run("Traffic split without profile", func(t *testing.T) {
listener := watcher.NewBufferingProfileListener()
- adaptor := newTrafficSplitAdaptor(listener, watcher.ServiceID{Name: "foo", Namespace: "ns"}, watcher.Port(80))
+ adaptor := newTrafficSplitAdaptor(listener, watcher.ServiceID{Name: "foo", Namespace: "ns"}, watcher.Port(80), "cluster.local")
adaptor.UpdateTrafficSplit(split)
@@ -78,7 +78,7 @@ func TestTrafficSplitAdaptor(t *testing.T) {
t.Run("Profile merged with traffic split", func(t *testing.T) {
listener := watcher.NewBufferingProfileListener()
- adaptor := newTrafficSplitAdaptor(listener, watcher.ServiceID{Name: "foo", Namespace: "ns"}, watcher.Port(80))
+ adaptor := newTrafficSplitAdaptor(listener, watcher.ServiceID{Name: "foo", Namespace: "ns"}, watcher.Port(80), "cluster.local")
adaptor.Update(profile)
adaptor.UpdateTrafficSplit(split) |
Contributor
|
@arminbuerkle Go ahead and push the changes to the |
4c25d9c to
42306c1
Compare
Contributor
Author
|
Changes for the traffic split adapter are pushed @adleong |
Contributor
|
@arminbuerkle I just did some testing on your branch, and noticed that the ✗ k -n linkerd logs linkerd-tap-78497c5456-zmcwz tap
time="2019-08-26T15:44:39Z" level=info msg="running version git-99c34ad9"
time="2019-08-26T15:44:39Z" level=fatal msg="failed to read config file: open /var/run/linkerd/config/global: no such file or directory"This diff will fix the problem: diff --git a/charts/linkerd2/templates/tap.yaml b/charts/linkerd2/templates/tap.yaml
index 9c6b0cfb..c552f694 100644
--- a/charts/linkerd2/templates/tap.yaml
+++ b/charts/linkerd2/templates/tap.yaml
@@ -91,6 +91,8 @@ spec:
securityContext:
runAsUser: {{.ControllerUID}}
volumeMounts:
+ - mountPath: /var/run/linkerd/config
+ name: config
- mountPath: /var/run/linkerd/tls
name: tls
readOnly: true
@@ -101,6 +103,9 @@ spec:
{{ end -}}
serviceAccountName: linkerd-tap
volumes:
+ - configMap:
+ name: linkerd-config
+ name: config
- {{- include "partials.proxy.volumes.identity" . | indent 8 | trimPrefix (repeat 7 " ") }}
- name: tls
secret: |
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de> Move fetching cluster domain for tap server to cmd main
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
42306c1 to
52121f0
Compare
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
52121f0 to
11c33f7
Compare
Contributor
Author
ihcsim
approved these changes
Aug 27, 2019
Contributor
ihcsim
left a comment
There was a problem hiding this comment.
@arminbuerkle No worries at all! All tests passed on my machine 👍. We're almost there!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continue of #2930.
8643398 is required so the api server also uses the right service profile address when a custom cluster domain is set.
c9b78c3 i'm not a 100% sure on this since it was only recently added but i think it needs to reference the right cluster domain too.
With this and #3277 hopefully only the cli flag is missing to support custom cluster domains throughout linkerd 😄