-
Notifications
You must be signed in to change notification settings - Fork 63
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: enable webhooks in sidero-controller-manager
#812
Conversation
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 webhooks work for the caps-controller-manager
as well?
They should, but I haven't really verified that. |
It might be nice to verify that, e.g. with some validation once again, even if that's temporary. |
Yep, there actually was an issue with the configs. I've fixed that: |
Based on siderolabs#794, but instead of using a dedicated controller for all webhooks keep all of them in each own controller. Additionally, implement validation webhooks for `Server` resource, which validates that `bootFromDiskMethod` and `configPatches` are using proper values. Signed-off-by: Artem Chernyshev <artem.chernyshev@talos-systems.com>
/m |
@@ -216,6 +217,7 @@ func main() { | |||
os.Exit(1) | |||
} | |||
|
|||
setupWebhooks(mgr) |
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 still don't get how this would work. In #794 I used a dedicated controller for the webhooks because the reconcilers above this line are actively using the conversion webhooks.
To my understanding this will cause Sidero to be unable to start when any of the CRD's change like proposed in #735, because in that case the reconcilers will try to convert them using the conversion webhooks defined here (but at that point, they are not yet available).
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.
@lion7 I'm going to take a closer look today at your PR and do more testing for the conversion
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.
👍🏻 that would be great, in that case I'll wait with rebasing #735 so it won't accidentally break (I have a working deployment of it running on my cluster).
Based on #794, but instead of
using a dedicated controller for all webhooks keep all of them in each
own controller.
Additionally, implement validation webhooks for
Server
resource, whichvalidates that
bootFromDiskMethod
andconfigPatches
are using propervalues.
Signed-off-by: Artem Chernyshev artem.chernyshev@talos-systems.com