From e1d0c523400186e2eb142180707405ddd4e59fa2 Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Mon, 27 Feb 2023 13:20:53 +0300 Subject: [PATCH] all: imp code; fix notes --- CHANGELOG.md | 8 +++-- internal/aghnet/hostgen.go | 8 ++--- internal/home/config.go | 33 +++++++++--------- internal/querylog/http.go | 38 ++++++++++----------- internal/querylog/querylog.go | 7 ++-- internal/stats/http.go | 25 ++++++++------ internal/stats/http_test.go | 64 +++++++++++++++-------------------- internal/stats/stats.go | 8 ++--- openapi/CHANGELOG.md | 6 +++- openapi/openapi.yaml | 55 ++++++------------------------ 10 files changed, 106 insertions(+), 146 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a79b4cad..5ae3823f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,14 +62,18 @@ In this release, the schema version has changed from 16 to 17. - The `GET /control/stats_info` HTTP API; use the new `GET /control/stats/config` API instead. - **NOTE:** If interval is custom then it will be equal to `90` days for + **NOTE:** If `interval` was configured by editing configuration file or new + HTTP API call `PUT /control/stats/config/update` and it's not equal to + previous allowed enum values then it will be equal to `90` days for compatibility reasons. - The `POST /control/stats_config` HTTP API; use the new `PUT /control/stats/config/update` API instead. - The `GET /control/querylog_info` HTTP API; use the new `GET /control/querylog/config` API instead. - **NOTE:** If interval is custom then it will be equal to `90` days for + **NOTE:** If `interval` was configured by editing configuration file or new + HTTP API call `PUT /control/querylog/config/update` and it's not equal to + previous allowed enum values then it will be equal to `90` days for compatibility reasons. - The `POST /control/querylog_config` HTTP API; use the new `PUT /control/querylog/config/update` API instead. diff --git a/internal/aghnet/hostgen.go b/internal/aghnet/hostgen.go index 3138bf32..b1421a64 100644 --- a/internal/aghnet/hostgen.go +++ b/internal/aghnet/hostgen.go @@ -71,16 +71,16 @@ func GenerateHostname(ip net.IP) (hostname string) { func NewDomainNameSet(list []string) (set *stringutil.Set, err error) { set = stringutil.NewSet() - for _, v := range list { + for i, v := range list { host := strings.ToLower(strings.TrimSuffix(v, ".")) - // TODO(a.garipov): Think about ignoring empty (".") names in - // the future. + // TODO(a.garipov): Think about ignoring empty (".") names in the + // future. if host == "" { return nil, errors.Error("host name is empty") } if set.Has(host) { - return nil, fmt.Errorf("duplicate host name %q", host) + return nil, fmt.Errorf("duplicate host name %q at index %d", host, i) } set.Add(host) diff --git a/internal/home/config.go b/internal/home/config.go index 6af776e4..613ffdcd 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -218,33 +218,32 @@ type tlsConfigSettings struct { } type queryLogConfig struct { + // Ignored is the list of host names, which should not be written to log. + Ignored []string `yaml:"ignored"` + + // Interval is the interval for query log's files rotation. + Interval timeutil.Duration `yaml:"interval"` + + // MemSize is the number of entries kept in memory before they are flushed + // to disk. + MemSize uint32 `yaml:"size_memory"` + // Enabled defines if the query log is enabled. Enabled bool `yaml:"enabled"` // FileEnabled defines, if the query log is written to the file. FileEnabled bool `yaml:"file_enabled"` - - // Interval is the interval for query log's files rotation. - Interval timeutil.Duration `yaml:"interval"` - - // MemSize is the number of entries kept in memory before they are - // flushed to disk. - MemSize uint32 `yaml:"size_memory"` - - // Ignored is the list of host names, which should not be written to - // log. - Ignored []string `yaml:"ignored"` } type statsConfig struct { - // Enabled defines if the statistics are enabled. - Enabled bool `yaml:"enabled"` - - // Interval is the time interval for flushing statistics to the disk. - Interval timeutil.Duration `yaml:"interval"` - // Ignored is the list of host names, which should not be counted. Ignored []string `yaml:"ignored"` + + // Interval is the retention interval for statistics. + Interval timeutil.Duration `yaml:"interval"` + + // Enabled defines if the statistics are enabled. + Enabled bool `yaml:"enabled"` } // config is the global configuration structure. diff --git a/internal/querylog/http.go b/internal/querylog/http.go index 2224dc41..0b6976e0 100644 --- a/internal/querylog/http.go +++ b/internal/querylog/http.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "net/url" + "sort" "strconv" "strings" "time" @@ -26,8 +27,8 @@ type configJSON struct { // fractional numbers and not mess the API users by changing the units. Interval float64 `json:"interval"` - // Enabled shows if the querylog is enabled. It is an [aghalg.NullBool] - // to be able to tell when it's set without using pointers. + // Enabled shows if the querylog is enabled. It is an aghalg.NullBool to + // be able to tell when it's set without using pointers. Enabled aghalg.NullBool `json:"enabled"` // AnonymizeClientIP shows if the clients' IP addresses must be anonymized. @@ -38,24 +39,22 @@ type configJSON struct { // getConfigResp is the JSON structure for the querylog configuration. type getConfigResp struct { - // Enabled shows if the querylog is enabled. It is an - // [aghalg.NullBool] to be able to tell when it's set without using - // pointers. - Enabled aghalg.NullBool `json:"enabled"` - - // AnonymizeClientIP shows if the clients' IP addresses must be - // anonymized. It is an [aghalg.NullBool] to be able to tell when it's - // set without using pointers. - // - // TODO(a.garipov): Consider using separate setting for statistics. - AnonymizeClientIP aghalg.NullBool `json:"anonymize_client_ip"` + // Ignored is the list of host names, which should not be written to log. + Ignored []string `json:"ignored"` // Interval is the querylog rotation interval in milliseconds Interval float64 `json:"interval"` - // Ignored is the list of host names, which should not be written to - // log. - Ignored []string `json:"ignored"` + // Enabled shows if the querylog is enabled. It is an aghalg.NullBool to + // be able to tell when it's set without using pointers. + Enabled aghalg.NullBool `json:"enabled"` + + // AnonymizeClientIP shows if the clients' IP addresses must be anonymized. + // It is an aghalg.NullBool to be able to tell when it's set without using + // pointers. + // + // TODO(a.garipov): Consider using separate setting for statistics. + AnonymizeClientIP aghalg.NullBool `json:"anonymize_client_ip"` } // Register web handlers @@ -109,8 +108,8 @@ func (l *queryLog) handleQueryLogInfo(w http.ResponseWriter, r *http.Request) { ok := checkInterval(ivl) if !ok { - // NOTE: If interval is custom we set it to 90 days for - // compatibility with old API. + // NOTE: If interval is custom we set it to 90 days for compatibility + // with old API. ivl = timeutil.Day * 90 } @@ -128,6 +127,7 @@ func (l *queryLog) handleGetQueryLogConfig(w http.ResponseWriter, r *http.Reques defer l.lock.Unlock() ignored := l.conf.Ignored.Values() + sort.Strings(ignored) _ = aghhttp.WriteJSONResponse(w, r, getConfigResp{ Enabled: aghalg.BoolToNullBool(l.conf.Enabled), Interval: float64(l.conf.RotationIvl.Milliseconds()), @@ -257,7 +257,7 @@ func (l *queryLog) handlePutQueryLogConfig(w http.ResponseWriter, r *http.Reques if len(newConf.Ignored) > 0 { set, serr := aghnet.NewDomainNameSet(newConf.Ignored) if serr != nil { - aghhttp.Error(r, w, http.StatusUnprocessableEntity, "ignored: %s", serr) + aghhttp.Error(r, w, http.StatusUnprocessableEntity, "ignored: duplicate or empty host") return } diff --git a/internal/querylog/querylog.go b/internal/querylog/querylog.go index 9d076835..f52b30f9 100644 --- a/internal/querylog/querylog.go +++ b/internal/querylog/querylog.go @@ -157,11 +157,8 @@ func newQueryLog(conf Config) (l *queryLog, err error) { l.conf = &Config{} *l.conf = conf - if conf.RotationIvl < time.Hour { - return nil, errors.Error("interval: less than an hour") - } - if conf.RotationIvl > timeutil.Day*365 { - return nil, errors.Error("interval: more than a year") + if conf.RotationIvl < time.Hour || conf.RotationIvl > timeutil.Day*365 { + return nil, errors.Error("unsupported interval") } return l, nil diff --git a/internal/stats/http.go b/internal/stats/http.go index f7c37700..2496e5e2 100644 --- a/internal/stats/http.go +++ b/internal/stats/http.go @@ -5,6 +5,7 @@ package stats import ( "encoding/json" "net/http" + "sort" "time" "github.com/AdguardTeam/AdGuardHome/internal/aghalg" @@ -68,15 +69,15 @@ type configResp struct { // getConfigResp is the response to the GET /control/stats_info. type getConfigResp struct { - // Enabled shows if statistics are enabled. It is an [aghalg.NullBool] - // to be able to tell when it's set without using pointers. - Enabled aghalg.NullBool `json:"enabled"` + // Ignored is the list of host names, which should not be counted. + Ignored []string `json:"ignored"` // Interval is the statistics rotation interval in milliseconds. Interval float64 `json:"interval"` - // Ignored is the list of host names, which should not be counted. - Ignored []string `json:"ignored"` + // Enabled shows if statistics are enabled. It is an aghalg.NullBool to be + // able to tell when it's set without using pointers. + Enabled aghalg.NullBool `json:"enabled"` } // handleStatsInfo handles requests to the GET /control/stats_info endpoint. @@ -90,8 +91,8 @@ func (s *StatsCtx) handleStatsInfo(w http.ResponseWriter, r *http.Request) { ok := checkInterval(days) if !ok || (s.enabled && days == 0) { - // NOTE: If interval is custom we set it to 90 days for - // compatibility with old API. + // NOTE: If interval is custom we set it to 90 days for compatibility + // with old API. days = 90 } @@ -102,15 +103,19 @@ func (s *StatsCtx) handleStatsInfo(w http.ResponseWriter, r *http.Request) { _ = aghhttp.WriteJSONResponse(w, r, resp) } -// handleGetStatsConfig handles requests to the GET /control/stats_info endpoint. +// handleGetStatsConfig handles requests to the GET /control/stats_info +// endpoint. func (s *StatsCtx) handleGetStatsConfig(w http.ResponseWriter, r *http.Request) { s.lock.Lock() defer s.lock.Unlock() + ignored := s.ignored.Values() + sort.Strings(ignored) + resp := getConfigResp{ Enabled: aghalg.BoolToNullBool(s.enabled), Interval: float64(s.limit.Milliseconds()), - Ignored: s.ignored.Values(), + Ignored: ignored, } err := aghhttp.WriteJSONResponse(w, r, resp) if err != nil { @@ -182,7 +187,7 @@ func (s *StatsCtx) handlePutStatsConfig(w http.ResponseWriter, r *http.Request) if len(reqData.Ignored) > 0 { set, serr := aghnet.NewDomainNameSet(reqData.Ignored) if serr != nil { - aghhttp.Error(r, w, http.StatusUnprocessableEntity, "ignored: %s", serr) + aghhttp.Error(r, w, http.StatusUnprocessableEntity, "ignored: duplicate or empty host") return } diff --git a/internal/stats/http_test.go b/internal/stats/http_test.go index 95229571..df8e11a9 100644 --- a/internal/stats/http_test.go +++ b/internal/stats/http_test.go @@ -17,22 +17,30 @@ import ( ) func TestHandleStatsConfig(t *testing.T) { - var ( - halfHour = time.Hour / 2 - hour = time.Hour - year = timeutil.Day * 365 + const ( + smallIvl = 1 * time.Minute + minIvl = 1 * time.Hour + maxIvl = 365 * timeutil.Day ) + conf := Config{ + Filename: filepath.Join(t.TempDir(), "stats.db"), + Limit: time.Hour * 24, + Enabled: true, + UnitID: func() (id uint32) { return 0 }, + ConfigModified: func() {}, + } + testCases := []struct { name string body getConfigResp wantCode int wantErr string }{{ - name: "set_ivl_1_hour", + name: "set_ivl_1_minIvl", body: getConfigResp{ Enabled: aghalg.NBTrue, - Interval: float64(hour.Milliseconds()), + Interval: float64(minIvl.Milliseconds()), Ignored: nil, }, wantCode: http.StatusOK, @@ -41,7 +49,7 @@ func TestHandleStatsConfig(t *testing.T) { name: "small_interval", body: getConfigResp{ Enabled: aghalg.NBTrue, - Interval: float64(halfHour.Milliseconds()), + Interval: float64(smallIvl.Milliseconds()), Ignored: []string{}, }, wantCode: http.StatusUnprocessableEntity, @@ -50,16 +58,16 @@ func TestHandleStatsConfig(t *testing.T) { name: "big_interval", body: getConfigResp{ Enabled: aghalg.NBTrue, - Interval: float64(year.Milliseconds() + hour.Milliseconds()), + Interval: float64(maxIvl.Milliseconds() + minIvl.Milliseconds()), Ignored: []string{}, }, wantCode: http.StatusUnprocessableEntity, wantErr: "unsupported interval\n", }, { - name: "set_ignored_ivl_1_year", + name: "set_ignored_ivl_1_maxIvl", body: getConfigResp{ Enabled: aghalg.NBTrue, - Interval: float64(year.Milliseconds()), + Interval: float64(maxIvl.Milliseconds()), Ignored: []string{ "ignor.ed", }, @@ -70,30 +78,30 @@ func TestHandleStatsConfig(t *testing.T) { name: "ignored_duplicate", body: getConfigResp{ Enabled: aghalg.NBTrue, - Interval: float64(hour.Milliseconds()), + Interval: float64(minIvl.Milliseconds()), Ignored: []string{ "ignor.ed", "ignor.ed", }, }, wantCode: http.StatusUnprocessableEntity, - wantErr: "ignored: duplicate host name \"ignor.ed\"\n", + wantErr: "ignored: duplicate or empty host\n", }, { name: "ignored_empty", body: getConfigResp{ Enabled: aghalg.NBTrue, - Interval: float64(hour.Milliseconds()), + Interval: float64(minIvl.Milliseconds()), Ignored: []string{ "", }, }, wantCode: http.StatusUnprocessableEntity, - wantErr: "ignored: host name is empty\n", + wantErr: "ignored: duplicate or empty host\n", }, { name: "enabled_is_null", body: getConfigResp{ Enabled: aghalg.NBNull, - Interval: float64(hour.Milliseconds()), + Interval: float64(minIvl.Milliseconds()), Ignored: []string{}, }, wantCode: http.StatusUnprocessableEntity, @@ -102,23 +110,6 @@ func TestHandleStatsConfig(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - handlers := map[string]http.Handler{} - - conf := Config{ - Filename: filepath.Join(t.TempDir(), "stats.db"), - Limit: time.Hour * 24, - Enabled: true, - UnitID: func() (id uint32) { return 0 }, - HTTPRegister: func( - _ string, - url string, - handler http.HandlerFunc, - ) { - handlers[url] = handler - }, - ConfigModified: func() {}, - } - s, err := New(conf) require.NoError(t, err) @@ -136,11 +127,10 @@ func TestHandleStatsConfig(t *testing.T) { req := httptest.NewRequest(http.MethodPut, configPut, bytes.NewReader(buf)) rw := httptest.NewRecorder() - handlers[configPut].ServeHTTP(rw, req) + s.handlePutStatsConfig(rw, req) require.Equal(t, tc.wantCode, rw.Code) if tc.wantCode != http.StatusOK { - assert.Equal(t, tc.wantCode, rw.Code) assert.Equal(t, tc.wantErr, rw.Body.String()) return @@ -149,12 +139,12 @@ func TestHandleStatsConfig(t *testing.T) { resp := httptest.NewRequest(http.MethodGet, configGet, nil) rw = httptest.NewRecorder() - handlers[configGet].ServeHTTP(rw, resp) + s.handleGetStatsConfig(rw, resp) require.Equal(t, http.StatusOK, rw.Code) ans := getConfigResp{} - jerr := json.Unmarshal(rw.Body.Bytes(), &ans) - require.NoError(t, jerr) + err = json.Unmarshal(rw.Body.Bytes(), &ans) + require.NoError(t, err) assert.Equal(t, tc.body, ans) }) diff --git a/internal/stats/stats.go b/internal/stats/stats.go index 6fa05a54..3254e9ed 100644 --- a/internal/stats/stats.go +++ b/internal/stats/stats.go @@ -128,12 +128,8 @@ func New(conf Config) (s *StatsCtx, err error) { ignored: conf.Ignored, } - if conf.Limit < time.Hour { - return nil, errors.Error("interval: less than an hour") - } - - if conf.Limit > timeutil.Day*365 { - return nil, errors.Error("interval: more than a year") + if conf.Limit < time.Hour || conf.Limit > timeutil.Day*365 { + return nil, errors.Error("unsupported interval") } s.limit = conf.Limit diff --git a/openapi/CHANGELOG.md b/openapi/CHANGELOG.md index 8508e6de..f32771de 100644 --- a/openapi/CHANGELOG.md +++ b/openapi/CHANGELOG.md @@ -6,18 +6,20 @@ -## v0.107.25: API changes +## v0.107.26: API changes ### Deprecated statistics APIs * The `GET /control/stats_info` HTTP API; use the new `GET /control/stats/config` API instead. + * The `POST /control/stats_config` HTTP API; use the new `PUT /control/stats/config/update` API instead. ### New statistics APIs * The new `GET /control/stats/config` HTTP API. + * The new `PUT /control/stats/config/update` HTTP API allows config updates. These `control/stats/config/update` and `control/stats/config` APIs accept and @@ -35,12 +37,14 @@ return a JSON object with the following format: * The `GET /control/querylog_info` HTTP API; use the new `GET /control/querylog/config` API instead. + * The `POST /control/querylog_config` HTTP API; use the new `PUT /control/querylog/config/update` API instead. ### New query log APIs * The new `GET /control/querylog/config` HTTP API. + * The new `PUT /control/querylog/config/update` HTTP API allows config updates. These `control/querylog/config/update` and `control/querylog/config` APIs diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 459eec4a..217e0555 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -227,10 +227,12 @@ '/querylog_info': 'get': 'deprecated': true - 'description': > + 'description': | Deprecated: Use `GET /querylog/config` instead. - NOTE: If interval is custom then it will be equal to `90` days for + NOTE: If `interval` was configured by editing configuration file or new + HTTP API call `PUT /querylog/config/update` and it's not equal to + previous allowed enum values then it will be equal to `90` days for compatibility reasons. 'tags': - 'log' @@ -322,10 +324,12 @@ '/stats_info': 'get': 'deprecated': true - 'description': > + 'description': | Deprecated: Use `GET /stats/config` instead. - NOTE: If interval is custom then it will be equal to `90` days for + NOTE: If `interval` was configured by editing configuration file or new + HTTP API call `PUT /stats/config/update` and it's not equal to + previous allowed enum values then it will be equal to `90` days for compatibility reasons. 'tags': - 'stats' @@ -1748,24 +1752,7 @@ 'items': 'type': 'string' 'PutStatsConfigUpdateRequest': - 'type': 'object' - 'description': 'Statistics configuration' - 'required': - - 'enabled' - - 'interval' - - 'ignored' - 'properties': - 'enabled': - 'description': 'Are statistics enabled' - 'type': 'boolean' - 'interval': - 'description': 'Statistics rotation interval' - 'type': 'number' - 'ignored': - 'description': 'List of host names, which should not be counted' - 'type': 'array' - 'items': - 'type': 'string' + '$ref': '#/components/schemas/GetStatsConfigResponse' 'DhcpConfig': 'type': 'object' 'properties': @@ -2194,29 +2181,7 @@ 'items': 'type': 'string' 'PutQueryLogConfigUpdateRequest': - 'type': 'object' - 'description': 'Query log configuration' - 'required': - - 'enabled' - - 'interval' - - 'anonymize_client_ip' - - 'ignored' - 'properties': - 'enabled': - 'type': 'boolean' - 'description': 'Is query log enabled' - 'interval': - 'description': > - Time period for query log rotation. - 'type': 'number' - 'anonymize_client_ip': - 'type': 'boolean' - 'description': "Anonymize clients' IP addresses" - 'ignored': - 'description': 'List of host names, which should not be written to log' - 'type': 'array' - 'items': - 'type': 'string' + '$ref': '#/components/schemas/GetQueryLogConfigResponse' 'ResultRule': 'description': 'Applied rule.' 'properties':