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

Support custom entities in Railgun #1339

Merged
merged 5 commits into from
May 25, 2021
Merged

Support custom entities in Railgun #1339

merged 5 commits into from
May 25, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented May 20, 2021

What this PR does / why we need it:
Add the kong-custom-entities-secret flag to Railgun, load the specified Secret, and pass it to sendconfig.PerformUpdate().

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1306

Special notes for your reviewer:
Testing: good question! For https://docs.konghq.com/kubernetes-ingress-controller/1.2.x/guides/configuring-custom-entities/ you need a plugin that actually defines custom entities. The core plugin suite doesn't really have any of these--oauth2 does but it's not available in DB-less mode. We're kinda stuck for custom entity integration tests unless we want to build a custom Kong image that includes such a plugin.

Add the kong-custom-entities-secret flag to Railgun, load the specified
Secret, and pass it to sendconfig.PerformUpdate().
@rainest rainest requested a review from a team as a code owner May 20, 2021 18:53
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #1339 (7751650) into next (9075a8d) will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1339      +/-   ##
==========================================
+ Coverage   55.60%   55.74%   +0.13%     
==========================================
  Files          38       38              
  Lines        3566     3577      +11     
==========================================
+ Hits         1983     1994      +11     
  Misses       1434     1434              
  Partials      149      149              
Impacted Files Coverage Δ
...n/internal/proxy/clientgo_cached_proxy_resolver.go 63.72% <100.00%> (+4.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9075a8d...7751650. Read the comment docs.

@shaneutt
Copy link
Contributor

I have a couple comments, but I'm also wondering how we can add tests for this. Perhaps should we put an issue into KTF to add plug-in utilities that would help us better test things like this?

Travis Raines added 2 commits May 24, 2021 15:40
- Add test for fetchCustomEntities
- Tidy code for custom entity fetching
@rainest
Copy link
Contributor Author

rainest commented May 24, 2021

I have a couple comments, but I'm also wondering how we can add tests for this. Perhaps should we put an issue into KTF to add plug-in utilities that would help us better test things like this?

Effort/benefit ratio is rather skewed towards the former here. Custom entities aren't particularly common and the controller's interactions with them are quite limited. We literally just append some arbitrary user-provided YAML objects to generated configuration and trust that you wrote them properly in DB-less mode and do nothing with them at all in DB-backed mode (we expect that you'll manage them independently--the DB-less functionality exists because it's impossible to manage config independently in that mode). Making them available would require a non-standard image, which feels like a fairly weighty thing to add to KTF for this alone--if we have other reasons for it that'd make more sense.

For this, added some basic unit stuff to confirm that the entity retrieval function does what it should.

@rainest rainest requested a review from shaneutt May 24, 2021 23:02
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Thanks for the patches, LGTM 🖖

@rainest rainest merged commit 6ef21e5 into next May 25, 2021
@rainest rainest deleted the feat/rg-cust-ents branch May 25, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants