✨ Networking config for default mode webhooks #379
Conversation
bhperry
force-pushed
the
default-webhook-config
branch
from
ffa7865 to
6bcba1c
Compare
|
Warning Rate limit exceeded@bhperry has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
## Walkthrough
This change extends the ClusterManager CustomResourceDefinition and its Go type definitions to support detailed webhook server configuration for both Default and Hosted deployment modes. It introduces new configuration fields for webhook servers, updates validation rules, augments deep copy logic, and enhances Swagger documentation to reflect these additions. Additionally, a test was updated to reflect changes in default port values for Hosted mode webhook configurations and to add coverage for Default mode default ports.
## Changes
| Files/Paths | Change Summary |
|-----------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| operator/v1/0000_01_operator.open-cluster-management.io_...yaml | Extended CRD schema: added `default` under `deployOption` with webhook server configuration fields; added new fields (`healthProbeBindAddress`, `hostNetwork`, `metricsBindAddress`) to both `default` and `hosted` webhook configs; added `port` with default 9443 for `default` mode; updated hosted mode webhook configs with new fields and retained port default 443; refined descriptions and validation for webhook configuration fields. |
| operator/v1/types_clustermanager.go | Added `DefaultClusterManagerConfiguration` struct; updated `ClusterManagerDeployOption` to include `Default`; extended `WebhookConfiguration` with new fields (`HealthProbeBindAddress`, `MetricsBindAddress`, `HostNetwork`); introduced separate `DefaultWebhookConfiguration` (with `Port` default 9443) and `HostedWebhookConfiguration` (with required `Address` and `Port` default 443); changed `Address` to required only in Hosted mode. |
| operator/v1/zz_generated.deepcopy.go | Added deep copy methods for `DefaultClusterManagerConfiguration`, `DefaultWebhookConfiguration`, and `HostedWebhookConfiguration`; updated `ClusterManagerDeployOption` deep copy logic to handle new `Default` field. |
| operator/v1/zz_generated.swagger_doc_generated.go | Added SwaggerDoc methods for `DefaultClusterManagerConfiguration`, `DefaultWebhookConfiguration`, and `HostedWebhookConfiguration`; updated docs for `ClusterManagerDeployOption` and `WebhookConfiguration` to include new fields and detailed descriptions; removed `address` and `port` from base `WebhookConfiguration` Swagger docs and moved them to hosted/default specific docs. |
| test/integration/api/clustermanager_test.go | Replaced `WebhookConfiguration` with `HostedWebhookConfiguration` in Hosted mode tests; clarified default port expectations for Hosted mode; added new test case verifying default port values (9443) for Default mode webhook configurations; retained existing test logic and error checks. |✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bhperry
force-pushed
the
default-webhook-config
branch
2 times, most recently
from
849e553 to
b2da1b0
Compare
operator/v1/types_clustermanager.go
Outdated
| RegistrationWebhookConfiguration WebhookConfiguration `json:"registrationWebhookConfiguration,omitempty"` | ||
|
|
||
| // WorkWebhookConfiguration represents the customized webhook-server configuration of work. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="self.address != ''",message="Address is required for hosted webhook configuration" |
There was a problem hiding this comment.
it seems cel validation is not added in the crd definition? Is it because we are using a lower version of crd gen?
There was a problem hiding this comment.
@qiujian16 Pushed change to inline common settings in separate Default and Hosted configuration structs. I didn't like that Address was left unused in Default mode. Also we get to keep the simple required Address validation for hosted (instead of CEL) as well as different default ports for each. What do you think?
There was a problem hiding this comment.
I think it is ok. We could have a better name for it I think.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bhperry, qiujian16 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 |
bhperry
force-pushed
the
default-webhook-config
branch
2 times, most recently
from
b859cf1 to
ab6dcf9
Compare
Signed-off-by: Ben Perry <[email protected]>
bhperry
force-pushed
the
default-webhook-config
branch
from
ab6dcf9 to
dc6f14d
Compare
| } | ||
|
|
||
| // WebhookConfiguration has two properties: Address and Port. | ||
| // WebhookConfiguration represents customization of webhook servers |
There was a problem hiding this comment.
actually it is not WebhookConfiguration, I think we could also use this for controller to set healthiness and metrics endpoint?
Summary
API changes to support customizing webhook ports and hostNetwork in Default installation mode
Related issue(s)
open-cluster-management-io/ocm#1035
Summary by CodeRabbit