Skip to content
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: loadbalancer source range #611

Merged
merged 6 commits into from
Oct 25, 2024

Conversation

jds9090
Copy link
Contributor

@jds9090 jds9090 commented Oct 23, 2024

Closes #610

Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for kamaji-documentation ready!

Name Link
🔨 Latest commit 723827f
🔍 Latest deploy log https://app.netlify.com/sites/kamaji-documentation/deploys/6719f31a75ba2100070f8065
😎 Deploy Preview https://deploy-preview-611--kamaji-documentation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@chanho3114
Copy link

This is a feature I really want. You're a genius. I hope this PR gets merged into the release as soon as possible.

Let me know if you'd like any changes!

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely useful, it would be great if we could have CEL validation here, as we're doing with other fields.

In the TenantControlPlaneSpec struct something like this:

// +kubebuilder:validation:XValidation:rule="(self.controlPlane.service.serviceType != 'LoadBalancer') && size(self.networkProfile.loadBalancerSourceRanges) > 0", message="load balancer source ranges are supported only with LoadBalancer Service type"

@jds9090 may I ask you to give it a try? According to the CEL Playground it seems correct, but better having a real smoke test!

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated review

@@ -229,6 +229,7 @@ func NewCmd(scheme *runtime.Scheme) *cobra.Command {
},
},
handlers.TenantControlPlaneServiceCIDR{},
handlers.TenantControlPlaneLoadBalancerSourceRanges{},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're able to achieve the same result with CEL we can decrease the validation at the webhook server, and Kamaji will benefit a lot!

@jds9090
Copy link
Contributor Author

jds9090 commented Oct 24, 2024

This is definitely useful, it would be great if we could have CEL validation here, as we're doing with other fields.

In the TenantControlPlaneSpec struct something like this:

// +kubebuilder:validation:XValidation:rule="(self.controlPlane.service.serviceType != 'LoadBalancer') && size(self.networkProfile.loadBalancerSourceRanges) > 0", message="load balancer source ranges are supported only with LoadBalancer Service type"

@jds9090 may I ask you to give it a try? According to the CEL Playground it seems correct, but better having a real smoke test!

Thank you for the suggestion to leverage CEL for the validation logic. However, after further review, I realized that validating CIDR formats exceeds the capabilities of CEL. CEL is excellent for field existence checks, size validations, and relational comparisons but lacks the capability to handle complex logic such as network-related validation like CIDR parsing.

In our case, the ability to:

  • Ensure valid CIDR formats (e.g., identifying malformed inputs like 192.168.0.0/33).

it seems reasonable to implement the CIDR validation logic using a webhook.

In this case, we can leverage CEL for the simpler rule enforcement—like ensuring loadBalancerSourceRanges is used only with LoadBalancer services—while delegating the actual CIDR format validation to the webhook. This approach ensures correctness while minimizing the reliance on external webhooks.

If this sounds good, I’ll proceed with keeping the CIDR-specific logic to the webhook, moving only the basic checks in CEL.

Is this what you meant?

@prometherion
Copy link
Member

I got your point and you're right. Let's keep the handler just for the cidr validation, then, there shouldn't be any problem with CEL and custom validation.

@jds9090
Copy link
Contributor Author

jds9090 commented Oct 24, 2024

This is definitely useful, it would be great if we could have CEL validation here, as we're doing with other fields.

In the TenantControlPlaneSpec struct something like this:

// +kubebuilder:validation:XValidation:rule="(self.controlPlane.service.serviceType != 'LoadBalancer') && size(self.networkProfile.loadBalancerSourceRanges) > 0", message="load balancer source ranges are supported only with LoadBalancer Service type"

@jds9090 may I ask you to give it a try? According to the CEL Playground it seems correct, but better having a real smoke test!

I changed the rule because it is not valid in my test cases.

// +kubebuilder:validation:XValidation:rule="!has(self.networkProfile.loadBalancerSourceRanges) || (size(self.networkProfile.loadBalancerSourceRanges) == 0 || self.controlPlane.service.serviceType == 'LoadBalancer')", message="LoadBalancer source ranges are supported only with LoadBalancer service type"

Components of the Rule

  1. !has(self.networkProfile.loadBalancerSourceRanges)
    This checks whether the loadBalancerSourceRanges field is missing or not present. If the field is not present, the rule allows the object to pass validation.

  2. size(self.networkProfile.loadBalancerSourceRanges) == 0
    This checks if the loadBalancerSourceRanges is an empty list (i.e., []). If it is empty, the rule allows the object to pass.

  3. self.controlPlane.service.serviceType == 'LoadBalancer'
    This ensures that if there are non-empty CIDR ranges in loadBalancerSourceRanges, the serviceType must be 'LoadBalancer'. This means CIDR ranges are only allowed for LoadBalancer` type services.

  4. Logical OR (||)
    The OR logic ensures that any of the following conditions allows the resource to pass:

  • If loadBalancerSourceRanges is not provided.
  • If loadBalancerSourceRanges is an empty list.
  • If serviceType is 'LoadBalancer' when loadBalancerSourceRanges is non-empty.

This validation logic ensures the following:

  1. LoadBalancerSourceRanges is optional.
  2. If loadBalancerSourceRanges is provided, it can be empty (no specific CIDR restrictions).
  3. If non-empty CIDR ranges are provided, the serviceType must be 'LoadBalancer'`.
  4. If the serviceType is not 'LoadBalancer', CIDR ranges must not be provided (they can only be present for LoadBalancer services).

@prometherion prometherion merged commit 4e8c2b6 into clastix:master Oct 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add LoadBalancerSourceRanges Field and Validation Logic for TenantControlPlane
3 participants