Allow setting custom cluster domain in service profiles#3148
Conversation
| @@ -99,13 +99,13 @@ func newCmdProfile() *cobra.Command { | |||
| } | |||
|
|
|||
| if options.template { | |||
There was a problem hiding this comment.
Should we be reading the cluster domain from the config object?
There was a problem hiding this comment.
Yes and no:
I think there's two different use cases here:
- I want to create my default service profile with the cluster domain read from the config (default)
- I want to create a custom service profile with the cluster domain overwritten from cmd args
Allowing both uses cases at the same time shouldn't be a problem. I'd implement that behaviour in the (possible) next PR which introduces the cli flag to actually set a custom cluster domain.
There was a problem hiding this comment.
Why would you want to create a custom service profile with a different domain from the control plane?
There was a problem hiding this comment.
When you have multiple clusters, each with a different domain you don't want to switch into different kubectl contexts to create the appropriate service profile.
Instead it'd be easier to pass this as a cli flag so you can generate your service profile and then add it to source control.
So mostly out of convenience.
There was a problem hiding this comment.
@arminbuerkle I see what you're saying, but I think it will ultimately be confusing if the Linkerd CLI's behavior doesn't match with the current kubectl context. I'd prefer to maintain the assumption that the Linkerd CLI should always work with relation to the current kubectl context.
There was a problem hiding this comment.
@adleong I understand, although i wouldn't agree. I think having both options can only be beneficial especially when the default option (when --cluster-domain cli arg is NOT set) generates a service profile from the cluster config.
That being said: I implemented your suggestion in 8420bb3
Overwriting it from CLI wouldn't be part of this PR anyways.
arminbuerkle
force-pushed
the
add-cluster-domain-to-service-profiles-2
branch
from
fe738a4 to
0d2677c
Compare
arminbuerkle
force-pushed
the
add-cluster-domain-to-service-profiles-2
branch
from
0d2677c to
8420bb3
Compare
arminbuerkle
force-pushed
the
add-cluster-domain-to-service-profiles-2
branch
from
8420bb3 to
f0e61dd
Compare
Signed-off-by: Armin Buerkle <[email protected]>
Signed-off-by: Armin Buerkle <[email protected]>
arminbuerkle
force-pushed
the
add-cluster-domain-to-service-profiles-2
branch
from
f0e61dd to
ad19a8c
Compare
|
Just rebased to get the changes from #3187 in. Could someone take a look so we can get this merged? |
ihcsim
left a comment
ihcsim
left a comment
There was a problem hiding this comment.
LGTM 👍
@arminbuerkle IIRC, all that is left is the new CLI option and an update to the proxy container's environment variables (in the proxy injector), per #2930 (comment), right? Thanks again!
Followup to #3148 Wrong args order in call to `profiles.RenderOpenAPI` was generating an invalid service profile name. Signed-off-by: Alejandro Pedraza <[email protected]>
Followup to #3148 Wrong args order in call to `profiles.RenderOpenAPI` was generating an invalid service profile name. Signed-off-by: Alejandro Pedraza <[email protected]>
|
@ihcsim yes. CLI option and setting the proxy container env variable. |
4 participants
Continue of #2950.
I decided to check for the
clusterDomainin 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.