Improve log aggregator host/port validation #14333
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Simplify validation functions for logging settings - Abstract some host/port parsing logic out in utils.external_settings so the validation function can use it as settings are updated - Never crash-loop if somehow http logging settings are invalid, just log errors (to the console!) and write a default /dev/null syslog config. - Add a bunch of new test cases for various scenarios (most of them pass before the changes, but we throw some new errors now when validation fails). Fixes ansible#13823 Refs ansible#13829 Signed-off-by: Rick Elrod <[email protected]>
AlanCoding
reviewed
Aug 11, 2023
Signed-off-by: Rick Elrod <[email protected]>
AlanCoding
reviewed
Aug 25, 2023
| # It would be really nice to check that if a port number is set here, then it's not also set in LOG_AGGREGATOR_PORT. | ||
| # But that would be a breaking change vs. silently preferring the port number in LOG_AGGREGATOR_HOST. | ||
| try: | ||
| host, port, parsed = validate_http_host_port(new_host, throw=True) |
There was a problem hiding this comment.
nice to reuse the validation method in both places.
AlanCoding
reviewed
Aug 25, 2023
| 'LOG_AGGREGATOR_TYPE', | ||
| 'LOG_AGGREGATOR_PROTOCOL', | ||
| ) | ||
| if not serializer.instance or not all(hasattr(serializer.instance, attr) for attr in needed_attrs) or not attrs.get('LOG_AGGREGATOR_ENABLED', False): |
There was a problem hiding this comment.
This is messing me up. I'm trying to manually run your test cases, but essentially all cases are bailing early on this line and not throwing the 400 errors because of that.
Still not sure to do with this information, but that's the big thing for verifying the functionality of this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't merge yet - I want to add more tests.
SUMMARY
Simplify validation functions for logging settings
Abstract some host/port parsing logic out in utils.external_logging so the validation function can use it as settings are updated
Never crash-loop if somehow http logging settings are invalid, just log errors (to the console!) and write a default /dev/null syslog config.
Add a bunch of new test cases for various scenarios (most of them pass before the changes, but we throw some new errors now when validation fails).
Fixes #13823
Refs #13829
ISSUE TYPE
COMPONENT NAME