-
Notifications
You must be signed in to change notification settings - Fork 97
Initial Support for Implementation Emitters #262
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danehans The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@danehans We still need to debate some details of having implementation specific emitters, but I do want to get some discussion going on the implementation as I've been thinking about this too. I like this PR much more than the last one, but I do have some thoughts on how we want to do this. There are two points points. First, I would really want to have all the IR be provider agnostic. Admittedly, the code is not structured this way right now, but from a maintainability POV, having emitters read provider-specific IR is just not tractable as the number of providers and emitters grows. To start, we can have model this IR so its similar to the ingress nginx one. The second point (that I don't feel as strongly about) is to think about failures as a first class citizen that is embedded into the API. I'm not too sure how this would work, but I have some ideas. Each section of the IR would also have some sort of status. As the emitter parses it, it will write the status saying it was parsed and if there are any caveats the user should know about. That way if we add a new section of IR, they will be parsed as unamrked automatically by all emitters. I also realize that there is a good amount of urgency to get something working specifically for ingress nginx (I definitely feel it), but if we really want users to use this tool, then I think its worth taking a step back and thinking a bit. |
3414acf to
6c5ffb5
Compare
Signed-off-by: Daneyon Hansen <[email protected]>
6c5ffb5 to
d088dd2
Compare
|
@danehans: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@Stevenjin8 thanks for the review
+1 I added the Policy type to IngressNginxHTTPRouteIR, but this could be part of the top-level IR so it's not tied to a specific provider. IMHO refactoring all of the IR to be provider agnostic should not prevent us from proceeding with this PR or some other approach that supports emitting implementation-specific resources. Are you able to work on refacting the IR to be provider agnostic? If so, how long do you anticipate this taking?
+1 Is this something we can iterate on so we're not blocked by making progress with the foundational work? |
|
@danehans can you please review/engage on https://docs.google.com/document/d/1EJV0nFtcmIK96_4V0C9UV8jlKqjPmmF1MFVfmpF_6lc/edit?tab=t.0 |
I really think we should figure out the foundational work before we move onto things that rely on the foundation. I don't think this is hard code. So the faster we can get consensus, the faster we can make progress. I'm putting almost all my time into this project right now. |
|
xref https://docs.google.com/document/d/1EJV0nFtcmIK96_4V0C9UV8jlKqjPmmF1MFVfmpF_6lc/edit?usp=sharing for discussion related to implementation-specific emitters. |
|
Emitters design doc PR xref: #265 |
| errs = append(errs, conversionErrs...) | ||
|
|
||
| // Convert client.Objects to unstructured and append to GatewayExtensions. | ||
| for _, obj := range implObjs { |
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.
Ok, I was thinking about this but want to get your throughts.
Right now, GCE is the only provider that takes advantage of provider-specific extensions. Do you think that we could have a GCE case and a non-GCE case?
|
|
||
| // Emit can mutate the IR (e.g. to inject HTTPRoute filters) and returns | ||
| // implementation-specific objects. | ||
| Emit(ir *intermediate.IR) ([]client.Object, error) |
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 do you think of having a ProviderIR and a EmitterIR. ProviderIR would be the current IR (and we could probably shed a bunch of unused fields). It would serve as an initial translation where all the Ingress resources are read without their annotations, the provider would then take that and do things like change certain httproutes to tlsroutes and fill in provider-agnostic data.
| type IngressNginxGatewayIR struct{} | ||
| type IngressNginxHTTPRouteIR struct{} | ||
|
|
||
| type IngressNginxHTTPRouteIR 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.
I think we should refactor the way we are doing provider-specific extensions. That is, we should get rid of as many of them as possible. Right now, only GCE uses them and we could keep them around for backcompatibility if we wanted. Ideally, any IR passed to the emitter would have no reference to the provider (or any ingress-related resource).
Stevenjin8
left a comment
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 left these as comments in the code too, but I'll write them again.
My one wish is for the IR fed to the emitter to be provider neutral, and to me, that means having to two IRs, one for the provider and one for the emitter.
The providerIR would be a translation of the Ingress without any implementation specific features and source maps. The providers could, if it wants to, use said IR to flush out the translation and add any provider neutral IR. Then, the emitter would finish the translation.
Stevenjin8
left a comment
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.
.
|
@danehans sorry I thought this was a new PR. Do you still want me to do the provider-neutral emitters. It will probably completely overwrite this PR. |
|
Here is my (WIP) PR #273 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #260
Does this PR introduce a user-facing change?: