Pull request: 3933 client sort

Merge in DNS/adguard-home from 3933-client-sort to master

* commit '6a6f926bd04fb05232dab628f5fc877d9307f175':
  all: imp hooks, opt
  home: imp docs, opt
  home: imp client handling
  all: doc changes, imp names
  Prevent spurious diffs in config file by sorting Client objects before writing

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
 
 
 

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) {
+// addFromConfig initializes the clients containter with objects from the
-	return clients.allTags.Has(tag)
+// configuration file.
-}
+func (clients *clientsContainer) addFromConfig(objects []*clientObject) {
-
+	for _, o := range objects {
-func (clients *clientsContainer) addFromConfig(objects []clientObject) {
-	for _, cy := range objects {
 		cli := &Client{
-			Name:                cy.Name,
+			Name: o.Name,
-			IDs:                 cy.IDs,
-			UseOwnSettings:      !cy.UseGlobalSettings,
-			FilteringEnabled:    cy.FilteringEnabled,
-			ParentalEnabled:     cy.ParentalEnabled,
-			SafeSearchEnabled:   cy.SafeSearchEnabled,
-			SafeBrowsingEnabled: cy.SafeBrowsingEnabled,
 
-			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 {
+		for _, s := range o.BlockedServices {
-			if !filtering.BlockedSvcKnown(s) {
+			if filtering.BlockedSvcKnown(s) {
-				log.Debug("clients: skipping unknown blocked-service %q", s)
+				cli.BlockedServices = append(cli.BlockedServices, s)
-				continue
+			} else {
+				log.Info("clients: skipping unknown blocked service %q", s)
 			}
-			cli.BlockedServices = append(cli.BlockedServices, s)
 		}
 
-		for _, t := range cy.Tags {
+		for _, t := range o.Tags {
-			if !clients.tagKnown(t) {
+			if clients.allTags.Has(t) {
-				log.Debug("clients: skipping unknown tag %q", t)
+				cli.Tags = append(cli.Tags, t)
-				continue
+			} 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
+// forConfig returns all currently known persistent clients as objects for the
-func (clients *clientsContainer) WriteDiskConfig(objects *[]clientObject) {
+// 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{
+		o := &clientObject{
-			Name:                     cli.Name,
+			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)
+		objs = append(objs, o)
-		cy.IDs = stringutil.CloneSlice(cli.IDs)
-		cy.BlockedServices = stringutil.CloneSlice(cli.BlockedServices)
-		cy.Upstreams = stringutil.CloneSlice(cli.Upstreams)
-
-		*objects = append(*objects, 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(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)
 		}
 	}

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 contains the YAML representations of the persistent clients.
-	Clients []clientObject `yaml:"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)
 

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 {

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
+# Only show interactive prompts if there is a terminal attached.  This
-# if there are any, because the commit could be in a temporary branch.
+# should work on all of our supported Unix systems.
-git grep -e 'TODO.*!!' -- ':!scripts/hooks/pre-commit' | cat || :
+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