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

Utilise FTL's native config upgrade functionality #1683

Merged
merged 6 commits into from
Feb 18, 2025

Conversation

PromoFaux
Copy link
Member

What does this PR aim to accomplish?:

Taking inspiration from pi-hole/pi-hole#5830 - we use the new functionality within FTL to ensure V5 configs are properly migrated to V6.

Also taken a couple of tricks to improve the output of the docker container.

Example output from a new v6 container with 5 volumes:

pihole  |   [i] v5 files detected that have not yet been migrated to v6
pihole  | 
pihole  |   [i] Migrating dnsmasq configuration files
pihole  | 
pihole  |   [i] Migrating config to Pi-hole v6.0 format
pihole  |       Reading legacy config files from /etc/pihole/pihole-FTL.conf
pihole  |          PIDFILE: Empty path is not possible, using default
pihole  |          SETUPVARSFILE: Empty path is not possible, using default
pihole  |          GRAVITYDB: Empty path is not possible, using default
pihole  |          WEBROOT: Empty path is not possible, using default
pihole  |          WEBHOME: Empty path is not possible, using default
pihole  |          API_INFO_LOG: Empty path is not possible, using default
pihole  |          WEBDOMAIN: Empty path is not possible, using default
pihole  |       Moving /etc/pihole/pihole-FTL.conf to /etc/pihole/migration_backup_v6/pihole-FTL.conf
pihole  |       Migrating config from /etc/pihole/setupVars.conf
pihole  |       setupVars.conf:WEBPASSWORD -> Setting webserver.api.pwhash to 06eae68f1ca3493598cb993d99a9e64179d6e740627f8426a2bfa9ed9a26733f
pihole  |       setupVars.conf:BLOCKING_ENABLED -> Setting dns.blocking.active to true
pihole  |       setupVars.conf:TEMPERATURE_LIMIT -> Not set
pihole  |       setupVars.conf:TEMPERATURE_UNIT -> Not set
pihole  |       setupVars.conf:WEBUIBOXEDLAYOUT -> Not set
pihole  |       setupVars.conf:WEBTHEME -> Not set
pihole  |       setupVars.conf:PIHOLE_DNS_1 -> Setting dns.upstreams[1] = 8.8.8.8
pihole  |       setupVars.conf:PIHOLE_DNS_2 -> Setting dns.upstreams[2] = 8.8.4.4
pihole  |       setupVars.conf:PIHOLE_DOMAIN -> Not set
pihole  |       setupVars.conf:DNS_FQDN_REQUIRED -> Not set
pihole  |       setupVars.conf:DNS_FQDN_REQUIRED -> Not set
pihole  |       setupVars.conf:DNS_bogusPriv -> Not set
pihole  |       setupVars.conf:DNSSEC -> Not set
pihole  |       setupVars.conf:PIHOLE_INTERFACE -> Setting dns.interface to eth0
pihole  |       setupVars.conf:HOSTRECORD -> Not set
pihole  |       setupVars.conf:DNSMASQ_LISTENING -> Not set
pihole  |       setupVars.conf:REV_SERVER -> Not set
pihole  |       setupVars.conf:DHCP_ACTIVE -> Not set
pihole  |       setupVars.conf:DHCP_START -> Not set
pihole  |       setupVars.conf:DHCP_END -> Not set
pihole  |       setupVars.conf:DHCP_ROUTER -> Not set
pihole  |       setupVars.conf:DHCP_LEASETIME -> Not set
pihole  |       setupVars.conf:DHCP_IPv6 -> Not set
pihole  |       setupVars.conf:DHCP_RAPID_COMMIT -> Not set
pihole  |       setupVars.conf:queryLogging -> Not set
pihole  |       setupVars.conf:GRAVITY_TMPDIR -> Not set
pihole  |       setupVars.conf:WEB_PORTS -> Not set
pihole  |       Moved /etc/pihole/setupVars.conf to /etc/pihole/migration_backup_v6/setupVars.conf
pihole  |       setupVars.conf migration complete
pihole  |       Moving /etc/pihole/custom.list to /etc/pihole/migration_backup_v6/custom.list
pihole  |       Config initialized with webserver ports 80 (HTTP) and 443 (HTTPS), IPv6 support is enabled
pihole  |       Wrote config file:
pihole  |        - 151 total entries
pihole  |        - 145 entries are default
pihole  |        - 6 entries are modified
pihole  |        
pihole  |       Config file written to /etc/pihole/pihole.toml
pihole  | 
pihole  |   [i] Setting up user & group for the pihole user
pihole  |   [i] PIHOLE_UID not set in environment, using default (100)
pihole  |   [i] PIHOLE_GID not set in environment, using default (101)
pihole  | 
pihole  |   [i] Starting FTL configuration
pihole  |   [i] Assigning password defined by Environment Variable
pihole  |   [i] Starting crond for scheduled scripts. Randomizing times for gravity and update checker
pihole  | 
pihole  |   [i] Ensuring logrotate script exists in /etc/pihole
pihole  | 
pihole  |   [i] Gravity migration checks
pihole  |   [i] No adlist file found, creating one with a default blocklist
pihole  |   [i] Existing gravity database found - schema will be upgraded if necessary
pihole  |         Upgrading gravity database from version 15 to 16
pihole  |         Upgrading gravity database from version 16 to 17
pihole  |         Upgrading gravity database from version 17 to 18
pihole  |         Upgrading gravity database from version 18 to 19
pihole  | 
pihole  |   [i] pihole-FTL pre-start checks
pihole  |   [i] Setting capabilities on pihole-FTL where possible
pihole  |   [i] Applying the following caps to pihole-FTL:
pihole  |         * CAP_CHOWN
pihole  |         * CAP_NET_BIND_SERVICE
pihole  |         * CAP_NET_RAW
pihole  |         * CAP_NET_ADMIN
pihole  | 
pihole  |   [i] Starting pihole-FTL (no-daemon) as pihole
pihole  | 
pihole  |   [i] Version info:
pihole  |       Core
pihole  |           Version is efaa0f4 (Latest: null)
pihole  |           Branch is development
pihole  |           Hash is efaa0f42 (Latest: efaa0f42)
pihole  |       Web
pihole  |           Version is 603f35d (Latest: null)
pihole  |           Branch is development
pihole  |           Hash is 603f35d5 (Latest: 603f35d5)
pihole  |       FTL
pihole  |           Version is vDev-d01a265 (Latest: null)
pihole  |           Branch is development
pihole  |           Hash is d01a2652 (Latest: d01a2652)
pihole  | 
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: ########## FTL started on c5127ed948b8! ##########
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: FTL branch: development
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: FTL version: vDev-d01a265
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: FTL commit: d01a2652
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: FTL date: 2025-01-14 18:59:14 +0100
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: FTL user: pihole
pihole  | 2025-01-14 18:17:57.592 GMT [67M] INFO: Compiled for linux/arm64/v8 (compiled on CI) using cc (Alpine 14.2.0) 14.2.0
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO: 2 FTLCONF environment variables found (2 used, 0 invalid, 0 ignored)
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO:    [✓] FTLCONF_webserver_api_password is used
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO:    [✓] FTLCONF_dns_upstreams is used
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO: Wrote config file:
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO:  - 151 total entries
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO:  - 145 entries are default
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO:  - 6 entries are modified
pihole  | 2025-01-14 18:17:57.683 GMT [67M] INFO:  - 1 entry is forced through environment
pihole  | 2025-01-14 18:17:57.686 GMT [67M] INFO: Parsed config file /etc/pihole/pihole.toml successfully
pihole  | 2025-01-14 18:17:57.686 GMT [67M] WARNING: Insufficient permissions to set process priority to -10 (CAP_SYS_NICE required), process priority remains at 0

Subsequent runs will not display the output messages as the configs will have already been migrated.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

@PromoFaux PromoFaux requested review from DL6ER and a team January 14, 2025 18:24
rdwebdesign
rdwebdesign previously approved these changes Jan 30, 2025
Copy link
Member

@rdwebdesign rdwebdesign left a comment

Choose a reason for hiding this comment

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

Approving base on successful tests.

Please check the MM message about "Operation not permitted" errors.

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

I tested this PR and noted two things.


We need to make sure, everyone still has mounted /etc/dnsmasq.d mapped into the container at the first start for a successful migration. Currently or example compose file says, its not needed for most users.

https://github.com/pi-hole/docker-pi-hole/tree/development?tab=readme-ov-file#quick-start


Second, the assigned UID&GID

pihole  |   [i] PIHOLE_UID not set in environment, using default (100)
pihole  |   [i] PIHOLE_GID not set in environment, using default (101)

Normal user UID and GID usually start > 1000, but as the container runs as root they will be assigned 100 and 101, respectively.
Is this by design? Or should we change it?

addgroup -S pihole && adduser -S pihole -G pihole && \

@DL6ER
Copy link
Member

DL6ER commented Feb 6, 2025

@yubiuser @PromoFaux What happens if they do not have dnsmasq.d mounted externally? Will it be empty because the v5 scripts generating the content of this directory is gone?

@yubiuser
Copy link
Member

yubiuser commented Feb 6, 2025

Those configs are not migrated, otherwise it's fine

# Move all conf files originally created by Pi-hole into this directory
# - 01-pihole.conf
# - 02-pihole-dhcp.conf
# - 04-pihole-static-dhcp.conf
# - 05-pihole-custom-cname.conf
# - 06-rfc6761.conf


My comment was aiming on proper warning for the release notes or a note to the Readme

@rdwebdesign
Copy link
Member

What happens if they do not have dnsmasq.d mounted externally?

If /etc/dnsmasq.d is not stored in a volume or bind mount its contents will be lost when the container is destroyed/recreated.
This is expected and it would happen in a v6 migration or in a normal upgrade to a new v5 image.

If a new container uses the same volumes used by the previous v5 container, the migration process will read the files and migrate them.

We need to make sure, everyone still has mounted /etc/dnsmasq.d mapped into the container at the first start for a successful migration.

This is only needed when the user has custom files in this directory. Most users don't change these files.
Users with custom dnsmasq files will know they need the mount, otherwise how Pi-hole would know they exist.

@DL6ER
Copy link
Member

DL6ER commented Feb 7, 2025

Okay, so it is really only about the comment on the README. They will also need it when they have static DHCP leases or CNAMEs. I don't think they have been stored elsewhere on v5. In v6, everything is in pihole.toml

@PromoFaux
Copy link
Member Author

Is this by design? Or should we change it?

No, not by design - I guess it's just whatever the alpine image assigns when the user is created. We do have environment variables to set the u/gid's to whatever the user wishes. Maybe we just to make it clearer in the documentation that they can change it to match any ids on the host system if needed?

@yubiuser
Copy link
Member

I amended the Readme and rebased the branch

PromoFaux and others added 5 commits February 10, 2025 17:12
… to defer the startup of FTL as we were previously doing.

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
…fit with the container output. Supress message in FTL migration output about environment variables, as these are not read in until FTLs first proper start

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: yubiuser <github@yubiuser.dev>
Co-authored-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: yubiuser <github@yubiuser.dev>
@yubiuser
Copy link
Member

No, not by design - I guess it's just whatever the alpine image assigns when the user is created. We do have environment variables to set the u/gid's to whatever the user wishes. Maybe we just to make it clearer in the documentation that they can change it to match any ids on the host system if needed?

#1690

@PromoFaux PromoFaux merged commit b6f909f into development Feb 18, 2025
7 checks passed
@PromoFaux PromoFaux deleted the explicit-migrate branch February 18, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants