-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
caddytls: Run the replacer on automation policy subjects #5459
Conversation
I confirmed this fixes the case of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah, well thanks for being brave and doing this -- I do wonder if we could make this a little simpler though... (also the use of *Raw
makes me think of a module field)
// replace placeholders in subjects to allow environment variables | ||
repl := caddy.NewReplacer() | ||
subjects := make([]string, len(ap.SubjectsRaw)) | ||
for i, sub := range ap.SubjectsRaw { | ||
subjects[i] = repl.ReplaceAll(sub, "") | ||
} | ||
ap.subjects = subjects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work if we just replace the subjects in-place? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because then if you do curl localhost:2019/config/
you get a config without the placeholders in it (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that might be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well let's give it a shot - thanks!
Also renamed the field to SubjectsRaw, which can be considered a breaking change but I don't expect this to affect much.
5ccb00f
to
13cd375
Compare
Also renamed the field to SubjectsRaw, which can be considered a breaking change but I don't expect this to affect much.
This has come up semi-frequently. I think it's fine to support
{env.DOMAIN}
for subjects. I'm not 100% sure if this will work in all the cases we expect it to, but it should at least be better supported than before.