Add Arvancloud provider #1722
Add Arvancloud provider #1722
Conversation
| raise Exception("create_record: %s", err.response.json()["errors"]) | ||
|
|
||
| LOGGER.debug("create_record: %s", created) | ||
| return created |
There was a problem hiding this comment.
You can simply return True and forget about the created variable.
| # type, name and content are used to filter records. | ||
| # If possible filter during the query, otherwise filter after response is received. | ||
| def list_records(self, rtype=None, name=None, content=None): | ||
| filter = {"per_page": 10} |
There was a problem hiding this comment.
Do you want/can increase the number of items per page to improve network performances?
| "id": record["id"], | ||
| } | ||
|
|
||
| if content: |
There was a problem hiding this comment.
You can use a single if and directly test that content in processed_record['content']. A None value will always evaluate this expression to False.
| ) | ||
|
|
||
| output.update( | ||
| { |
There was a problem hiding this comment.
Globally I feel that your implementation is leaking out the Arvancloud internal representation of a DNS record to Lexicon. It would mean that what you pass in the content field something very different than for the other providers if you select the arvancloud provider.
Lexicon is aiming to propose a unified interface that is independent from a given provider. In that extent, the expected value passed in content should be only a singular value (host for CNAME, ip for A/AAAA, free text for TXT etc.).
So I would prefer that we stick as much as possible to this unified API and avoid to carry specific data in the content field.
This mostly impacts complex records with multiple elements like MX, or SRV, and also CNAME, ANAME and A/AAAA in your case:
- for
CNAMEandANAME, I think we could agree that only thesourceoption proposed by Arvancloud is relevant, - for
MXandSRV, the priority can be set with the dedicated flag--priorityproposed by Lexicon CLI and relevant for this case (you will access it in the provider usingself._get_lexicon_option('priority'), - for the remaining elements in
SRV, it is undecided at Lexicon CLI level and it depends on the provider implementation: here you can continue to split the value to extract the target, port and weight, until we define a better interface at Lexicon level to describe these records, - finally
A/AAAA; this is the first time I see things likeportorweightfor this kind of records, and I do not see which meaning in could have for Arvancloud. In my opinion, we should keep things simple as for all other providers. AndA/AAAArecord contains just an IPv4/IPv6 singular value, and we put defaultport/weightvalues (or no values if supported) for the calls to Arvancloud API.
Sounds good to you?
| "TXT": (lambda: {"value": {"text": content}}), | ||
| "SPF": (lambda: {"value": {"text": content}}), | ||
| "DKIM": (lambda: {"value": {"text": content}}), | ||
| }.get(rtype, lambda: {})() |
There was a problem hiding this comment.
Very smart way to have execution branches thanks to deferred evaluations, I like it!
|
|
||
| content_split = content.split(" ") | ||
|
|
||
| if rtype in ["A", "AAAA"]: |
There was a problem hiding this comment.
See my comment below about how to format A and AAAA records content in Lexicon.
|
|
||
| output.update( | ||
| { | ||
| "A": (lambda: {"value": ", ".join([" ".join(str(value) if value is not None and value != "" else "" for value in item.values()) for item in input])}), |
There was a problem hiding this comment.
See my discussion above about how records should be represented, but at the end I think it should simply be "A": (lambda: {"value": input['ip']}).
Also current implementation triggers mypy: the input field of this method is certainly not a string.
Finally, what about the other supported records, like AAAA?
| @@ -95,6 +96,8 @@ def __init__(self): | |||
| self.provider_module = None | |||
|
|
|||
| def setup_method(self, _): | |||
| warnings.filterwarnings("ignore", category=ResourceWarning, | |||
There was a problem hiding this comment.
I am concerned by this filter. The test is generating alerts about unclosed sockets? Seems like a problem of resource cleaning in the provider implementation.
ResourceWarningforunclosed ssl.SSLSocket