Skip to content

Conversation

knowald
Copy link

@knowald knowald commented Jul 23, 2025

Summary

This PR adds validation to reject invalid duration units (like d, w, mo, y) when used
with the CERTIFICATE_EXPIRATION condition. Go's time.Duration only supports units up to
hours (h), so using larger units like days would silently fail and evaluate to 0.

Problem

Previously, when users attempted to use conditions like:

conditions:
  - "[CERTIFICATE_EXPIRATION] > 14d"

The 14d would fail to parse as a valid duration and default to 0, causing the condition to
behave unexpectedly without any clear error message.

Related issues:

Solution

Added validation that:

  1. Detects invalid duration units during condition validation
  2. Returns an error message suggesting the use of hours instead

Example Error Message

When using an invalid duration unit:
invalid duration unit in condition '[CERTIFICATE_EXPIRATION] > 14d': Go's time.Duration only supports units up to 'h' (hours). Use hours instead (e.g., '336h' instead of '14d')

Changes

  • Added isDurationWithInvalidUnit() helper to detect invalid duration formats
  • Updated Condition.Validate() to check for invalid units in CERTIFICATE_EXPIRATION
    conditions

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Updated documentation in README.md, if applicable.

@github-actions github-actions bot added the feature New feature or request label Jul 23, 2025
@TwiN
Copy link
Owner

TwiN commented Jul 23, 2025

Adding support for other duration units just for certificate expiration would create confusion.

Beyond that, I'm ok with adding support for days, but weeks, months and years I think is too much, reason being the m of month overlaps with the m of minute, which may create additional confusion.

@knowald
Copy link
Author

knowald commented Jul 23, 2025

@TwiN This PR doesn't add support for new duration types - I understand your preference for sticking with time.Duration types only. Instead, it provides clearer feedback to users who try to use unsupported formats. When I initially tried using 14d (before the README note about unsupported units was added), it wasn't clear why it didn't work. This change helps prevent similar confusion for other users by explicitly indicating when they're using an unsupported unit.

@knowald
Copy link
Author

knowald commented Jul 24, 2025

Tests have been updated and are passing. If this approach doesn't suit the project, I'm open to revising it.

Comment on lines +93 to +94
// invalidDurationUnitError is the error message for invalid duration units, see https://pkg.go.dev/time#Duration
invalidDurationUnitError = "invalid duration unit in condition '%s': Go's time.Duration only supports units up to 'h' (hours). Use hours instead (e.g., '336h' instead of '14d')"
Copy link
Owner

Choose a reason for hiding this comment

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

For the sake of consistency, the error should be something like

var (
	ErrInvalidDurationUnit = errors.New("invalid duration...")
)

as opposed to a constant string. Maybe at some point I'll change my mind, but since the entirety of the code uses this approach, I'd rather remain consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants