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

chore: refactor dataplane components (client, synchronizer, and parser) #2296

Merged
merged 16 commits into from
Mar 1, 2022

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Feb 28, 2022

What this PR does / why we need it:

This significantly refactors the internal/dataplane code the original motivation was to make it easier to later add options to parser for #1492.

Most notably this PR splits what was previous called the Proxy interface, which had both the aspects of a high level data-plane API client and a synchronization server for the data-plane. It's responsibilities are now split into dataplane.KongClient and dataplane.Synchronizer and while the diff is large, the code has mainly just been moved around and the resulting objects are much easier to read and understand.

High level refactoring notes:

  • the Proxy interface no longer exists, it's concerns have been separated into a dataplane.Client and a dataplane.Synchronizer
  • the dataplane.KongClient is now responsible for updates to the dataplane: internal/dataplane/sendconfig/common_workflows.go has been consumed
  • internal/dataplane/parser is now an object which can be configured to allow attributes to affect runtime logic instead of having to continually add more function parameters and feed them down the call chain, this will make it easier to add options that are specific to some resource types, or otherwise influence parsing with lighter weight PRs
  • translate code (and tests) now have their own files to help make it easier to locate the specific object translator you're looking for
  • the data-plane has in general become more abstract: higher level components are more able to treat the data-plane as opaque than before, and not have to think about Kong specific semantics. We don't have current plans to support any data-plane APIs other than the Kong Admin API, but this change will inherently make it easier to support alternative APIs in the future.

Which issue this PR fixes

Resolves #2295

Special notes for your reviewer:

No user facing changes, and despite the fact that the diff looks large functionality was just moved not added.

@shaneutt shaneutt added priority/medium area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. labels Feb 28, 2022
@shaneutt shaneutt self-assigned this Feb 28, 2022
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:34 Inactive
@shaneutt shaneutt changed the title shaneutt/refactor dataplane client chore: refactor dataplane components into client, synchronizer, and parser Feb 28, 2022
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:34 Inactive
@shaneutt shaneutt force-pushed the shaneutt/refactor-dataplane-client branch from 2cc29ca to ce22962 Compare February 28, 2022 20:36
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:36 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:36 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:52 Inactive
@shaneutt shaneutt force-pushed the shaneutt/refactor-dataplane-client branch from ce22962 to c18862a Compare February 28, 2022 20:59
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:59 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 20:59 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:15 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:17 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:17 Inactive
@shaneutt shaneutt force-pushed the shaneutt/refactor-dataplane-client branch from 6eabf63 to 0ef9bf5 Compare February 28, 2022 21:19
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:19 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:19 Inactive
@shaneutt shaneutt force-pushed the shaneutt/refactor-dataplane-client branch from 0ef9bf5 to 31fa138 Compare February 28, 2022 21:22
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:22 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:22 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 21:38 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 22:04 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 22:04 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 23:29 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 23:29 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 23:44 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 28, 2022 23:44 Inactive
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Some minor file name bikeshedding and a dead link.

Can you clarify the "make it easier to later add options to parser" refactor/indicate what we should look at that isn't just reorganization? What did we do before (I think this is "add new parameters to NewCacheBasedProxyWithStagger() and friends) and how do we handle that after this change?

internal/dataplane/parser/translate_httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_kong.go Outdated Show resolved Hide resolved
@shaneutt shaneutt force-pushed the shaneutt/refactor-dataplane-client branch from a6f0a43 to 28e90f7 Compare March 1, 2022 20:21
@shaneutt shaneutt temporarily deployed to Configure ci March 1, 2022 20:21 Inactive
@shaneutt
Copy link
Contributor Author

shaneutt commented Mar 1, 2022

Can you clarify the "make it easier to later add options to parser" refactor/indicate what we should look at that isn't just reorganization? What did we do before (I think this is "add new parameters to NewCacheBasedProxyWithStagger() and friends) and how do we handle that after this change?

Before options were passed as function parameters and would need to be passed through several layers of function calls (options like the logger for instance). Now the parser is a struct with the logger (again, for instance) as an attribute. Future iterations will now be able to feed options from the dataplane.KongClient to the parser as object attributes that can be accessed in methods.

@shaneutt shaneutt requested a review from rainest March 1, 2022 20:23
@shaneutt shaneutt temporarily deployed to Configure ci March 1, 2022 20:26 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci March 1, 2022 20:27 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci March 1, 2022 20:43 Inactive
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

A more concrete condensed pseudocode example would still help understanding the refactor better, but eh, looks reasonable enough, I suppose that can wait til a PR for #1492.

@shaneutt shaneutt merged commit bc2bc26 into main Mar 1, 2022
@shaneutt shaneutt deleted the shaneutt/refactor-dataplane-client branch March 1, 2022 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. ci/license/unchanged priority/medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor parser build
2 participants