Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use strict teleport.yaml validation in warning mode #5057

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

awly
Copy link
Contributor

@awly awly commented Dec 4, 2020

Strict YAML validation catches the cases where a valid config key is
placed in the wrong location in the config. These errors were not
caught by the old validation.
The failure is always reported, but only fails startup when both old and
new validations fail. This will let the users fix their configs during
6.0 release and we will start enforcing it in 7.0.

Example:

auth_service:
  data_dir: "/foo" # this field must live under "teleport:", not "auth_service:"

Output:

$ teleport start -c teleport-invalid.yaml
ERRO             "Teleport configuration is invalid: yaml: unmarshal errors:\n  line 6: field data_dir not found in type config.Auth." config/fileconf.go:303
ERRO             This error will be enforced in the next Teleport release. config/fileconf.go:304
[AUTH]         Auth service 5.0.0-dev:v4.4.0-alpha.1-262-g307040886-dirty is starting on 0.0.0.0:3025.
... continues startup ...

Updates #5056

Copy link
Contributor

@a-palchikov a-palchikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm except for the time.Sleep - is logging always configured to output to stderr so the user can actually see it?

@awly
Copy link
Contributor Author

awly commented Dec 4, 2020

@a-palchikov my reason for the sleep is that most users don't configure/run teleport manually, but provision it via some tooling (ansible, k8s, etc). So they don't see the logs most of the time, unless something goes wrong.
A startup delay might nudge them to investigate and actually look at logs.

This is kind of speculative, so if others also thing the sleep is a bad idea, I'll remove it.

Copy link
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awly can you fix the output to remove newlines? see in the output how the line is quoted

@awly
Copy link
Contributor Author

awly commented Dec 4, 2020

@klizhentas that output is coming from yaml.UnmarshalStrict, not from our code.
Should I strings.Replace all the newlines in the returned error?

@klizhentas
Copy link
Contributor

@awly yes, I would remove newlines from the output

@awly awly requested a review from klizhentas December 7, 2020 18:11
@awly
Copy link
Contributor Author

awly commented Dec 7, 2020

Removed newlines, output looks like:

$ teleport start -c teleport-invalid.yaml
ERRO             Teleport configuration is invalid: yaml: unmarshal errors:  line 8: field data_dir not found in type config.Auth. config/fileconf.go:306
ERRO             This error will be enforced in the next Teleport release. config/fileconf.go:307

@awly
Copy link
Contributor Author

awly commented Dec 14, 2020

@klizhentas PTAL

Andrew Lytvynov added 2 commits December 18, 2020 13:58
Strict YAML validation catches the cases where a valid config key is
placed in the wrong location in the config. These errors were not
caught by the old validation.
The failure is always reported, but only fails startup when both old and
new validations fail. This will let the users fix their configs during
6.0 release and we will start enforcing it in 7.0.

Example:
```yaml
auth_service:
  data_dir: "/foo" # this field must live under "teleport:", not "auth_service:"
```

Output:
```
$ teleport start -c teleport-invalid.yaml
ERRO             "Teleport configuration is invalid: yaml: unmarshal errors:\n  line 6: field data_dir not found in type config.Auth." config/fileconf.go:303
ERRO             This error will be enforced in the next Teleport release. config/fileconf.go:304
[AUTH]         Auth service 5.0.0-dev:v4.4.0-alpha.1-262-g307040886-dirty is starting on 0.0.0.0:3025.
... continues startup ...
```
@awly awly force-pushed the andrew/config-strict-validation-warning branch from a704fc2 to d1e844b Compare December 18, 2020 21:58
@awly awly merged commit 225777c into master Dec 18, 2020
@awly awly deleted the andrew/config-strict-validation-warning branch December 18, 2020 22:11
awly pushed a commit that referenced this pull request Apr 20, 2021
Strict validation was added in warning mode in
#5057 and released in 6.0.

For 7.0, we can drop the legacy custom validation logic, with the
assumption that all bad configs were migrated.
awly pushed a commit that referenced this pull request Apr 21, 2021
Strict validation was added in warning mode in
#5057 and released in 6.0.

For 7.0, we can drop the legacy custom validation logic, with the
assumption that all bad configs were migrated.
awly pushed a commit that referenced this pull request Apr 21, 2021
* Enforce strict teleport.yaml validation

Strict validation was added in warning mode in
#5057 and released in 6.0.

For 7.0, we can drop the legacy custom validation logic, with the
assumption that all bad configs were migrated.

* Implement 'teleport configure --test' command

This command tests an existing config for errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants