From 1f8740589ac6b28f7d9ce19bff989462e63cce2a Mon Sep 17 00:00:00 2001 From: Adam Connelly Date: Sat, 20 Jun 2020 13:11:33 +0100 Subject: [PATCH] Ensure that schedule is a valid Cron expression I've added the framework to perform additional validation on a field, and implemented a Cron expression validator for the scraping schedule. Each field can have a collection of zero or more validators that implement `IFieldValidator`, and the validator uses the error reporter to report any validation errors. Fixes #1103 --- changelog/content/experimental/unreleased.md | 3 +- .../Serialization/Deserializer.cs | 1 + .../FieldDeserializationContext.cs | 17 ++++- .../Serialization/FieldDeserializationInfo.cs | 16 ++++- .../FieldDeserializationInfoBuilder.cs | 12 +++- .../CronExpressionValidator.cs | 50 +++++++++++++ .../FieldValidators/IFieldValidator.cs | 19 +++++ .../v1/Core/ScrapingDeserializer.cs | 3 +- .../Promitor.Core.Scraping.csproj | 1 + .../DeserializationContextTests.cs | 3 +- .../DeserializerTests/ValidationTests.cs | 18 +++++ .../CronExpressionValidatorTests.cs | 72 +++++++++++++++++++ 12 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/CronExpressionValidator.cs create mode 100644 src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/IFieldValidator.cs create mode 100644 src/Promitor.Tests.Unit/Serialization/FieldValidators/CronExpressionValidatorTests.cs diff --git a/changelog/content/experimental/unreleased.md b/changelog/content/experimental/unreleased.md index bbac4cfc5..a92cbe5fa 100644 --- a/changelog/content/experimental/unreleased.md +++ b/changelog/content/experimental/unreleased.md @@ -8,4 +8,5 @@ version: - {{% tag added %}} New validation rule to ensure at least one resource or resource collection is configured to scrape - {{% tag removed %}} Support for Swagger 2.0 ([deprecation notice](https://changelog.promitor.io/#swagger-2-0)) - {{% tag removed %}} Support for Swagger UI 2.0 ([deprecation notice](https://changelog.promitor.io/#swagger-ui-2-0)) -- {{% tag added %}} Provide suggestions when unknown fields are found in the metrics config. [#1105](https://github.com/tomkerkhove/promitor/issues/1105). \ No newline at end of file +- {{% tag added %}} Provide suggestions when unknown fields are found in the metrics config. [#1105](https://github.com/tomkerkhove/promitor/issues/1105). +- {{% tag added %}} Add validation to ensure the scraping schedule is a valid Cron expression. [#1103](https://github.com/tomkerkhove/promitor/issues/1103). \ No newline at end of file diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs index 97001b34c..5fb26518f 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs @@ -42,6 +42,7 @@ public virtual TObject Deserialize(YamlMappingNode node, IErrorReporter errorRep { fieldContext.SetValue( result, GetFieldValue(child, fieldContext.DeserializationInfo, errorReporter)); + fieldContext.Validate(child, errorReporter); } else { diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationContext.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationContext.cs index 29cf85eec..d4b25c2e8 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationContext.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationContext.cs @@ -1,4 +1,6 @@ -using GuardNet; +using System.Collections.Generic; +using GuardNet; +using YamlDotNet.RepresentationModel; namespace Promitor.Core.Scraping.Configuration.Serialization { @@ -41,6 +43,19 @@ public void SetValue(TObject target, object value) HasBeenSet = true; } + /// + /// Runs any custom validators that have been defined for the field. + /// + /// The pair of nodes defining the name of the field, and its value. + /// Used to report any validation errors. + public void Validate(KeyValuePair fieldNodes, IErrorReporter errorReporter) + { + foreach (var validator in DeserializationInfo.Validators) + { + validator.Validate(fieldNodes.Value.ToString(), fieldNodes, errorReporter); + } + } + /// /// Sets the field to its default value. /// diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfo.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfo.cs index 34f317028..653aa39c2 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfo.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfo.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Reflection; +using Promitor.Core.Scraping.Configuration.Serialization.FieldValidators; using YamlDotNet.RepresentationModel; namespace Promitor.Core.Scraping.Configuration.Serialization @@ -20,7 +21,14 @@ public class FieldDeserializationInfo /// The default value to use for the field if not specified. /// A custom function to use for getting the value of the field. /// A deserializer to use to deserialize the field. - public FieldDeserializationInfo(PropertyInfo propertyInfo, bool isRequired, object defaultValue, Func, IErrorReporter, object> customMapperFunc, IDeserializer deserializer = null) + /// Any custom validators to validate the field using. + public FieldDeserializationInfo( + PropertyInfo propertyInfo, + bool isRequired, + object defaultValue, + Func, IErrorReporter, object> customMapperFunc, + IDeserializer deserializer, + IReadOnlyCollection validators) { YamlFieldName = GetName(propertyInfo); CustomMapperFunc = customMapperFunc; @@ -28,6 +36,7 @@ public FieldDeserializationInfo(PropertyInfo propertyInfo, bool isRequired, obje IsRequired = isRequired; DefaultValue = defaultValue; Deserializer = deserializer; + Validators = validators; } /// @@ -60,6 +69,11 @@ public FieldDeserializationInfo(PropertyInfo propertyInfo, bool isRequired, obje /// public IDeserializer Deserializer { get; } + /// + /// Gets the custom validators for the field. + /// + public IReadOnlyCollection Validators { get; } + private static string GetName(MemberInfo propertyInfo) { return char.ToLowerInvariant(propertyInfo.Name[0]) + propertyInfo.Name.Substring(1); diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfoBuilder.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfoBuilder.cs index 962574acb..d3cd42e66 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfoBuilder.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldDeserializationInfoBuilder.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Linq.Expressions; using System.Reflection; +using Promitor.Core.Scraping.Configuration.Serialization.FieldValidators; using YamlDotNet.RepresentationModel; namespace Promitor.Core.Scraping.Configuration.Serialization @@ -11,6 +12,7 @@ namespace Promitor.Core.Scraping.Configuration.Serialization /// public class FieldDeserializationInfoBuilder : IFieldDeserializationInfoBuilder { + private readonly List _validators = new List(); private PropertyInfo _propertyInfo; private bool _isRequired; private object _defaultValue; @@ -88,7 +90,15 @@ public FieldDeserializationInfoBuilder MapUsingDeserializer(ID public FieldDeserializationInfo Build() { return new FieldDeserializationInfo( - _propertyInfo, _isRequired, _defaultValue, _customMapperFunc, _deserializer); + _propertyInfo, _isRequired, _defaultValue, _customMapperFunc, _deserializer, _validators); + } + + /// + /// Indicates that this field should be validated as a Cron expression. + /// + public void ValidateCronExpression() + { + _validators.Add(new CronExpressionValidator()); } } } \ No newline at end of file diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/CronExpressionValidator.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/CronExpressionValidator.cs new file mode 100644 index 000000000..a73caff49 --- /dev/null +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/CronExpressionValidator.cs @@ -0,0 +1,50 @@ +using System.Collections.Generic; +using Cronos; +using YamlDotNet.RepresentationModel; + +namespace Promitor.Core.Scraping.Configuration.Serialization.FieldValidators +{ + /// + /// Ensures that a field is a valid Cron expression. + /// + public class CronExpressionValidator : IFieldValidator + { + /// + public void Validate(string value, KeyValuePair fieldNodes, IErrorReporter errorReporter) + { + if (!string.IsNullOrEmpty(value)) + { + if (!IsValidStandardExpression(value) && !IsValidExpressionWithSeconds(value)) + { + errorReporter.ReportError(fieldNodes.Value, $"'{value}' is not a valid value for '{fieldNodes.Key}'. The value must be a valid Cron expression."); + } + } + } + + private static bool IsValidStandardExpression(string value) + { + try + { + CronExpression.Parse(value); + return true; + } + catch (CronFormatException) + { + return false; + } + } + + private static bool IsValidExpressionWithSeconds(string value) + { + try + { + CronExpression.Parse(value, CronFormat.IncludeSeconds); + return true; + } + catch (CronFormatException) + { + return false; + } + } + } +} \ No newline at end of file diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/IFieldValidator.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/IFieldValidator.cs new file mode 100644 index 000000000..c0af309cf --- /dev/null +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/FieldValidators/IFieldValidator.cs @@ -0,0 +1,19 @@ +using System.Collections.Generic; +using YamlDotNet.RepresentationModel; + +namespace Promitor.Core.Scraping.Configuration.Serialization.FieldValidators +{ + /// + /// Provides validation for a yaml field. + /// + public interface IFieldValidator + { + /// + /// Validates a field, reporting any errors via the error reporter. + /// + /// The value of the field. + /// A KeyValuePair where the Key is the field name, and the Value is its value. + /// The error reporter. + void Validate(string value, KeyValuePair fieldNodes, IErrorReporter errorReporter); + } +} \ No newline at end of file diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/ScrapingDeserializer.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/ScrapingDeserializer.cs index 9454ec5c9..851c30a68 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/ScrapingDeserializer.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/ScrapingDeserializer.cs @@ -8,7 +8,8 @@ public class ScrapingDeserializer : Deserializer public ScrapingDeserializer(ILogger logger) : base(logger) { Map(scraping => scraping.Schedule) - .IsRequired(); + .IsRequired() + .ValidateCronExpression(); } } } diff --git a/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj b/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj index ad8857d9b..71f719121 100644 --- a/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj +++ b/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj @@ -17,6 +17,7 @@ + diff --git a/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs b/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs index 19e91b7c4..ecbef055d 100644 --- a/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs +++ b/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs @@ -1,6 +1,7 @@ using System.Collections.Generic; using System.Reflection; using Promitor.Core.Scraping.Configuration.Serialization; +using Promitor.Core.Scraping.Configuration.Serialization.FieldValidators; using Xunit; namespace Promitor.Tests.Unit.Serialization @@ -88,7 +89,7 @@ public void GetSuggestions_IgnoredFields_IncludesIgnoredFieldsInSuggestions() private FieldDeserializationInfo CreateField(PropertyInfo propertyInfo) { - return new FieldDeserializationInfo(propertyInfo, false, null, null, null); + return new FieldDeserializationInfo(propertyInfo, false, null, null, null, new List()); } public class Person diff --git a/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs b/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs index 6ef54ea09..e1074ea05 100644 --- a/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs +++ b/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs @@ -168,6 +168,21 @@ public void Deserialize_UnknownField_ReturnsSuggestion() _errorReporter.Verify(r => r.ReportWarning(nameTagNode, "Unknown field 'nmae'. Did you mean 'name'?")); } + [Fact] + public void Deserialize_CronSyntaxInvalid_ReportsError() + { + // Arrange + var node = YamlUtils.CreateYamlNode("schedule: 12345"); + var dayValueNode = node.Children.Single(c => c.Key.ToString() == "schedule").Value; + + // Act / Assert + YamlAssert.ReportsError( + _deserializer, + node, + dayValueNode, + "'12345' is not a valid value for 'schedule'. The value must be a valid Cron expression."); + } + [Fact] public void Deserialize_UnknownField_ReturnsMultipleSuggestions() { @@ -190,6 +205,7 @@ private class TestConfigObject public DayOfWeek Day { get; set; } public string Date { get; set; } public TimeSpan Interval { get; set; } + public string Schedule { get; set; } } private class TestDeserializer: Deserializer @@ -201,6 +217,8 @@ public TestDeserializer() : base(NullLogger.Instance) Map(t => t.Day); Map(t => t.Date); Map(t => t.Interval); + Map(t => t.Schedule) + .ValidateCronExpression(); IgnoreField("customField"); } } diff --git a/src/Promitor.Tests.Unit/Serialization/FieldValidators/CronExpressionValidatorTests.cs b/src/Promitor.Tests.Unit/Serialization/FieldValidators/CronExpressionValidatorTests.cs new file mode 100644 index 000000000..324e9885e --- /dev/null +++ b/src/Promitor.Tests.Unit/Serialization/FieldValidators/CronExpressionValidatorTests.cs @@ -0,0 +1,72 @@ +using System.Linq; +using Moq; +using Promitor.Core.Scraping.Configuration.Serialization; +using Promitor.Core.Scraping.Configuration.Serialization.FieldValidators; +using Xunit; +using YamlDotNet.RepresentationModel; + +namespace Promitor.Tests.Unit.Serialization.FieldValidators +{ + public class CronExpressionValidatorTests + { + private readonly CronExpressionValidator _validator = new CronExpressionValidator(); + + [Fact] + public void Validate_InvalidExpression_ReportsError() + { + AssertExpressionIsNotValid("invalid-expression"); + } + + [Fact] + public void Validate_ValidExpression_DoesNotReportError() + { + AssertExpressionIsValid("*/5 * * * *"); + } + + [Fact] + public void Validate_ValidExpressionWithSeconds_DoesNotReportError() + { + AssertExpressionIsValid("* */5 * * * *"); + } + + [Fact] + public void Validate_EmptyString_DoesNotReportError() + { + AssertExpressionIsValid(string.Empty); + } + + private void AssertExpressionIsNotValid(string cronExpression) + { + // Arrange + var fieldNodes = YamlUtils + .CreateYamlNode($"schedule: '{cronExpression}'") + .Children + .First(); + var errorReporter = new Mock(); + + // Act + _validator.Validate(fieldNodes.Value.ToString(), fieldNodes, errorReporter.Object); + + // Assert + errorReporter.Verify( + e => e.ReportError(fieldNodes.Value, $"'{cronExpression}' is not a valid value for 'schedule'. The value must be a valid Cron expression.")); + } + + private void AssertExpressionIsValid(string cronExpression) + { + // Arrange + var fieldNodes = YamlUtils + .CreateYamlNode($"schedule: '{cronExpression}'") + .Children + .First(); + var errorReporter = new Mock(); + + // Act + _validator.Validate(fieldNodes.Value.ToString(), fieldNodes, errorReporter.Object); + + // Assert + errorReporter.Verify( + e => e.ReportError(It.IsAny(), It.IsAny()), Times.Never); + } + } +} \ No newline at end of file