From b2ec5a704edc43edaecafefb46613168fa0dc5e8 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 11:19:45 -0400 Subject: [PATCH 1/8] feat: add a log stifling util to sendconfig updates --- pkg/sendconfig/common_workflows.go | 4 ++++ pkg/sendconfig/sendconfig.go | 31 ++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/pkg/sendconfig/common_workflows.go b/pkg/sendconfig/common_workflows.go index d78c563eba..48130ec77b 100644 --- a/pkg/sendconfig/common_workflows.go +++ b/pkg/sendconfig/common_workflows.go @@ -11,6 +11,10 @@ import ( "github.com/kong/kubernetes-ingress-controller/pkg/store" ) +// ----------------------------------------------------------------------------- +// Sendconfig - Workflow Functions +// ----------------------------------------------------------------------------- + // UpdateKongAdminSimple is a helper function for the most common usage of PerformUpdate() with only minimal // upfront configuration required. This function is specialized and highly opinionated. // diff --git a/pkg/sendconfig/sendconfig.go b/pkg/sendconfig/sendconfig.go index 3fa9dfba2d..5561028b7a 100644 --- a/pkg/sendconfig/sendconfig.go +++ b/pkg/sendconfig/sendconfig.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "reflect" + "sync" "github.com/kong/deck/diff" "github.com/kong/deck/dump" @@ -33,7 +34,6 @@ func PerformUpdate(ctx context.Context, customEntities []byte, oldSHA []byte, ) ([]byte, error) { - newSHA, err := deckgen.GenerateSHA(targetContent, customEntities) if err != nil { return oldSHA, err @@ -42,7 +42,9 @@ func PerformUpdate(ctx context.Context, if !reverseSync { // use the previous SHA to determine whether or not to perform an update if equalSHA(oldSHA, newSHA) { - log.Info("no configuration change, skipping sync to kong") + if !hasUpdateAlreadyBeenReported(newSHA) { + log.Info("no configuration change, skipping sync to kong") + } return oldSHA, nil } } @@ -186,3 +188,28 @@ func onUpdateDBMode(ctx context.Context, } return nil } + +// ----------------------------------------------------------------------------- +// Sendconfig - Logging & Reporting Helper Functions +// ----------------------------------------------------------------------------- + +var ( + latestReportedSHA []byte + shaLock sync.RWMutex +) + +// hasUpdateAlreadyBeenReported is a helper function to allow sendconfig internals to be aware of the last logged/reported +// update to the Kong Admin API. Given the most recent update SHA, it will return true/false whether or not that SHA has previously +// been reported (logged, e.t.c.) so that the caller can make decisions such as staggering or stifling duplicate log lines. +// +// TODO: This is a bit of a hack for now to keep backwards compat, but in the future we might configure rolling this into +// some object/interface which has this functionality as an inherent behavior. +func hasUpdateAlreadyBeenReported(latestUpdateSHA []byte) bool { + shaLock.Lock() + defer shaLock.Unlock() + if equalSHA(latestReportedSHA, latestUpdateSHA) { + return true + } + latestReportedSHA = latestUpdateSHA + return false +} From 008d83564654d6d95217ef341034a657b9d07f2a Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 11:21:23 -0400 Subject: [PATCH 2/8] fix: increase DefaultSyncSeconds in my testing, DBLESS and Postgres mode have both shown problems with repetitive sub-second updates. This is something we're tracking as a potential bug elsewhere, but for now increasing the interval seems to make the most sense for stability, as testing has shown it reduces/removes the timing problems which we've encountered with subsecond interval updates. --- railgun/internal/proxy/proxy.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/railgun/internal/proxy/proxy.go b/railgun/internal/proxy/proxy.go index 4dd178ca31..73bc4da024 100644 --- a/railgun/internal/proxy/proxy.go +++ b/railgun/internal/proxy/proxy.go @@ -17,9 +17,7 @@ import ( const ( // DefaultSyncSeconds indicates the time.Duration (minimum) that will occur between // updates to the Kong Proxy Admin API when using the NewProxy() constructor. - // - // NOTE: this default was originally inherited from KIC v1.x. - DefaultSyncSeconds float32 = 0.3 + DefaultSyncSeconds float32 = 1.0 // DefaultObjectBufferSize is the number of client.Objects that the server will buffer // before it starts rejecting new objects while it processes the originals. From 98572912a95f8e86a5e7ea1dd16f751d7797bbc4 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 11:22:20 -0400 Subject: [PATCH 3/8] chore: reduce ingress wait time in integration tests Recent updates have made our controller faster and having such a long Ingress timeout doesn't make sense any longer: it just increasing the time it takes to reach a timeout condition on certain kinds of failures. --- railgun/test/integration/suite_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railgun/test/integration/suite_test.go b/railgun/test/integration/suite_test.go index b8baa59af0..a21db2b5f3 100644 --- a/railgun/test/integration/suite_test.go +++ b/railgun/test/integration/suite_test.go @@ -40,7 +40,7 @@ const ( waitTick = time.Second * 1 // ingressWait is the default amount of time to wait for any particular ingress resource to be provisioned. - ingressWait = time.Minute * 5 + ingressWait = time.Minute * 3 // httpcTimeout is the default client timeout for HTTP clients used in tests. httpcTimeout = time.Second * 3 From ae9038f8f6c4597f08d9378401819f92de4da544 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 11:29:13 -0400 Subject: [PATCH 4/8] docs: add changelog updates for sendconfig logging differences --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d86a7031fc..ba38d58c66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,11 @@ released and the release notes may change significantly before then. #### Under the hood +- the controller manager will no longer log multiple entries for `nil` updates + to the Kong Admin API. The result is that operators will no longer see multiple + "no configuration change, skipping sync to kong" entries for any single update, + instead it will only report this `nil` update scenario the first time it is + encountered for any particular SHA derived from the configuration contents. - the `--sync-rate-limit` is now deprecated in favor of `--sync-time-seconds`. This functionality no longer blocks goroutines until the provided number of seconds has passed to enforce rate limiting, now instead it configures a From cdfaf57a87aff4f0df489332fa71759509b4c9e7 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 11:34:09 -0400 Subject: [PATCH 5/8] fix: lint line len for docs --- pkg/sendconfig/sendconfig.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pkg/sendconfig/sendconfig.go b/pkg/sendconfig/sendconfig.go index 5561028b7a..233470ef6e 100644 --- a/pkg/sendconfig/sendconfig.go +++ b/pkg/sendconfig/sendconfig.go @@ -198,12 +198,17 @@ var ( shaLock sync.RWMutex ) -// hasUpdateAlreadyBeenReported is a helper function to allow sendconfig internals to be aware of the last logged/reported -// update to the Kong Admin API. Given the most recent update SHA, it will return true/false whether or not that SHA has previously -// been reported (logged, e.t.c.) so that the caller can make decisions such as staggering or stifling duplicate log lines. +// hasUpdateAlreadyBeenReported is a helper function to allow +// sendconfig internals to be aware of the last logged/reported +// update to the Kong Admin API. Given the most recent update SHA, +// it will return true/false whether or not that SHA has previously +// been reported (logged, e.t.c.) so that the caller can make +// decisions (such as staggering or stifling duplicate log lines). // -// TODO: This is a bit of a hack for now to keep backwards compat, but in the future we might configure rolling this into -// some object/interface which has this functionality as an inherent behavior. +// TODO: This is a bit of a hack for now to keep backwards compat, +// but in the future we might configure rolling this into +// some object/interface which has this functionality as an +// inherent behavior. func hasUpdateAlreadyBeenReported(latestUpdateSHA []byte) bool { shaLock.Lock() defer shaLock.Unlock() From 81e76be4197007e8d6f7468fa7ea0025b1eab89f Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 12:33:20 -0400 Subject: [PATCH 6/8] test: add test func for new update report util --- pkg/sendconfig/sendconfig_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/sendconfig/sendconfig_test.go b/pkg/sendconfig/sendconfig_test.go index 5758484619..a88bede302 100644 --- a/pkg/sendconfig/sendconfig_test.go +++ b/pkg/sendconfig/sendconfig_test.go @@ -7,6 +7,7 @@ import ( "github.com/kong/deck/file" "github.com/kong/go-kong/kong" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" ) func Test_renderConfigWithCustomEntities(t *testing.T) { @@ -116,3 +117,14 @@ func Test_renderConfigWithCustomEntities(t *testing.T) { }) } } + +func Test_updateReportingUtilities(t *testing.T) { + assert.False(t, hasUpdateAlreadyBeenReported([]byte("fake-sha"))) + assert.True(t, hasUpdateAlreadyBeenReported([]byte("fake-sha"))) + assert.False(t, hasUpdateAlreadyBeenReported([]byte("another-fake-sha"))) + assert.True(t, hasUpdateAlreadyBeenReported([]byte("another-fake-sha"))) + assert.False(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.True(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.True(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.True(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) +} From e798686b7c7127c9bd36dcc76e6397ee745aa0cf Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 12:42:27 -0400 Subject: [PATCH 7/8] Update railgun/internal/proxy/proxy.go --- railgun/internal/proxy/proxy.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/railgun/internal/proxy/proxy.go b/railgun/internal/proxy/proxy.go index 73bc4da024..35f660a1f8 100644 --- a/railgun/internal/proxy/proxy.go +++ b/railgun/internal/proxy/proxy.go @@ -17,6 +17,12 @@ import ( const ( // DefaultSyncSeconds indicates the time.Duration (minimum) that will occur between // updates to the Kong Proxy Admin API when using the NewProxy() constructor. + // this 1s default was based on local testing wherein it appeared sub-second updates + // to the Admin API could be problematic (or at least operate differently) based on + // which storage backend was in use (i.e. "dbless", "postgres"). This is a workaround + // for improvements we still need to investigate upstream. + // + // See Also: https://github.com/Kong/kubernetes-ingress-controller/issues/1398 DefaultSyncSeconds float32 = 1.0 // DefaultObjectBufferSize is the number of client.Objects that the server will buffer From 033906afd982b5a0556eae9c98d189d6e747d398 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Fri, 4 Jun 2021 14:00:13 -0400 Subject: [PATCH 8/8] chore: function naming cleanup for code rev --- pkg/sendconfig/sendconfig.go | 6 +++--- pkg/sendconfig/sendconfig_test.go | 16 ++++++++-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/sendconfig/sendconfig.go b/pkg/sendconfig/sendconfig.go index 233470ef6e..391f78c27c 100644 --- a/pkg/sendconfig/sendconfig.go +++ b/pkg/sendconfig/sendconfig.go @@ -42,7 +42,7 @@ func PerformUpdate(ctx context.Context, if !reverseSync { // use the previous SHA to determine whether or not to perform an update if equalSHA(oldSHA, newSHA) { - if !hasUpdateAlreadyBeenReported(newSHA) { + if !hasSHAUpdateAlreadyBeenReported(newSHA) { log.Info("no configuration change, skipping sync to kong") } return oldSHA, nil @@ -198,7 +198,7 @@ var ( shaLock sync.RWMutex ) -// hasUpdateAlreadyBeenReported is a helper function to allow +// hasSHAUpdateAlreadyBeenReported is a helper function to allow // sendconfig internals to be aware of the last logged/reported // update to the Kong Admin API. Given the most recent update SHA, // it will return true/false whether or not that SHA has previously @@ -209,7 +209,7 @@ var ( // but in the future we might configure rolling this into // some object/interface which has this functionality as an // inherent behavior. -func hasUpdateAlreadyBeenReported(latestUpdateSHA []byte) bool { +func hasSHAUpdateAlreadyBeenReported(latestUpdateSHA []byte) bool { shaLock.Lock() defer shaLock.Unlock() if equalSHA(latestReportedSHA, latestUpdateSHA) { diff --git a/pkg/sendconfig/sendconfig_test.go b/pkg/sendconfig/sendconfig_test.go index a88bede302..5c7b766c22 100644 --- a/pkg/sendconfig/sendconfig_test.go +++ b/pkg/sendconfig/sendconfig_test.go @@ -119,12 +119,12 @@ func Test_renderConfigWithCustomEntities(t *testing.T) { } func Test_updateReportingUtilities(t *testing.T) { - assert.False(t, hasUpdateAlreadyBeenReported([]byte("fake-sha"))) - assert.True(t, hasUpdateAlreadyBeenReported([]byte("fake-sha"))) - assert.False(t, hasUpdateAlreadyBeenReported([]byte("another-fake-sha"))) - assert.True(t, hasUpdateAlreadyBeenReported([]byte("another-fake-sha"))) - assert.False(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) - assert.True(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) - assert.True(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) - assert.True(t, hasUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.False(t, hasSHAUpdateAlreadyBeenReported([]byte("fake-sha"))) + assert.True(t, hasSHAUpdateAlreadyBeenReported([]byte("fake-sha"))) + assert.False(t, hasSHAUpdateAlreadyBeenReported([]byte("another-fake-sha"))) + assert.True(t, hasSHAUpdateAlreadyBeenReported([]byte("another-fake-sha"))) + assert.False(t, hasSHAUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.True(t, hasSHAUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.True(t, hasSHAUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) + assert.True(t, hasSHAUpdateAlreadyBeenReported([]byte("yet-another-fake-sha"))) }