Repository: Kong/kubernetes-ingress-controller
Pull request #6617 (2024-11-04 15:40:56)

refactor: do not allocate for SanitizedCopy()
**What this PR does / why we need it**:

`SanitizedCopy()` functions unnecessarily allocate objects where it's not needed.

This PR removes those allocations in several places and adds a benchmark to prove the results of this optimization:

before

```
go test -benchmem -bench BenchmarkSanitizedCopy -count 1 ./internal/dataplane/kongstate
goos: darwin
goarch: arm64
pkg: github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate
cpu: Apple M1 Max
BenchmarkSanitizedCopy-10           1837            609341 ns/op         3216834 B/op       1036 allocs/op
PASS
ok      github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate   1.788s
```

after

```
go test -benchmem -bench BenchmarkSanitizedCopy -count 1 ./internal/dataplane/kongstate
goos: darwin
goarch: arm64
pkg: github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate
cpu: Apple M1 Max
BenchmarkSanitizedCopy-10           4982            242434 ns/op          778484 B/op          5 allocs/op
PASS
ok      github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate   2.750s
```
 
```
benchstat -confidence 0.9 old new
goos: darwin
goarch: arm64
pkg: github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/kongstate
cpu: Apple M1 Max
                 │     old     │                new                 │
                 │   sec/op    │   sec/op     vs base               │
SanitizedCopy-10   624.5µ ± 1%   241.3µ ± 1%  -61.37% (p=0.008 n=5)

                 │      old      │                 new                 │
                 │     B/op      │     B/op      vs base               │
SanitizedCopy-10   3141.4Ki ± 0%   760.2Ki ± 0%  -75.80% (p=0.008 n=5)

                 │      old      │                new                │
                 │   allocs/op   │ allocs/op   vs base               │
SanitizedCopy-10   1036.000 ± 0%   5.000 ± 0%  -99.52% (p=0.008 n=5)
```

## Special notes 


This can be taken a step further by removing the allocations for credentials i.e. https://github.com/Kong/kubernetes-ingress-controller/blob/4b60bf8cd8074fb310b8270e623fe8f080cdd934/internal/dataplane/kongstate/credentials.go#L163-L234 but that requires a bit more refactoring.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) Report
Attention: Patch coverage is `92.85714%` with `4 lines` in your changes missing coverage. Please review.
> Project coverage is 72.3%. Comparing base [(`f3bd1d9`)](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/commit/f3bd1d9ea2d9b82925a14d64b3bce112def0d934?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) to head [(`bab378b`)](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/commit/bab378b9bbe3780abdf652c645a7a9ec178c6617?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong).

| [Files with missing lines](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Patch % | Lines |
|---|---|---|
| [internal/dataplane/kongstate/kongstate.go](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?src=pr&el=tree&filepath=internal%2Fdataplane%2Fkongstate%2Fkongstate.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-aW50ZXJuYWwvZGF0YXBsYW5lL2tvbmdzdGF0ZS9rb25nc3RhdGUuZ28=) | 85.0% | [3 Missing :warning: ](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) |
| [internal/dataplane/kongstate/consumer.go](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?src=pr&el=tree&filepath=internal%2Fdataplane%2Fkongstate%2Fconsumer.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-aW50ZXJuYWwvZGF0YXBsYW5lL2tvbmdzdGF0ZS9jb25zdW1lci5nbw==) | 96.8% | [1 Missing :warning: ](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) |

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##            main   #6617     +/-   ##
=======================================
- Coverage   74.4%   72.3%   -2.2%     
=======================================
  Files        202     202             
  Lines      25801   25980    +179     
=======================================
- Hits       19219   18800    -419     
- Misses      5549    6168    +619     
+ Partials    1033    1012     -21     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/Kong/kubernetes-ingress-controller/pull/6617?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong).


Patch:
diff --git a/internal/dataplane/kongstate/consumer.go b/internal/dataplane/kongstate/consumer.go
index 21d9621e3..f6f86911a 100644
--- a/internal/dataplane/kongstate/consumer.go
+++ b/internal/dataplane/kongstate/consumer.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"github.com/kong/go-kong/kong"
+	"github.com/samber/lo"
 
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
 	kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1"
@@ -28,39 +29,49 @@ type Consumer struct {
 }
 
 // SanitizedCopy returns a shallow copy with sensitive values redacted best-effort.
-func (c *Consumer) SanitizedCopy(uuidGenerator util.UUIDGenerator) *Consumer {
-	return &Consumer{
+func (c *Consumer) SanitizedCopy(uuidGenerator util.UUIDGenerator) Consumer {
+	return Consumer{
 		Consumer: c.Consumer,
 		Plugins:  c.Plugins,
-		KeyAuths: func() (res []*KeyAuth) {
-			for _, v := range c.KeyAuths {
-				res = append(res, v.SanitizedCopy(uuidGenerator))
+		KeyAuths: func() []*KeyAuth {
+			if c.KeyAuths == nil {
+				return nil
 			}
-			return
+			return lo.Map(c.KeyAuths, func(c *KeyAuth, _ int) *KeyAuth {
+				return c.SanitizedCopy(uuidGenerator)
+			})
 		}(),
-		HMACAuths: func() (res []*HMACAuth) {
-			for _, v := range c.HMACAuths {
-				res = append(res, v.SanitizedCopy())
+		HMACAuths: func() []*HMACAuth {
+			if c.HMACAuths == nil {
+				return nil
 			}
-			return
+			return lo.Map(c.HMACAuths, func(c *HMACAuth, _ int) *HMACAuth {
+				return c.SanitizedCopy()
+			})
 		}(),
-		JWTAuths: func() (res []*JWTAuth) {
-			for _, v := range c.JWTAuths {
-				res = append(res, v.SanitizedCopy())
+		JWTAuths: func() []*JWTAuth {
+			if c.JWTAuths == nil {
+				return nil
 			}
-			return
+			return lo.Map(c.JWTAuths, func(c *JWTAuth, _ int) *JWTAuth {
+				return c.SanitizedCopy()
+			})
 		}(),
-		BasicAuths: func() (res []*BasicAuth) {
-			for _, v := range c.BasicAuths {
-				res = append(res, v.SanitizedCopy())
+		BasicAuths: func() []*BasicAuth {
+			if c.BasicAuths == nil {
+				return nil
 			}
-			return
+			return lo.Map(c.BasicAuths, func(c *BasicAuth, _ int) *BasicAuth {
+				return c.SanitizedCopy()
+			})
 		}(),
-		Oauth2Creds: func() (res []*Oauth2Credential) {
-			for _, v := range c.Oauth2Creds {
-				res = append(res, v.SanitizedCopy())
+		Oauth2Creds: func() []*Oauth2Credential {
+			if c.Oauth2Creds == nil {
+				return nil
 			}
-			return
+			return lo.Map(c.Oauth2Creds, func(c *Oauth2Credential, _ int) *Oauth2Credential {
+				return c.SanitizedCopy()
+			})
 		}(),
 		ACLGroups:       c.ACLGroups,
 		MTLSAuths:       c.MTLSAuths,
diff --git a/internal/dataplane/kongstate/kongstate.go b/internal/dataplane/kongstate/kongstate.go
index 6342f738d..954d54ad8 100644
--- a/internal/dataplane/kongstate/kongstate.go
+++ b/internal/dataplane/kongstate/kongstate.go
@@ -46,25 +46,33 @@ func (ks *KongState) SanitizedCopy(uuidGenerator util.UUIDGenerator) *KongState
 	return &KongState{
 		Services:  ks.Services,
 		Upstreams: ks.Upstreams,
-		Certificates: func() (res []Certificate) {
-			for _, v := range ks.Certificates {
-				res = append(res, *v.SanitizedCopy())
+		Certificates: func() []Certificate {
+			if ks.Certificates == nil {
+				return nil
 			}
-			return
+
+			return lo.Map(ks.Certificates, func(c Certificate, _ int) Certificate {
+				return c.SanitizedCopy()
+			})
 		}(),
 		CACertificates: ks.CACertificates,
 		Plugins:        ks.Plugins,
-		Consumers: func() (res []Consumer) {
-			for _, v := range ks.Consumers {
-				res = append(res, *v.SanitizedCopy(uuidGenerator))
+		Consumers: func() []Consumer {
+			if ks.Consumers == nil {
+				return nil
 			}
-			return
+
+			return lo.Map(ks.Consumers, func(c Consumer, _ int) Consumer {
+				return c.SanitizedCopy(uuidGenerator)
+			})
 		}(),
-		Licenses: func() (res []License) {
-			for _, v := range ks.Licenses {
-				res = append(res, *v.SanitizedCopy())
+		Licenses: func() []License {
+			if ks.Licenses == nil {
+				return nil
 			}
-			return
+			return lo.Map(ks.Licenses, func(l License, _ int) License {
+				return l.SanitizedCopy()
+			})
 		}(),
 		ConsumerGroups: ks.ConsumerGroups,
 		Vaults:         ks.Vaults,
diff --git a/internal/dataplane/kongstate/license.go b/internal/dataplane/kongstate/license.go
index 2d714cbb6..52b064da3 100644
--- a/internal/dataplane/kongstate/license.go
+++ b/internal/dataplane/kongstate/license.go
@@ -10,8 +10,8 @@ type License struct {
 }
 
 // SanitizedCopy returns a shallow copy with sensitive values redacted best-effort.
-func (l License) SanitizedCopy() *License {
-	return &License{
+func (l License) SanitizedCopy() License {
+	return License{
 		License: kong.License{
 			ID:        l.ID,
 			Payload:   redactedString,
diff --git a/internal/dataplane/kongstate/types.go b/internal/dataplane/kongstate/types.go
index fcfa7afe5..9c47856b8 100644
--- a/internal/dataplane/kongstate/types.go
+++ b/internal/dataplane/kongstate/types.go
@@ -54,8 +54,8 @@ type Certificate struct {
 }
 
 // SanitizedCopy returns a shallow copy with sensitive values redacted best-effort.
-func (c *Certificate) SanitizedCopy() *Certificate {
-	return &Certificate{
+func (c *Certificate) SanitizedCopy() Certificate {
+	return Certificate{
 		kong.Certificate{
 			ID:        c.ID,
 			Cert:      c.Cert,


Test patch:
diff --git a/internal/dataplane/kongstate/consumer_test.go b/internal/dataplane/kongstate/consumer_test.go
index 8933b9de3..b45e51f07 100644
--- a/internal/dataplane/kongstate/consumer_test.go
+++ b/internal/dataplane/kongstate/consumer_test.go
@@ -65,7 +65,7 @@ func TestConsumer_SanitizedCopy(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			got := *tt.in.SanitizedCopy(mocks.StaticUUIDGenerator{UUID: "52fdfc07-2182-454f-963f-5f0f9a621d72"})
+			got := tt.in.SanitizedCopy(mocks.StaticUUIDGenerator{UUID: "52fdfc07-2182-454f-963f-5f0f9a621d72"})
 			assert.Equal(t, tt.want, got)
 		})
 	}
diff --git a/internal/dataplane/kongstate/kongstate_test.go b/internal/dataplane/kongstate/kongstate_test.go
index 23c532e87..4f964243e 100644
--- a/internal/dataplane/kongstate/kongstate_test.go
+++ b/internal/dataplane/kongstate/kongstate_test.go
@@ -147,6 +147,46 @@ func TestKongState_SanitizedCopy(t *testing.T) {
 	ensureAllKongStateFieldsAreCoveredInTest(t, testedFields.UnsortedList())
 }
 
+func BenchmarkSanitizedCopy(b *testing.B) {
+	const count = 1000
+	ks := KongState{
+		Certificates: func() []Certificate {
+			certificates := make([]Certificate, 0, count)
+			for i := 0; i < count; i++ {
+				certificates = append(certificates,
+					Certificate{kong.Certificate{ID: kong.String(strconv.Itoa(i)), Key: kong.String("secret")}},
+				)
+			}
+			return certificates
+		}(),
+		Consumers: func() []Consumer {
+			consumers := make([]Consumer, 0, count)
+			for i := 0; i < count; i++ {
+				consumers = append(consumers,
+					Consumer{
+						Consumer: kong.Consumer{ID: kong.String(strconv.Itoa(i))},
+					},
+				)
+			}
+			return consumers
+		}(),
+		Licenses: func() []License {
+			licenses := make([]License, 0, count)
+			for i := 0; i < count; i++ {
+				licenses = append(licenses,
+					License{kong.License{ID: kong.String(strconv.Itoa(i)), Payload: kong.String("secret")}},
+				)
+			}
+			return licenses
+		}(),
+	}
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		ret := ks.SanitizedCopy(mocks.StaticUUIDGenerator{UUID: "52fdfc07-2182-454f-963f-5f0f9a621d72"})
+		_ = ret
+	}
+}
+
 // extractNotEmptyFieldNames returns the names of all non-empty fields in the given KongState.
 // This is to programmatically find out what fields are used in a test case.
 func extractNotEmptyFieldNames(s KongState) []string {
diff --git a/internal/dataplane/kongstate/types_test.go b/internal/dataplane/kongstate/types_test.go
index adb201c2e..bf74b9ba5 100644
--- a/internal/dataplane/kongstate/types_test.go
+++ b/internal/dataplane/kongstate/types_test.go
@@ -34,7 +34,7 @@ func TestCertificate_SanitizedCopy(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			got := *tt.in.SanitizedCopy()
+			got := tt.in.SanitizedCopy()
 			assert.Equal(t, tt.want, got)
 		})
 	}