From 04f9441275539dabcd89b62a4b84b05366c3a933 Mon Sep 17 00:00:00 2001 From: Gabriele Gerbino Date: Tue, 17 Oct 2023 11:50:19 +0200 Subject: [PATCH 1/2] fix: don't overwrite fields set to empty arrays when default exists This commit fixes a bug preventing users to set fields to emtpy arrays when a default for those fields exist. For example, imagine we have an `openid-connect` plugin configured via the UI having the `login_tokens` set to an empty array (note: this field has a default value of `["id_token"]`: ``` $ http :8001/plugins/$id | jq .config.login_tokens [] ``` If we do a `deck dump`, the configuration will be reflected into a local state file: ``` $ deck dump --yes $ cat kong.yaml | grep login_tokens login_tokens: [] ``` But, if we now run `deck diff` we see that decK will detect a difference because of the `login_tokens` field's default value: ``` $ deck diff updating plugin openid-connect (global) { "config": { "anonymous": null, ... ... "login_redirect_mode": "fragment", "login_redirect_uri": null, "login_tokens": [ + "id_token" ], "logout_methods": [ "POST", "DELETE" ], ... ... } Summary: Created: 0 Updated: 1 Deleted: 0 ``` This commit corrects this defect by allowing decK to set empty arrays when default values exist for a given field. --- kong/utils.go | 5 +- kong/utils_test.go | 133 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 3 deletions(-) diff --git a/kong/utils.go b/kong/utils.go index 3b1b2f200..eef5f5ad3 100644 --- a/kong/utils.go +++ b/kong/utils.go @@ -236,9 +236,8 @@ func fillConfigRecord(schema gjson.Result, config Configuration) Configuration { } case []interface{}: if value.Get(fname).Get("elements.type").String() != "record" && - config[fname] != nil && - len(config[fname].([]interface{})) > 0 { - // Non empty array with elements which are not of type record + config[fname] != nil { + // Non nil array with elements which are not of type record // this means field is already set return true } diff --git a/kong/utils_test.go b/kong/utils_test.go index a4fde4505..b1e738513 100644 --- a/kong/utils_test.go +++ b/kong/utils_test.go @@ -2212,3 +2212,136 @@ func Test_FillPluginsDefaults_Acme(t *testing.T) { }) } } + +const NonEmptyDefaultArrayFieldSchema = `{ + "fields": [ + { + "protocols": { + "default": [ + "grpc", + "grpcs", + "http", + "https" + ], + "elements": { + "len_min": 1, + "one_of": [ + "grpc", + "grpcs", + "http", + "https" + ], + "required": true, + "type": "string" + }, + "required": true, + "type": "set" + } + }, + { + "config": { + "fields": [ + { + "issuer": { + "required": true, + "type": "string" + } + }, + { + "login_tokens": { + "default": [ + "id_token" + ], + "elements": { + "one_of": [ + "id_token", + "access_token", + "refresh_token", + "tokens", + "introspection" + ], + "type": "string" + }, + "required": false, + "type": "array" + } + } + ], + "type": "record" + } + } + ] +}` + +func Test_FillPluginsDefaults_NonEmptyDefaultArrayField(t *testing.T) { + client, err := NewTestClient(nil, nil) + require.NoError(t, err) + require.NotNil(t, client) + + tests := []struct { + name string + plugin *Plugin + expected *Plugin + }{ + { + name: "not setting login_tokens should be overwritten by default value", + plugin: &Plugin{ + Config: Configuration{ + "issuer": "https://accounts.google.com", + }, + }, + expected: &Plugin{ + Config: Configuration{ + "issuer": "https://accounts.google.com", + "login_tokens": []any{"id_token"}, + }, + }, + }, + { + name: "setting empty array for login_tokens should not be overwritten by default value", + plugin: &Plugin{ + Config: Configuration{ + "issuer": "https://accounts.google.com", + "login_tokens": []any{}, + }, + }, + expected: &Plugin{ + Config: Configuration{ + "issuer": "https://accounts.google.com", + "login_tokens": []any{}, + }, + }, + }, + { + name: "setting non-empty login_tokens should not be overwritten by default value", + plugin: &Plugin{ + Config: Configuration{ + "issuer": "https://accounts.google.com", + "login_tokens": []any{"access_token", "refresh_token"}, + }, + }, + expected: &Plugin{ + Config: Configuration{ + "issuer": "https://accounts.google.com", + "login_tokens": []any{"access_token", "refresh_token"}, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + plugin := tc.plugin + var fullSchema map[string]interface{} + err := json.Unmarshal([]byte(NonEmptyDefaultArrayFieldSchema), &fullSchema) + + require.NoError(t, err) + require.NotNil(t, fullSchema) + assert.NoError(t, FillPluginsDefaults(plugin, fullSchema)) + opts := cmpopts.IgnoreFields(*plugin, "Enabled", "Protocols") + if diff := cmp.Diff(plugin, tc.expected, opts); diff != "" { + t.Errorf(diff) + } + }) + } +} From 1457275918c46a4af281c94234fef385b089ea18 Mon Sep 17 00:00:00 2001 From: Gabriele Gerbino Date: Tue, 17 Oct 2023 17:54:32 +0200 Subject: [PATCH 2/2] addressing comments --- CHANGELOG.md | 4 ++++ kong/utils_test.go | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7fa01d6c..02ed24f84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,10 @@ ## Unreleased +- Fix a bug preventing users to set fields to emtpy arrays when + a default for those fields exist. + [#374](https://github.com/Kong/go-kong/pull/374) + ## [v0.47.0] > Release date: 2023/08/29 diff --git a/kong/utils_test.go b/kong/utils_test.go index b1e738513..4546d58a2 100644 --- a/kong/utils_test.go +++ b/kong/utils_test.go @@ -2330,12 +2330,12 @@ func Test_FillPluginsDefaults_NonEmptyDefaultArrayField(t *testing.T) { } for _, tc := range tests { + tc := tc t.Run(tc.name, func(t *testing.T) { plugin := tc.plugin var fullSchema map[string]interface{} - err := json.Unmarshal([]byte(NonEmptyDefaultArrayFieldSchema), &fullSchema) + require.NoError(t, json.Unmarshal([]byte(NonEmptyDefaultArrayFieldSchema), &fullSchema)) - require.NoError(t, err) require.NotNil(t, fullSchema) assert.NoError(t, FillPluginsDefaults(plugin, fullSchema)) opts := cmpopts.IgnoreFields(*plugin, "Enabled", "Protocols")