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

Update Dynu code and .ini file #483

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Update Dynu code and .ini file #483

merged 4 commits into from
Oct 22, 2024

Conversation

panosangel
Copy link
Contributor

@panosangel panosangel commented May 23, 2024

linuxserver.io


  • I have read the contributing guideline and understand that I have made the correct modifications

Description:

Running SWAG for the first time I got errors while setting it up to work with Dynu DNS challenge. The code for DNSPLUGIN flag was wrongly named and the .ini config file had the wrong name.

Benefits of this PR and context:

If it's not fixed, every single Dynu DNS user will have to make the same adjustments.

How Has This Been Tested?

I kept making adjustments to docker-compose.yml and the filesystem until there were no errors shown.

Source / References:

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for opening this pull request! Be sure to follow the pull request template!

@aptalca
Copy link
Member

aptalca commented May 23, 2024

dynu is a different plugin and it's abandoned

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.10.0-pkg-b255070a-dev-05b586d6df5df4fb3d648db62de9273bd3329190-pr-483/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.10.0-pkg-b255070a-dev-05b586d6df5df4fb3d648db62de9273bd3329190-pr-483/shellcheck-result.xml

Tag Passed
amd64-2.10.0-pkg-b255070a-dev-05b586d6df5df4fb3d648db62de9273bd3329190-pr-483
arm64v8-2.10.0-pkg-b255070a-dev-05b586d6df5df4fb3d648db62de9273bd3329190-pr-483

@panosangel
Copy link
Contributor Author

According to docker logs DNSPLUGIN=dynudns is not a recognized value and dynu is suggested. Could it be a validation issue?

swag  | [migrations] started
swag  | [migrations] 01-nginx-site-confs-default: executing...
swag  | [migrations] 01-nginx-site-confs-default: succeeded
swag  | [migrations] done
swag  | ───────────────────────────────────────
swag  | 
swag  |       ██╗     ███████╗██╗ ██████╗
swag  |       ██║     ██╔════╝██║██╔═══██╗
swag  |       ██║     ███████╗██║██║   ██║
swag  |       ██║     ╚════██║██║██║   ██║
swag  |       ███████╗███████║██║╚██████╔╝
swag  |       ╚══════╝╚══════╝╚═╝ ╚═════╝
swag  | 
swag  |    Brought to you by linuxserver.io
swag  | ───────────────────────────────────────
swag  | 
swag  | To support the app dev(s) visit:
swag  | Certbot: https://supporters.eff.org/donate/support-work-on-certbot
swag  | 
swag  | To support LSIO projects visit:
swag  | https://www.linuxserver.io/donate/
swag  | 
swag  | ───────────────────────────────────────
swag  | GID/UID
swag  | ───────────────────────────────────────
swag  | 
swag  | User UID:    1000
swag  | User GID:    1000
swag  | ───────────────────────────────────────
swag  | 
swag  | Setting resolver to  127.0.0.11
swag  | Setting worker_processes to 8
swag  | generating self-signed keys in /config/keys, you can replace these with your own keys if required
swag  | ....+...........+...+...+.......+..+.........+.+......+........+.+.....................+++++++++++++++++++++++++++++++++++++++*....+........+.............+..+.+..+...+....+.....+......+.+++++++++++++++++++++++++++++++++++++++*....+...++++++
swag  | ..+..+...+...+....+......+..+...+++++++++++++++++++++++++++++++++++++++*......+.......+.....+................+.....+.+......+...+++++++++++++++++++++++++++++++++++++++*.+..+...+......+.+...+...............+.....+......+...............................+..................+.....+.+......+.....+..........+...+............+...+.....+......+...++++++
swag  | -----
swag  | Variables set:
swag  | PUID=1000
swag  | PGID=1000
swag  | TZ=Europe/Athens
swag  | URL=domain.com
swag  | SUBDOMAINS=wildcard
swag  | EXTRA_DOMAINS=
swag  | ONLY_SUBDOMAINS=false
swag  | VALIDATION=dns
swag  | CERTPROVIDER=
swag  | DNSPLUGIN=dynudns
swag  | [email protected]
swag  | STAGING=true
swag  | 
swag  | Please set the DNSPLUGIN variable to one of the following:
swag  | acmedns
swag  | aliyun
swag  | azure
swag  | bunny
swag  | cloudflare
swag  | cpanel
swag  | desec
swag  | digitalocean
swag  | directadmin
swag  | dnsimple
swag  | dnsmadeeasy
swag  | dnspod
swag  | do
swag  | domeneshop
swag  | dreamhost
swag  | duckdns
swag  | dynu
swag  | freedns
swag  | gandi
swag  | gehirn
swag  | glesys
swag  | godaddy
swag  | google
swag  | google-domains
swag  | he
swag  | hetzner
swag  | infomaniak
swag  | inwx
swag  | ionos
swag  | linode
swag  | loopia
swag  | luadns
swag  | namecheap
swag  | netcup
swag  | njalla
swag  | nsone
swag  | ovh
swag  | porkbun
swag  | rfc2136
swag  | route53
swag  | sakuracloud
swag  | standalone
swag  | transip
swag  | vultr

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-c57ee935-dev-b81abc31dc9b1d3030cd67185a379639b99d65dc-pr-483/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-c57ee935-dev-b81abc31dc9b1d3030cd67185a379639b99d65dc-pr-483/shellcheck-result.xml

Tag Passed
amd64-2.11.0-pkg-c57ee935-dev-b81abc31dc9b1d3030cd67185a379639b99d65dc-pr-483
arm64v8-2.11.0-pkg-c57ee935-dev-b81abc31dc9b1d3030cd67185a379639b99d65dc-pr-483

@lucas-cornette
Copy link

I'm also using dynudns and can confirm these changes have to be made to be able to use the dynu plugin.
I have renamed the ./config/dns-conf/dynu-credentials.ini file to dynu.ini and changed the DNSPLUGIN variable in my dockerfile from dynudns to dynu. After these changes, certbot is successfully generating certs for my domain.

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-b36b718b-dev-35aeb158a0afb43b50c2347138bdcaf9017659bd-pr-483/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-b36b718b-dev-35aeb158a0afb43b50c2347138bdcaf9017659bd-pr-483/shellcheck-result.xml

Tag Passed
amd64-2.11.0-pkg-b36b718b-dev-35aeb158a0afb43b50c2347138bdcaf9017659bd-pr-483
arm64v8-2.11.0-pkg-b36b718b-dev-35aeb158a0afb43b50c2347138bdcaf9017659bd-pr-483

@LinuxServer-CI
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. This might be due to missing feedback from OP. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link
Member

@nemchik nemchik left a comment

Choose a reason for hiding this comment

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

Noting that https://pypi.org/project/certbot-dns-dynudns/ and https://github.com/DustyRah/certbot-dns-dynudns/ do not mention the new config file name, but I'm guessing the old plugin that was deprecated in #439 was due to not being updated to newer code standards in certbot, and the new plugin introduced in #463 may have been a fork of the original that updated the code but maybe did not update the readme. I'm not positive, but the rename makes sense given how other current plugins work with the config naming.

README.md Outdated Show resolved Hide resolved
@aptalca
Copy link
Member

aptalca commented Oct 21, 2024

This change is not desired. There are two separate dynu plugins. One is old and abandoned (dynu), and the other is new and maintained by the dynu employees (dynudns).

The old and abandoned plugin force downgrades a bunch of the certbot dependencies because it has old deps hardcoded. We do not want that because it affects everyone, not just dynu users.

It seems the real issue here is, when the dynu employees forked the abandoned plugin and published it under a different name, they only changed the name externally (package name). Internally it still uses the old references.

This requires putting in an exception to use the new plugin package but with the old internal name. I'm short on time these days and have no ability to test and trouble shoot so it will have to come from someone else.

EDIT:

I take it back. While most of what I wrote above is true, I took another look at the full log and now see that it still has the new plugin installed, but it references the old plugin's name. I assumed it would force install the old plugin on startup but doesn't. So while not a proper solution, I guess it would work.

@nemchik
Copy link
Member

nemchik commented Oct 21, 2024

It seems the real issue here is, when the dynu employees forked the abandoned plugin and published it under a different name, they only changed the name externally (package name). Internally it still uses the old references.

This requires putting in an exception to use the new plugin package but with the old internal name. I'm short on time these days and have no ability to test and trouble shoot so it will have to come from someone else.

EDIT:

I take it back. While most of what I wrote above is true, I took another look at the full log and now see that it still has the new plugin installed, but it references the old plugin's name. I assumed it would force install the old plugin on startup but doesn't. So while not a proper solution, I guess it would work.

Yeah it looks like we removed the old plugin in #439 and added the new plugin in #463 but there was some confusion on our part.

4b4c103 added root/defaults/dns-conf/dynu-credentials.ini which should have been root/defaults/dns-conf/dynudns.ini because of how our init handles the credentials file here

DNSCREDENTIALFILE="/config/dns-conf/${DNSPLUGIN}.ini"
and here
set_ini_value "dns-${DNSPLUGIN}-credentials" "${DNSCREDENTIALFILE}" /config/etc/letsencrypt/cli.ini

Apparently even if we had done the above correctly, the new dynudns plugin's entrypoint is still dns-dynu as shown here https://github.com/DustyRah/certbot-dns-dynudns/blob/da9df1b91f4615c8e83a7213a701dd4b87e3505c/setup.py#L69 and our init uses that entrypoint for the authenticator in certbot's cli.ini here

set_ini_value "authenticator" "dns-${DNSPLUGIN}" /config/etc/letsencrypt/cli.ini
so despite the package name being dynudns the correct naming we need here is just dynu.

Copy link
Member

@nemchik nemchik left a comment

Choose a reason for hiding this comment

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

Merge conflicts need to be resolved.

The changes in README.md can be completely reverted (no need to fix the merge conflict in this file, if you just revert it entirely our build system will update README.md from readme-vars.yml).

readme-vars.yml needs a changelog entry.

@panosangel
Copy link
Contributor Author

To be honest I'm kind of confused which approach we'll take now so I can make the appropriate adjustments.

@nemchik
Copy link
Member

nemchik commented Oct 21, 2024

To be honest I'm kind of confused which approach we'll take now so I can make the appropriate adjustments.

I pushed some adjustments for you to resolve the merge conflict and add a changelog entry.

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-d68acf16-dev-8decebad67eedcb5d5b27d7a00ecf8e96a458473-pr-483/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-d68acf16-dev-8decebad67eedcb5d5b27d7a00ecf8e96a458473-pr-483/shellcheck-result.xml

Tag Passed
amd64-2.11.0-pkg-d68acf16-dev-8decebad67eedcb5d5b27d7a00ecf8e96a458473-pr-483
arm64v8-2.11.0-pkg-d68acf16-dev-8decebad67eedcb5d5b27d7a00ecf8e96a458473-pr-483

@nemchik nemchik merged commit 08e91b3 into linuxserver:master Oct 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants