Determine credential type from label #4825

rainest · 2023-10-12T00:03:16Z

What this PR does / why we need it:

Adds a label alternative to the kongCredType Secret field. Setting konghq.com/credential: CREDTYPE is now equivalent to setting a kongCredType=CREDTYPE Secret field.

Which issue this PR fixes:

Fix #2502

Special notes for your reviewer:

#2502 (comment) contains a migration script that will eventually go in docs. There isn't a clear place in 3.x docs to stick an upgrade section, so #4854 to follow up. As a stopgap, the changelog links to the comment.

Adds new system, does not remove old system. Not sure we want to single release breaking change this; I think we can do the final removal after 3.0 unless we want to bundle as many breaking changes at once, even if they're larger ones that didn't previously have a deprecation period: #4853

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

codecov · 2023-10-12T00:33:12Z

Codecov Report

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

Comparison is base (2089f87) 77.7% compared to head (68533b3) 77.9%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4825     +/-   ##
=======================================
+ Coverage   77.7%   77.9%   +0.2%     
=======================================
  Files        164     165      +1     
  Lines      18399   18455     +56     
=======================================
+ Hits       14298   14389     +91     
+ Misses      3281    3256     -25     
+ Partials     820     810     -10

Files	Coverage Δ
...ion/validation/consumers/credentials/validation.go	`96.2% <100.0%> (+6.8%)`	⬆️
internal/dataplane/parser/parser.go	`92.6% <100.0%> (+2.6%)`	⬆️
internal/util/credential.go	`100.0% <100.0%> (ø)`
internal/admission/handler.go	`61.2% <0.0%> (ø)`
internal/dataplane/kongstate/kongstate.go	`83.0% <92.3%> (+2.8%)`	⬆️

... and 13 files with indirect coverage changes

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

rainest · 2023-10-13T22:37:11Z

Observed a flake in https:/Kong/kubernetes-ingress-controller/actions/runs/6511654773/attempts/1?pr=4825 but Github isn't showing any artifacts in the summary 😢

2023-10-13T18:32:19.8431950Z     translation_failures_test.go:287: received event's message (failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)) does not contain the expected reason: 'failed to construct certificate from secret'
2023-10-13T18:32:19.8456195Z     translation_failures_test.go:306: waiting for events, received so far:
2023-10-13T18:32:19.8458524Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)"
2023-10-13T18:32:19.8460802Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to construct certificate from secret"
2023-10-13T18:32:19.8462572Z         * Secret/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2: "failed to construct certificate from secret"
2023-10-13T18:32:20.8435119Z     translation_failures_test.go:287: received event's message (failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)) does not contain the expected reason: 'failed to construct certificate from secret'
2023-10-13T18:32:20.8456966Z     translation_failures_test.go:306: waiting for events, received so far:
2023-10-13T18:32:20.8459510Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)"
2023-10-13T18:32:20.8461884Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to construct certificate from secret"
2023-10-13T18:32:20.8463723Z         * Secret/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2: "failed to construct certificate from secret"
2023-10-13T18:32:21.8396897Z     translation_failures_test.go:261: 
2023-10-13T18:32:21.8398816Z         	Error Trace:	/home/runner/work/kubernetes-ingress-controller/kubernetes-ingress-controller/test/integration/translation_failures_test.go:261
2023-10-13T18:32:21.8400316Z         	Error:      	Condition never satisfied
2023-10-13T18:32:21.8401419Z         	Test:       	TestTranslationFailures/ingress_referring_a_secret_with_no_valid_TLS_key-pair

internal/admission/handler.go

internal/admission/validation/consumers/credentials/validation.go

internal/dataplane/kongstate/kongstate.go

internal/admission/validation/consumers/credentials/validation.go

pmalek · 2023-10-17T15:08:30Z

Observed a flake in https:/Kong/kubernetes-ingress-controller/actions/runs/6511654773/attempts/1?pr=4825 but Github isn't showing any artifacts in the summary 😢

2023-10-13T18:32:19.8431950Z     translation_failures_test.go:287: received event's message (failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)) does not contain the expected reason: 'failed to construct certificate from secret'
2023-10-13T18:32:19.8456195Z     translation_failures_test.go:306: waiting for events, received so far:
2023-10-13T18:32:19.8458524Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)"
2023-10-13T18:32:19.8460802Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to construct certificate from secret"
2023-10-13T18:32:19.8462572Z         * Secret/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2: "failed to construct certificate from secret"
2023-10-13T18:32:20.8435119Z     translation_failures_test.go:287: received event's message (failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)) does not contain the expected reason: 'failed to construct certificate from secret'
2023-10-13T18:32:20.8456966Z     translation_failures_test.go:306: waiting for events, received so far:
2023-10-13T18:32:20.8459510Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to fetch the secret (f5538e3a-6211-440e-bcbd-3650432100a6/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2)"
2023-10-13T18:32:20.8461884Z         * Ingress/translation-failures-c86b6152-5e04-4aed-bdbe-bda6254336fc: "failed to construct certificate from secret"
2023-10-13T18:32:20.8463723Z         * Secret/translation-failures-d45b20b5-31e4-40fa-8fd2-e48520d3d0b2: "failed to construct certificate from secret"
2023-10-13T18:32:21.8396897Z     translation_failures_test.go:261: 
2023-10-13T18:32:21.8398816Z         	Error Trace:	/home/runner/work/kubernetes-ingress-controller/kubernetes-ingress-controller/test/integration/translation_failures_test.go:261
2023-10-13T18:32:21.8400316Z         	Error:      	Condition never satisfied
2023-10-13T18:32:21.8401419Z         	Test:       	TestTranslationFailures/ingress_referring_a_secret_with_no_valid_TLS_key-pair

Should be fixed by #4871

internal/util/credential.go

CHANGELOG.md

* feat(credentials) accept label credential type * pr: remove bad copy/paste resource failure * pr: reuse credential type code and add unit * pr: log instead of failure, align test with new error * use correct label in changelog --------- Co-authored-by: Grzegorz Burzyński <[email protected]>

pmalek · 2023-10-19T10:12:06Z

CHANGELOG.md

+ credential type instead of the `kongCredType` field. This allows controller
+ compontents to avoid caching unnecessary Secrets. The `kongCredType` field is
+ still supported but is now deprecated. A script to generate commands to
+ update Secrets is available at https:/Kong/kubernetes-ingress-controller/issues/2502#issuecomment-1758213596


I suggest we track changing this changelog entry to reference a proper docs link (or an equivalent) rather than a github issue link

* feat(credentials) accept label credential type * pr: remove bad copy/paste resource failure * pr: reuse credential type code and add unit * pr: log instead of failure, align test with new error * use correct label in changelog --------- Co-authored-by: Grzegorz Burzyński <[email protected]>

pmalek · 2023-10-19T10:15:43Z

I'd assume we need to add validation to webhook to check secrets with the introduced label? (so that eventually we'd only check those instead of inspecting fields?)

czeslavo · 2023-10-19T10:51:33Z

I'd assume we need to add validation to webhook to check secrets with the introduced label? (so that eventually we'd only check those instead of inspecting fields?)

Yeah, that should do the job: #4896

pull-request-size bot added the size/L label Oct 12, 2023

rainest force-pushed the feat/label-secrets branch 3 times, most recently from 13dc7ee to 4fef47b Compare October 13, 2023 00:18

rainest mentioned this pull request Oct 13, 2023

Remove kongCredType support entirely #4853

Closed

3 tasks

rainest force-pushed the feat/label-secrets branch from 4fef47b to 56e6d15 Compare October 13, 2023 18:11

rainest marked this pull request as ready for review October 13, 2023 22:37

rainest requested a review from a team as a code owner October 13, 2023 22:37

czeslavo requested changes Oct 17, 2023

View reviewed changes

rainest force-pushed the feat/label-secrets branch from c14822d to 236697d Compare October 17, 2023 20:40

rainest added 3 commits October 17, 2023 14:03

feat(credentials) accept label credential type

70a6985

pr: remove bad copy/paste resource failure

cd36c2d

pr: reuse credential type code and add unit

3da7474

rainest force-pushed the feat/label-secrets branch from 236697d to 3da7474 Compare October 17, 2023 21:03

czeslavo reviewed Oct 17, 2023

View reviewed changes

internal/util/credential.go Outdated Show resolved Hide resolved

pr: log instead of failure, align test with new error

899b80b

rainest force-pushed the feat/label-secrets branch from cb16248 to 899b80b Compare October 17, 2023 22:35

czeslavo mentioned this pull request Oct 18, 2023

Credentials secrets are not validated if not referenced by a consumer #4885

Closed

1 task

rainest requested a review from czeslavo October 18, 2023 20:11

czeslavo reviewed Oct 19, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

use correct label in changelog

68533b3

czeslavo approved these changes Oct 19, 2023

View reviewed changes

czeslavo merged commit 1ceb6a8 into main Oct 19, 2023
35 checks passed

czeslavo deleted the feat/label-secrets branch October 19, 2023 09:12

pmalek added this to the KIC v3.0.0 milestone Oct 19, 2023

pmalek reviewed Oct 19, 2023

View reviewed changes

pmalek mentioned this pull request Oct 19, 2023

fix: run basic credentials validation despite relation with consumers #4887

Merged

1 task

This was referenced Nov 2, 2023

fix: fix webhook validation of KongConsumers referring Secrets with konghq.com/credential label #5067

Merged

KongConsumers referring Secrets with konghq.com/credential label without keyCredType field are rejected #5069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Determine credential type from label #4825

Determine credential type from label #4825

rainest commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

rainest commented Oct 13, 2023

pmalek commented Oct 17, 2023

pmalek Oct 19, 2023

pmalek commented Oct 19, 2023

czeslavo commented Oct 19, 2023

Determine credential type from label #4825

Determine credential type from label #4825

Conversation

rainest commented Oct 12, 2023 • edited Loading

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

rainest commented Oct 13, 2023

pmalek commented Oct 17, 2023

pmalek Oct 19, 2023

Choose a reason for hiding this comment

pmalek commented Oct 19, 2023

czeslavo commented Oct 19, 2023

rainest commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading