feat(stackable-webhook)!: Add support for mutating webhooks #1119
Conversation
NickLarsenNZ
left a comment
NickLarsenNZ
left a comment
There was a problem hiding this comment.
Left some comments.
Also a few things for me to check into which shouldn't block you.
Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>
Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>
Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>
Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>
Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>
Techassi
left a comment
Techassi
left a comment
There was a problem hiding this comment.
Blocking for now, because there are a whole bunch of notes I have on this. These are best discussed verbally instead of writing back and forth.
This reverts commit bea8241. It actually caused lifetime problems in commons-operator
| /// ``` | ||
| pub struct WebhookServer { | ||
| options: WebhookServerOptions, | ||
| webhooks: Vec<Box<dyn Webhook>>, |
There was a problem hiding this comment.
note: In my opinion, this crate is only aimed at implementing Kubernetes webhooks, not any generic webhook. Kubernetes has a well-known set of supported webhooks: the conversion and admission webhooks. As such, we should use an enum in combination with enum_dispatch.
| #[derive(Clone, Debug)] | ||
| pub struct WebhookServerOptions { |
There was a problem hiding this comment.
note: This had a whole bunch of documentation. Why is this removed? I spent a significant amount of time writing up proper documentation for all the public items in this crate.
note: This had a builder. Why did it get removed?
| /// Creates a new webhook server with the given config and list of [`Webhook`]s. | ||
| /// | ||
| /// The server listens on `socket_addr` which is provided via the [`WebhookOptions`] and handles | ||
| /// routing based on the provided Axum `router`. Most of the time it is sufficient to use | ||
| /// [`WebhookOptions::default()`]. See the documentation for [`WebhookOptions`] for more details | ||
| /// on the default values. | ||
| /// | ||
| /// To start the server, use the [`WebhookServer::run()`] function. This will | ||
| /// run the server using the Tokio runtime until it is terminated. | ||
| /// | ||
| /// ### Basic Example | ||
| /// | ||
| /// ``` | ||
| /// use stackable_webhook::{WebhookServer, WebhookOptions}; | ||
| /// use axum::Router; | ||
| /// | ||
| /// # async fn test() { | ||
| /// let router = Router::new(); | ||
| /// let (server, cert_rx) = WebhookServer::new(router, WebhookOptions::default()) | ||
| /// .await | ||
| /// .expect("failed to create WebhookServer"); | ||
| /// # } | ||
| /// ``` | ||
| /// | ||
| /// ### Example with Custom Options | ||
| /// | ||
| /// ``` | ||
| /// use stackable_webhook::{WebhookServer, WebhookOptions}; | ||
| /// use axum::Router; | ||
| /// | ||
| /// # async fn test() { | ||
| /// let options = WebhookOptions::builder() | ||
| /// .bind_address([127, 0, 0, 1], 8080) | ||
| /// .add_subject_alterative_dns_name("my-san-entry") | ||
| /// .build(); | ||
| /// | ||
| /// let router = Router::new(); | ||
| /// let (server, cert_rx) = WebhookServer::new(router, options) | ||
| /// .await | ||
| /// .expect("failed to create WebhookServer"); | ||
| /// # } | ||
| /// ``` | ||
| /// Please read their documentation for details. |
There was a problem hiding this comment.
note: Again, this had so much more documentation before, which is still valid after the change. Also, it should still provide a (basic) example on how to use it, and not just refer to the downstream impl.
| for webhook in &webhooks { | ||
| router = webhook.register_routes(router); | ||
| } |
There was a problem hiding this comment.
suggestion: We can add a debug statement here to log the registration of routes we add the the core webhook server.
There was a problem hiding this comment.
Are you thinking of
tracing::debug!("Registering webhook routes");or something more advanced? I can't really think of what else to log here 🙈
There was a problem hiding this comment.
Ah, we don't have access to which route we are adding at this point, but we can maybe add such a log statement where we do know the routes.
| /// | ||
| /// ### Example usage | ||
| /// | ||
| /// TODO |
There was a problem hiding this comment.
note: Well... We should add that...
| handler_state: Arc<S>, | ||
| _resource: PhantomData<R>, | ||
|
|
||
| disable_validating_webhook_configuration_maintenance: bool, |
There was a problem hiding this comment.
suggestion: This is a mouthful with a bunch of superfluous information: The fact that we are talking about validation webhooks is already indicated by the struct name itself and the maintenance is dealing with the validating webhook config.
| disable_validating_webhook_configuration_maintenance: bool, | |
| disable_maintenance: bool, |
| handler_state: Arc<S>, | ||
| _resource: PhantomData<R>, | ||
|
|
||
| disable_mutating_webhook_configuration_maintenance: bool, |
There was a problem hiding this comment.
note: Same as in the validating webhook.
| validating_webhook_configuration: ValidatingWebhookConfiguration, | ||
| handler: H, | ||
| handler_state: Arc<S>, | ||
| _resource: PhantomData<R>, | ||
|
|
||
| disable_validating_webhook_configuration_maintenance: bool, | ||
| client: Client, |
There was a problem hiding this comment.
note: Add documentation to the fields.
| validating_webhook_configuration: ValidatingWebhookConfiguration, | ||
| handler: H, | ||
| handler_state: Arc<S>, | ||
| disable_validating_webhook_configuration_maintenance: bool, | ||
| client: Client, | ||
| field_manager: String, |
There was a problem hiding this comment.
note: We should reduce the number of parameters here.
Description
Part of stackabletech/commons-operator#111
Used by stackabletech/commons-operator#387
BREAKING: Refactor the entire
WebhookServermechanism, so multiple webhooks can run in parallel.Put individual webhooks (currently
ConversionWebhookandMutatingWebhook) behind theWebhooktrait.Definition of Done Checklist
Author
Reviewer
Acceptance