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

pdns: reconstruct zone URLs to enable non-root folder API endpoints #2141

Merged
merged 2 commits into from
May 27, 2024

Conversation

jotasi
Copy link
Contributor

@jotasi jotasi commented Mar 15, 2024

As I was trying to both better understand the behavior and create a patch for myself that makes lego work for my DNS provider's API, I have implemented a fix for #2128 even as the discussion in the issue has not yet progressed any further. If you require further discussion before this PR should be considered, please let me know.

This corresponds to the first of the two solutions, that I've proposed in #2128. It replaces the usage of the (at least for my provider) absolute URL zone.URL, which resulted in incorrect URLs being used when the API endpoint is not located at the server's root, by relative URLs. AFAICS, these should now work in all cases (both if the API endpoint is located at the server's root and if it isn't).

The two added test cases for non-root API endpoints also demonstrate the issue (if you add them without the changes to the code, the tests fail).

The constructed URLs reflect the URLs as they are documented in the PDNS API documentation for the update for a specific zone and for notifying for a specific zone. The description in the API is not 100% unambiguous as {zone_id} is not explicitly defined as to be the same as the Zone's ID returned by the API for that zone (as "id"). But both conceptually (as the zone's ID's main feature is URL-embedability) and from looking at the PDNS code (at least to the level that I could dig through it without spending a lot of time on it), this should be the case. If this is true, I don't see a downside of constructing the URLs in this way for existing use cases and it enables mine.

Sadly, I don't have a PDNS installation to play with and test this further (and also no time to set one up for this). Therefore, I cannot verify myself that it properly works in all cases except for the automated tests and my considerations listed above.

Fixes #2128

@ldez ldez self-requested a review March 15, 2024 20:01
@jotasi jotasi force-pushed the feature/enable_pdns_rooted_at_path branch from b54178a to df7dfff Compare March 24, 2024 14:13
@jotasi
Copy link
Contributor Author

jotasi commented Mar 24, 2024

I rebased onto the current master to keep the branch up to date. I'll do so every once in a while. If this is not helpful for your review, please let me know.

@ldez
Copy link
Member

ldez commented Mar 24, 2024

You don't need to rebase unless there are conflicts.

@ldez ldez force-pushed the feature/enable_pdns_rooted_at_path branch from df7dfff to 1805ea5 Compare May 27, 2024 20:14
@ldez ldez added the bug label May 27, 2024
@ldez ldez added this to the v4.17 milestone May 27, 2024
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 5eb8768 into go-acme:master May 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

pdns: API endpoint not at URL root resulting in incorrect URL queried and thus failing with error code 404
2 participants