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: initial impl of removing usage gloo proxy #10397

Merged
merged 51 commits into from
Dec 21, 2024
Merged

feat: initial impl of removing usage gloo proxy #10397

merged 51 commits into from
Dec 21, 2024

Conversation

yuval-k
Copy link
Contributor

@yuval-k yuval-k commented Dec 10, 2024

this is part of #10363

To enable opaque extendibility, we can't use protobuf based IR.

This pr changes the plugin model:

  • Each plugin can contribute CRDs that will be translated opaquely
  • Each plugin translates its own CRDs opaquely to the rest of the system.
  • The system handles policy attachment for the plugins

2 plugin stages:

  • Gateway api to gateway IR (uses krt); this creates a gateway IR with all the policies attached
  • gateway IR to xDS (pure function, no IO)

Added some example CRDs, by no means in final form.

still WIP, there's a bunch of intentional panics in the code; created draft PR for early feedback

@@ -47,10 +48,11 @@ func filterDelegatedChildren(
// make a copy; multiple parents can delegate to the same child so we can't modify a shared reference
clone := c.Clone()

child, ok := clone.Object.(*gwv1.HTTPRoute)
origChild, ok := clone.Object.(*ir.HttpRouteIR)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/origChild/childIR

}

// in case multiple policies attached to the same resouce, we sort by policy creation time.
func (d *directResponse) CreationTime() time.Time {
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this will be very repeated and build some spaghetti. Is there a way we can make this not a policy requirement or have a standard load of it into a policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason i introduced it is so i can sort policies by creation time in the IR (in the attached policies slice)
this helps with:

  • prevent translation from happening when no policies changed (krt will dedupe events if the previous IR is equal to the new IR).
  • given that all of these go in a slice, wanted to have consistent order incase order matters (though it probably shouldnt)

Copy link
Contributor

@ilrudie ilrudie left a comment

Choose a reason for hiding this comment

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

couple of comments from a partial review pass

Comment on lines +92 to +97
func(c skubeclient.ClientGetter, namespace string, o metav1.ListOptions) (runtime.Object, error) {
return ourCli.GatewayV1alpha1().Upstreams(namespace).List(context.Background(), o)
},
func(c skubeclient.ClientGetter, namespace string, o metav1.ListOptions) (watch.Interface, error) {
return ourCli.GatewayV1alpha1().Upstreams(namespace).Watch(context.Background(), o)
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Capturing ourCli as a closure in the registered list/watch functions. Feels odd but I don't see right off hand why it wouldn't work...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the motivation here is that i can use fake client to test these;
i think client getter basically does the same thing using the istio client.. cc @stevenctl WDYT?

projects/gateway2/extensions2/plugins/upstream/plugin.go Outdated Show resolved Hide resolved
func (d *builtinPlugin) Equals(in any) bool {
// we don't really need equality check here, because this policy is embedded in the httproute,
// and we have generation based equality checks for that already.
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be false so that these are never equal and will always behave as though they need to update under any possible change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should be true, false will cause excessive jitter.
in this specific case, it is not a problem because a built in type is embdded in the httproute CR, so we will catch it in the httproute equals (that is based off of the generation).

i'll try to see if its easy to add equals here for readability sake

Copy link
Contributor

Choose a reason for hiding this comment

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

I had not really grokked how this works when I wrote the comment. I had assumed we create a collection of builtinplugins and so assuming every plugin was equal would break but we don't use it that way.

That being said, should this code live somewhere else, maybe under translater/httproute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's here because policy attachment happens in the collections in policy.go..
I do agree that it might worth moving policy.go to translator package; but prefer to do it in a follow-up

}
}

func getFactionPercent(f gwv1.HTTPRequestMirrorFilter) *envoy_config_core_v3.RuntimeFractionalPercent {
Copy link
Contributor

@ilrudie ilrudie Dec 16, 2024

Choose a reason for hiding this comment

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

Suggested change
func getFactionPercent(f gwv1.HTTPRequestMirrorFilter) *envoy_config_core_v3.RuntimeFractionalPercent {
func getFractionPercent(f gwv1.HTTPRequestMirrorFilter) *envoy_config_core_v3.RuntimeFractionalPercent {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the suggestion has an r in the end so i can't apply it. will push this in shortly

Copy link
Contributor

Choose a reason for hiding this comment

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

updated, but you probably already pushed, good either way

Comment on lines 17 to 22
func InitCollectionsWithGateways(ctx context.Context,
kubeRawGateways krt.Collection[*gwv1.Gateway],
httpRoutes krt.Collection[*gwv1.HTTPRoute],
tcproutes krt.Collection[*gwv1a2.TCPRoute],
refgrants *RefGrantIndex,
extensions extensionsplug.Plugin, istioClient kube.Client, krtopts krtutil.KrtOptions) (*GatweayIndex, *RoutesIndex, krt.Collection[ir.Upstream], krt.Collection[ir.EndpointsForUpstream]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting

@yuval-k yuval-k marked this pull request as ready for review December 19, 2024 13:47
return i.getUpstream(kctx, resolved.GetGroupKind(), types.NamespacedName{Namespace: resolved.Namespace, Name: resolved.Name}, ref.Port)
}

type GatweayIndex struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type GatweayIndex struct {
type GatewayIndex struct {

@@ -72,30 +78,31 @@ func filterDelegatedChildren(
}

for _, match := range rule.Matches {
match := *match.DeepCopy()
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a deepcopy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to make sure we are not changing what's in the krt cache.
I believe controller runtime used to do that automatically for us

Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to completely delete these plugins already?
alternatively we can keep them around until the necessary functionality is ported into the new framework

Copy link
Contributor

@nfuden nfuden left a comment

Choose a reason for hiding this comment

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

A great start that we can start reimplementing alot such as the required checks that we havent quite gotten passing

@yuval-k yuval-k merged commit 5d4e603 into main Dec 21, 2024
12 of 17 checks passed
@yuval-k yuval-k deleted the no-proxy branch December 21, 2024 00:11
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.

6 participants