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

Clock matcher #225

Merged
merged 5 commits into from
Aug 2, 2024
Merged

Clock matcher #225

merged 5 commits into from
Aug 2, 2024

Conversation

vnxme
Copy link
Collaborator

@vnxme vnxme commented Aug 1, 2024

Summary

The clock matcher allows to match connections using the time when their wrapping/matching occurs. Restrict access to some resources in the night? Use more proxy upstreams during the busy hours? Such scenarios can't be realised without this matcher.

Syntax

Caddyfile. Generally clock <time_after> <time_before> [<time_zone>] should be used, but clock <after|from> <time_after> [<time_zone>] and clock <before|till|to|until> <time_before> [<time_zone>] shortcuts are also available for convenience.

JSON. The matcher has 2 mandatory string options: after and before. Each one accepts time in 15:04:05 format. If before has 00:00:00 value, it is treated as 24:00:00. If after is greater than before, these time points are automatically swapped. Besides, there is the third timezone option, which may be used to match the time points in any IANA time zone location or a custom fixed time zone defined by an offset (e.g. +02, -03:30 or even +12:34:56) other than the default UTC (use "Local" to have the system's local time zone). The matcher returns true, if the connection wrapping/matching time is greater than or equal to after AND less than before.

Example

{
	layer4 {
		:8080 {
			@night_m clock before 05:00:00
			@morning clock 05:00:00 12:00:00
			@afternoon clock 12:00:00 17:00:00
			@evening clock 17:00:00 21:00:00
			@night_e clock after 21:00:00
			route @night_m @night_e {
				proxy 00.upstream.local:8080
			}
			route @morning {
				proxy 01.upstream.local:8080 02.upstream.local:8080
			}
			route @afternoon {
				proxy 03.upstream.local:8080 04.upstream.local:8080 05.upstream.local:8080
			}
			route @evening {
				proxy 06.upstream.local:8080 07.upstream.local:8080
			}
		}
		:8888 {
			@la_is_awake clock 08:00:00 20:00:00 America/Los_Angeles
			route @la_is_awake {
				proxy existing.machine.local:8888
			}
			@la_is_asleep not clock 08:00:00 20:00:00 America/Los_Angeles
			route @la_is_asleep {
				proxy non-existing.machine.local:8888
			}
		}
	}
}

@mholt
Copy link
Owner

mholt commented Aug 2, 2024

Ha, neat. This is something I actually thought of when originally designing Caddy v2 back in 2019 -- I figured it could be fun to have something like this for the HTTP handler chain, to make routing decisions based on time of day. 🤷‍♂️

Anyway, I like this.

@IceCodeNew
Copy link
Collaborator

Fantastic idea! I find the implementation very straightforward and lucid, and I admire how simple the example configuration is for users to get started.

However, I have some concerns regarding the current design. It doesn't mandate users to declare the timezone explicitly, which could result in unforeseen complications.

Take the official Caddy Docker image as an example; it lacks the tzdata package. Even if users set their local timezone with TZ=xxx, the Caddy container will still run in the UTC timezone. And that may take users by surprise.

Moreover, the same configuration could behave differently across machines due to the variance in timezone settings—I do not prefer such a design.

I advocate mandating users to declare the timezone they desire to prevent unexpected behavior.

@vnxme
Copy link
Collaborator Author

vnxme commented Aug 2, 2024

@IceCodeNew, thank you for a valid concern. I've added time zone support.

Take the official Caddy Docker image as an example; it lacks the tzdata package. Even if users set their local timezone with TZ=xxx, the Caddy container will still run in the UTC timezone. And that may take users by surprise.

I suppose that's a serious issue, and the tzdata package has to be included in the official Caddy Docker image.

For this matcher, the tzdata package is not required due to the embedded time zone data (_ "time/tzdata" in imports).

Copy link
Collaborator

@IceCodeNew IceCodeNew left a comment

Choose a reason for hiding this comment

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

I prefer to mandatory the timezone, and @morning clock 05:00:00 12:00:00 should be considered invalid.

IMO that is the only way to make sure users have full awareness of how caddy would behave across machines, no matter how those machines are configured.
And I think it matters ;-)

@vnxme
Copy link
Collaborator Author

vnxme commented Aug 2, 2024

I see your point, but can't fully agree with you. In my view, mandatory timezone definition for each clock matcher occurrence in Caddyfile is too much. If the user is really concerned about time zones, one will definitely have the tzdata package installed along with Caddy and the system's local time zone adjusted as needed.

Generally nothing prevents you from defining time zones explicitly if you want. But it's mentioned in the comments to the code what time zone is used by default, and it's basically the user's problem if one doesn't read and understand the docs (hope someone will write it when caddy-l4 gets merged into the mainline).

@IceCodeNew
Copy link
Collaborator

I see your point, but can't fully agree with you. In my view, mandatory timezone definition for each clock matcher occurrence in Caddyfile is too much. If the user is really concerned about time zones, one will definitely have the tzdata package installed along with Caddy and the system's local time zone adjusted as needed.

Generally nothing prevents you from defining time zones explicitly if you want. But it's mentioned in the comments to the code what time zone is used by default, and it's basically the user's problem if one doesn't read and understand the docs (hope someone will write it when caddy-l4 gets merged into the mainline).

That is OK, I am not totally against the current design.
Just want to clarify that the problem around the caddy container images is solve (by the latest commit, terrific).
I am concernded about various environment where caddy could be kicked start, the official container images are just one thing that came up to me first.

Besides missing tzdata, there are still things remains that could go wrong. For example there are locations practicing daylight saving time , what would @morning clock 05:00:00 12:00:00 mean in such a case? Is 05:00:00 the time been set forward or not?

@francislavoie
Copy link
Collaborator

IMO most people who will be using this will understand what UTC is. Default to UTC (not system time), have an option to specify TZ or local_time (that's how logging works, see https://caddyserver.com/docs/caddyfile/directives/log#format-modules time_local). Document that TZ requires having system support for it (e.g. tzdata in Docker). Good enough I think, as long as the behaviour is documented.

Personally I'm not sure how common a need this would be that it has to be a standard inclusion in caddy-l4, but it's simple enough that I don't mind it. I do think this should be ported to Caddy's HTTP matchers too, where it would likely be used more often (just a wider userbase).

@vnxme
Copy link
Collaborator Author

vnxme commented Aug 2, 2024

For example there are locations practicing daylight saving time , what would @morning clock 05:00:00 12:00:00 mean in such a case? Is 05:00:00 the time been set forward or not?

This matcher takes into account the daylight savings based on the location. @morning clock 05:00:00 12:00:00 America/Los_Angeles will match the morning in California in both summer and winter. In other words, nothing to worry about.

@vnxme
Copy link
Collaborator Author

vnxme commented Aug 2, 2024

Default to UTC (not system time), have an option to specify TZ or local_time

Changed defaults to UTC. It is possible to use "Local" value to have the system's local time zone instead of UTC.

Document that TZ requires having system support for it (e.g. tzdata in Docker).

Time zone data is embedded by golang, so no extra packages are really required for this matcher to work.

I do think this should be ported to Caddy's HTTP matchers too, where it would likely be used more often (just a wider userbase).

It's possible to use layer4 as a listener wrapper around http. As long as caddy-l4 is merged into the mainline, no need to port anything.

@IceCodeNew
Copy link
Collaborator

UTC by default instead of the local timezone LGTM. I have no concerns about current implementation.

Copy link
Owner

@mholt mholt left a comment

Choose a reason for hiding this comment

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

This looks cool. Just one nit about docs and format and I think we can merge it

Comment on lines 34 to 36
After string `json:"after,omitempty"`
Before string `json:"before,omitempty"`
Timezone string `json:"timezone,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

The expected format(s) should be documented here, maybe an example as well. HH:MM:SS (24-hour format).

Also, I find it easier to specify UTC offsets for timezones, or 3-letter tz abbreviations.. like "-6:00" or "MST" or something like that. That's just my preference and I have no idea the implications of allowing that, but... what are your thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've provided comments for the matcher fileds. Besides, I've added custom offset support (e.g -06:00 is a valid timezone value now). 3-letter timezone abbreviations are ambiguous and not included in the standard time library.

Copy link
Owner

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome; thanks for the contribution!

@mholt mholt merged commit 3524134 into mholt:master Aug 2, 2024
6 checks passed
@vnxme vnxme deleted the clock-matcher branch August 7, 2024 09:32
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