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

refactor: align using CEL for target ref validation #364

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Dec 8, 2023

  • refactor: use standard lib instead for slices
    • The experimental slices package has been moved to the standard library from go 1.21 which allows us to remove this as a direct dependency
  • refactor: remove redundant validation code from auth policy
    • Target Ref validation is already covered by CEL validation
  • refactor: use kubebuilder validation for supported target refs for rlp
    • Align with AuthPolicy which uses CEL validation instead
  • test: target ref validation integrations tests for RLP and AP
    • refactor unit tests to integration tests to check that target ref CEL is working as expected
  • refactor: integration test auth policy route selector CEL
    • Route selector validation is already covered by CEL. Remove redundant validation code in Validation() function and add new integration tests for testing CEL validation
  • refactor: rlp route selector validation using CEL
    • Align using CEL for RLP route selector validation like in AP. Add new integration test for testing route selector CEL for RLP

@KevFan KevFan requested a review from a team as a code owner December 8, 2023 11:49
@KevFan KevFan self-assigned this Dec 8, 2023
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #364 (aa55a03) into main (13c75de) will decrease coverage by 0.93%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   66.14%   65.21%   -0.93%     
==========================================
  Files          35       35              
  Lines        3843     3795      -48     
==========================================
- Hits         2542     2475      -67     
- Misses       1113     1126      +13     
- Partials      188      194       +6     
Flag Coverage Δ
integration 70.73% <ø> (-1.34%) ⬇️
unit 58.90% <ø> (-0.65%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1beta1 (u) ∅ <ø> (∅)
pkg/common (u) 76.92% <ø> (ø)
pkg/istio (u) 37.11% <ø> (ø)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 33.21% <ø> (ø)
pkg/rlptools (u) 56.46% <ø> (ø)
controllers (i) 70.73% <ø> (-1.34%) ⬇️
Files Coverage Δ
api/v1beta2/authpolicy_types.go 72.85% <ø> (-4.72%) ⬇️
api/v1beta2/ratelimitpolicy_types.go 17.85% <ø> (-9.01%) ⬇️
controllers/authpolicy_status.go 94.44% <ø> (ø)
controllers/kuadrant_status.go 73.72% <ø> (-3.39%) ⬇️
controllers/ratelimitpolicy_istio_wasmplugin.go 77.77% <ø> (-1.67%) ⬇️
controllers/ratelimitpolicy_status.go 98.11% <ø> (ø)
pkg/common/apimachinery_status_conditions.go 100.00% <ø> (ø)
pkg/common/common.go 88.39% <ø> (ø)
pkg/common/gatewayapi_utils.go 68.23% <ø> (ø)

... and 4 files with indirect coverage changes

@KevFan KevFan added kind/enhancement New feature or request size/small area/api Changes user facing APIs dependencies labels Dec 8, 2023
@KevFan KevFan force-pushed the minor-refactor branch 2 times, most recently from ce7f1db to a0a1d63 Compare December 8, 2023 13:08
@alexsnaps alexsnaps added area/dependencies Changes dependencies and removed dependencies labels Dec 8, 2023
@alexsnaps alexsnaps added this to the v0.6.0 milestone Dec 8, 2023
@@ -116,6 +116,8 @@ func (l Limit) CountersAsStringList() []string {
// RateLimitPolicySpec defines the desired state of RateLimitPolicy
type RateLimitPolicySpec struct {
// TargetRef identifies an API object to apply policy to.
// +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'"
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

🧇

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

great contribution!

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

I wonder if we aren't ready to remove as well the validation at:

// prevents usage of routeSelectors in a gateway AuthPolicy

and
// prevents usage of routeSelectors in a gateway RLP

WDYT?

@KevFan
Copy link
Contributor Author

KevFan commented Dec 11, 2023

I wonder if we aren't ready to remove as well the validation at:

// prevents usage of routeSelectors in a gateway AuthPolicy

and

// prevents usage of routeSelectors in a gateway RLP

WDYT?

@guicassolato Yes, I think AuthPolicy is covered

// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.routeSelectors)",message="route selectors not supported when targeting a Gateway"
// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules.authentication) || !self.rules.authentication.exists(x, has(self.rules.authentication[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules.metadata) || !self.rules.metadata.exists(x, has(self.rules.metadata[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules.authorization) || !self.rules.authorization.exists(x, has(self.rules.authorization[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules.response) || !has(self.rules.response.success) || !has(self.rules.response.success.headers) || !self.rules.response.success.headers.exists(x, has(self.rules.response.success.headers[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules.response) || !has(self.rules.response.success) || !has(self.rules.response.success.dynamicMetadata) || !self.rules.response.success.dynamicMetadata.exists(x, has(self.rules.response.success.dynamicMetadata[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules.callbacks) || !self.rules.callbacks.exists(x, has(self.rules.callbacks[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
so I'll remove the redundant validation.

But RateLimitPolicy is not covered

// RateLimitPolicySpec defines the desired state of RateLimitPolicy

I'll add a new commit to align RateLimitPolicy with AuthPolicy for this bit also and add new integration tests 👍

@KevFan KevFan merged commit 5ab0be7 into Kuadrant:main Dec 13, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Changes user facing APIs area/dependencies Changes dependencies kind/enhancement New feature or request size/small
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants