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.

Sign up for GitHub

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

Jump to bottom

fix(dao) invalidate client certificate cache when updating #8934

Merged
merged 4 commits into from
Jun 16, 2022
Merged

fix(dao) invalidate client certificate cache when updating #8934

merged 4 commits into from
Jun 16, 2022

Conversation

liverpool8056
Copy link
Contributor

@liverpool8056 liverpool8056 commented Jun 13, 2022 •
edited
Loading

When updating client certificate, the certificate in core_cache should be invalidation, and the corresponding cache item for the new certificate should be used. Currently this logic doesn't work.

Summary

The logic for invalidating a certificate in core_cache doesn't work when a update event occurs.

Full changelog

change the logic how to get cache_key for certificate in core_cache and add a test

@liverpool8056 liverpool8056 requested a review from a team as a code owner June 13, 2022 07:37
@CLAassistant
Copy link

CLAassistant commented Jun 13, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

mayocream
mayocream suggested changes Jun 13, 2022
View reviewed changes
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
@mayocream mayocream changed the title fix(certificate) fix certificate cache when updating certificate fix(dao) invalidate client certificate cache when updating Jun 13, 2022
@liverpool8056 liverpool8056 requested a review from mayocream June 14, 2022 05:48
mayocream
mayocream suggested changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@mayocream mayocream left a comment

Choose a reason for hiding this comment

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

Could you move the test to spec/02-integration/05-proxy/18-upstream_tls_spec.lua? this file contains both service and upstream client cert tests.

spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
spec/02-integration/04-admin_api/21-client_certificate.lua Outdated Show resolved Hide resolved
@liverpool8056 liverpool8056 requested a review from windmgc June 14, 2022 07:43
@mayocream
Copy link
Contributor

Could you move the test to spec/02-integration/05-proxy/18-upstream_tls_spec.lua? this file contains both service and upstream client cert tests.

@liverpool8056 ^, please also sign the CLA above.

@pull-request-size pull-request-size bot added size/M and removed size/L labels Jun 15, 2022
@liverpool8056 liverpool8056 reopened this Jun 15, 2022
@mayocream @liverpool8056 @windmgc
fix(dao) invalidate client certificate cache when updating
4257248 
Co-authored-by: robin.xiang <[email protected]>
Co-authored-by: Qirui(Keery) Nie <[email protected]>
@mayocream mayocream force-pushed the FTI-4078-fix-certificate-cache branch from 53afe3e to 4257248 Compare June 15, 2022 03:31
@liverpool8056
Copy link
Contributor Author

liverpool8056 commented Jun 15, 2022 •
edited
Loading

Could you move the test to spec/02-integration/05-proxy/18-upstream_tls_spec.lua? this file contains both service and upstream client cert tests.

@liverpool8056 ^, please also sign the CLA above.

already sign

@github-actions github-actions bot removed the core/db label Jun 15, 2022
@liverpool8056 liverpool8056 requested a review from mayocream June 15, 2022 06:28
windmgc
windmgc approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@windmgc windmgc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm from my side

mayocream
mayocream suggested changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@mayocream mayocream left a comment

Choose a reason for hiding this comment

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

Please add an entry for this PR in the CHANGELOG.md

@liverpool8056 liverpool8056 requested a review from mayocream June 15, 2022 10:29
mayocream
mayocream reviewed Jun 15, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@liverpool8056 liverpool8056 requested a review from mayocream June 15, 2022 12:32
@fffonion fffonion merged commit 1fe3ce1 into master Jun 16, 2022
@fffonion fffonion deleted the FTI-4078-fix-certificate-cache branch June 16, 2022 06:23
@mayocream mayocream mentioned this pull request Nov 2, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@windmgc windmgc windmgc approved these changes

@mayocream mayocream mayocream approved these changes

@chronolaw chronolaw Awaiting requested review from chronolaw

Assignees
No one assigned
Labels
changelog core/proxy size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liverpool8056 @CLAassistant @mayocream @windmgc @chronolaw @fffonion