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

fix(admission): use Ignore failure policy in tests (same as charts) #5063

Merged
merged 8 commits into from
Nov 3, 2023

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Nov 2, 2023

What this PR does / why we need it:

Uses the same failure policy (Ignore) in the test admission webhook configuration as the one used in charts and deploy-admission-controller.sh script.

The policy tells Kubernetes what to do in case status 500 is returned from the webhook:

  • Ignore - accept the object.
  • Fail - do not accept the object.

Given that we're using Ignore in production, we should use the same in tests. Without that, we may have test cases where we expect an object to be rejected, but we returned status 500, which in production would still accept the object.

What validations will this bring back to life for installations using default failurePolicy:

  • HTTPRoute:
    • queryparam matching is supported with expression router only
    • group is not a supported group for httproute backendRefs, only core is supported
    • kind is not a supported kind for httproute backendRefs, only Service is supported
    • no parentRef matched gateway
    • sectionname referenced listener was not found on gateway
    • no listeners could be found for gateway
    • HTTPRoute not supported by listener
    • HTTPRoute failed schema validation (route could not be generated in parser)
    • referenced gateway not found
    • referenced gatewayclass not found
  • KongConsumer:
    • failed to fetch managed KongConsumers from cache
    • consumer referenced non-existent credentials secret
    • consumer credential failed validation

Which issue this PR fixes:

Related issue: #4680

Special notes for your reviewer:

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

@czeslavo czeslavo self-assigned this Nov 2, 2023
@czeslavo czeslavo added fix and removed size/XS labels Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (6fb8469) 73.3% compared to head (3e3c7cb) 75.6%.
Report is 3 commits behind head on main.

❗ Current head 3e3c7cb differs from pull request most recent head ac4860d. Consider uploading reports for the commit ac4860d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5063     +/-   ##
=======================================
+ Coverage   73.3%   75.6%   +2.2%     
=======================================
  Files        167     167             
  Lines      18909   18911      +2     
=======================================
+ Hits       13867   14299    +432     
+ Misses      4218    3788    -430     
  Partials     824     824             
Files Coverage Δ
internal/admission/handler.go 60.5% <100.0%> (-0.6%) ⬇️
internal/admission/validation/gateway/httproute.go 90.0% <81.8%> (+<0.1%) ⬆️
internal/admission/validator.go 72.7% <42.8%> (-0.7%) ⬇️

... and 25 files with indirect coverage changes

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

@czeslavo czeslavo force-pushed the fix/use-webhook-ignore-failure-policy branch from d677c33 to 178b999 Compare November 2, 2023 17:53
@czeslavo czeslavo changed the title fix: use Ignore failure policy in tests (same as charts) fix: use Ignore failure policy in tests (same as charts) Nov 2, 2023
@czeslavo czeslavo force-pushed the fix/use-webhook-ignore-failure-policy branch from 36dcaca to 1132773 Compare November 2, 2023 19:29
@czeslavo czeslavo force-pushed the fix/use-webhook-ignore-failure-policy branch 2 times, most recently from dc12bf7 to 19fc915 Compare November 2, 2023 19:32
@czeslavo czeslavo marked this pull request as ready for review November 2, 2023 19:32
@czeslavo czeslavo requested a review from a team as a code owner November 2, 2023 19:32
@czeslavo czeslavo changed the title fix: use Ignore failure policy in tests (same as charts) fix(admission): use Ignore failure policy in tests (same as charts) Nov 2, 2023
@czeslavo czeslavo force-pushed the fix/use-webhook-ignore-failure-policy branch from 19fc915 to d6a692b Compare November 3, 2023 09:07
@czeslavo czeslavo force-pushed the fix/use-webhook-ignore-failure-policy branch from d6a692b to 3e3c7cb Compare November 3, 2023 09:46
CHANGELOG.md Outdated Show resolved Hide resolved
@czeslavo czeslavo requested a review from pmalek November 3, 2023 13:38
@czeslavo czeslavo enabled auto-merge (squash) November 3, 2023 13:42
@czeslavo czeslavo merged commit a364f35 into main Nov 3, 2023
41 checks passed
@czeslavo czeslavo deleted the fix/use-webhook-ignore-failure-policy branch November 3, 2023 13:58
github-actions bot pushed a commit that referenced this pull request Nov 3, 2023
czeslavo added a commit that referenced this pull request Nov 3, 2023
czeslavo added a commit that referenced this pull request Nov 3, 2023
…#5063) (#5078)

(cherry picked from commit a364f35)

Co-authored-by: Grzegorz Burzyński <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants