fix(diff+file): avoid filling defaults in config for kong (#133)

* fix(diff+file): avoid filling defaults in config for kong the previous logic was filling defaults in the configuration that was passed to Kong. This was problematic, especially where nils were populated as defaults, e.g. if a shorthand_field was passed with some value and the corresponding new field is auto-populated as `nil` by decK , the auto-populated nil value would take precedence in Kong thus causing the shorthand_field to be ignored (https://konghq.atlassian.net/browse/KAG-5157). This change applies the default values only to configurations used for diff, the original configuration is always passed to Kong as is. * Test/avoid filling defaults in config for kong (#135) * add tests for plugin filling default values --------- Co-authored-by: samugi <[email protected]> --------- Co-authored-by: Tao Yi <[email protected]>
Kong · Aug 27, 2024 · e72f4c2 · e72f4c2
1 parent 64dd801
commit e72f4c2
Show file tree

Hide file tree

Showing 7 changed files with 235 additions and 56 deletions.
diff --git a/go.mod b/go.mod
@@ -20,6 +20,7 @@ require (
 	github.com/hexops/gotextdiff v1.0.3
 	github.com/kong/deck v1.34.0
 	github.com/kong/go-kong v0.55.0
+	github.com/samber/lo v1.47.0
 	github.com/shirou/gopsutil/v3 v3.24.5
 	github.com/ssgelm/cookiejarparser v1.0.1
 	github.com/stretchr/testify v1.9.0
@@ -115,11 +116,11 @@ require (
 	github.com/yusufpapurcu/wmi v1.2.4 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa // indirect
-	golang.org/x/mod v0.14.0 // indirect
-	golang.org/x/net v0.23.0 // indirect
+	golang.org/x/mod v0.17.0 // indirect
+	golang.org/x/net v0.25.0 // indirect
 	golang.org/x/sys v0.23.0 // indirect
-	golang.org/x/text v0.14.0 // indirect
-	golang.org/x/tools v0.16.1 // indirect
+	golang.org/x/text v0.16.0 // indirect
+	golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect
 	google.golang.org/protobuf v1.33.0 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect

diff --git a/go.sum b/go.sum
@@ -282,6 +282,8 @@ github.com/sagikazarmark/locafero v0.4.0 h1:HApY1R9zGo4DBgr7dqsTH/JJxLTTsOt7u6ke
 github.com/sagikazarmark/locafero v0.4.0/go.mod h1:Pe1W6UlPYUk/+wc/6KFhbORCfqzgYEpgQ3O5fPuL3H4=
 github.com/sagikazarmark/slog-shim v0.1.0 h1:diDBnUNK9N/354PgrxMywXnAwEr1QZcOr6gto+ugjYE=
 github.com/sagikazarmark/slog-shim v0.1.0/go.mod h1:SrcSrq8aKtyuqEI1uvTDTK1arOWRIczQRv+GVI1AkeQ=
+github.com/samber/lo v1.47.0 h1:z7RynLwP5nbyRscyvcD043DWYoOcYRv3mV8lBeqOCLc=
+github.com/samber/lo v1.47.0/go.mod h1:RmDH9Ct32Qy3gduHQuKJ3gW1fMHAnE/fAzQuf6He5cU=
 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 h1:lZUw3E0/J3roVtGQ+SCrUrg3ON6NgVqpn3+iol9aGu4=
 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1/go.mod h1:uToXkOrWAZ6/Oc07xWQrPOhJotwFIyu2bBVN41fcDUY=
 github.com/sergi/go-diff v1.1.0/go.mod h1:STckp+ISIX8hZLjrqAeVduY0gWCT9IjLuqbuNXdaHfM=
@@ -364,16 +366,16 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
-golang.org/x/crypto v0.21.0 h1:X31++rzVUdKhX5sWmSOFZxx8UW/ldWx55cbf08iNAMA=
-golang.org/x/crypto v0.21.0/go.mod h1:0BP7YvVV9gBbVKyeTG0Gyn+gZm94bibOW5BjDEYAOMs=
+golang.org/x/crypto v0.23.0 h1:dIJU/v2J8Mdglj/8rJ6UUOM3Zc9zLZxVZwwxMooUSAI=
+golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8=
 golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa h1:FRnLl4eNAQl8hwxVVC17teOw8kdjVDVAiFMtgUdTSRQ=
 golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa/go.mod h1:zk2irFbV9DP96SEBUUAy67IdHUaZuSnrz1n472HUCLE=
 golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
 golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
-golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0=
-golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
+golang.org/x/mod v0.17.0 h1:zY54UmvipHiNd+pm+m0x9KhZ9hl1/7QNMyxXbc6ICqA=
+golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
 golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
 golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
@@ -386,8 +388,8 @@ golang.org/x/net v0.0.0-20210428140749-89ef3d95e781/go.mod h1:OJAsFXCWl8Ukc7SiCT
 golang.org/x/net v0.0.0-20220225172249-27dd8689420f/go.mod h1:CfG3xpIq0wQ8r1q4Su4UZFWDARRcnwPjda9FqA0JpMk=
 golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
 golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
-golang.org/x/net v0.23.0 h1:7EYJ93RZ9vYSZAIb2x3lnuvqO5zneoD6IvWjuhfxjTs=
-golang.org/x/net v0.23.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
+golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac=
+golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM=
 golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
@@ -440,16 +442,16 @@ golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
 golang.org/x/text v0.3.8/go.mod h1:E6s5w1FMmriuDzIBO73fBruAKo1PCIq6d2Q6DHfQ8WQ=
 golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8=
-golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ=
-golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
+golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
+golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.0.0-20200505023115-26f46d2f7ef8/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/tools v0.0.0-20201224043029-2b0845dc783e/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
 golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU=
-golang.org/x/tools v0.16.1 h1:TLyB3WofjdOEepBHAU20JdNC1Zbg87elYofWYAY5oZA=
-golang.org/x/tools v0.16.1/go.mod h1:kYVVN6I1mBNoB1OX+noeBjbRk4IUEPa7JJ+TJMEooJ0=
+golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d h1:vU5i/LfpvrRCpgM/VPfJLg5KjxD3E+hfT1SH+d9zLwg=
+golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go
@@ -599,6 +599,36 @@ func (sc *Syncer) Solve(ctx context.Context, parallelism int, dry bool, isJSONOu
 		var err error
 		var result crud.Arg
 
+		// This variable holds the original event with the unchanged configuration
+		// Below the configuration in `e` may be modified. This is done solely for
+		// the purpose of displaying a correct diff and should not affect the
+		// configuration that is sent to Kong.
+		originalE := e
+
+		// If the event is for a plugin, inject defaults in the plugin's config
+		// that will be used for the diff. This is needed to avoid highlighting
+		// default values that were populated by Kong as differences.
+		if plugin, ok := e.Obj.(*state.Plugin); ok {
+			pluginCopy := &state.Plugin{
+				Plugin: *plugin.DeepCopy(),
+				Meta:   plugin.Meta,
+			}
+			e.Obj = pluginCopy
+
+			exists, err := utils.WorkspaceExists(ctx, sc.kongClient)
+			if err == nil && exists {
+				var schema map[string]interface{}
+				schema, err = sc.kongClient.Plugins.GetFullSchema(ctx, pluginCopy.Plugin.Name)
+				if err != nil {
+					return nil, err
+				}
+
+				if err := kong.FillPluginsDefaults(&pluginCopy.Plugin, schema); err != nil {
+					return nil, fmt.Errorf("failed filling plugin defaults: %w", err)
+				}
+			}
+		}
+
 		c := e.Obj.(state.ConsoleString)
 		objDiff := map[string]interface{}{
 			"old": e.OldObj,
@@ -680,7 +710,7 @@ func (sc *Syncer) Solve(ctx context.Context, parallelism int, dry bool, isJSONOu
 		if !dry {
 			// sync mode
 			// fire the request to Kong
-			result, err = sc.processor.Do(ctx, e.Kind, e.Op, e)
+			result, err = sc.processor.Do(ctx, originalE.Kind, originalE.Op, originalE)
 			// TODO https://github.com/Kong/go-database-reconciler/issues/22 this does not print, but is switched on
 			// sc.enableEntityActions because the existing behavior returns a result from the anon Run function.
 			// Refactoring should use only the channel and simplify the return, probably to just an error (all the other

diff --git a/pkg/file/builder.go b/pkg/file/builder.go
@@ -1443,44 +1443,6 @@ func (b *stateBuilder) ingestRoute(r FRoute) error {
 	return nil
 }
 
-func (b *stateBuilder) getPluginSchema(pluginName string) (map[string]interface{}, error) {
-	var schema map[string]interface{}
-
-	// lookup in cache
-	if schema, ok := b.schemasCache[pluginName]; ok {
-		return schema, nil
-	}
-
-	exists, err := utils.WorkspaceExists(b.ctx, b.client)
-	if err != nil {
-		return nil, fmt.Errorf("ensure workspace exists: %w", err)
-	}
-	if !exists {
-		return schema, ErrWorkspaceNotFound
-	}
-
-	schema, err = b.client.Plugins.GetFullSchema(b.ctx, &pluginName)
-	if err != nil {
-		return schema, err
-	}
-	b.schemasCache[pluginName] = schema
-	return schema, nil
-}
-
-func (b *stateBuilder) addPluginDefaults(plugin *FPlugin) error {
-	if b.client == nil {
-		return nil
-	}
-	schema, err := b.getPluginSchema(*plugin.Name)
-	if err != nil {
-		if errors.Is(err, ErrWorkspaceNotFound) {
-			return nil
-		}
-		return fmt.Errorf("retrieve schema for %v from Kong: %w", *plugin.Name, err)
-	}
-	return kong.FillPluginsDefaults(&plugin.Plugin, schema)
-}
-
 func (b *stateBuilder) ingestPlugins(plugins []FPlugin) error {
 	for _, p := range plugins {
 		p := p
@@ -1504,9 +1466,6 @@ func (b *stateBuilder) ingestPlugins(plugins []FPlugin) error {
 		if err != nil {
 			return err
 		}
-		if err := b.addPluginDefaults(&p); err != nil {
-			return fmt.Errorf("add defaults to plugin '%v': %w", *p.Name, err)
-		}
 		utils.MustMergeTags(&p, b.selectTags)
 		if plugin != nil {
 			p.Plugin.CreatedAt = plugin.CreatedAt

diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go
@@ -7,9 +7,13 @@ import (
 	"crypto/sha1"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"os"
 	"strings"
 	"testing"
@@ -19,8 +23,10 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	deckDiff "github.com/kong/go-database-reconciler/pkg/diff"
 	deckDump "github.com/kong/go-database-reconciler/pkg/dump"
+	"github.com/kong/go-database-reconciler/pkg/state"
 	"github.com/kong/go-database-reconciler/pkg/utils"
 	"github.com/kong/go-kong/kong"
+	"github.com/samber/lo"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -5522,3 +5528,102 @@ func TestSync_License(t *testing.T) {
 		require.Empty(t, licenses)
 	})
 }
+
+func Test_Sync_PluginDoNotFillDefaults(t *testing.T) {
+
+	client, err := getTestClient()
+
+	require.NoError(t, err)
+	ctx := context.Background()
+	t.Run("empty_fields_of_plugin_config", func(t *testing.T) {
+		mustResetKongState(ctx, t, client, deckDump.Config{})
+
+		currrentState, err := fetchCurrentState(ctx, client, deckDump.Config{})
+		require.NoError(t, err)
+		targetState := stateFromFile(ctx, t,
+			"testdata/sync/033-plugin-with-empty-fields/kong.yaml",
+			client,
+			deckDump.Config{},
+		)
+
+		kongURL, err := url.Parse(client.BaseRootURL())
+		require.NoError(t, err)
+		p := NewRecordRequestProxy(kongURL)
+		s := httptest.NewServer(p)
+		c, err := utils.GetKongClient(utils.KongClientConfig{
+			Address: s.URL,
+		})
+		require.NoError(t, err)
+
+		syncer, err := deckDiff.NewSyncer(deckDiff.SyncerOpts{
+			CurrentState: currrentState,
+			TargetState:  targetState,
+
+			KongClient: c,
+		})
+		stats, errs, changes := syncer.Solve(ctx, 1, false, true)
+		require.Empty(t, errs, "Should have no errors in syncing")
+		require.NoError(t, err)
+
+		require.Equal(t, int32(1), stats.CreateOps.Count(), "Should create 1 entity")
+		require.Len(t, changes.Creating, 1, "Should have 1 creating record in changes")
+
+		// The change records which are returned in `diff` command should fill default values.
+		t.Run("should fill default values in change records", func(t *testing.T) {
+			body, ok := changes.Creating[0].Body.(map[string]any)
+			require.True(t, ok)
+			plugin, ok := body["new"].(*state.Plugin)
+			require.True(t, ok)
+
+			path, ok := plugin.Config["path"]
+			require.True(t, ok)
+			require.Equal(t, "/tmp/file.log", path, "path should be same as specified in file")
+
+			reopen, ok := plugin.Config["reopen"]
+			require.True(t, ok, "'reopen' field should be filled")
+			require.Equal(t, false, reopen, "should be the same as default value")
+
+			custom_fields_by_lua, ok := plugin.Config["custom_fields_by_lua"]
+			require.True(t, ok, "'custom_fields_by_lua' field should be filled")
+			require.Nil(t, custom_fields_by_lua, "should be an explicit nil")
+		})
+
+		// But the default values should not be filled in request sent to Kong.
+		t.Run("should not fill default values in requests sent to Kong", func(t *testing.T) {
+			reqs := p.dumpRequests()
+			req, found := lo.Find(reqs, func(r *http.Request) bool {
+				return r.Method == "PUT" && strings.Contains(r.URL.Path, "/plugins")
+			})
+			require.True(t, found, "Should find request to create plugin")
+			buf, err := io.ReadAll(req.Body)
+			require.NoError(t, err, "Should read request body from record")
+			plugin := state.Plugin{}
+			err = json.Unmarshal(buf, &plugin)
+			require.NoError(t, err, "Should unmarshal request body to plugin type")
+
+			path, ok := plugin.Config["path"]
+			require.True(t, ok)
+			require.Equal(t, "/tmp/file.log", path, "path should be same as specified in file")
+
+			_, ok = plugin.Config["reopen"]
+			require.False(t, ok, "'reopen' field should not be filled")
+
+			_, ok = plugin.Config["custom_fields_by_lua"]
+			require.False(t, ok, "'custom_fields_by_lua' field should not be filled")
+		})
+
+		// Should update Kong state successfully.
+		t.Run("Should get the plugin config from update Kong", func(t *testing.T) {
+			newState, err := fetchCurrentState(ctx, client, deckDump.Config{})
+			require.NoError(t, err)
+			plugins, err := newState.Plugins.GetAll()
+			require.NoError(t, err)
+			require.Len(t, plugins, 1)
+			plugin := plugins[0]
+			require.Equal(t, "file-log", *plugin.Name)
+			path, ok := plugin.Config["path"]
+			require.True(t, ok)
+			require.Equal(t, "/tmp/file.log", path, "path should be same as specified in file")
+		})
+	})
+}