Fix debug service not being disabled by configuration #46032
Conversation
tool/teleport/common/teleport.go
Outdated
| start.Flag("disable-debug-service", | ||
| "Disables debug service.").Hidden(). | ||
| BoolVar(&ccf.DisabledDebugService) |
There was a problem hiding this comment.
I would name the flag debug-service, make it a BoolVar() and .Default(true).
This will automatically generate a --no-debug-service option that can be used to disable it.
There was a problem hiding this comment.
Wouldn't adding such a flag make the configuration obsolete? Regarding configuration parsing, the command-line flags have priority over the default and user-provided configurations. For example, given that the value would be true, even when users change it to debug_service.enabled, it would always get overwritten by the CLI value.
We could name it the --no-debug-service (without creating the --debug-service) flag. That way, we can keep the configuration in the file so it won't change the current behavior. What do you think?
There was a problem hiding this comment.
I'd like to avoid the double negatives if we can. For example, tsh ssh --help shows this:
--[no-]no-resume Disable SSH connection resumption
--no-no-resume is a very awkward UX.
There was a problem hiding this comment.
I see. I wasn't aware that the `--no—' option cannot be disabled.
To keep the name as --no-debug-service and the configuration file, we would need only to use the value of this flag if it is set to false (similar to what we do for file config). The UX difference is that the --debug-service wouldn't affect the final configuration.
Also, this flag will be hidden, so I don't think there will be any problem with the auto-generated option (regarding the flag usage).
There was a problem hiding this comment.
Regarding configuration parsing, the command-line flags have priority over the default and user-provided configurations. For example, given that the value would be
true, even when users change it todebug_service.enabled, it would always get overwritten by the CLI value.
Ah, okay, I see this now. We cannot have BoolVar with Default(true) because it'd always take precedence over whatever is in the config file and the only way to turn off the debug service would be to pass the --no-debug-service flag.
This feels cursed, but after thinking about it for a sec, this behavior makes sense from kingpin's PoV – you say you have a boolean flag and you want the default to be true, even if the flag wasn't specified.
So ultimately wouldn't the solution be to have a bool flag called debug-service with no default on the flag? Would it then work as expected? As in:
- No flag specified and no config:
debug_service.enabledthrough setting defaults on the config. - No flag and config:
debug_service.enabledset to whatever is in the config. - Flag and config: the flag takes precedence over the config.
We could name it the
--no-debug-service(without creating the--debug-service) flag. That way, we can keep the configuration in the file so it won't change the current behavior. What do you think?
Looking at the output of teleport start --help, I think we definitely should go with --no-debug-service. There's no other flag that starts with disable. I also don't really see why it would need to be hidden.
There was a problem hiding this comment.
The thing is, we don't have a way to know whether the flag was set to false by the user or if it uses the field struct bool default value. So, if you perform a teleport start without any flag and no default value to debug-service, it would be set to false.
There was a problem hiding this comment.
It feels like something we should address at some point, otherwise I guess we'll run into more and more situations like this one.
What about going with no-debug-service without supporting debug-service as you suggested in #46032 (comment)? I feel like we should try to make the flag public if possible.
There was a problem hiding this comment.
I've updated the name to no-debug-service and made it public. I agree the double negative (--[no-]no-debug-service) is not good, but it seems like having a flag (e.g., --debug-service) that does not affect its standard format (non-negative) is worse. Let me know what you think.
* fix(config): update conditional check to disabled debug service * refactor(teleport): rename flag and make it public * test(config): add a test case with config file and command line arg * chore(config): fix command line flag name typo
gabrielcorado
deleted the
gabrielcorado/fix-debug-service-not-being-disabled
branch
|
@gabrielcorado See the table below for backport results.
|
#45464 introduced a bug that prevented the debug service from being disabled. The problem was during the
ApplyFileConfig, called during theteleport startcommand as part of the CLI flags, file config, and default values merge. Since the change, the debug service value would always be enabled by default, meaning that during the merge, we should, instead of checking if it is enabled, check if it is disabled and change the value accordingly.This PR also adds a new hidden flag to
teleport start:--disable-debug-service. This flag can turn off the debug service without requiring a configuration file.changelog: Fixed debug service not being turned off by configuration; Connect My Computer in Teleport Connect should no longer fail with "bind: invalid argument"