root: Set Helper.State for other directives/plugins to use

[henderkes]

Assistance Disclosure

No AI was used.

I feel like it may be useful to also generate warnings in case a root statement overwrites an existing one, e.g.

localhost

root /path
root * /path2

# or

root * /path1
root * /path2

But that may be a separate discussion.

[henderkes]

see: https://github.com/henderkes/caddy/blob/05997e06f55695e6b7f7a954455d9b8b96be970e/caddyconfig/httpcaddyfile/httptype.go#L1437

[henderkes]

we have both accesses with strings and with constants (e.g. namedRoute), so I wasn't sure if I should add one here

[francislavoie]

Overwriting root has many legitimate usecases, so let's not add a warning for that. For example you'd have a subpath like handle /uploads/* which changes the root to somewhere else, or intercept which changes the root to allow X-Accel-Redirect to work.

[francislavoie]

LGTM

[francislavoie]

Worth noting though, this could misbehave:

example.com {
	root /foo

	handle_path /bar* {
		root /bar
		file_server
	}

	php_server # oops, root is /bar now
}

I'm not 100% sure but needs checking.

[henderkes]

Worth noting though, this could misbehave:

example.com {
	root /foo

	handle_path /bar* {
		root /bar
		file_server
	}

	php_server # oops, root is /bar now
}

I'm not 100% sure but needs checking.

You're right, that breaks it, it overwrites the root to /bar.

[henderkes]

:80 {
	root * public # * optional
	handle_path /bar* {
		root bar # gets initial copy of State, but later changes to parent may not be picked up?
		php_server { # sibling to php_server, sharing State
			worker index.php
		}
	}

	php_server { # sibling to root directive, sharing State
		env APP_ENV test
		worker index.php
	}
}

This works for our purposes now, but I am not sure if the lack of later propagation of parent -> child could cause an issue. You're more familiar with the ways the Caddyfile is parsed and what may need access to later-updated values in the State.

Although it kind of feels weird to me that this was the behaviour in the first place.

Edit: also, since this is a shallow copy, point values in the existing map would still be shared. Should I change this to a deep copy instead?

[francislavoie]

I didn't even remember we had State tbh 😂 so I don't remember what it gets used for these days.

A quick scan of the existing usages suggest it's only used for server-level config stuff like tls, logs, and invoke.

I think it could break invoke if it doesn't write back into the parent state because those get pulled out later at the end of server parsing, so if it was hidden within the block scope it would change how that behaves

caddy/caddyconfig/httpcaddyfile/httptype.go

Lines 164 to 169 in 5d189af

	if state[namedRouteKey] != nil {
	for name := range state[namedRouteKey].(map[string]struct{}) {
	result := ConfigValue{Class: namedRouteKey, Value: name}
	sb.pile[result.Class] = append(sb.pile[result.Class], result)
	}
	state[namedRouteKey] = nil

[francislavoie]

Maybe it would be better to introduce a new BlockState map which is the same as State except it's scoped like you set up.

[henderkes]

Thank you for the suggestion, but I think we'd all want to avoid duplicating that map if there's any helping it. It's too late tonight, but I'll try to figure out how this could potentially cause issues (or first verify whether invoke does). Can't hurt to get a bit more familiar with Caddy.

[henderkes]

• ["root"] is only used by this PR, shouldn't propagate back
• ["tlsCertTags"] inner reference map, changes will still propagate back to the parent if it had set them. only if parent tlsCertTags wasn't set, it wouldn't see changes by children
• • never seems to check for it, as far as I can see, so no problem
• ["logCounter"] would need to propagate back... but I don't think it can actually be increased in child directives of a site block?

The real question is what plugins do, outside of Caddy itself. I don't know if any of the would use State in a way where propagation back to the parent matters, but I assume it might. Unfortunately, creating a BlockState for things that should not carry back up seems to be the only way to prevent breakage for sure.

The question is whether it wouldn't be easier to just expose the parentBlock to child handlers instead. That would also satisfy our needs in frankenphp and we could iterate up through parent blocks to find the correct root. Though the BlockState would make it easier for us, of course.