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

KIC 2.0 - Proxy interface and Async Cache Resolution Server #1274

Merged
merged 5 commits into from
May 4, 2021

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Apr 30, 2021

While working on the problem of asynchronous object caching, conversion and Admin API updating resolution I took a look at how this was handled in 1.x to decide whether I wanted to try and use the existing logic or experiment with a new solution.

After careful consideration and syncing with @hbagdi and @mflendrich about some ideas, this PR is a proposal to provide a new interface and backend server mechanism to handle the conversion of Kubernetes API object changes to Kong Admin API updates.

Some of the advantages intended by this new implementation include (but are not limited to):

- abstract interface allows for potential multiple backend implementations (including test implementations which can make it easier to unit test some areas of the code)
- greatly improved logging capabilities of the object resolution process

Resolves #1267

Followup Items:

@shaneutt shaneutt added priority/high area/perf Performance Related Issues labels Apr 30, 2021
@shaneutt shaneutt requested review from rainest, mflendrich, hbagdi and a team April 30, 2021 20:52
@shaneutt shaneutt self-assigned this Apr 30, 2021
@github-actions
Copy link

Licenses differ between commit e92b8f4d195cd4e30de981126fc746eb2ca35d1d and base:

+++ pr_licenses.csv	2021-04-30 20:54:12.524444863 +0000
@@ -65,7 +65,7 @@
 github.com/kong/kubernetes-ingress-controller/railgun/controllers/corev1,Unknown,Unknown
 github.com/kong/kubernetes-ingress-controller/railgun/hack/generators/controllers/networking,Unknown,Unknown
 github.com/kong/kubernetes-ingress-controller/railgun/internal/ctrlutils,Unknown,Unknown
-github.com/kong/kubernetes-ingress-controller/railgun/internal/mgrutils,Unknown,Unknown
+github.com/kong/kubernetes-ingress-controller/railgun/internal/proxy,Unknown,Unknown
 github.com/kong/kubernetes-ingress-controller/railgun/manager,Unknown,Unknown
 github.com/kong/kubernetes-ingress-controller/railgun/pkg/clientset,Unknown,Unknown
 github.com/kong/kubernetes-ingress-controller/railgun/pkg/clientset/fake,Unknown,Unknown```

@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #1274 (0ab031c) into next (bc62211) will decrease coverage by 0.45%.
The diff coverage is 0.00%.

❗ Current head 0ab031c differs from pull request most recent head e633a05. Consider uploading reports for the commit e633a05 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1274      +/-   ##
==========================================
- Coverage   56.73%   56.28%   -0.46%     
==========================================
  Files          34       34              
  Lines        3356     3383      +27     
==========================================
  Hits         1904     1904              
- Misses       1311     1338      +27     
  Partials      141      141              
Impacted Files Coverage Δ
pkg/store/store.go 32.45% <0.00%> (-3.16%) ⬇️

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 bc62211...e633a05. Read the comment docs.

@shaneutt shaneutt force-pushed the proxy-interface branch from 1719a0a to 156017d Compare May 3, 2021 17:34
pkg/store/store.go Outdated Show resolved Hide resolved
@rainest
Copy link
Contributor

rainest commented May 3, 2021

How does

UpdateObject() and DeleteObject() methods will start throwing errors and you'll need to retry queing the object at a later time.

work in practice? Will attempts to submit an Ingress/KongPlugin/etc. to the K8S API server fail, similar to something that fails an admission webhook check?

How will syncTicker and fullResyncTicker work differently from the current sync loop? It looks like we'll go from always generating configuration and comparing the hash before updating the admin API to updating on new unhandled events? Does that potentially send unnecessary updates? IIRC we track events that don't actually end up contributing to config (e.g. Services that aren't actually tied to an Ingress we consume) and filter them after when generating config, though there may be other changes in Railgun that skip those now.

shaneutt added 4 commits May 4, 2021 11:14
The new Proxy interface allows async implementations
that controllers and a controller-runtime based
controller managers can use to update the Kong
Proxy's Admin APIs using only K8s objects and having
no need to understand Kong DSL.
@shaneutt shaneutt force-pushed the proxy-interface branch from e126cb7 to 534b3b7 Compare May 4, 2021 15:14
@shaneutt
Copy link
Contributor Author

shaneutt commented May 4, 2021

How does

UpdateObject() and DeleteObject() methods will start throwing errors and you'll need to retry queing the object at a later time.
work in practice? Will attempts to submit an Ingress/KongPlugin/etc. to the K8S API server fail, similar to something that fails an admission webhook check?

I've actually pulled this out of scope for now and added that scope to https://github.com/Kong/kubernetes-ingress-controller/issues/1275, the intention now is to decide how this works in a later and isolated scope.

How will syncTicker and fullResyncTicker work differently from the current sync loop? It looks like we'll go from always generating configuration and comparing the hash before updating the admin API to updating on new unhandled events? Does that potentially send unnecessary updates? IIRC we track events that don't actually end up contributing to config (e.g. Services that aren't actually tied to an Ingress we consume) and filter them after when generating config, though there may be other changes in Railgun that skip those now.

As of updates today, there is a simple boolean that flips to indicate whether an update needs to be performed, based on whether an object has been processed in the cycle. For this point in the alpha it does mean there can be some nil updates that occur if an object is reconciled that is not needed, there is a follow up item that intends to only cache objects which matter:

#1259

Otherwise if you still feel there's more that needs to be resolved here we can look at other approaches?

@shaneutt shaneutt requested a review from rainest May 4, 2021 15:27
@shaneutt shaneutt marked this pull request as ready for review May 4, 2021 15:27
@shaneutt shaneutt force-pushed the proxy-interface branch from 4c83903 to f585e65 Compare May 4, 2021 15:28
railgun/manager/manager.go Outdated Show resolved Hide resolved
@shaneutt shaneutt force-pushed the proxy-interface branch from 0ab031c to e633a05 Compare May 4, 2021 15:45
@shaneutt shaneutt requested a review from rainest May 4, 2021 19:07
@shaneutt shaneutt merged commit bbe221f into next May 4, 2021
@shaneutt shaneutt deleted the proxy-interface branch May 4, 2021 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants