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

add support for misaka.io #556

Closed
wants to merge 1 commit into from
Closed

add support for misaka.io #556

wants to merge 1 commit into from

Conversation

ym
Copy link
Contributor

@ym ym commented Sep 3, 2020

Hello,

I'd like to propose adding support for misaka.io DNS service.

Regards,

@packagr-io-beta
Copy link
Contributor

Hi.

I'm an automated pull request bot named Packagr. I handle testing, versioning and package releases for this project.

  • If you're the owner of this repo, you can click the button below to kick off the Packagr build pipeline and create an automated release.'
  • If not, don't worry, someone will be by shortly to check on your pull request.

Packagr


If you're interested in learning more about Packagr, or adding it to your project, you can check it out here

Copy link
Collaborator

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! Here is my review.

if name:
name = self._relative_name(name)

new_identifier = f'{self.domain_id}/recordsets/{name}/{rtype}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

So misaka.io does not implement identifiers in their API, and you need to generate these IDs on Lexicon side.

Storing the parameters in the identifier to retrieve the record associated is a smart move. However, it has some drawbacks on CLI usability, since from user side you need to deal with a complex string instead of usual simple identifiers expected here (serie of number or some letters).

I personally prefer to use another approach in this situation. I calculate dynamically the identifier to use from an hash-baset approach. Typical code is:

@staticmethod
def _identifier(record):
sha256 = hashlib.sha256()
sha256.update(('type=' + record.get('type', '') + ',').encode('utf-8'))
sha256.update(('name=' + record.get('name', '') + ',').encode('utf-8'))
sha256.update(('data=' + record.get('data', '') + ',').encode('utf-8'))
return sha256.hexdigest()[0:7]

Then, when an identifier is provided to update or delete a record, I get all records of the zone, calculate the identifier for each of them, and select the one corresponding to the identifier provider.

This way, you have an identifier that is not exposing some internals of the fetching logic, and keep the identifier simple (here 8 characters).

It can be a problem however if fetching all records is quite slow for the DNS API.

What do you think of this approach?

Copy link
Contributor Author

@ym ym Sep 5, 2020

Choose a reason for hiding this comment

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

As a compromise, we can remove {self.domain_id}/recordsets/ part from the identifier.
Since domain is a mandatory argument we don't need it in identifier.

Local-calculated dynamic identifier does make it easier when typing manually on CLI but its cost is a little bit higher and I would like to avoid it if it's not required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it would leave only {name}/{type}. Ok I think we can work from that. I propose you the following:

  • we always use the relative_name, not the full_name for the name parameter, since full_name can be processed from relative_name using the domain if needed.
  • we encode the {name}/{type} in Base64URL (base64.urlsafe_b64encode()) to get the actual identifier for maximum portability
  • as a bonus we can strip the trailing padding = characters with something like https://gist.github.com/cameronmaske/f520903ade824e4c30ab, but I think it not that necessary

@ym ym requested a review from AnalogJ as a code owner January 10, 2021 17:38
@adferrand adferrand mentioned this pull request Apr 19, 2022
@adferrand
Copy link
Collaborator

Closed by #1205

@adferrand adferrand closed this Apr 19, 2022
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