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

caddytls: verifier: caddyfile: re-add Caddyfile support #6127

Merged
merged 3 commits into from
Feb 25, 2024

Conversation

mohammed90
Copy link
Member

This was inadvertently missed during #5784. Re-adding Caddyfile support with a test of a dummy verifier to ensure it isn't broken in the future. I know LeafCertClientAuth is there, but it doesn't have Caddyfile support. This PR is faster to fix the gap until the LeafCertClientAuth module supports Caddyfile.

CC/ @Gr33nbl00d

@mohammed90 mohammed90 added this to the v2.8.0 milestone Feb 25, 2024
@mohammed90 mohammed90 marked this pull request as ready for review February 25, 2024 08:09
@mohammed90
Copy link
Member Author

The namespace of the verifiers isn't optimal. It's under tls.client_auth. It's better if it were under tls.client_auth.verifier, but unsure how much breakage that'd cause.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Feb 25, 2024
@francislavoie
Copy link
Member

I think it's just the one plugin which uses it, so I think it might be okay to change the namespace if @Gr33nbl00d agrees.

@Gr33nbl00d
Copy link
Contributor

Usually i am not a big fan of breaking changes for just better naming. But i agree if you think its better this way and woth it i will create a version 2 of my plugin which works for caddy 2.8.0+

@francislavoie
Copy link
Member

Unfortunately because of how Go works, a v2 would be a new module ID. I'd recommend just staying at v1 indefinitely to avoid that. Just bump the go.mod to require Caddy 2.8.0 as minimum.

@Gr33nbl00d
Copy link
Contributor

Unfortunately because of how Go works, a v2 would be a new module ID. I'd recommend just staying at v1 indefinitely to avoid that. Just bump the go.mod to require Caddy 2.8.0 as minimum.

Ah ok then i will do so thanks for the hint :)

@mohammed90
Copy link
Member Author

Unfortunately because of how Go works, a v2 would be a new module ID. I'd recommend just staying at v1 indefinitely to avoid that. Just bump the go.mod to require Caddy 2.8.0 as minimum.

Ah ok then i will do so thanks for the hint :)

Also you're not breaking your users. We're breaking your module. So the v2 isn't necessary anyways. As Francis said, just make a new tag that depends on Caddy v2.8.0.

I'll push the commit later.

@Gr33nbl00d
Copy link
Contributor

Unfortunately because of how Go works, a v2 would be a new module ID. I'd recommend just staying at v1 indefinitely to avoid that. Just bump the go.mod to require Caddy 2.8.0 as minimum.

Ah ok then i will do so thanks for the hint :)

Also you're not breaking your users. We're breaking your module. So the v2 isn't necessary anyways. As Francis said, just make a new tag that depends on Caddy v2.8.0.

I'll push the commit later.

Not completly true. If that compiler limitation wouldnt be there it makes sense. If you want to still be able to compile against older version of caddy it makes usually sense to create a new major version as this is not backward compatible change. If i would need to submit urgent hotfixes. Users will be forced to update to latest caddy otherwise. I usually would create a new v1 branch for maintenance purpose and continue v2 on main. So i can still release v1 hotfixes.

@mohammed90 mohammed90 merged commit 03f703a into master Feb 25, 2024
25 checks passed
@mohammed90 mohammed90 deleted the verifier-caddyfile branch February 25, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants