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.

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

feat: implement --dedup-plugin-configs flag for dump #581

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

GGabriele
Copy link
Collaborator

_plugin_configs feature of decK allows users to abstract out configuration
of plugins that are used repeatedly and then reference them in
services/routes/consumers. This helps with de-duplication of configuration
code. While users would like to take advantage of this, most of them cannot
because de-duplicating the configuration in the first place is a cumbersome
and an error-prone process.

#341 (doc)

This implements such flag and feature for the dump command.

Test:
sync this file first (before.yaml.txt), having some shared plugins across different consumers, routes and services.

$ deck sync
<--omitted->
$ deck dump --yes --dedup-plugin-configs

$ deck sync
Summary:
  Created: 0
  Updated: 0
  Deleted: 0

Result: after.yaml.txt

@GGabriele GGabriele requested a review from a team as a code owner February 4, 2022 14:28
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

Good start.

A few corner cases to think about:

  • What if a plugin configuration is unique and is only present once in the entire state? The code shouldn't introduce an indirection (via plugin source name) in such a case.
  • What if a plugin has multiple parents at the same time? Does this logic hold correctly in such a case? I'm not sure either way - please give that some thought and we will certainly need tests around it.

dump/dump.go Outdated
// }
// }
func dedupPluginsConfig(state *utils.KongRawState, plugins []*kong.Plugin) {
state.SharedPluginMap = make(map[string]map[string]utils.SharedPlugin)
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend making this function unit-testable:

  • Take state and plugins as input
  • Return the "SharedPluginMap", and then set "state.SharedPluginMap" to the returned result in the caller's code.

Avoid changing input arguments in place - this codebase doesn't follow this consistently but it is for the best. I myself didn't know better before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored a bit

fp.ConfigSource = kong.String(name)
break
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggest two functions: one takes in the plugin map and the relation IDs, and returns the name of the plugin config source (if found), and this one then sets the config to nil and source to non-nil. This way you can unit-test the function that looks through sharedPlugins map.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored a bit

@GGabriele GGabriele force-pushed the dedup_plugins branch 5 times, most recently from dd3c2cd to 8ea7c1d Compare February 11, 2022 15:09
if p.Route != nil && p.Route.ID != nil {
sharedPlugin.Routes = append(sharedPlugin.Routes, *p.Route.ID)
}
return sharedPlugin
Copy link
Member

Choose a reason for hiding this comment

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

The tracking here loses the relationship of a plugin with multiple entities.
Consider the following plugin:

  plugins:
    name: rate-limiting
    service:
       id: svc-id
    consumer:
       id: consumer-id
    config:
      day: null
      fault_tolerant: true
      hide_client_headers: false
      hour: null
      limit_by: consumer
      minute: 10
      month: null
      policy: redis
      redis_database: 0
      redis_host: redis.common.svc
      redis_password: null
      redis_port: 6379
      redis_timeout: 2000
      second: null
      year: null
    enabled: true
    protocols:
    - http
    - https

In this case, this data structure would track the plugin as a plugin that is applied to the consumer, and the same plugin is applied to the service, in reality, that is not actually true, the plugin is present only once, on a combination of service and consumer.

The code that uses this data structure, later on, is structured in a way that one never runs into this bug currently. But that is a bit too risky to rely on. Please consider a data structure that captures the entire relationship explicitly.

_plugin_configs feature of decK allows users to abstract out configuration
of plugins that are used repeatedly and then reference them in
services/routes/consumers. This helps with de-duplication of configuration
code. While users would like to take advantage of this, most of them cannot
because de-duplicating the configuration in the first place is a cumbersome
and an error-prone process.

This implements such flag and feature for the dump command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants