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

Fix bug introduced in PR#3891, auto_https should never be set to on #3930

Closed

Conversation

sdellysse
Copy link

upstream docs: https://caddyserver.com/docs/caddyfile/options#auto-https

For some reason, the caddy directive auto_https cannot take a value of on, but instead just shouldn't be called.

Bug was introduced here: https://github.com/opnsense/plugins/pull/3891/files?w=1#diff-c905c5709a75510121b03ead89789a33e45e9514b7e7facd591d8bcdf503263bL215-R277

The auto_https directive is quite confusing, so I also added a comment explaining why this fix should remain, so that it's not accidentally undone in the future.

upstream docs: https://caddyserver.com/docs/caddyfile/options#auto-https

For some reason, the caddy directive `auto_https` cannot take a value of `on`, but instead just shouldn't be called.
@Monviech
Copy link
Sponsor Member

Monviech commented Apr 21, 2024

This doesn't need to be fixed, it was an accidental regression caused by having an explicit "on" stored in the configuration in versions before 1.5.3, and having replaced it with an empty value in 1.5.4 using a blanc_desc (check the pull request of 1.5.4 that explains it.)

The oversight was that it would have needed a migration script so that it removes "on" from the stored configuration.

a628ebf

@sdellysse
Copy link
Author

Gotcha, I didn't realize that, so I went to Service->Caddy->General, toggled the value from On to Off to On again, and now everything works. Thanks!

(Putting my specific solution above in case someone googles this and ends up here)

@sdellysse sdellysse closed this Apr 21, 2024
@Monviech
Copy link
Sponsor Member

Thank you for the effort though. I'm making sure the next time I change something like this, I offer a migration script again. (Like the time I change descriptions around)

https://github.com/opnsense/plugins/blob/master/www/caddy/src/opnsense/mvc/app/models/OPNsense/Caddy/Migrations/M1_1_3.php

Monviech added a commit to Monviech/opnsense-plugins that referenced this pull request Apr 21, 2024
This migration script empties out "on" and "none" options that were replaced with empty values by this pull request. It fixes the regression that Caddy will refuse to start because invalid values for AutoHttps and DnsProvider will remain stored in the config without a manual config change from the user.

Compare to: opnsense@a628ebf

Issue was tracked multiple times:
https://forum.opnsense.org/index.php?topic=40075.0
opnsense#3930
opnsense#3917 (comment)
@Monviech
Copy link
Sponsor Member

I have created a migration script. It will fix that issue for users in the future when they upgrade.

@sdellysse sdellysse deleted the sdellysse/caddy-auto_https-fix branch April 22, 2024 04:53
fichtner pushed a commit that referenced this pull request Apr 22, 2024
…ing able to start. (Regression in 1.5.4) (#3931)

* Create M1_1_7.php

This migration script empties out "on" and "none" options that were replaced with empty values by this pull request. It fixes the regression that Caddy will refuse to start because invalid values for AutoHttps and DnsProvider will remain stored in the config without a manual config change from the user.

Compare to: a628ebf

Issue was tracked multiple times:
https://forum.opnsense.org/index.php?topic=40075.0
#3930
#3917 (comment)

* Bump Migration script and Model version to 1.1.8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants