Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(utils): add ClearUnmatchingDeprecations function to align configs #473

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+915 −302
Draft

fix(utils): add ClearUnmatchingDeprecations function to align configs #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
280 changes: 135 additions & 145 deletions kong/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,76 +208,6 @@ func getConfigSchema(schema gjson.Result) (gjson.Result, error) {
return schema, fmt.Errorf("no 'config' field found in schema")
}

// traverseConfigMap recursively traverses a plugin config
// and returns the value at the specified path.
// The path is represented as a slice of strings, where each string is a key in the map.
//
// If the path is empty, nil is returned.
//
// If the path cannot be fully traversed (e.g., a non-existent key is encountered),
// this function returns nil and an appropriate error.
//
// This function can be helpful to fetch the nested config value from a backward translation
// path provided with deprecated fields.
//
// Example usage:
//
// configMap := map[string]interface{}{
// "foo": map[string]interface{}{
// "bar": 42,
// },
// }
// value, err := traverseConfigMap(configMap, []string{"foo", "bar"})
// // value comes 42 here
func traverseConfigMap(currentConfigMap map[string]interface{}, path []string) (interface{}, error) {
if len(path) == 0 {
return nil, nil
}

pathElement := path[0]
value, ok := currentConfigMap[pathElement]
if !ok {
return nil, fmt.Errorf("key %q not found in map", pathElement)
}

switch v := value.(type) {
case map[string]interface{}:
// Traversing the map recursively, dissecting the path each time
return traverseConfigMap(v, path[1:])
default:
return v, nil
}
}

// backfillResultConfigMap recursively traverses a nested Configuration struct
// and sets the value at the specified path to the provided configValue.
// The path is represented as a slice of strings, where each string is a key
// in the nested map[string]interface{} fields of the Configuration struct.
//
// If the path cannot be fully traversed (e.g., a non-existent key is encountered),
// this function returns an appropriate error.
//
// An example usage here is when for a plugin redis_port is changed, we can change
// redis.port from the config struct too.
func backfillResultConfigMap(res Configuration, path []string, configValue interface{}) error {
// Traverse the map to the second-to-last level
for i, p := range path {
if i == len(path)-1 {
// Last element in the path, update the value
res[p] = configValue
return nil
}
// Traverse to the next level
next, ok := res[p].(map[string]interface{})
if !ok {
return fmt.Errorf("backward_translation path %q incorrect", p)
}
res = next
}

return nil
}

type FillRecordOptions struct {
FillDefaults bool
FillAuto bool
Expand All @@ -288,7 +218,6 @@ func fillConfigRecord(schema gjson.Result, config Configuration, opts FillRecord
res := config.DeepCopy()
configFields := schema.Get("fields")
// Fetch deprecated fields
shortHandFields := schema.Get("shorthand_fields")
defaultRecordValue := schema.Get("default")

configFields.ForEach(func(_, value gjson.Result) bool {
Expand Down Expand Up @@ -423,80 +352,6 @@ func fillConfigRecord(schema gjson.Result, config Configuration, opts FillRecord
return true
})

// Filling defaults for deprecated fields
// Required for deck sync/diff inorder
// Otherwise, users keep seeing updates in these fields despite of no change
shortHandFields.ForEach(func(_, value gjson.Result) bool {
ms := value.Map()
fname := ""
for k := range ms {
fname = k
break
}

var deprecatedFieldValue interface{}

// check if key is already set in the config
if v, ok := config[fname]; ok {
if v != nil {
// This config's value should be retained.
// Also, the result config 'res' may have a different value for some nested fields than this.
// As per current conventions, shorthand fields take priority when different values are present
// in equivalent shorthand configs and normal nested configs.
// Backfilling nested configs to reduce inconsistencies.
deprecatedFieldValue = v
}
}

// Using path provided in backwards translation to get
// the defaults for deprecated fields from the already formed default config
backwardTranslation := value.Get(fname + ".translate_backwards")

if !backwardTranslation.Exists() {
// Checking for replaced_with path if it exists in the deprecation block
var replacePath gjson.Result
replacedWith := value.Get(fname + ".deprecation.replaced_with")
if replacedWith.IsArray() {
for _, item := range replacedWith.Array() {
if pathArray := item.Get("path"); pathArray.Exists() && pathArray.IsArray() {
replacePath = pathArray
}
}
}

if !replacePath.Exists() {
// This block attempts to fill defaults for deprecated fields.
// Thus, not erroring out here, as it is not vital.
return true
}

backwardTranslation = replacePath
}

configPathForBackwardTranslation := make([]string, 0, len(backwardTranslation.Array()))
for _, value := range backwardTranslation.Array() {
configPathForBackwardTranslation = append(configPathForBackwardTranslation, value.Str)
}

if deprecatedFieldValue != nil {
// This block attempts to fill defaults for deprecated fields.
// Thus, not erroring out here, as it is not vital.
_ = backfillResultConfigMap(res, configPathForBackwardTranslation, deprecatedFieldValue)
return true
}

configValue, err := traverseConfigMap(res, configPathForBackwardTranslation)
if err != nil {
// This block attempts to fill defaults for deprecated fields.
// Thus, not erroring out here, as it is not vital.
return true
}

res[fname] = configValue

return true
})

return res
}

Expand Down Expand Up @@ -741,3 +596,138 @@ func FillPluginsDefaults(plugin *Plugin, schema Schema) error {
func FillPluginsDefaultsWithOpts(plugin *Plugin, schema map[string]interface{}, opts FillRecordOptions) error {
return fillConfigRecordDefaultsAutoFields(plugin, schema, opts)
}

func deleteAndCollapseMap(config map[string]interface{}, path []string) {
key := path[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check for the possibility of an empty path and empty map before this to avoid panics during runtime?

if len(path) == 1 {
delete(config, key)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can return after this delete to avoid the else block.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... does it matter 🤔 ? I mean - it won't enter else block and having a return right after this line doesn't change anything right? Like there won't be any other instruction either way.

Oh... maybe you mean like:

if condition {
  return do_something()
}
// here would be the else part?

?
I'm not sure I fully understand 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant something like this:

  if condition {
    // do something
    return
  }
  
  // rest of the code, no else

This helps with readability as well.

} else {
if nested, ok := config[key].(map[string]interface{}); ok {
deleteAndCollapseMap(nested, path[1:])
if len(nested) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, when can this case arise?

delete(config, key)
}
}
}
}

type pathExistsInConfigOptions struct {
acceptNullValue bool
}

func pathExistsInConfig(config map[string]interface{}, path []string, opts pathExistsInConfigOptions) bool {
key := path[0]
if len(path) == 1 {
value, ok := config[key]
if opts.acceptNullValue {
return ok
}

return value != nil
} else if nested, ok := config[key].(map[string]interface{}); ok {
return pathExistsInConfig(nested, path[1:], opts)
}

return false
}

func clearUnmatchingDeprecationsHelper(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add comments over the functions to explain what they do and in which scenarios they may be useful?

newPluginConfig Configuration,
oldPluginConfig Configuration,
schema *gjson.Result,
) {
configFields := schema.Get("fields")
// Fetch deprecated fields
shortHandFields := schema.Get("shorthand_fields")

shortHandFields.ForEach(func(_, value gjson.Result) bool {
field := value.Map()
for deprecatedFieldName, shorthandFieldConfig := range field {
if deprecatedFieldValue, ok := newPluginConfig[deprecatedFieldName]; ok {
// deprecatedFieldName is used in new plugin configuration
// verify if the fields that this depractedField is replaced with
// are also sent in new plugin configuration - if not clear them from old plugin configuration
replacements := shorthandFieldConfig.Get("deprecation.replaced_with.#.path")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backward compatibility, can we also use "translate_backwards" here? Otherwise, for older gateway versions where schema doesn't have "replaced_with", this won't work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I haven't figured out how to deal with backwards compatibility yet 🤔 but that's a good point.

replacements.ForEach(func(_, value gjson.Result) bool {
replacementPathAsStringArray := make([]string, len(value.Array()))
for i, pathSegment := range value.Array() {
replacementPathAsStringArray[i] = pathSegment.String()
}

// We know that deprecated value is defined in new config and we also have information
// on how this deprecated value is replaced. If the new plugin configuration contains
// both old and new values we don't need to adjust old plugin configuration.
// However if the new key is missing in new plugin configuration then we need to
// delete it from old plugin configuration in order for them to match.
// --
// There is also a possibility that the new field exists in new config but only because
// it was filled with `null` value due to decK logic of filling empty keys. In that case
// if the deprecated value is different from nil then we need to clear it both in new and old config.
acceptNullValue := deprecatedFieldValue == nil
if !pathExistsInConfig(newPluginConfig,
replacementPathAsStringArray,
pathExistsInConfigOptions{acceptNullValue: acceptNullValue},
) {
if !acceptNullValue {
deleteAndCollapseMap(newPluginConfig, replacementPathAsStringArray)
}
deleteAndCollapseMap(oldPluginConfig, replacementPathAsStringArray)
}

return true
})

} else {
// Here the opposite is true - the new plugin configuration does not contain deprecated fields
// however for backwards compatibility Kong sends deprecated fields as well in the response.
// Now in order to make diffs the same we need to delete those deprecated fields from the old plugin
// configuration that Kong sent us.
delete(oldPluginConfig, deprecatedFieldName)
}
}

return true
})

configFields.ForEach(func(_, value gjson.Result) bool {
field := value.Map()

for fieldName, fieldConfig := range field {
if fieldType := fieldConfig.Get("type"); fieldType.String() == "record" {
var nestedNewPluginConfig map[string]interface{}
if f, ok := newPluginConfig[fieldName].(map[string]interface{}); ok {
nestedNewPluginConfig = f
}

var nestedOldPluginConfig map[string]interface{}
if f, ok := oldPluginConfig[fieldName].(map[string]interface{}); ok {
nestedOldPluginConfig = f
}

if nestedNewPluginConfig != nil && nestedOldPluginConfig != nil {
clearUnmatchingDeprecationsHelper(nestedNewPluginConfig, nestedOldPluginConfig, &fieldConfig)
}
}
}

return true
})
}

func ClearUnmatchingDeprecations(newPlugin *Plugin, oldPlugin *Plugin, schema map[string]interface{}) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are we supposed to be using this function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a PR that'll be using this: https://github.com/Kong/go-database-reconciler/pull/145/files

For testing purposes I've changed mine go.mod to:

replace github.com/kong/go-kong => <<my local path>>/go-kong and the tests in tests/integration/diff_test.go are passing.

jsonb, err := json.Marshal(&schema)
if err != nil {
return err
}
gjsonSchema := gjson.ParseBytes((jsonb))
configSchema, err := getConfigSchema(gjsonSchema)
if err != nil {
return err
}

if newPlugin != nil && oldPlugin != nil {
clearUnmatchingDeprecationsHelper(newPlugin.Config, oldPlugin.Config, &configSchema)
}

return nil
}
Loading
Loading