-
Notifications
You must be signed in to change notification settings - Fork 188
feat: Add httproute support for vmauth. #1628
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
Conversation
d39103a to
96734bb
Compare
|
overall looks good, please add unit (internal/controller/operator/factory/vmauth/vmauth_test.go) and e2e (test/e2e/vmauth_test.go) tests for this feature |
Thanks! I've already added the tests. |
|
I'm not sure if this is an issue. When I add the following to vmauth_controller: and then run |
it doesn't add manifests to config/rbac/, please add it manually |
I have added the RBAC manifests to config/rbac/. Regarding the Gateway API, I added a check for the HTTPRoute CRD. Currently, if the CRD is missing, the operator simply skips creating the HTTPRoute. However, should the operator update the Status to inform the user that their requested HTTPRoute was not created due to the missing CRD? |
|
if CreateOrUpdate returns error it will automatically set status to Failed. Just need to write proper log message and text in error message |
|
Please let me know if there’s anything else I should update. |
Co-authored-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
|
Great work! I am interested in this feature as well so thanks for the effort <3 Quick points I noticed during review:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
Thank you for the feedback!
I’ll add the missing reference to VMAuthSpec in the documentation.
It looks like this is a documentation mistake. @AndrewChubatiuk Should I update it in this PR? |
right, looks like a documentation issue. no need to do it |
|
Hi, may I ask if this is ready to be merged? |
Co-authored-by: Vadim Rutkovsky <roignac@gmail.com>
Co-authored-by: Vadim Rutkovsky <roignac@gmail.com>
Co-authored-by: Vadim Rutkovsky <roignac@gmail.com>
5907bef to
69a8294
Compare
vrutkovs
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.
Looks good, two more code change suggestions
Co-authored-by: Vadim Rutkovsky <roignac@gmail.com>
Co-authored-by: Vadim Rutkovsky <roignac@gmail.com>
f41gh7
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.
Overall looks good! Please address minor GetCRD comment
f41gh7
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.
LGTM
|
Thanks for contribution! |
feat: #1625
Hello,
I’ve extended the vmspec.spec to include an httpRoute definition, for example:
This configuration creates an HTTPRoute resource that binds the VMAuth instance to the specified Gateway.
Additionally, the httpRoute spec looks like this: