Allow custom cluster domain in controller components#2950
Allow custom cluster domain in controller components#2950ihcsim merged 7 commits intolinkerd:masterfrom
Conversation
arminbuerkle
changed the title
arminbuerkle
force-pushed
the
add-cluster-domain-to-controller
branch
from
f8af8c4 to
d30e940
Compare
arminbuerkle
force-pushed
the
add-cluster-domain-to-controller
branch
2 times, most recently
from
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Armin Buerkle <[email protected]>
Signed-off-by: Armin Buerkle <[email protected]>
arminbuerkle
force-pushed
the
add-cluster-domain-to-controller
branch
from
dc1c949 to
f0d0ddc
Compare
arminbuerkle
left a comment
arminbuerkle
left a comment
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 <[email protected]>
Signed-off-by: Armin Buerkle <[email protected]>
arminbuerkle
force-pushed
the
add-cluster-domain-to-controller
branch
from
0b8dcd5 to
fe3788e
Compare
ihcsim
left a comment
ihcsim
left a comment
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 <[email protected]>
|
Thank you @ihcsim for testing. Your suggestion indeed fixed that. |
ihcsim
left a comment
ihcsim
left a comment
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 <[email protected]>
2 participants
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":""}