[master] Acme fixes#65721
Closed
jeffclay wants to merge 7 commits intosaltstack:masterfrom
jeffclay:acme-fixes
Closed
[master] Acme fixes#65721jeffclay wants to merge 7 commits intosaltstack:masterfrom jeffclay:acme-fixes
jeffclay wants to merge 7 commits intosaltstack:masterfrom
jeffclay:acme-fixes
Conversation
Author
|
This is a retry for PR #64685 since I mangled the re-base. |
Two new parameters were added to the ACME salt state to enhance configuration flexibility. The first, 'dns_plugin_propagate_seconds', allows specifying a delay for DNS propagation. The second, 'replace_staging', enables replacement of a same-name staging (test) certificate with a production one.
This commit introduces two new functions, "revoke" and "delete", to the ACME module, empowering users with the ability to revoke and delete certificates. An additional optional argument, "propagation_seconds", is also added for the Cloudflare DNS plugin which affects certificate renewal process.
Added `versionadded` directive to the `revoke` and `delete` functions in the acme module
/home/runner/work/salt/salt/salt/modules/acme.py:docstring of salt.modules.acme.delete:2: WARNING: Explicit markup ends without a blank line; unexpected unindent. /home/runner/work/salt/salt/salt/modules/acme.py:docstring of salt.modules.acme.revoke:2: WARNING: Explicit markup ends without a blank line; unexpected unindent.
The return statements have been added to the `delete` and `revocation` functions in the acme.py module. This ensures that each function will return a value of True upon successful completion
The tests for the acme module have been simplified and streamlined for certificate revocation and deletion. The changes replace verbose setup dictionaries with more direct MagicMock patches and assertions, making the test cases more readable and maintainable. The code also better covers edge cases through more specific field checks in the assertion stage.
Author
|
@dwoz Do I need to do anything here? |
twangboy
approved these changes
Jan 2, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Acme State
Added
dns_plugin_propogate_secondsoption to cert().Added option to replace an existing staging/test certificate with a production cert.
Acme Module
Added
dns_plugin_propogate_secondsoption to cert(). This was previously in the doc string but not actually coded anywhere.Added revoke() with optional reason.
Added delete().
What issues does this PR fix or reference?
Fixes:
#63700
#64686
Previous Behavior
dns_plugin_propogate_secondsonly existed in doc strings.test_certflag then removing the flag, the result was nothing happening due to:salt/salt/states/acme.py
Line 104 in 75269c4
New Behavior
dns_plugin_propogate_secondsexists in Acme state and modulereplace_stagingthat will revoke and delete a staging certificate as long as "STAGING" is in the cert issuer dictionary andtest_certis no longer requested.revokecapability which will delete a cert after revocation due to the--non-interactivecmd switch.deletecapability which deletes certificates. (No need to call this after revoke in sls state or Acme state, just seemed like a natural thing to add along withrevoke.)Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.