-
Notifications
You must be signed in to change notification settings - Fork 57
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
OpenAPI Provider #157
OpenAPI Provider #157
Conversation
Welcome @guicassolato! |
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 Once the patch is verified, the new status will be reflected by the 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. |
@arkodg @youngnick @LiorLieberman et al Sharing this as a draft for some initial feedback. Quite a few TODOs ahead still, including:
Apart from more testing of course. Please check out the test cases already covered under |
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.
Thanks @guicassolato, that a very useful contribution! left some comments
pkg/i2gw/providers/openapi/README.md
Outdated
## Known limitations | ||
|
||
* Only offline translation supported – i.e. `--input_file` required | ||
* All API operation [paths](https://swagger.io/specification/v3/#paths-object) treated as `Exact` type – i.e. no support for [path templating](https://swagger.io/specification/v3/#path-templating), therefore no `PathPrefix`, nor `RegularExpression` path types output |
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.
Curious to know why no support for path templating? a limitation you encountered ?
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'm afraid there aren't many good options for that. At least, not that I could think of.
- Delete the part in each path from the first variable placeholder, all the way to the end of the path, and use
PathPrefix
type → some nasty stuff could happen with the ordering of the route rules - Use
RegularExpression
type → not core; what flavour of regex would we use?
Path templates are not like server variables that in some case give us at least a closed set of possible values (enum.)
spec, err := loader.LoadFromFile(filename) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to load OpenAPI spec: %w", err) | ||
} | ||
|
||
if err := spec.Validate(ctx); err != nil { | ||
return nil, fmt.Errorf("invalid OpenAPI 3.x spec: %w", err) | ||
} | ||
|
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.
@levikobi @mlavacca I think we should think about whether we want to preserve this all supported providers by default functionality.
For example here, if we merge this PR, if someone provides --input-file
but doesn't provide provider this will always fail (because a regular yaml/json input file would never be a valid openapi spec.
@guicassolato, regardless to the above, I think running openapi
provider should always be mutually exclusive to other providers.
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 use case for converting to all supported providers anyway?
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.
thats how we started, I think we need to change it, but will have a separate issue for that
s.mu.Lock() | ||
defer s.mu.Unlock() |
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.
why do we need a lock? couldn't find a place where you run addResource
or getResource
concurrently
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.
It's a good practice in general. Since the dependency is injected, the converter cannot prevent external usage of the storage.
In fact, I now realise my implementation is poor. A concurrent call to ReadResourcesFromFile
would cause the entire storage to be invalidated while completely bypassing the lock, when arguably the right thing to do would be to use the context to infer whether reading a resource should be additive to the storage or clear it first. At the very least, do the latter, but not forcibly recreate the storage. I will fix that.
From the perspective of the openapi
package, there is no way to tell what a client will do with those two competing processes.
Signed-off-by: Guilherme Cassolato <guicassolato@gmail.com>
a70bfe9
to
acc95e9
Compare
8df08c1
to
f9d8d13
Compare
/cc @mlavacca |
I want to mark this as ready for review, but not before coming to a decision about how to proceed for 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:
|
/cc @arkodg |
+1 on this approach. having access to generic
+1 on introducing a |
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.
Thanks for this PR! I left some initial generic comments in the review
} | ||
|
||
type storage struct { | ||
mu sync.RWMutex |
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.
Why do we need thread safety here? I haven't seen any concurrency in the code, did I miss something?
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 tried to answer it here: #157 (comment)
TL;DR - I think not having it is a road to problems in the future.
Not supposed to sound unnecessarily ominous here... I do believe things change, such as – I'm making stuff up here, ok? – start supporting multiple input files (or an input directory equivalently) and, if so, why not calling ReadResourcesFromFile
concurrently?
It's a small price to pay IMO, but if more people believe we're better off without the mutex, I can remove it – even if I don't understand why.
gatewayResources := i2gw.GatewayResources{ | ||
Gateways: make(map[types.NamespacedName]gatewayv1.Gateway), | ||
HTTPRoutes: make(map[types.NamespacedName]gatewayv1.HTTPRoute), | ||
TLSRoutes: make(map[types.NamespacedName]gatewayv1alpha2.TLSRoute), | ||
TCPRoutes: make(map[types.NamespacedName]gatewayv1alpha2.TCPRoute), | ||
ReferenceGrants: make(map[types.NamespacedName]gatewayv1beta1.ReferenceGrant), | ||
} |
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.
Do we really need to perform this initialization here?
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 TLSRoutes
, TCPRoutes
and ReferenceGrants
probably not.
Nil maps are safe to read and it kinds of make sense to me that the caller will not try to write back on the GatewayResources
struct without checking first. Well, if it does, it's the caller's problem anyway.
Besides, returning nil
for TLSRoutes
, TCPRoutes
and ReferenceGrants
seems semantically the right thing to do, I reckon. I'll fix it.
Thanks!
… (mapping, filtering), instead of custom implementation
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.
Thanks @guicassolato !
added my comments, mostly nits.
backendRef: toBackendRef(""), | ||
} | ||
|
||
if ps := conf.ProviderSpecific[ProviderName]; ps != nil { |
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 ensures that all those flags are set if ps != nil?
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.
They don't have to. toNamespacedName
and toBackendRef
funcs will fallback to the defaults, which are analogous in effect to an empty gatewayClassName
.
IOW, we need this nil check not to panic, but we don't need to check each individual flag.
resourcesNamePrefixes[resourcesNamePrefix]++ | ||
if resourcesNamePrefixes[resourcesNamePrefix] > 1 { | ||
resourcesNamePrefix = fmt.Sprintf("%s-%d", resourcesNamePrefix, resourcesNamePrefixes[resourcesNamePrefix]+1) |
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 add logs and/or comments here to explain the logic?
It will also be useful for @Devaansh-Kumar project to build notifications system for conversion results.
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.
similar comments for the the helper functions below, adding a line explaining when we you take a not obvious decisions will be very useful
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.
Sure. I can add the comment.
Out of curiosity though, what would a more obvious decision here be in your opinion?
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've added comments. Please let me know if we want logs as well, in which case I'd probably also expect some definition regarding #157 (comment), to avoid multiple formats of log messages.
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.
sorry, I did not meant that your choice was not obvious as "I would choose something else". But when a user is invoking the tool, I am not sure it will be straight forward to them.
@Devaansh-Kumar is working on a notification system, so he might consider this as an INFO notification, and comments in the code will help him.
Hopefully this makes sense.
No need for logs.
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.
No problem, @LiorLieberman! You're absolutely right about making things clear. What's obvious to some people may not be to others indeed. I'm glad you brought it up. Thanks!
log.Printf("%s provider: invalid OpenAPI 3.x spec: %v", ProviderName, err) | ||
return nil, nil |
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.
why nil and not 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.
Maybe I didn't get what you meant at #157 (comment)? Or shall we make readSpecFromFile
return an error, then ReadResourcesFromFile
catch the error and, depending on the type, raise it or not?
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 I meant in #157 (comment) is the latter. And yes, for that comment it was important that ReadResourcesFromFile
would not return error, otherwise the tool will fail for all providers
.
However, I opened #158 to change the default mode to choose no providers. Will soon get to do this.
So I think for now you should make readSpecFromFile
return an error, log this error in ReadResourcesFromFile
and return nil. Once I make a PR for #158 I will change that to return the 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.
Did you implemented this?
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 guess I missed this one. Doing it now...
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.
Done
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.
Oops. Forgot to fix the tests. Doing it now.
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.
Now done.
* provider-specific conf renamed as provided-specific flags * mutex to read/write provider-specific flag definitions wrapped within a type along with the definitions themselves * minor string handling enhancements (concatenation, trim prefix) * additional comments explaining logics and reasoning throughout the code (thread-safety, helper funcs and expressions, etc)
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 I answered all the open comments.
This looks good to me, going to approve it and I asked @mlavacca to review and leave final lgtm.
Thanks Gui!
/approve
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.
Looks great to me, thanks @guicassolato!
/ok-to-test
/approve
/lgtm
/hold for implementing the requested changes from last review. |
33f49b0
to
b6d703e
Compare
b6d703e
to
b174c23
Compare
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.
/lgtm
I'll let @LiorLieberman take a final look and remove the unhold the PR
[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:
Approvers can indicate their approval by writing |
/unhold |
@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 😞 |
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?:
Try this PR out: