-
-
Notifications
You must be signed in to change notification settings - Fork 300
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 Arvancloud provider #1722
base: master
Are you sure you want to change the base?
Add Arvancloud provider #1722
Conversation
) | ||
|
||
output.update( | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
CNAME
andANAME
, I think we could agree that only thesource
option proposed by Arvancloud is relevant, - for
MX
andSRV
, the priority can be set with the dedicated flag--priority
proposed 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 likeport
orweight
for 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
/AAAA
record contains just an IPv4/IPv6 singular value, and we put defaultport
/weight
values (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ResourceWarning
forunclosed ssl.SSLSocket