add support for mijn.host DNS challenge #3749
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
Add support for mijn.host dns challenge |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes include the addition of support for the "mijn.host" DNS service in the Let's Encrypt project. This update encompasses modifications across multiple files, including the changelog, documentation, Dockerfile, build configuration, and settings files. The updates introduce new variables, configuration options, and instructions to facilitate the integration of the new DNS provider, enhancing the overall functionality of the system. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
letsencrypt/config.yaml (1)
79-79: New Configuration Field ApprovedThe addition of the
mijn_host_api_keyfield is correctly implemented. It's crucial to ensure that documentation is updated to guide users on how to obtain and use this API key.
|
This needs a newer ACME version. Let's update Certbot first before adding more DNS challenge options. |
|
okay, I have raised PR in certbot repo, waiting for the maintainers to merge it. |
|
Maybe I was a bit unclear: mijn certbot plug-in requires ACME 2.11.0, but we ship with 2.7.4 currently: Maybe you could ask |
|
thanks, I saw you new PR where you upgrade the acme version |
|
by when can you merge this pr @agners |
|
@shubhamku044 with #3751 merged, you can now rebase/merge this PR and move forward. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
letsencrypt/config.yaml (1)
103-104: LGTM: New DNS providers added correctly.The addition of
dns-mijn-hostanddns-google-domainsto the provider list is correct and aligns with the PR objective. However, consider maintaining alphabetical order for better readability:dns-gandi|dns-gehirn|dns-godaddy|dns-google|dns-google-domains|\ dns-hetzner|dns-infomaniak|dns-joker|dns-linode|dns-luadns|dns-mijn-host|dns-namecheap|dns-netcup|dns-njalla|dns-nsone|\
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- letsencrypt/CHANGELOG.md (1 hunks)
- letsencrypt/build.yaml (1 hunks)
- letsencrypt/config.yaml (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
letsencrypt/CHANGELOG.md (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.letsencrypt/build.yaml (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.letsencrypt/config.yaml (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (3)
letsencrypt/build.yaml (1)
27-27: LGTM! Verify consistency with PR objectives.The addition of
CERTBOT_DNS_MIJN_HOST_VERSION: 0.0.3is consistent with the existing structure and aligns with the PR objective of adding support for the mijn.host DNS challenge.To ensure this change is properly integrated, please run the following verification:
This script will help verify the proper integration of the mijn.host DNS challenge support and identify any potential conflicts or missing configurations.
letsencrypt/config.yaml (1)
77-77: LGTM: New API key field for mijn.host added correctly.The addition of the
mijn_host_api_keyfield is consistent with the PR objective and follows the existing pattern for DNS provider configurations.letsencrypt/CHANGELOG.md (1)
3-6: Changelog entry looks good.The new entry for version 5.2.1 is correctly formatted, follows the versioning convention, and clearly describes the added feature. It aligns well with the PR objectives of adding support for the mijn.host DNS challenge.
|
Hm, seems there is still a merge conflict: I guess the bump now bumped cryptography too high (to 43.0.1 🙈 ). Can the plug-in maybe loosen that restriction 🤔 |
|
sure, I will make the necessary changes |
|
FWIW, PR #3776 reverts Cryptography to 42 also for another reason, so you can rebase on this instead. |
Co-authored-by: Stefan Agner <[email protected]>
|
hey @agners I have changed the version of cryptography, now it will work fine |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (1)
64-64: Fix extra whitespace in blank lines.Remove the extra spaces in these blank lines to maintain consistent formatting with the rest of the file.
- +Also applies to: 69-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
letsencrypt/CHANGELOG.md(2 hunks)letsencrypt/DOCS.md(4 hunks)letsencrypt/Dockerfile(2 hunks)letsencrypt/build.yaml(1 hunks)letsencrypt/config.yaml(2 hunks)letsencrypt/rootfs/etc/cont-init.d/file-structure.sh(1 hunks)letsencrypt/rootfs/etc/services.d/lets-encrypt/run(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- letsencrypt/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (4)
letsencrypt/DOCS.md (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
letsencrypt/Dockerfile (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
letsencrypt/build.yaml (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
letsencrypt/config.yaml (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (10)
letsencrypt/build.yaml (1)
28-28: LGTM! Version dependencies are properly aligned
The changes correctly integrate the mijn.host DNS provider while maintaining compatibility with:
- ACME 2.11.0 (required by the plugin)
- Updated cryptography version (42.0.8)
letsencrypt/config.yaml (2)
80-80: LGTM: API key field follows schema conventions.
The new mijn_host_api_key field is correctly defined as an optional string parameter, maintaining consistency with other DNS provider configurations.
107-107: LGTM: Provider list updated correctly.
The addition of dns-mijn-host to the provider list follows the naming convention and maintains proper alphabetical ordering.
letsencrypt/Dockerfile (2)
24-24: LGTM: Build argument follows naming convention.
The new build argument CERTBOT_DNS_MIJN_HOST_VERSION is correctly declared and follows the established naming pattern for DNS provider version arguments.
75-75: Version compatibility check required.
While the plugin installation follows the correct format, there are important version compatibility concerns:
- The PR comments indicate that
certbot-dns-mijn-hostrequires ACME 2.11.0, but the current version is 2.7.4. - There were reported conflicts with the
cryptographylibrary version.
Let's verify the version requirements:
Consider the following approach:
- Wait for the ACME version upgrade PR mentioned by reviewer
agners - Once merged, rebase this PR on top of the updated ACME version
- Verify that all version conflicts are resolved before proceeding
letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (1)
38-38: LGTM! The mijn.host DNS provider integration looks good.
The addition follows the established pattern for DNS provider configurations and handles the API key securely.
Let's verify that the configuration option is properly defined:
✅ Verification successful
The mijn.host DNS provider configuration is properly defined and documented ✅
The verification confirms:
- The
mijn_host_api_keyconfiguration option is correctly defined inconfig.yamlas an optional string parameter - The documentation in
DOCS.mdincludes:- Complete example configuration
- Clear instructions for obtaining the API key
- Proper integration with the DNS challenge providers list
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mijn.host API key configuration is defined in config.yaml
# Test: Check if the configuration option exists in config.yaml
rg -A 2 'mijn_host_api_key:' letsencrypt/config.yaml
# Test: Ensure the configuration is documented
rg -A 5 'mijn.host' letsencrypt/DOCS.md
Length of output: 818
letsencrypt/rootfs/etc/services.d/lets-encrypt/run (2)
66-68: LGTM! Implementation follows the established pattern.
The implementation correctly:
- Checks for the DNS challenge type and provider
- Requires the necessary API key configuration
- Uses consistent argument structure with propagation seconds
66-68: Verify version compatibility with dependencies.
Based on the PR objectives, this implementation requires ACME 2.11.0 while the current version is 2.7.4. Additionally, there were mentioned conflicts with the cryptography library versions.
Please ensure that:
- The ACME version has been updated to 2.11.0
- The
cryptographyversion conflicts have been resolved
✅ Verification successful
Version requirements are satisfied
Based on the build.yaml and CHANGELOG.md files, the implementation is fully compatible with the required versions:
- ACME and Certbot are both at version 2.11.0 (upgraded from 2.7.4)
- Cryptography is at version 42.0.8
- The new
certbot-dns-mijn-hostplugin is installed at version 0.0.3
The version conflicts mentioned in the PR objectives have been resolved as evidenced by the successful version upgrades documented in the changelog: "Update Certbot/ACME to 2.11.0 & update all plugins to their latest version"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check installed versions of dependencies
# Test: Check installed certbot version
certbot --version
# Test: Check installed cryptography version
python3 -c "import cryptography; print(cryptography.__version__)"
# Test: Check if certbot-dns-mijn-host plugin is installed
pip list | grep certbot-dns-mijn-host
Length of output: 349
Script:
#!/bin/bash
# Let's check the dependency specifications in configuration files
# Look for requirements.txt or similar files
fd -e txt -e ini requirements
# Check Dockerfile or similar files for version specifications
fd -e dockerfile Dockerfile
fd -H -g "*Dockerfile*"
# Check package configurations
fd -e json package.json
fd -e toml pyproject.toml
# Search for version specifications in the codebase
rg -i "certbot.*version|acme.*version|cryptography.*version" -A 2 -B 2
# Look specifically in the letsencrypt directory
ls -la letsencrypt/
cat letsencrypt/Dockerfile 2>/dev/null
cat letsencrypt/build.json 2>/dev/null
Length of output: 13285
letsencrypt/DOCS.md (2)
63-63: LGTM: DNS provider correctly added to the supported providers list.
The new provider "dns-mijn-host" is properly added in alphabetical order in both locations of the supported providers list.
Also applies to: 1103-1103
1053-1072: LGTM: Configuration example is well-documented.
The mijn.host DNS challenge configuration example:
- Follows the established documentation pattern
- Includes all necessary configuration fields
- Provides clear instructions for obtaining the API key
- Includes recommended propagation seconds value
…ns into dns-mijn-host
|
hi @agners |
|
When will this PR be merged into master? I was planning developing support for this provider, but then I saw there was this PR already with the functionality. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores