all: imp style, imp tests

Author: masterkusok

internal/cmd/args.go

@@ -439,8 +439,8 @@ func parseCmdLineOptions(conf *configuration) (err error) {
 		timeoutIdx:                  &conf.Timeout,
 		cacheMinTTLIdx:              &conf.CacheMinTTL,
 		cacheMaxTTLIdx:              &conf.CacheMaxTTL,
-		cacheOptimisticAnswerTTLIdx: &conf.CacheOptimisticAnswerTTL,
-		cacheOptimisticMaxAgeIdx:    &conf.CacheOptimisticMaxAge,
+		cacheOptimisticAnswerTTLIdx: &conf.OptimisticAnswerTTL,
+		cacheOptimisticMaxAgeIdx:    &conf.OptimisticMaxAge,
 		cacheSizeBytesIdx:           &conf.CacheSizeBytes,
 		ratelimitIdx:                &conf.Ratelimit,
 		ratelimitSubnetLenIPv4Idx:   &conf.RatelimitSubnetLenIPv4,

internal/cmd/config.go

@@ -107,13 +107,13 @@ type configuration struct {
 	// greater.
 	CacheMaxTTL uint32 `yaml:"cache-max-ttl"`
 
-	// CacheOptimisticAnswerTTL is the default TTL for expired cached responses
+	// OptimisticAnswerTTL is the default TTL for expired cached responses
 	// in seconds.
-	CacheOptimisticAnswerTTL timeutil.Duration `yaml:"optimistic-answers-ttl"`
+	OptimisticAnswerTTL timeutil.Duration `yaml:"optimistic-answers-ttl"`
 
-	// CacheOptimisticMaxAge is the maximum time entries remain in the cache
+	// OptimisticMaxAge is the maximum time entries remain in the cache
 	// when cache is optimistic.
-	CacheOptimisticMaxAge timeutil.Duration `yaml:"optimistic-max-age"`
+	OptimisticMaxAge timeutil.Duration `yaml:"optimistic-max-age"`
 
 	// CacheSizeBytes is the cache size in bytes.  Default is 64k.
 	CacheSizeBytes int `yaml:"cache-size"`
@@ -206,16 +206,16 @@ type configuration struct {
 // no options have been parsed, it returns a suitable exit code and an error.
 func parseConfig() (conf *configuration, exitCode int, err error) {
 	conf = &configuration{
-		HTTPSServerName:          "dnsproxy",
-		UpstreamMode:             string(proxy.UpstreamModeLoadBalance),
-		CacheSizeBytes:           64 * 1024,
-		Timeout:                  timeutil.Duration(10 * time.Second),
-		CacheOptimisticAnswerTTL: timeutil.Duration(30 * time.Second),
-		CacheOptimisticMaxAge:    timeutil.Duration(12 * time.Hour),
-		RatelimitSubnetLenIPv4:   24,
-		RatelimitSubnetLenIPv6:   56,
-		HostsFileEnabled:         true,
-		PendingRequestsEnabled:   true,
+		HTTPSServerName:        "dnsproxy",
+		UpstreamMode:           string(proxy.UpstreamModeLoadBalance),
+		CacheSizeBytes:         64 * 1024,
+		Timeout:                timeutil.Duration(10 * time.Second),
+		OptimisticAnswerTTL:    timeutil.Duration(30 * time.Second),
+		OptimisticMaxAge:       timeutil.Duration(12 * time.Hour),
+		RatelimitSubnetLenIPv4: 24,
+		RatelimitSubnetLenIPv6: 56,
+		HostsFileEnabled:       true,
+		PendingRequestsEnabled: true,
 	}
 
 	err = parseCmdLineOptions(conf)

internal/cmd/proxy.go

@@ -63,8 +63,8 @@ func createProxyConfig(
 		CacheSizeBytes:           conf.CacheSizeBytes,
 		CacheMinTTL:              conf.CacheMinTTL,
 		CacheMaxTTL:              conf.CacheMaxTTL,
-		CacheOptimisticAnswerTTL: uint32(time.Duration(conf.CacheOptimisticAnswerTTL).Seconds()),
-		CacheOptimisticMaxAge:    uint32(time.Duration(conf.CacheOptimisticMaxAge).Seconds()),
+		CacheOptimisticAnswerTTL: time.Duration(conf.OptimisticAnswerTTL),
+		CacheOptimisticMaxAge:    time.Duration(conf.OptimisticMaxAge),
 		CacheOptimistic:          conf.CacheOptimistic,
 		RefuseAny:                conf.RefuseAny,
 		HTTP3:                    conf.HTTP3,

proxy/cache.go

@@ -40,21 +40,20 @@ type cache struct {
 	// those again.
 	optimistic bool
 
-	// optimisticTTL is the default TTL for expired cached responses in seconds.
-	optimisticTTL uint32
+	// optimisticTTL is the default TTL for expired cached responses.
+	optimisticTTL time.Duration
 
 	// optimisticMaxAge is the maximum time entries remain in the cache when
 	// cache is optimistic.
-	optimisticMaxAge uint32
+	optimisticMaxAge time.Duration
 }
 
 // cacheItem is a single cache entry.  It's a helper type to aggregate the
 // item-specific logic.
 type cacheItem struct {
-	createdTime time.Time
-	m           *dns.Msg
-	u           string
-	ttl         uint32
+	m   *dns.Msg
+	u   string
+	ttl uint32
 }
 
 // respToItem converts the pair of the response and upstream resolved the one
@@ -71,10 +70,9 @@ func (c *cache) respToItem(m *dns.Msg, u upstream.Upstream, l *slog.Logger) (ite
 	}
 
 	return &cacheItem{
-		m:           m,
-		u:           upsAddr,
-		ttl:         ttl,
-		createdTime: time.Now(),
+		m:   m,
+		u:   upsAddr,
+		ttl: ttl,
 	}
 }
 
@@ -85,12 +83,9 @@ const (
 	// expTimeSz is the exact length of byte slice capable to store the
 	// expiration time the response.  It's essentially the size of a uint32.
 	expTimeSz = 4
-	// createdTimeSz is the exact length of byte slice capable to store the
-	// creation time.  It's essentially the size of a uint32.
-	createdTimeSz = 4
 
 	// minPackedLen is the minimum length of the packed cacheItem.
-	minPackedLen = expTimeSz + createdTimeSz + packedMsgLenSz
+	minPackedLen = expTimeSz + packedMsgLenSz
 )
 
 // pack converts the ci into bytes slice.
@@ -102,11 +97,8 @@ func (ci *cacheItem) pack() (packed []byte) {
 	// Put expiration time.
 	binary.BigEndian.PutUint32(packed, uint32(time.Now().Unix())+ci.ttl)
 
-	// Put creation time.
-	binary.BigEndian.PutUint32(packed[expTimeSz:], uint32(ci.createdTime.Unix()))
-
 	// Put the length of the packed message.
-	binary.BigEndian.PutUint16(packed[expTimeSz+createdTimeSz:], uint16(pmLen))
+	binary.BigEndian.PutUint16(packed[expTimeSz:], uint16(pmLen))
 
 	// Put the packed message itself.
 	packed = append(packed, pm...)
@@ -119,25 +111,26 @@ func (ci *cacheItem) pack() (packed []byte) {
 
 // unpackItem converts the data into cacheItem using req as a request message.
 // expired is true if the item exists but expired.  The expired cached items are
-// only returned if c is optimistic.  req must not be nil.
+// only returned if c is optimistic and optimistic max age is not exceeded.  req
+// must not be nil.
 func (c *cache) unpackItem(data []byte, req *dns.Msg) (ci *cacheItem, expired bool) {
 	if len(data) < minPackedLen {
 		return nil, false
 	}
 
 	b := bytes.NewBuffer(data)
-	expire := int64(binary.BigEndian.Uint32(b.Next(expTimeSz)))
-	createdTime := time.Unix(int64(binary.BigEndian.Uint32(b.Next(createdTimeSz))), 0)
-	now := time.Now().Unix()
+	expire := time.Unix(int64(binary.BigEndian.Uint32(b.Next(expTimeSz))), 0)
+	now := time.Now()
 	var ttl uint32
-	if expired = expire <= now; expired {
-		if !c.optimistic {
+	if expired = now.After(expire); expired {
+		optimisticExpire := expire.Add(c.optimisticMaxAge)
+		if !c.optimistic || now.After(optimisticExpire) {
 			return nil, expired
 		}
 
-		ttl = c.optimisticTTL
+		ttl = uint32(c.optimisticTTL.Seconds())
 	} else {
-		ttl = uint32(expire - now)
+		ttl = uint32(expire.Unix() - now.Unix())
 	}
 
 	l := int(binary.BigEndian.Uint16(b.Next(packedMsgLenSz)))
@@ -165,9 +158,8 @@ func (c *cache) unpackItem(data []byte, req *dns.Msg) (ci *cacheItem, expired bo
 	filterMsg(res, m, req.AuthenticatedData, doBit, ttl)
 
 	return &cacheItem{
-		m:           res,
-		u:           string(b.Next(b.Len())),
-		createdTime: createdTime,
+		m: res,
+		u: string(b.Next(b.Len())),
 	}, expired
 }
 
@@ -181,35 +173,50 @@ func (p *Proxy) initCache() {
 
 	size := p.CacheSizeBytes
 	p.logger.Info("cache enabled", "size", size)
-	p.cache = newCache(
-		size,
-		p.CacheOptimisticAnswerTTL,
-		p.CacheOptimisticMaxAge,
-		p.EnableEDNSClientSubnet,
-		p.CacheOptimistic,
-	)
+	p.cache = newCache(&cacheConfig{
+		size:             size,
+		optimisticTTL:    p.CacheOptimisticAnswerTTL,
+		optimisticMaxAge: p.CacheOptimisticMaxAge,
+		withECS:          p.EnableEDNSClientSubnet,
+		optimistic:       p.CacheOptimistic,
+	})
 	p.shortFlighter = newOptimisticResolver(p)
 }
 
+// cacheConfig is the configuration structure for [cache].
+type cacheConfig struct {
+	// size is the cache size in bytes.
+	size int
+
+	// optimisticTTL is the default TTL for expired cached responses when
+	// optimistic is enabled.
+	optimisticTTL time.Duration
+
+	// optimisticMaxAge is the maximum time entries remain in the cache when
+	// cache is optimistic.
+	optimisticMaxAge time.Duration
+
+	// withECS enables EDNS Client Subnet support for cache.
+	withECS bool
+
+	// optimistic defines if the cache should return expired items and resolve
+	// those again.
+	optimistic bool
+}
+
 // newCache returns a properly initialized cache.  logger must not be nil.
-func newCache(
-	size int,
-	optimisticTTL,
-	optimisticMaxAge uint32,
-	withECS,
-	optimistic bool,
-) (c *cache) {
+func newCache(conf *cacheConfig) (c *cache) {
 	c = &cache{
 		itemsLock:           &sync.RWMutex{},
 		itemsWithSubnetLock: &sync.RWMutex{},
-		items:               createCache(size),
-		optimistic:          optimistic,
-		optimisticTTL:       optimisticTTL,
-		optimisticMaxAge:    optimisticMaxAge,
+		items:               createCache(conf.size),
+		optimistic:          conf.optimistic,
+		optimisticTTL:       conf.optimisticTTL,
+		optimisticMaxAge:    conf.optimisticMaxAge,
 	}
 
-	if withECS {
-		c.itemsWithSubnet = createCache(size)
+	if conf.withECS {
+		c.itemsWithSubnet = createCache(conf.size)
 	}
 
 	return c
@@ -236,21 +243,9 @@ func (c *cache) get(req *dns.Msg) (ci *cacheItem, expired bool, key []byte) {
 		c.items.Del(key)
 	}
 
-	if c.isOptimisticExpired(ci) {
-		c.items.Del(key)
-
-		return nil, false, key
-	}
-
 	return ci, expired, key
 }
 
-// isOptimisticExpired returns true if cache is optimistic and expired item
-// should be deleted explicitly.  ci must not be nil.
-func (c *cache) isOptimisticExpired(ci *cacheItem) (expired bool) {
-	return c.optimistic && uint32(time.Since(ci.createdTime).Seconds()) > ci.ttl+c.optimisticMaxAge
-}
-
 // getWithSubnet returns cached item for the req if it's found by n.  expired
 // is true if the item's TTL is expired.  k is the resulting key for req.  It's
 // returned to avoid recalculating it afterwards.