✨ Networking config for default mode webhooks #379
Conversation
bhperry
force-pushed
the
default-webhook-config
branch
from
ffa7865 to
6bcba1c
Compare
WalkthroughAdds mode-specific webhook configuration: introduces a Default mode with bind settings and extends Hosted mode to include bindConfiguration; updates CRD schema and Go API types (new BindConfiguration, Default/Hosted webhook types), refreshes generated deepcopy code, and updates integration tests to use HostedWebhookConfiguration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
bhperry
force-pushed
the
default-webhook-config
branch
2 times, most recently
from
849e553 to
b2da1b0
Compare
bhperry
force-pushed
the
default-webhook-config
branch
3 times, most recently
from
ab6dcf9 to
dc6f14d
Compare
|
@bhperry would you like to rebase the PR and update accordingly? I think it is ready to be merged in general |
Signed-off-by: Ben Perry <[email protected]>
bhperry
force-pushed
the
default-webhook-config
branch
from
dc6f14d to
948c0b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
226-233: Add minimum constraint to hosted external portsThe hosted webhook configurations in the CRD are missing a
minimum: 1to enforce that the external port is always ≥ 1. Please update both occurrences inoperator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:
- hosted.registrationWebhookConfiguration.port (around lines 226–233)
- hosted.workWebhookConfiguration.port (around lines 282–288)
port: default: 443 description: Port represents the external port of a webhook-server. The default value of Port is 443. format: int32 + minimum: 1 maximum: 65535 type: integer
♻️ Duplicate comments (1)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
94-176: CRD descriptions: replace “BindAddress… port” with “Port…” for clarity.Multiple descriptions still say “BindAddress represents the primary listen port…”. Update to “Port represents the primary listen port…”, consistent with the Go type.
You’ll get this automatically if you update the Go comment and regenerate; however, if you’re curating this CRD in-repo, apply these in the sections below:
- spec.deployOption.default.registrationWebhookConfiguration.bindConfiguration.port.description
- spec.deployOption.default.workWebhookConfiguration.bindConfiguration.port.description
Example diff (one occurrence shown):
- description: BindAddress represents the primary listen port of a server. The default value is 9443. + description: Port represents the primary listen port of a server. The default value is 9443.
🧹 Nitpick comments (1)
operator/v1/types_clustermanager.go (1)
284-294: Optional: consider pointer fields for DefaultWebhookConfiguration to express optionality.These are marked +optional in comments, but the fields are value types. In recent Go versions, omitempty will drop zero-value structs, but using pointers is the conventional pattern across K8s APIs and avoids serializing
{}objects when no configuration is provided.If you want to follow the prevailing style, consider:
type DefaultClusterManagerConfiguration struct { - RegistrationWebhookConfiguration DefaultWebhookConfiguration `json:"registrationWebhookConfiguration,omitempty"` - WorkWebhookConfiguration DefaultWebhookConfiguration `json:"workWebhookConfiguration,omitempty"` + RegistrationWebhookConfiguration *DefaultWebhookConfiguration `json:"registrationWebhookConfiguration,omitempty"` + WorkWebhookConfiguration *DefaultWebhookConfiguration `json:"workWebhookConfiguration,omitempty"` }This is optional, not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(3 hunks)operator/v1/types_clustermanager.go(3 hunks)operator/v1/zz_generated.deepcopy.go(5 hunks)test/integration/api/clustermanager_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/api/clustermanager_test.go
🔇 Additional comments (10)
operator/v1/types_clustermanager.go (3)
295-304: Hosted webhook type swap looks correct and aligns with comments/resolution.Switching to HostedWebhookConfiguration for both registration and work is consistent with the discussion and CRD. No issues spotted.
335-340: LGTM: Using a nested BindConfiguration pointer allows mode-specific bind tuning.This fits the goal of supporting hostNetwork and per-listener ports without leaking this into Hosted’s external endpoint semantics.
375-378: LGTM: New Default deploy option block is well-scoped.Including the Default-specific configuration under DeployOption.default is a clean separation and won’t affect Hosted consumers.
operator/v1/zz_generated.deepcopy.go (6)
78-93: LGTM: Deep-copy for BindConfiguration is correct.Value copy is sufficient given all fields are scalars.
186-197: LGTM: DeepCopy now handles Default deploy option.Allocating and deep-copying Default ensures mode-specific config is correctly duplicated.
315-332: LGTM: DefaultClusterManagerConfiguration deep copy covers both nested webhook configs.No shallow-copy leaks.
333-353: LGTM: DefaultWebhookConfiguration deep-copies BindConfiguration pointer.Using allocation plus value copy is fine because BindConfiguration has no nested pointers.
433-439: LGTM: HostedClusterManagerConfiguration deep copy fixed to deep-copy nested webhook configs.Prevents aliasing between source and destination.
451-471: LGTM: HostedWebhookConfiguration deep copy.Address is a string; BindConfiguration is deep-copied. Looks correct.
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
106-135: Confirm intentional allowance of <=0 for health/metrics ports.CRD currently permits negative values (no minimum) for healthProbePort and metricsPort, consistent with your “<= 0 disables” semantics. If that’s intentional, it’s fine; otherwise, set a minimum of 0. No change required if you want to keep the disable behavior.
No action if intended; just calling it out to avoid accidental schema tightening later.
Also applies to: 145-174, 196-225, 251-280
bhperry
force-pushed
the
default-webhook-config
branch
from
948c0b1 to
1648f89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (2)
226-233: Hosted external port should enforce Minimum=1External port 0/negative is not meaningful. Add Minimum=1 in the Go type (HostedWebhookConfiguration.Port), then regenerate the CRD.
Apply in Go types:
type HostedWebhookConfiguration struct { // ... - // +kubebuilder:default=443 + // +kubebuilder:default=443 + // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=65535 Port int32 `json:"port,omitempty"` }
281-287: Enforce Minimum=1 for hosted work external port as wellMirror the Minimum=1 validation for this port field by updating the Go types and regenerating the CRD.
operator/v1/types_clustermanager.go (2)
341-359: Hosted external port: enforce Minimum=1; optional IPv6 note
- Add Minimum=1 to HostedWebhookConfiguration.Port to reject non-positive values.
- Optional: If IPv6 endpoints should be supported in the future, the regex on Address will need widening (current pattern allows IPv4-style dotted labels but not IPv6).
Proposed change:
// +kubebuilder:default=443 + // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=65535 Port int32 `json:"port,omitempty"`
341-359: Lingering WebhookConfiguration references & missing Port minimum validation
test/integration/api/clustermanager_test.go (≈ lines 154–158):
Rename any mentions ofWebhookConfigurationtoHostedWebhookConfiguration, e.g.:- Context("Set customized WebhookConfiguration", func() { + Context("Set customized HostedWebhookConfiguration", func() {operator/v1/types_clustermanager.go – enforce a minimum port value:
// +optional // +kubebuilder:default=443 // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=65535 Port int32 `json:"port,omitempty"`Regenerate CRDs and verify
validation: minimum: 1appears in
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlBackward-compatibility: if this module is consumed as a library, either introduce a deprecated alias
type WebhookConfiguration = HostedWebhookConfigurationor confirm no downstreams import the old symbol.
♻️ Duplicate comments (4)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (3)
137-176: Mirror the Port minimum/doc fixes for work webhook bind configSame concerns as registration: enforce Minimum=1 for port, and fix “BindAddress” wording via the Go type change so both blocks regenerate correctly.
192-225: Hosted BindConfiguration inherits the same Port minimum/doc issuesPlease rely on the Go-type changes suggested above; the generated hosted.bindConfiguration schema should then gain Minimum=1 and the corrected description automatically.
247-280: Same hosted bindConfiguration fixes apply to work webhookThis block will pick up the corrected Port minimum and description once the Go types are updated and CRD is regenerated.
operator/v1/types_clustermanager.go (1)
306-334: Fix Port validation and description; minor comment punctuation
- Port: require Minimum=1 to prevent invalid/ephemeral binding.
- Doc: “BindAddress represents…” should be “Port represents…”.
- Minor: add a period to the MetricsPort default sentence.
Apply this diff:
type BindConfiguration struct { - // BindAddress represents the primary listen port of a server. The default value is 9443. + // Port represents the primary listen port of a server. The default value is 9443. + // Port must be >= 1. // +optional // +kubebuilder:default=9443 + // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=65535 Port int32 `json:"port,omitempty"` // HealthProbePort represents the bind port of a webhook-server's healthcheck endpoint. The default value is 8000. // Healthchecks may be disabled by setting a value less than or equal to 0. // +optional // +kubebuilder:default=8000 // +kubebuilder:validation:Maximum=65535 HealthProbePort int32 `json:"healthProbePort"` - // MetricsPort represents the bind port for a webhook-server's metric endpoint. The default value is 8080 + // MetricsPort represents the bind port for a webhook-server's metric endpoint. The default value is 8080. // Metrics may be disabled by setting a value less than or equal to 0. // +optional // +kubebuilder:default=8080 // +kubebuilder:validation:Maximum=65535 MetricsPort int32 `json:"metricsPort"` // HostNetwork enables running webhook pods with hostNetwork: true // This may be required in some installations, such as EKS with Calico CNI, // to allow the API Server to communicate with the webhook pods. // +optional HostNetwork bool `json:"hostNetwork,omitempty"` }
🧹 Nitpick comments (2)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
114-120: HostNetwork + identical ports can cause collisions; add guidanceWhen hostNetwork: true is set for both registration and work webhooks, using the same bind port (default 9443) can collide if pods land on the same node. Consider documenting that these ports must differ when hostNetwork is enabled, or add validation in the operator to prevent identical bind ports when both are hostNetwork=true.
Also applies to: 154-159, 205-210, 260-265
operator/v1/types_clustermanager.go (1)
306-334: Operator-side validation/doc for hostNetwork port collisionsWhen hostNetwork=true on both registration and work, identical BindConfiguration.Port defaults (9443) may collide on the same node. Consider:
- Adding a note to the field comments warning that ports should differ if hostNetwork is enabled for multiple webhooks.
- Implementing a validating webhook (in the operator) to prevent identical bind ports when both hostNetwork flags are set.
Happy to draft the comment text or a validation check if you confirm where it belongs (API docs vs. operator validation).
Also applies to: 341-359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(3 hunks)operator/v1/types_clustermanager.go(3 hunks)operator/v1/zz_generated.deepcopy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- operator/v1/zz_generated.deepcopy.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: verify
🔇 Additional comments (4)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
94-101: Default-mode webhook config addition looks goodIntroducing spec.deployOption.default with registration/work webhook blocks is clear and aligns with the API type split. No schema conflicts spotted.
operator/v1/types_clustermanager.go (3)
284-293: DefaultClusterManagerConfiguration addition is well-structuredUsing a dedicated Default-mode struct with a BindConfiguration indirection allows clean mode-specific semantics without overloading Hosted. Good separation of concerns.
299-304: Hosted webhook config type swap looks correctSwitching Hosted to a specific HostedWebhookConfiguration (with address/port and optional bind config) matches the CRD and intended behavior.
375-378: New Default field in ClusterManagerDeployOption is appropriatePointer makes it truly optional; the omitempty on nested structs avoids needless noise in serialized specs. Looks good.
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Ben Perry <[email protected]>
bhperry
force-pushed
the
default-webhook-config
branch
from
1648f89 to
0294dea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/v1/types_clustermanager.go (1)
341-359: Enforce Minimum Port and Clarify IPv4-only Address ValidationThe current regex only matches FQDN labels (and IPv4 dotted-decimal) but not IPv6, while the comment says “IP or fqdn.” Also,
Porthas no minimum constraint, so users can explicitly set it to 0 or negative.Required:
- Add a minimum validation of 1 to the external
Portto prevent invalid (≤0) values.Optional:
- Either update the comment to say “IPv4 address or fully-qualified domain name (FQDN)” to match the existing regex, or expand the pattern to accept IPv6 literals if needed.
Diff (operator/v1/types_clustermanager.go):
@@ -341,7 +341,7 @@ // HostedWebhookConfiguration represents customization of webhook servers running in hosted installation mode type HostedWebhookConfiguration struct { // Address represents the address of a webhook-server. - // It could be in IP format or fqdn format. + // It may be an IPv4 address or a fully-qualified domain name (FQDN). // The Address must be reachable by apiserver of the hub cluster. // +required // +kubebuilder:validation:Required @@ -351,6 +351,7 @@ // +optional // +kubebuilder:default=443 + // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=65535 Port int32 `json:"port,omitempty"`
♻️ Duplicate comments (1)
operator/v1/types_clustermanager.go (1)
306-333: Enforce Port >= 1 for BindConfiguration.PortWithout a minimum, 0/negative ports can slip through and produce unusable listeners. Health/Metrics intentionally allow <=0 to disable; the primary Port should not.
Apply this diff to add the minimum:
// +kubebuilder:default=9443 + // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=65535 Port int32 `json:"port,omitempty"`Optional: to prevent hostNetwork collisions, consider a CEL rule to ensure positive ports are distinct when HostNetwork=true. Example (if/when CEL is acceptable in this API):
- Ports must be unique across {Port, HealthProbePort, MetricsPort} when HostNetwork is true and each is >0.
🧹 Nitpick comments (2)
operator/v1/zz_generated.deepcopy.go (2)
333-353: Nit: prefer using DeepCopyInto for BindConfiguration for consistencySemantically equivalent today, but using DeepCopyInto mirrors other generated code and future-proofs if BindConfiguration ever grows non-primitive fields.
Example change inside DefaultWebhookConfiguration.DeepCopyInto:
- *out = new(BindConfiguration) - **out = **in + *out = new(BindConfiguration) + (*in).DeepCopyInto(*out)
451-471: Nit: same DeepCopyInto preference for HostedWebhookConfigurationAs above, using DeepCopyInto for BindConfiguration keeps style consistent and resilient to future changes.
- *out = new(BindConfiguration) - **out = **in + *out = new(BindConfiguration) + (*in).DeepCopyInto(*out)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(3 hunks)operator/v1/types_clustermanager.go(3 hunks)operator/v1/zz_generated.deepcopy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧰 Additional context used
🧬 Code Graph Analysis (1)
operator/v1/zz_generated.deepcopy.go (1)
operator/v1/types_clustermanager.go (5)
BindConfiguration(307-333)DefaultClusterManagerConfiguration(285-293)HostedClusterManagerConfiguration(296-304)DefaultWebhookConfiguration(336-339)HostedWebhookConfiguration(342-359)
🔇 Additional comments (8)
operator/v1/types_clustermanager.go (4)
284-293: Default-mode webhook configuration struct looks goodClear separation for Default mode via DefaultClusterManagerConfiguration and per-component DefaultWebhookConfiguration aligns with the PR objective.
295-304: Hosted-mode webhook configuration switch LGTMSwitching to HostedWebhookConfiguration per component is consistent with the new design and improves clarity around external vs bind settings.
335-340: DefaultWebhookConfiguration wrapper is cleanPointer BindConfiguration keeps the API minimal by default and only surfaces bind knobs when set. Good.
375-378: New DeployOption.default block is appropriateThe optional Default configuration under DeployOption matches the mode-specific split and keeps the top-level Spec tidy.
operator/v1/zz_generated.deepcopy.go (4)
78-93: DeepCopy for BindConfiguration is correctValue fields only; shallow copy is sufficient.
186-197: DeepCopy includes Default pointer now — goodClusterManagerDeployOption.DeepCopyInto now handles Default; prevents aliasing bugs.
315-332: DefaultClusterManagerConfiguration deepcopy is correctDeep-copies both nested webhook configs; no issues spotted.
433-439: HostedClusterManagerConfiguration deepcopy change LGTMSwitching to DeepCopyInto on nested types is correct.
|
verify fails, you may need to rerun the make update |
Signed-off-by: Ben Perry <[email protected]>
|
/approve |
|
[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 |
openshift-merge-bot
bot
merged commit 67d4817
into
open-cluster-management-io:main
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
New Features
Documentation
Tests