-
Notifications
You must be signed in to change notification settings - Fork 474
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
initial dev doc #10776
base: main
Are you sure you want to change the base?
initial dev doc #10776
Conversation
|
||
// Errors processing it for status. | ||
// note: these errors are based on policy itself, regardless of whether it's attached to a resource. | ||
// TODO: change for conditions | ||
Errors []error | ||
|
||
// original object. ideally with structural errors removed. | ||
// The IR of the policy objects. ideally with structural errors removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "with structural errors removed" mean?
DEV.md
Outdated
@@ -0,0 +1,80 @@ | |||
Architecture: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Architecture: | |
# Architecture: |
DEV.md
Outdated
@@ -0,0 +1,80 @@ | |||
Architecture: | |||
|
|||
KGateway is a control plane for envoy based on the gateway-api. This means that we translate K8s objects like Gateways, HttpRoutes, Service, EndpointSlices and User policy into envoy configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KGateway is a control plane for envoy based on the gateway-api. This means that we translate K8s objects like Gateways, HttpRoutes, Service, EndpointSlices and User policy into envoy configuration. | |
Kgateway is a control plane for envoy based on the gateway-api. This means that we translate K8s objects like Gateways, HttpRoutes, Service, EndpointSlices and User policy into envoy configuration. |
nit: the capitalized form is Kgateway
with lowercase g, there are a few more instances below too
DEV.md
Outdated
|
||
Our goals with the architecture of the project are to make it scalable and extendable. | ||
|
||
To make the project scalable, its importnat to keep the computation minimal when changes occur. For example, when a pod changes, or a policy is updated, we don't do the minimum amount of computation to update the envoy configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make the project scalable, its importnat to keep the computation minimal when changes occur. For example, when a pod changes, or a policy is updated, we don't do the minimum amount of computation to update the envoy configuration. | |
To make the project scalable, it's important to keep the computation minimal when changes occur. For example, when a pod changes, or a policy is updated, we do the minimum amount of computation to update the envoy configuration. |
it says "we don't do the minimum amount" .. assume it should say "we do"?
DEV.md
Outdated
The system will make use of the traget refs to attach the policy IR to Listners and HttpRoutes. You will then | ||
get access to the IR during translation (more on that later). | ||
|
||
The second field, `NewGatewayTranslationPass` allocates a new translation state for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the lifecycle of the plugin (i.e. when does NewPlugin get called) vs. the translation pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point - NewPlugin is called just once; i'll update
DEV.md
Outdated
HttpRoutes and Gateways are handled by KGateway. Kgateway builds an IR for HttpRoutes and Gateways, that looks very similar to | ||
the original object, but in additional has an `AttachedPolicies` struct that contains all the policies that are attached to the object. | ||
|
||
KGateway uses the `TargetRefs` field in the PolicyWrapper to opaquely attach the policy to the HttpRoute or Gateway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we mention extensionRef too?
DEV.md
Outdated
|
||
## Translation to xDS | ||
|
||
When we reach this phase, we already ahve the Policy -> IR translation done; and we have all the HttpRoutes and Gateways in IR form with the policies attached to them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should group the 2 phases as sub-headings in this doc: 1) policy -> IR translation (when NewPlugin is called?), 2) IR to xDS (when the ApplyFor* funcs are called)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the policy -> IR translation
happens earlier; i'll expand on it in the earlier section
@@ -65,12 +65,13 @@ type ProxyTranslationPass interface { | |||
pCtx *ListenerContext, | |||
out *envoy_config_listener_v3.Listener, | |||
) | |||
// called 1 time per filter chain after listeners | |||
// called 1 time per filter chain after listeners and allows tweaking HCM settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having each plugin adhere to this monolothic interface, have we considered defining different plugin types so each plugin only needs to implement the funcs that are relevant? (similar to how gloo did it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did; i prefered this way so its very clear which function is called in which order, and which functions are allowed to share state. I recently added a default impl, so that plugins can only override the functions they need.
@@ -65,12 +65,13 @@ type ProxyTranslationPass interface { | |||
pCtx *ListenerContext, | |||
out *envoy_config_listener_v3.Listener, | |||
) | |||
// called 1 time per filter chain after listeners | |||
// called 1 time per filter chain after listeners and allows tweaking HCM settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we somewhere (either in the readme or in code) list explicitly the order in which these functions are called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the order should be the order they appear in the interface; i can mention that in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few nits but otherwise LGTM. This doc explains a plugin that contributes policy to the translation (ContributesPolicies) but it would be helpful to explain how a plugin contributes a backend (ContributesBackends). We should consider a few follow-up items related to this work:
- A "day in the life of a translation". Explain how a Gateway is translated to an Envoy proxy. Do the same for HTTPRoute and Policies that get attached at different places.
- Add a simple example plugin to the repo that is not enabled by default. The plugin should contribute backends and policies or have separate example plugins for each.
DEV.md
Outdated
@@ -0,0 +1,80 @@ | |||
Architecture: | |||
|
|||
KGateway is a control plane for envoy based on the gateway-api. This means that we translate K8s objects like Gateways, HttpRoutes, Service, EndpointSlices and User policy into envoy configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/gateway-api/Gateway API /
DEV.md
Outdated
|
||
Our goals with the architecture of the project are to make it scalable and extendable. | ||
|
||
To make the project scalable, its importnat to keep the computation minimal when changes occur. For example, when a pod changes, or a policy is updated, we don't do the minimum amount of computation to update the envoy configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For we don't
, do you mean we do
? If not, the statement contractions itself.
DEV.md
Outdated
|
||
To make the project scalable, its importnat to keep the computation minimal when changes occur. For example, when a pod changes, or a policy is updated, we don't do the minimum amount of computation to update the envoy configuration. | ||
|
||
With extendability, we KGateway to be the basis on-top of which users can easily add their own custom logic. to that end we have a plugin system that allows users to add their own custom logic to the control plane in a way that's opaque to the core code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/we KGateway to be/KGateway is/
DEV.md
Outdated
With extendability, we KGateway to be the basis on-top of which users can easily add their own custom logic. to that end we have a plugin system that allows users to add their own custom logic to the control plane in a way that's opaque to the core code. | ||
|
||
|
||
Going down further, to enable these goals we use KRT based system. KRT gives us a few advantages: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For KRT, can you provide a link to the best reference for a user to gain additional context? Maybe this one: https://github.com/istio/istio/tree/master/pkg/kube/krt#krt-kubernetes-declarative-controller-runtime
DEV.md
Outdated
|
||
|
||
Going down further, to enable these goals we use KRT based system. KRT gives us a few advantages: | ||
- The ability to complement controllers of custom Intermediate representation (henceforth IR). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Intermediate representation/Intermediate Representation/
Can you expand or reword this sentence to better explain your point?
DEV.md
Outdated
For policies, this is pluggable. Plugins can Contribute a policy to kgateway. Contributing a policy means that we add a policy collection to kgateway. It's the users plugin responsibility to convert the policy CRD to the IR form. Ideally, the IR should look as close as possible to the envoy configuration, so this translation only happens when the policy CRD changes. | ||
|
||
You can see in the Plugin interface a field called `ContributesPolicies` which is a map of GK -> `PolicyPlugin`. | ||
The policy plugin contains a bunch of fields, but for out discussion we'll focus on these two: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/out/our/
I agree that not all fields should be discussed here but it will be helpful to improvise the godocs throughout, especially for internal/kgateway/extensions2/plugin/plugin.go
and IR types.
DEV.md
Outdated
} | ||
``` | ||
Policies is a the collection of policies that the plugin contributes. The plugin is responsible to create | ||
this collection, usually by started with a CRD collection, and then translating to a `ir.PolicyWrapper` struct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/started/starting/
DEV.md
Outdated
|
||
## Translation to xDS | ||
|
||
When we reach this phase, we already ahve the Policy -> IR translation done; and we have all the HttpRoutes and Gateways in IR form with the policies attached to them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ahve/have/
ApplyHCM(ctx context.Context, | ||
pCtx *HcmContext, | ||
out *envoy_hcm.HttpConnectionManager) error | ||
|
||
// called 1 time for all the routes in a filter chain. | ||
// called 1 time for all the routes in a filter chain. Use this to set default PerFilterConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you state "called 1 time" means 1 time per translation pass, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, correct
internal/kgateway/ir/iface.go
Outdated
ApplyForRoute( | ||
ctx context.Context, | ||
pCtx *RouteContext, | ||
out *envoy_config_route_v3.Route) error | ||
// runs for policy applied | ||
|
||
// Appliesa policy attached to a specific Backend (via extensionRef on the BackendRef). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Appliesa/Applies a/
// called 0 or more times | ||
// called 0 or more times (one for each route) | ||
// Applies policy for an HTTPRoute that has a policy attached via a targetRef. | ||
// The output configures the envoy_config_route_v3.Route | ||
ApplyForRoute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to be done in this PR, but i wonder if we can make some of these function names more self-explanatory, e.g. ApplyForRoute and ApplyForRouteBackend sound similar, just the attachment method is different. not sure what a better name is offhand..will have to think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApplyPolicyToHttpRoute
, ApplyPolicyToBackendViaRoute
, ApplyForBackend
? (I think adding the policy part might make it more clear?)
Dev doc to explain how plugins work