diff --git a/CHANGELOG.md b/CHANGELOG.md index e27fab44..6e0a0ae7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ and this project adheres to ### Changed +- Client objects in the configuration file are now sorted ([#3933]). - Responses from cache are now labeled ([#3772]). - Better error message for ED25519 private keys, which are not widely supported ([#3737]). @@ -232,6 +233,7 @@ In this release, the schema version has changed from 10 to 12. [#3772]: https://github.com/AdguardTeam/AdGuardHome/issues/3772 [#3815]: https://github.com/AdguardTeam/AdGuardHome/issues/3815 [#3890]: https://github.com/AdguardTeam/AdGuardHome/issues/3890 +[#3933]: https://github.com/AdguardTeam/AdGuardHome/pull/3933 diff --git a/internal/home/clients.go b/internal/home/clients.go index 02c87f03..7a478679 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -106,7 +106,7 @@ type clientsContainer struct { // dhcpServer: optional // Note: this function must be called only once func (clients *clientsContainer) Init( - objects []clientObject, + objects []*clientObject, dhcpServer *dhcpd.Server, etcHosts *aghnet.HostsContainer, ) { @@ -175,56 +175,65 @@ type clientObject struct { UseGlobalBlockedServices bool `yaml:"use_global_blocked_services"` } -func (clients *clientsContainer) tagKnown(tag string) (ok bool) { - return clients.allTags.Has(tag) -} - -func (clients *clientsContainer) addFromConfig(objects []clientObject) { - for _, cy := range objects { +// addFromConfig initializes the clients containter with objects from the +// configuration file. +func (clients *clientsContainer) addFromConfig(objects []*clientObject) { + for _, o := range objects { cli := &Client{ - Name: cy.Name, - IDs: cy.IDs, - UseOwnSettings: !cy.UseGlobalSettings, - FilteringEnabled: cy.FilteringEnabled, - ParentalEnabled: cy.ParentalEnabled, - SafeSearchEnabled: cy.SafeSearchEnabled, - SafeBrowsingEnabled: cy.SafeBrowsingEnabled, + Name: o.Name, - UseOwnBlockedServices: !cy.UseGlobalBlockedServices, + IDs: o.IDs, + Upstreams: o.Upstreams, - Upstreams: cy.Upstreams, + UseOwnSettings: !o.UseGlobalSettings, + FilteringEnabled: o.FilteringEnabled, + ParentalEnabled: o.ParentalEnabled, + SafeSearchEnabled: o.SafeSearchEnabled, + SafeBrowsingEnabled: o.SafeBrowsingEnabled, + UseOwnBlockedServices: !o.UseGlobalBlockedServices, } - for _, s := range cy.BlockedServices { - if !filtering.BlockedSvcKnown(s) { - log.Debug("clients: skipping unknown blocked-service %q", s) - continue + for _, s := range o.BlockedServices { + if filtering.BlockedSvcKnown(s) { + cli.BlockedServices = append(cli.BlockedServices, s) + } else { + log.Info("clients: skipping unknown blocked service %q", s) } - cli.BlockedServices = append(cli.BlockedServices, s) } - for _, t := range cy.Tags { - if !clients.tagKnown(t) { - log.Debug("clients: skipping unknown tag %q", t) - continue + for _, t := range o.Tags { + if clients.allTags.Has(t) { + cli.Tags = append(cli.Tags, t) + } else { + log.Info("clients: skipping unknown tag %q", t) } - cli.Tags = append(cli.Tags, t) } + sort.Strings(cli.Tags) _, err := clients.Add(cli) if err != nil { - log.Tracef("clientAdd: %s", err) + log.Error("clients: adding clients %s: %s", cli.Name, err) } } } -// WriteDiskConfig - write configuration -func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) { +// forConfig returns all currently known persistent clients as objects for the +// configuration file. +func (clients *clientsContainer) forConfig() (objs []*clientObject) { clients.lock.Lock() + defer clients.lock.Unlock() + + objs = make([]*clientObject, 0, len(clients.list)) for _, cli := range clients.list { - cy := clientObject{ - Name: cli.Name, + o := &clientObject{ + Name: cli.Name, + + Tags: stringutil.CloneSlice(cli.Tags), + IDs: stringutil.CloneSlice(cli.IDs), + BlockedServices: stringutil.CloneSlice(cli.BlockedServices), + Upstreams: stringutil.CloneSlice(cli.Upstreams), + UseGlobalSettings: !cli.UseOwnSettings, FilteringEnabled: cli.FilteringEnabled, ParentalEnabled: cli.ParentalEnabled, @@ -233,14 +242,16 @@ func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) { UseGlobalBlockedServices: !cli.UseOwnBlockedServices, } - cy.Tags = stringutil.CloneSlice(cli.Tags) - cy.IDs = stringutil.CloneSlice(cli.IDs) - cy.BlockedServices = stringutil.CloneSlice(cli.BlockedServices) - cy.Upstreams = stringutil.CloneSlice(cli.Upstreams) - - *objects = append(*objects, cy) + objs = append(objs, o) } - clients.lock.Unlock() + + // Maps aren't guaranteed to iterate in the same order each time, so the + // above loop can generate different orderings when writing to the config + // file: this produces lots of diffs in config files, so sort objects by + // name before writing. + sort.Slice(objs, func(i, j int) bool { return objs[i].Name < objs[j].Name }) + + return objs } func (clients *clientsContainer) periodicUpdate() { @@ -526,7 +537,7 @@ func (clients *clientsContainer) check(c *Client) (err error) { } for _, t := range c.Tags { - if !clients.tagKnown(t) { + if !clients.allTags.Has(t) { return fmt.Errorf("invalid tag: %q", t) } } diff --git a/internal/home/config.go b/internal/home/config.go index 5d329fe2..99463a6a 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -84,8 +84,10 @@ type configuration struct { DHCP dhcpd.ServerConfig `yaml:"dhcp"` - // Note: this array is filled only before file read/write and then it's cleared - Clients []clientObject `yaml:"clients"` + // Clients contains the YAML representations of the persistent clients. + // This field is only used for reading and writing persistent client data. + // Keep this field sorted to ensure consistent ordering. + Clients []*clientObject `yaml:"clients"` logSettings `yaml:",inline"` @@ -316,8 +318,6 @@ func (c *configuration) write() error { c.Lock() defer c.Unlock() - Context.clients.WriteDiskConfig(&config.Clients) - if Context.auth != nil { config.Users = Context.auth.GetUsers() } @@ -365,10 +365,11 @@ func (c *configuration) write() error { config.DHCP = c } + config.Clients = Context.clients.forConfig() + configFile := config.getConfigFilename() log.Debug("Writing YAML file: %s", configFile) yamlText, err := yaml.Marshal(&config) - config.Clients = nil if err != nil { log.Error("Couldn't generate YAML file: %s", err) diff --git a/internal/home/home.go b/internal/home/home.go index ec103b1f..1238aaec 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -294,8 +294,8 @@ func setupConfig(args options) (err error) { return err } } + Context.clients.Init(config.Clients, Context.dhcpServer, Context.etcHosts) - config.Clients = nil // override bind host/port from the console if args.bindHost != nil { diff --git a/scripts/hooks/pre-commit b/scripts/hooks/pre-commit index b52735b1..9866e3ed 100755 --- a/scripts/hooks/pre-commit +++ b/scripts/hooks/pre-commit @@ -2,9 +2,72 @@ set -e -f -u -# Show all temporary todos to the programmer but don't fail the commit -# if there are any, because the commit could be in a temporary branch. -git grep -e 'TODO.*!!' -- ':!scripts/hooks/pre-commit' | cat || : +# Only show interactive prompts if there is a terminal attached. This +# should work on all of our supported Unix systems. +is_tty='0' +if [ -e /dev/tty ] +then + is_tty='1' +fi +readonly is_tty + +# prompt is a helper that prompts the user for interactive input if that +# can be done. If there is no terminal attached, it sleeps for two +# seconds, giving the programmer some time to react, and returns with +# a zero exit code. +prompt() { + if [ "$is_tty" -eq '0' ] + then + sleep 2 + + return 0 + fi + + while true + do + printf 'commit anyway? y/[n]: ' + read -r ans < /dev/tty + + case "$ans" + in + ('y'|'Y') + break + ;; + (''|'n'|'N') + exit 1 + ;; + (*) + continue + ;; + esac + done +} + +# Warn the programmer about unstaged changes and untracked files, but do +# not fail the commit, because those changes might be temporary or for +# a different branch. +awk_prog='substr($2, 2, 1) != "." { print $9; } $1 == "?" { print $2; }' +readonly awk_prog + +unstaged="$( git status --porcelain=2 | awk "$awk_prog" )" +readonly unstaged + +if [ "$unstaged" != "" ] +then + printf 'WARNING: you have unstaged changes:\n\n%s\n\n' "$unstaged" + prompt +fi + +# Warn the programmer about temporary todos, but do not fail the commit, +# because the commit could be in a temporary branch. +temp_todos="$( git grep -e 'TODO.*!!' -- ':!scripts/hooks/pre-commit' || : )" +readonly temp_todos + +if [ "$temp_todos" != "" ] +then + printf 'WARNING: you have temporary todos:\n\n%s\n\n' "$temp_todos" + prompt +fi verbose="${VERBOSE:-0}" readonly verbose