OpenAPI Provider

[guicassolato]

What type of PR is this? /kind feature

What this PR does / why we need it: Adds new openapi provider.

Which issue(s) this PR fixes: Closes #147

Does this PR introduce a user-facing change?:

- New OpenAPI Provider

Try this PR out:

make build
./ingress2gateway print --providers openapi3 --input-file=pkg/i2gw/providers/openapi3/fixtures/input/1-petstore3.yaml

[k8s-ci-robot]

Welcome @guicassolato!

It looks like this is your first PR to kubernetes-sigs/ingress2gateway 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/ingress2gateway has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @guicassolato. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[guicassolato]

@arkodg @youngnick @LiorLieberman et al

Sharing this as a draft for some initial feedback.

Quite a few TODOs ahead still, including:

• [x] Gateways and parentRefs
• [x] backendRefs
• [x] gatewayClass
• [x] Name objects after the OAS title (?)
• [ ] ~Input with multiple OAS docs (?)~

Apart from more testing of course.

Please check out the test cases already covered under providers/openapi/fixtures.

[mlavacca]

/cc @mlavacca

[guicassolato]

I want to mark this as ready for review, but not before coming to a decision about how to proceed for backendRefs and gatewayClassName.

It was suggested to use command-line args for those. I would like to know how people feel about that?

A few questions to drive the conversation next should we say this indeed is the desired approach:

• Should I just define those as flags of the print command?
• Since they don't have meaning for other providers converting from Ingress resources, will providers like istio, ingress-nginx, kong, etc ignore the values when supplied?
• Perhaps call those options --default-backend-name and --default-gateway-controller-name?
• Or they could be namespaced specifically for this provider – e.g. --openapi3-bakcend-name and --openapi3-gateway-controller-name – tho I can see a possible mess in the future should other providers ask for other similar options. If it is provider-specific, should the options be required when --providers includes openapi3 and forbidden otherwise?
• Provider-specific options could also be serialised into a single command-line flag (e.g. --provider-options "backend-name=x,controller-name=y")
• Is it starting to get a little bit too much to pass in the call to ToGatewayAPIResources? Should we change the API to accept a more generic PrintOptions type?

cc @youngnick @arkodg @LiorLieberman

[LiorLieberman]

/cc @arkodg

[mlavacca]

I want to mark this as ready for review, but not before coming to a decision about how to proceed for backendRefs and gatewayClassName.

It was suggested to use command-line args for those. I would like to know how people feel about that?

A few questions to drive the conversation next should we say this indeed is the desired approach:

• Should I just define those as flags of the print command?
• Since they don't have meaning for other providers converting from Ingress resources, will providers like istio, ingress-nginx, kong, etc ignore the values when supplied?
• Perhaps call those options --default-backend-name and --default-gateway-controller-name?
• Or they could be namespaced specifically for this provider – e.g. --openapi3-bakcend-name and --openapi3-gateway-controller-name – tho I can see a possible mess in the future should other providers ask for other similar options. If it is provider-specific, should the options be required when --providers includes openapi3 and forbidden otherwise?

+1 on this approach. having access to generic --default-backend-name, --default-gateway-controller-name flags might be confusing for users. I'd prefer having openapi3--prefixed command line flags to avoid such a confusion. openapi is a special case, and I do cannot think of many other such special cases in the future

• Provider-specific options could also be serialised into a single command-line flag (e.g. --provider-options "backend-name=x,controller-name=y")
• Is it starting to get a little bit too much to pass in the call to ToGatewayAPIResources? Should we change the API to accept a more generic PrintOptions type?

+1 on introducing a PrintOptions struct to pass all the needed parameters.

[arkodg]

hey reg naming, i'd vote for

• <openapi provider prefix>-backend - openapi provider specific CLI arg . How are you going to represent the backendRef here, as a fqdn ?
• gateway-controller - common to the entire CLI arg

[guicassolato]

Where I wrote controller-name please read gateway-class-name, since obviously we're talking about generating Gateways, not GatewayClasses. Apologies for my distraction before.

[guicassolato]

I was feeling somewhat uncomfortable with hard-coding provider-specific CLI flags when there's already a mechanism in place to register providers more dynamically. So please let me know what you think about https://github.com/kubernetes-sigs/ingress2gateway/pull/157/commits/12dd6a09cceb21f537a2c134e01d64f012665630.

I'm proposing a function to declare and a method to retrieve those provider-specific user input values, that keeps the common implementation clean from it. Explanation and example provided in PROVIDER.md.

I'm using the method to register --openapi3-backend and --openapi3-gateway-class-name both as specific to the openapi3 provider. Example of how the flags will show in command help:

$ ./ingress2gateway print --help
Prints Gateway API objects generated from ingress and provider-specific resources.

Usage:
  ingress2gateway print [flags]

Flags:
  -A, --all-namespaces                       If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even
                                             if specified with --namespace.
  -h, --help                                 help for print
      --input-file string                    Path to the manifest file. When set, the tool will read ingresses from the file instead of reading from the cluster. Supported files are yaml and json.
  -n, --namespace string                     If present, the namespace scope for this CLI request.
      --openapi3-backend string              Provider-specific: openapi3. The name of the backend service to use in the HTTPRoutes
      --openapi3-gateway-class-name string   Provider-specific: openapi3. The name of the gateway class to use in the Gateways
  -o, --output string                        Output format. One of: (json, yaml). (default "yaml")
      --providers strings                    If present, the tool will try to convert only resources related to the specified providers, supported values are [apisix ingress-nginx istio kong openapi3]. (default [apisix,ingress-nginx,istio,kong,openapi3])

Global Flags:
      --kubeconfig string   The kubeconfig file to use when talking to the cluster. If the flag is not set, a set of standard locations can be searched for an existing kubeconfig file.

@arkodg, if you think gateway-class-name is useful to all providers, I'm happy to hard-code this one as we normally would and remove it from openapi3-specific, even if for now only openapi3 will be doing something with it.

[arkodg]

@guicassolato no strong feelings, the project maintainers have a much better understand of naming and should decide

[guicassolato]

@guicassolato no strong feelings, the project maintainers have a much better understand of naming and should decide

cc @LiorLieberman @mlavacca

[LiorLieberman]

Thanks again for driving this Gui! Regarding gateway-class-name I hear you, I think what we ultimately want to do is to have IR (intermediate representation) and have --input-provider and --output-provider. Each provider would implement old config --> IR and IR --> Gateway configs (+gateway extensions). That at least what @arkodg and I discussed at KubeCon.

If we do plan move on to this logic, I think gateway-class-name will be derived from the --output-provider flag.

Since we dont support it now, an nor gateway extensions are supported, I am trying to think whether it will indeed be useful for other providers. The more I think about it - I get to the conclusion that we must move to input and output providers separately. So I'd vote leaving gateway-class-name prefixed

Regarding the backend flag - still not sure how you were going to represent it in a flag value? A formatted string with name,ns,port? (not loving this either)

About the general approach of registering provider specific flags dynamically. I like it but trying to think what are the use-cases for other providers. We have to ask ourselves how different the openapi provider from all other providers PLUS how much of the logic is similar, maybe we want to have a subcommand for openapi which will make things simpler? not saying we should but just throwing it some thought

Lastly I left a few random comments on https://github.com/kubernetes-sigs/ingress2gateway/commit/12dd6a09cceb21f537a2c134e01d64f012665630.

Thats really awesome, thanks again!

[guicassolato]

Hi @LiorLieberman. Thank you so much for taking the time to review this.

It turns out the biggest effort here has not been converting OAS to Gateway API per se, but the details... where, as we all know, lives the Devil. The PR has grown bigger and I still hope to be able to deliver valuable contribution with it.

A few comments in line...

Thanks again for driving this Gui! Regarding gateway-class-name I hear you, I think what we ultimately want to do is to have IR (intermediate representation) and have --input-provider and --output-provider. Each provider would implement old config --> IR and IR --> Gateway configs (+gateway extensions). That at least what @arkodg and I discussed at KubeCon.

I like the idea of IR. Of course, that only works if the abstraction in the middle is capable of absorbing all the complexity from all possible inputs/"old configs". In fact, the main reason why I didn't use Ingress as IR – and therefore do OAS -> Ingress -> Gateway API, relying on the standard ("common") conversion functions – was not being able to support capabilities that exist in the old config (OAS, in this case) but not in the IR, such as header and query matchers.

To be completely honest, I also wanted to avoid generating invalid HTTPRoutes with more rules or more matches per rule than it can take, without introducing breaking changes to existing providers. To give an example, even the Swagger Petstore API would activate this problem.

I believe a straight OAS -> Gateway API conversion for now allowed me solving both issues: capabilities and conformance.

If we do plan move on to this logic, I think gateway-class-name will be derived from the --output-provider flag.

Yes, I can see how that could work for gateway-class-name.

Since we dont support it now, an nor gateway extensions are supported, I am trying to think whether it will indeed be useful for other providers. The more I think about it - I get to the conclusion that we must move to input and output providers separately. So I'd vote leaving gateway-class-name prefixed

So, to be clear then, you're aligned with @mlavacca and with how I ended up implementing it, i.e. openapi3-gateway-class-name?

Regarding the backend flag - still not sure how you were going to represent it in a flag value? A formatted string with name,ns,port? (not loving this either)

Currently, it's namespace/name, no support for port number. We could instead establish a convention to use $SERVICE.$NAMESPACE.svc.cluster.local:$PORT, for example. Would that be preferable?

About the general approach of registering provider specific flags dynamically. I like it but trying to think what are the use-cases for other providers. We have to ask ourselves how different the openapi provider from all other providers PLUS how much of the logic is similar, maybe we want to have a subcommand for openapi which will make things simpler? not saying we should but just throwing it some thought

I was hoping the example provided in PROVIDER.md would convince you 🙂 It's a different use case than OpenAPI.

By that I don't mean to disagree that openapi3 is a special case, because it definitely is. To begin with, it doesn't count with a few crucial pieces of information that Ingress does, such as names of the backends, names of the gateway TLS certificate secrets, which gateway controller ultimately will be responsible for the generated Gateway API objects... And I'm sure we'll find others. However, I believe OAS to Gateway API is not special in the sense that, sooner or later, all providers will need something that is specific to its case.

I can ramble for several pages about the truth behind this last statement, but instead I'll turn to my other point of motivation for having flags dynamically registered. I'm concerned about the leakage of a provider specific detail hard-coded at the level that is supposed to abstract that. It feels like violating a principle, apart from opening a dangerous precedent IMO.

If you are sure we won't regret it (with the information we know now), I will keep it simple and hard code --openapi3-gateway-class-name, --openapi3-backend and (heads-up!) --openapi3-gateway-tls-secret directly inside func newPrintCommand() {...}. Otherwise, the work is done and nothing to worry about.

Lastly I left a few random comments on 12dd6a0.

Thanks for those too! Both valid points that I will be addressing next.

[LiorLieberman]

I believe a straight OAS -> Gateway API conversion for now allowed me solving both issues: capabilities and conformance.

Right, to clarify, I did not mean that you should have done OAS-->Ingress-->Gateway, instead I am saying that in future extensions we should do OAS-->IR, Istio CRDs -->IR, Ingress-->IR, and have output providers from the other side do IR-->provider

So, to be clear then, you're aligned with @mlavacca and with how I ended up implementing it, i.e. openapi3-gateway-class-name?

👍

Currently, it's namespace/name, no support for port number. We could instead establish a convention to use $SERVICE.$NAMESPACE.svc.cluster.local:$PORT, for example. Would that be preferable?

hmm not sure, why should a user include svc.cluster.local? it seems like we only have to support namespace, name, and port, right? so I am good with whatever convention you decide here as long as you document it.

Regarding the last bullet, I think what you added is great, so definitely good with keeping it.

Are there any more blockers/things we need to figure out before we can merge this ?

[guicassolato]

Currently, it's namespace/name, no support for port number. We could instead establish a convention to use $SERVICE.$NAMESPACE.svc.cluster.local:$PORT, for example. Would that be preferable? hmm not sure, why should a user include svc.cluster.local? it seems like we only have to support namespace, name, and port, right? so I am good with whatever convention you decide here as long as you document it.

I guess I was just trying not to come up with yet another convention. But you're probably right. Why requiring users to know the cluster domain if we don't actually need it?

I'll include the option for the port number. Let's make it [namespace/]name[:port]. Examples of valid values:

• --openapi3-backend=my-service
• --openapi3-backend=my-namespace/my-service
• --openapi3-backend=my-service:3000
• --openapi3-backend=my-namespace/my-service:3000

Are there any more blockers/things we need to figure out before we can merge this ?

After the above, I think we're good.

Thanks @LiorLieberman!

[guicassolato]

Awesome, before my final review.

Quite a few TODOs ahead still, including:

• [x] Gateways and parentRefs
• [ ] backendRefs
• [ ] gatewayClass
• [ ] Name objects after the OAS title (?)
• [ ] Input with multiple OAS docs (?)

Apart from more testing of course. Please check out the test cases already covered under providers/openapi/fixtures.

Is this up to date? I think you already took care of some unchecked stuff here (like gwc for example) Can you please update the comment whats already addressed and whats not, and then add TODOs (future/unsupported) things in provider.MD and ping me for final review?

I've updated the comment, @LiorLieberman.

I think the only item not covered in this iteration is the one of input files declaring multiple OAS docs, which I prefer leaving for a future improvement, if ever requested.

The limitation to a maximum of 1 OAS per input file is already mentioned in provider.MD.

[LiorLieberman]

/hold for implementing the requested changes from last review.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guicassolato, LiorLieberman, mlavacca

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [LiorLieberman,mlavacca]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[LiorLieberman]

/unhold

[LiorLieberman]

@guicassolato can you change the release notes to be slightly more detailed? It will make my life easier when releasing a new version

[guicassolato]

@guicassolato can you change the release notes to be slightly more detailed? It will make my life easier when releasing a new version

It was merged before I could address this, @LiorLieberman 😞