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

encode zone names in url paths #74

Merged
merged 2 commits into from
Jul 18, 2024
Merged

encode zone names in url paths #74

merged 2 commits into from
Jul 18, 2024

Conversation

freym
Copy link

@freym freym commented Jul 17, 2024

Hi
We host several RFC23171 zones such as "128/26.2.0.192.in-addr.arpa.". Normal operation like creating the zone or adding/updating new entries can be done with the plugin. If you activate the notify option, the following error is displayed because the zone name is not encoded:

requests.exceptions.HTTPError: 404 Client Error: Not Found for url: http://POWERNDNS/api/v1/servers/localhost/zones/128/26.2.0.192.in-addr.arpa./notify

This PR adds URL encoding for the zone name.

Powerdns uses a special encoding for URLs. Instead of "%2F" for a slash, the slash must be encoded with "=2F". (This must be done in version 4.7.3 from Debian, from version >= 4.8 Powerdns accepts “%2F” and “=2F” as path argument. The output of "/api/v1/servers/localhost/zones" still shows the zone URL with "=2F")

Footnotes

  1. https://datatracker.ietf.org/doc/html/rfc2317#section-4

Copy link
Contributor

@ross ross left a comment

Choose a reason for hiding this comment

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

This seems reasonable. Did you verify that the change doesn't cause problems when things are created unencoded? e.g. before this change, and then things use the encoded version from then on.

@freym
Copy link
Author

freym commented Jul 18, 2024

We have been using this patch for several days without any problems. But we don't have that many DNS zones. I can rewrite the patch so that encoding is optional and you have to explicitly turn it on to avoid endangering existing setups.

@ross
Copy link
Contributor

ross commented Jul 18, 2024

We have been using this patch for several days without any problems. But we don't have that many DNS zones. I can rewrite the patch so that encoding is optional and you have to explicitly turn it on to avoid endangering existing setups.

Playing around with it locally. I'd like to have it support the encoding cleaning/automatically, just want to make sure it doesn't break anything for existing uses. Doesn't seem like it should.

How are you all configuring things with the / in the name since it generally can't be used in (unix) filenames?

@ross
Copy link
Contributor

ross commented Jul 18, 2024

Testing locally seems to have no real problems when things are created before these changes and after. Going to make a couple minor stylistic tweaks to the PR and update the CHANGELOG and we should be good to move forward with it.

@freym
Copy link
Author

freym commented Jul 18, 2024

Great.

How are you all configuring things with the / in the name since it generally can't be used in (unix) filenames?

Our external DNS provider has selected these zone names. And we use PostgreSQL as the backend in Powerdns, which is fine with these names.

Even the linked RFC2317 says:

The examples here use "/" because it was felt to be more visible and
pedantic reviewers felt that the 'these are not hostnames' argument
needed to be repeated. We advise you not to be so pedantic, and to
not precisely copy the above examples, e.g. substitute a more
conservative character, such as hyphen, for "/".

@ross
Copy link
Contributor

ross commented Jul 18, 2024

Pushed up the changes. Only one of consequence was pulling the logic into a helper function and testing it a little bit. Otherwise just added a changelog entry. Please take a look at it and ✅ if it make sense to you.

@freym
Copy link
Author

freym commented Jul 18, 2024

Looks good and works in our test environment.

@ross ross merged commit 2524604 into octodns:main Jul 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants