From 774937728b13aba0a1c7e7b8acaea9edb362926a Mon Sep 17 00:00:00 2001 From: Keith Collister Date: Sun, 12 Dec 2021 11:46:13 +0100 Subject: [PATCH 1/5] Prevent spurious diffs in config file by sorting Client objects before writing --- internal/home/clients.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/home/clients.go b/internal/home/clients.go index 02c87f03..09472fbd 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -222,6 +222,9 @@ func (clients *clientsContainer) addFromConfig(objects []clientObject) { // WriteDiskConfig - write configuration func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) { clients.lock.Lock() + defer clients.lock.Unlock() + + clientObjects := []clientObject{} for _, cli := range clients.list { cy := clientObject{ Name: cli.Name, @@ -238,9 +241,17 @@ func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) { cy.BlockedServices = stringutil.CloneSlice(cli.BlockedServices) cy.Upstreams = stringutil.CloneSlice(cli.Upstreams) - *objects = append(*objects, cy) + clientObjects = append(clientObjects, cy) } - 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(clientObjects, func(i, j int) bool { + return clientObjects[i].Name < clientObjects[j].Name + }) + *objects = append(*objects, clientObjects...) } func (clients *clientsContainer) periodicUpdate() { From 0e1015a4ee7f4101ef21cb2b2688beb50a11dc83 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 13 Dec 2021 14:56:48 +0300 Subject: [PATCH 2/5] all: doc changes, imp names --- CHANGELOG.md | 2 ++ internal/home/clients.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 044b9688..b0a6d96d 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]). @@ -229,6 +230,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 09472fbd..78cc7101 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -224,9 +224,9 @@ func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) { clients.lock.Lock() defer clients.lock.Unlock() - clientObjects := []clientObject{} + var clientObjects []clientObject for _, cli := range clients.list { - cy := clientObject{ + cliObj := clientObject{ Name: cli.Name, UseGlobalSettings: !cli.UseOwnSettings, FilteringEnabled: cli.FilteringEnabled, @@ -236,18 +236,18 @@ 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) + cliObj.Tags = stringutil.CloneSlice(cli.Tags) + cliObj.IDs = stringutil.CloneSlice(cli.IDs) + cliObj.BlockedServices = stringutil.CloneSlice(cli.BlockedServices) + cliObj.Upstreams = stringutil.CloneSlice(cli.Upstreams) - clientObjects = append(clientObjects, cy) + clientObjects = append(clientObjects, cliObj) } // 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. + // 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(clientObjects, func(i, j int) bool { return clientObjects[i].Name < clientObjects[j].Name }) From 39da2f9eaa7ba858b7857da34a6ba498b9c9c46a Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 13 Dec 2021 15:18:21 +0300 Subject: [PATCH 3/5] home: imp client handling --- internal/home/clients.go | 85 ++++++++++++++++++++-------------------- internal/home/config.go | 12 +++--- internal/home/home.go | 2 +- 3 files changed, 50 insertions(+), 49 deletions(-) diff --git a/internal/home/clients.go b/internal/home/clients.go index 78cc7101..ba5b5161 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,59 +175,64 @@ 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() - var clientObjects []clientObject for _, cli := range clients.list { - cliObj := 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, @@ -236,22 +241,16 @@ func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) { UseGlobalBlockedServices: !cli.UseOwnBlockedServices, } - cliObj.Tags = stringutil.CloneSlice(cli.Tags) - cliObj.IDs = stringutil.CloneSlice(cli.IDs) - cliObj.BlockedServices = stringutil.CloneSlice(cli.BlockedServices) - cliObj.Upstreams = stringutil.CloneSlice(cli.Upstreams) - - clientObjects = append(clientObjects, cliObj) + objs = append(objs, o) } // 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(clientObjects, func(i, j int) bool { - return clientObjects[i].Name < clientObjects[j].Name - }) - *objects = append(*objects, clientObjects...) + sort.Slice(objs, func(i, j int) bool { return objs[i].Name < objs[j].Name }) + + return objs } func (clients *clientsContainer) periodicUpdate() { @@ -537,7 +536,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 3465c26f..368a4b51 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -84,8 +84,11 @@ 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"` + // Note: this array is filled only before file read/write and then it's + // cleared. + // + // TODO(a.garipov): Do something with this. + Clients []*clientObject `yaml:"clients"` logSettings `yaml:",inline"` @@ -316,8 +319,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 +366,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 { From 0d6f4f013902bbaea130335fbf5080575a958dc6 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 13 Dec 2021 15:24:35 +0300 Subject: [PATCH 4/5] home: imp docs, opt --- internal/home/config.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/home/config.go b/internal/home/config.go index 368a4b51..a6be26d9 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -84,10 +84,9 @@ type configuration struct { DHCP dhcpd.ServerConfig `yaml:"dhcp"` - // Note: this array is filled only before file read/write and then it's - // cleared. - // - // TODO(a.garipov): Do something with this. + // 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"` From 6a6f926bd04fb05232dab628f5fc877d9307f175 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Mon, 13 Dec 2021 15:28:12 +0300 Subject: [PATCH 5/5] all: imp hooks, opt --- internal/home/clients.go | 1 + scripts/hooks/pre-commit | 69 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/internal/home/clients.go b/internal/home/clients.go index ba5b5161..7a478679 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -224,6 +224,7 @@ 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 { o := &clientObject{ Name: cli.Name, 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