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(plugins/acme): fix certificate renew failure issue #12773

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

Water-Melon
Copy link
Contributor

Summary

Fix ACME renewal bug.

Using client.renew_certificate directly as the callback function in ngx_timer_at causes the parameter value to not be the plugin's config.

Checklist

Issue reference

KAG-4008
Fix #12442

@github-actions github-actions bot added plugins/acme cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Mar 22, 2024
@Water-Melon Water-Melon marked this pull request as ready for review March 22, 2024 09:31
@Water-Melon Water-Melon removed the request for review from chronolaw March 22, 2024 11:21
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

Please refine the changelog as customers might unable to understand it.

Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

One more question: there are two backport labels, did we decided to backport this fix?

@chronolaw
Copy link
Contributor

Do we have test case to verify this issue is fixed?

@Water-Melon
Copy link
Contributor Author

Water-Melon commented Mar 22, 2024

Do we have test case to verify this issue is fixed?

This issue was reproduced using my personal domain name + EC2 instance and was verified after the fix.

Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

In general, we should add tests for bugfix before merging.

@Water-Melon
Copy link
Contributor Author

The test for this ticket requires binding a public IP address to a domain name, and then configuring it according to the instructions in https://docs.konghq.com/hub/kong-inc/acme/how-to/ . Afterward, send a PATCH request (also in this document), and we will see this error log in Kong's logs:

... attempt to index local 'config' (a boolean value), context: ngx.timer

After fixing it, if we execute it again, there will be no error log output, and the API will still return {"message":"Renewal process started successfully"}.

Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

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

It seems that it is hard to write a test case for this PR, so I approve it.

@chronolaw chronolaw changed the title fix(acme): fix ACME renewal bug fix(plugins/acme): fix ACME renewal bug Mar 26, 2024
@ADD-SP ADD-SP marked this pull request as draft March 27, 2024 02:24
@ADD-SP
Copy link
Contributor

ADD-SP commented Mar 27, 2024

Missing tests, converted to a draft.

@Water-Melon Water-Melon marked this pull request as ready for review March 28, 2024 06:41
Using client.renew_certificate directly as the callback function in ngx_timer_at
causes the parameter value to not be the plugin's config.

KAG-4008
@ADD-SP ADD-SP changed the title fix(plugins/acme): fix ACME renewal bug fix(plugins/acme): fix a bug that causes failed to review certificates Mar 28, 2024
@ADD-SP ADD-SP changed the title fix(plugins/acme): fix a bug that causes failed to review certificates fix(plugins/acme): fix certificate renew failure Mar 28, 2024
@ADD-SP ADD-SP changed the title fix(plugins/acme): fix certificate renew failure fix(plugins/acme): fix certificate renew failure issue Mar 28, 2024
@ADD-SP ADD-SP merged commit ca2e67c into master Mar 28, 2024
50 checks passed
@ADD-SP ADD-SP deleted the fix/acme_renewal_bug branch March 28, 2024 09:00
github-actions bot pushed a commit that referenced this pull request Mar 28, 2024
Using client.renew_certificate directly as the callback function in ngx_timer_at
causes the parameter value to not be the plugin's config.

KAG-4008

(cherry picked from commit ca2e67c)
@team-gateway-bot
Copy link
Collaborator

Successfully created backport PR for release/3.5.x:

github-actions bot pushed a commit that referenced this pull request Mar 28, 2024
Using client.renew_certificate directly as the callback function in ngx_timer_at
causes the parameter value to not be the plugin's config.

KAG-4008

(cherry picked from commit ca2e67c)
@team-gateway-bot
Copy link
Collaborator

Successfully created backport PR for release/3.6.x:

@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee plugins/acme size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when triggering renewal of cert in acme plugin
6 participants