Repository navigation
Allow custom cluster domain in controller components - #2950
Conversation
f8af8c4 to
d30e940
Compare
f874004 to
dc1c949
Compare
|
@arminbuerkle Apologies for unable to get to this sooner. I will start reviewing this today. Meanwhile, can you rebase this branch with |
| "trust.domain", | ||
| false, | ||
| "service-name.service-ns.svc.cluster.local", | ||
| "service-name.service-ns.svc.mycluster.local", |
There was a problem hiding this comment.
AFAICT, this doesn't do anything, right? Consider reverting.
There was a problem hiding this comment.
makeEndpointTranslator calls newEndpointTranslator which then uses GetServiceAndPort to split the domain name into a service and port.
In GetServiceAndPort the domain was hard coded to always use cluster.local which shouldn't be the case and was changed in e67de3b
Update golden templates Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
The change relaxes the constrains of an authority requiring a `svc.cluster.local` suffix to only require `svc` as third part. A unit test could be added though the destination/server and endpoint watcher already test this behaviour. Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
dc1c949 to
f0d0ddc
Compare
There was a problem hiding this comment.
Sorry for taking so long to take get back to this. I'll try to schedule some time to add your suggestions.
| "trust.domain", | ||
| false, | ||
| "service-name.service-ns.svc.cluster.local", | ||
| "service-name.service-ns.svc.mycluster.local", |
There was a problem hiding this comment.
makeEndpointTranslator calls newEndpointTranslator which then uses GetServiceAndPort to split the domain name into a service and port.
In GetServiceAndPort the domain was hard coded to always use cluster.local which shouldn't be the case and was changed in e67de3b
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
0b8dcd5 to
fe3788e
Compare
There was a problem hiding this comment.
Thanks for the changes! I did some testing with linkerd upgrade, and noticed that the cluster domain value in the linkerd-config config map turned out as an empty string, if the upgrade was performed on an older version of Linkerd.
To reproduce,
$ linkerd version --client
Client version: stable-2.4.0
$ linkerd install | kubectl apply -f -
$ kubectl -n linkerd get cm linkerd-config -ojsonpath="{.data.global}" | jq .clusterDomain
null # this is correct because the stable-2.4.0 release doesn't support this configuration yet
$ bin/linkerd version --client
Client version: git-9287eb27
$ bin/linkerd upgrade | kubectl apply -f -
$ kubectl -n linkerd get cm linkerd-config -ojsonpath="{.data.global}" | jq .clusterDomain
"" # this should be cluster.local. Otherwise, all subsequent requests will be using the empty string as the cluster domainWe need to upgrade the upgrade code to account for this. So something like this should work:
diff --git a/cli/cmd/upgrade.go b/cli/cmd/upgrade.go
index 87c544fb..26a5369c 100644
--- a/cli/cmd/upgrade.go
+++ b/cli/cmd/upgrade.go
@@ -219,6 +219,9 @@ func (options *upgradeOptions) validateAndBuild(stage string, k kubernetes.Inter
}
configs.GetInstall().Flags = options.recordedFlags
configs.GetGlobal().OmitWebhookSideEffects = options.omitWebhookSideEffects
+ if configs.GetGlobal().GetClusterDomain() == "" {
+ configs.GetGlobal().ClusterDomain = defaultClusterDomain
+ }
var identity *installIdentityValues
idctx := configs.GetGlobal().GetIdentityContext()With this change, you should see the linkerd-config config map gets updated correctly:
$ kubectl -n linkerd get cm linkerd-config -ojsonpath="{.data.global}" | jq .clusterDomain
"cluster.local"
Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
|
Thank you @ihcsim for testing. Your suggestion indeed fixed that. |
There was a problem hiding this comment.
Approved! Thanks again 👍!
Continue of #2950. I decided to check for the `clusterDomain` in the config map in web server main for the same reasons as as pointed out here #3113 (comment) It decouples the server implementations from the config. Signed-off-by: Armin Buerkle <armin.buerkle@alfatraining.de>
Splits #2930 into smaller components: Part 1, controller components.
While it's still a lot of changes, most of them are auto generated (proto, .golden) or updating tests so a different domain than
cluster.localis used.I added the
cluster.localdefault domain to thecli/cmd/pkg, so the.goldenfiles will directly add it to their global configs. Otherwise they'd get initialised with an empty string:{..., "clusterDomain":""}