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(admission): validate path with regex supplied in Ingress #4647

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented Sep 12, 2023

What this PR does / why we need it:

Introduce custom validation for Ingress object:

  • uses the Kong Gateway endpoint for validating KongRoute by calling method RouteService.Validate
  • the above allows validating regex expressions configured by users in Ingress paths
  • introduces test coverage for both expressions router and old ones

Which issue this PR fixes:

Closes #4557

Special notes for your reviewer:

Run also test suit manually for Kong Gateway < 3.0 - kong-gateway:2.8.4.2 on this branch to ensure that integration tests that rely on messages returned from the Gateway are compatible

  • results - section run-integration-tests is relevant (it takes KIC code from the branch, not run it from prebuild container image latest)

Since integration tests are run in a matrix for expressions router and old ones it can be covered with tests that are exclusively run when the particular router is used.

Using two class matches ingressClassMatcher and ingressV1ClassMatcher replicates the approach used in other parts of the codebase. Refactoring this everywhere in separate PR in my opinion is worth consideration.

Also moving the whole internal/validation to internal/admission/validation is something to consider too.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@programmer04 programmer04 added area/feature New feature or request area/admission labels Sep 12, 2023
@programmer04 programmer04 added this to the KIC v2.12.0 milestone Sep 12, 2023
@programmer04 programmer04 requested a review from a team as a code owner September 12, 2023 11:33
@programmer04 programmer04 self-assigned this Sep 12, 2023
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 11:38 — with GitHub Actions Inactive
@programmer04 programmer04 enabled auto-merge (squash) September 12, 2023 11:40
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Patch coverage: 18.5% and project coverage change: -0.4% ⚠️

Comparison is base (74ac23c) 68.1% compared to head (b127b6c) 67.7%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4647     +/-   ##
=======================================
- Coverage   68.1%   67.7%   -0.4%     
=======================================
  Files        162     164      +2     
  Lines      19028   19130    +102     
=======================================
- Hits       12969   12966      -3     
- Misses      5290    5392    +102     
- Partials     769     772      +3     
Files Changed Coverage Δ
internal/admission/handler.go 32.7% <0.0%> (-2.6%) ⬇️
internal/util/builder/ingress.go 0.0% <0.0%> (ø)
internal/validation/gateway/httproute.go 70.0% <ø> (ø)
internal/validation/ingress/ingress.go 0.0% <0.0%> (ø)
internal/admission/validator.go 37.9% <14.2%> (-1.5%) ⬇️
internal/dataplane/parser/translate_ingress.go 96.0% <91.3%> (+1.0%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/validation/ingress/ingress.go Show resolved Hide resolved
internal/validation/ingress/ingress.go Outdated Show resolved Hide resolved
internal/validation/ingress/ingress.go Outdated Show resolved Hide resolved
test/integration/ingress_webhook_test.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_ingress.go Outdated Show resolved Hide resolved
test/integration/ingress_webhook_test.go Outdated Show resolved Hide resolved
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 temporarily deployed to gcloud September 12, 2023 16:12 — with GitHub Actions Inactive
@programmer04 programmer04 merged commit 832a7aa into main Sep 13, 2023
145 of 198 checks passed
@programmer04 programmer04 deleted the validate-ingress branch September 13, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admission Webhook validating user-supplied regular expressions
2 participants