dns/ddclient: add Netcup DNS API

[Curly060]

Netcup is a German hosting provider who offers an API for DNS manipulation which this plugin makes use of, see:

• Wiki: https://www.netcup-wiki.de/wiki/DNS_API
• Technical documentation: https://ccp.netcup.net/run/webservice/servers/endpoint.php

[Curly060]

This is a new DynDNS plugin for the DNS API of the German hosting provider Netcup. I have created this plugin to my best knowledge, trying to understand the existing new ddns client code.

Me and my friend have tested this on our own installations (ARM and x86) and we believe it works well.

However, I am not so happy with the naming of the input fields. I would have loved to specify my very own custom fields, but instead I had to re-use as many fields as possible. It would have been great if there had been a way that my plugin can state which fields it requires, but instead I am stuck with what is already there.
There is a crude way to only show some fields for certain services, but there is no way to actually hide unwanted fields. Is there a way to dynamically create form fields?

Anyway, I am happy for any remarks and will gladly make any necessary changes to get this accepted.

[fichtner]

TTL will be added in a more generic way in https://github.com/opnsense/plugins/pull/3550/files#diff-ba4d24cee94fcdc665a4223826a1daea10fde3e20971b1e4865ee261d8e2b5f4R174

we should try to avoid service-specific naming

[fichtner]

TTL is in master now

[Curly060]

I have rebased my code against latest master and made the following changes:

• all Netcup related fields were removed. The plugin now only uses existing fields. I have kept the API key password field separately, because
  • it's just so much more intuitive and the user has a help text directly there about the other fields without having to go to the Wiki
  • There is already a provider specific field (resourceId). Why not have this one?
  • The field is very easily hidden if not needed
• some polishing (removed unused exports, better logging and parameter checking)

[fichtner]

It would probably make more sense then to use user+pass as API key+pass and move customer to resourceID. But I think it's required and the GUI will not require it in this case? It needs some sort of constraint added but we can do that post-merge.

[Curly060]

Uh, that would be even more confusing, IMHO.
There is a service specific field for the azure plugin, so why can't another plugin have it's own specific field, too? Yes, with more plugins coming up this can clutter the configuration, I agree.
But abusing an already very specific field for yet another specific purpose is IMHO not a good way forward.

I still believe that every service should have it's very own custom fields with an exact help of what they are for. That would be the best option. But I guess that is not possible with the way the framework works, right?

In that case let me propose another way:
Get rid of all service specific fields and introduce a couple of general purpose fields. If a service then needs some special configuration, it can use those custom fields. These would then all be documented in the Wiki.

Does that sound like a way forward?

[Sumpfdotter]

I can't follow. You might know that it's common that some Web APIs require pairs of secrets. I have the feeling you are not really reading what others are writing. Please explain: Why is it okay to introduce a second visible field "resourceID" instead of concatenating it's value into the user ID field, but it's not okay to introduce a second invisible field "key" or "client secret"? Can you please come up with an logical explaination for that?

[fichtner]

Let’s try to keep this between author and reviewer. It’s hard enough to follow as is. 😉

[Sumpfdotter]

Curly060, can I be become Co-Author? Otherwise I'm not accept here.

Every DDNS has the same problem:

1. Determine the As-Is value (= quering the master DDNS server)
2. Determine and monitor the To-Be value
3. Update the value if needed

Only step no 3 is different for different providers. For that reason it is a good idea to have one OPNSense plugin for all DDNS tasks and sub plugins for the different provider. The proposal to come up with an own OPNSense plugin for Netcup is for sure an antipattern. The discussion for a solution for the missing secret field is blocked. So I cannot see a possible innovation progress here.

[fichtner]

Dude, nothing is blocked. Two proposals are open for implementation. If you think the suggestion to create a separate plugin is overkill then I don’t disagree. I was merely pointing out when it makes sense to use „netcup“ specific fields and when it doesn’t.

[Curly060]

Let's come to an end with this. I have now added a field called "clientSecret" of type "password" with label text "Additional client secret, e.g. an API key". Now the field has nothing to do with Netcup at all, except that the Netcup plugin is currently its only user. But since the name is quite generic, this might change with future plugins coming up. Plus there are already fields that are provider specific and have generic names (ttl, resourceId). So I think this is a compromise between usability and maintainability.

Is the current form acceptable now?

[fichtner]

API key is usually the password and the user name is left blank

[Curly060]

Just to clarify:
The Netcup DNS API requires a login which then returns an API Session ID. For the login the following input is required:

• customer number
• API password
• API key

The closest match to the existing fields were:

• customer number => username
• API password => password
• API key => no existing field, hence the introduction of a new field

It would also be possible to do something like this:
Enter both, API key and API password into the existing password field, separated with e.g. colon:
username field: :
The code could then split(':') the password field and get API key and password from it. However, this would require some documentation somewhere.

[fichtner]

Enter both, API key and API password into the existing password field

That would be my preference as well. Docs are here and a special note for cloudflare is there already: https://docs.opnsense.org/manual/dynamic_dns.html#provider-specific-configuration

If the API key has a fixed length input without separator would be possible as well.

[flennic]

Would be very interested in using this PR implementation. Is this still under active review after the discussion?

[fichtner]

The merge conditions have been laid out. Adding a new field is not an issue but using the API user as password and adding an API key field named clientSecret is highly counter-intuitive.

[fichtner]

See other comment.

[Sumpfdotter]

The merge conditions have been laid out. Adding a new field is not an issue but using the API user as password and adding an API key field named clientSecret is highly counter-intuitive.

I looked at the code: "using the API user as password " sounds like a misunderstanding.

The request uses the standard fields username and password perfectly well: username contains the customer number (Netcup username, which is not a secret password), and password contains the netcup API password (which is a secret password). Netcup requires an additional client secret (API Key) and for that the author introduced a new generic field for a client secret.

Sounds exactly as required?
Cheers!

[fichtner]

To me this makes no sense at all:

        self.netcupCustomerNr = self.settings.get('username')
        self.netcupAPIPassword = self.settings.get('password')
        self.netcupAPIKey = self.settings.get('clientSecret')

And here we are debating the choices of one provider but making a decision forward for ALL possible providers in the future. clientSecret is odd. The mapping is fuzzy.

[Sumpfdotter]

To me this makes no sense at all:

        self.netcupCustomerNr = self.settings.get('username')
        self.netcupAPIPassword = self.settings.get('password')
        self.netcupAPIKey = self.settings.get('clientSecret')

And here we are debating the choices of one provider but making a decision forward for ALL possible providers in the future. clientSecret is odd. The mapping is fuzzy.

Perfect mapping! But even if it does not make sense to you, the mapping does not belong to ALL possible providers, it belongs to the netcup plugin only.

Till now you've not been able to explain you concerns. You are rejecting for no transprant reason. I really like to have that feauture as well. The branch works perfectly in my setup and is very well designed and mapped according to netcup requirements and according to generalizability of the chosen new field.

[AdSchellevis]

let's agree to disagree, you can build your own software if you do want to release something that doesn't match our standards, just don't expect us to merge and maintain code in these cases. end of discussion until requested modifications are made.

[Sumpfdotter]

let's agree to disagree, you can build your own software if you do want to release something that doesn't match our standards, just don't expect us to merge and maintain code in these cases. end of discussion until requested modifications are made.

A standard is something that is written down and results can be verified against. Nobody explained what the criteria is to solve that problem. We all can be lucky that there is more contributors. You like to have some changes? Then you must explain it! You you are just rejecting. End of discussion? Wow, mighty person you are. Create a standard, write it down and then we can go further.

[AdSchellevis]

@Sumpfdotter well, we did write down quite a lot, also about our standards and development workflow (https://docs.opnsense.org/), what did you do in the meantime that would help others?? An empty github profile and complaining about other peoples efforts. Wow...

Feedback provided is mostly about normalization of data, which is quite a generic theme in software engineering. A lot of books have been written about these subjects, usually it's not needed to repeat those.

Consider this my last comment, I still don't mind if someone is willing to do the work needed, in the meantime, please be so kind to stop wasting other peoples time.

[Sumpfdotter]

Req 2023-08-17-Fichtner-1: "TTL will be added in a more generic way" --> resolved by Curly060
Req 2023-08-17-Fichtner-2: "we should try to avoid service-specific naming" --> resolved by Curly060
Req 2023-08-18-Fichtner-1: "Enter both, API key and API password into the existing password field, separated with e.g. colon" --> "That would be my preference as well" --> Possible solution, but ugly for the end user. So Curly came up with a different one:
Req 2023-08-19-Fichtner-1: "It would probably make more sense then to use user+pass as API key+pass and move customer to resourceID." --> "Curly060 explained "abusing an already very specific field for yet another specific purpose is IMHO not a good way forward" and came up with a clear description: "The closest match to the existing fields were: customer number => username, API password => password, API key => no existing field, hence the introduction of a new field"
==> Still no clear merge conditions expressed.

From that point on, wrong aspects has been used for rejection:
Fichtner today: "using the API user as password" --> Netcup does not have a client's "API User" which here would be used as password, it has has an "API password", so you're basically rejecting that a password which is called "API Password" would go to a password field. Please rethink about that.
Fichtner today: "we are debating the choices of one provider but making a decision forward for ALL possible " --> That's not true, the mapping you are rejecting is only part of netcup plugin, not affecting any other provider.
Fichtner today: "clientSecret is odd." --> Yes, this is to be discussed. Curly060 changed it to become as general as possible. So please come up with a name you would accept or a critera for chosing a name. What is wrong with "clientSecret"? It's about a secret information (****-field) that is assinged to the OPNSense router as a specific client. If you really debate that word without explaining what's wrong with it, look at the other "standards" curly came up with like "resourceID" etc. Come on, let's find a name, just define one.
Fichtner today: "The mapping is fuzzy" --> No explanaition given, what's fuzzy other than the reasons stated before - which are not correct, as exlained. Please describe a little bit more, what is disturbing you with that mapping? What do you expect Curly060 to do so that you accept the mapping? There is no clear statement given from your side. Would be really helpful.

"An empty github profile and complaining about other peoples efforts" --> now it becomes personal? For you, human beings are defined by the content of the github profile only? You think there are no other fields where humans can become worthy? Let's focus on the solution I'd suggest ...

[AdSchellevis]

... now it becomes personal? ....

best reread your own response.

....We all can be lucky that there is more contributors. You like to have some changes? Then you must explain it! You you are just rejecting. End of discussion? Wow, mighty person you are. Create a standard, write it down and then we can go further.

Giving you some time to cool off now.

[Curly060]

Could we please take the heat out of this discussion and start over? We have somehow lost the focus.
I did very well understand that provider specific fields are not an option. Hence I turned my provider specific field into a generic one: clientSecret.

This name is by no means unusual (in fact, e.g. in Azure it is quite common) and now the field even has the potential to be used in future DDNS plugins. It is in no way any more "harmful" than "resourceId" (which is provider specific) or "ttl" (which was AWS specific, but with the intention to be used by future plugins, which is exactly what happens with this plugin now).
I do believe that the new field makes using the GUI much more intuitive while at the same time not clutter the configuration.

That was my commit c16ad72 from September 2023 and is the current state. IMHO it does reflect everything that had been discussed up to that point back then, so I had asked if this new commit is acceptable but then never received any feedback.

So what exactly is wrong with the current state of the commit and what needs to be changed for this to be accepted? I see that "changes are requested", but it is no longer clear to me what that change should be.

[AdSchellevis]

@Curly060

Could we please take the heat out of this discussion and start over?

Sure, the most important consistency comment is the use of username/password. For all other machine to machine communications (including our own api) the key+secret combination equals username+password. Which in your case means the customer nummer is the one missing (and doesn't have a logical existing spot).

Some style cleanups might be needed in the python script itself, but I don't mind merging and skimming the code myself and letting you retest (as I don't have an account for this service).

How we ended up with other people commenting PR's trying to force things in still under review is beyond me, but apparently that's sadly how it works these days. Eventually if it breaks at some point, in our experience, complaints go to the maintainer. I guess I can only advise people to read into opensource before using it (I personally found this book on the subject quite enlightening https://www.amazon.com/Working-Public-Making-Maintenance-Software/dp/0578675862)

[Curly060]

Thanks, I am glad we can continue on this. Let's try one more time to describe the situation and find a solution:

In Netcup terminology the username is the customer number. For example if you go to the Netcup Customer Control Panel (CCP), you'll see it:
https://www.customercontrolpanel.de/?login_language=GB

It's a standard login page: username and password. Only that Netcup calls their username field "customer number". If it was possible to use their DNS API with just this, we would not even have had a discussion at all. We would have simply used username+password. Agreed?

But in order to use their DNS API a second secret is necessary. In netcup terminology they call this "API-key". You need to create it inside CCP.
Only then it is possible to use their DNS API, using these three values:

• username (=customer number in netcup terminology)
• password
• client secret (=API-Key in netcup terminology)

So that's two secret fields. Currently we have only one available in the GUI. So how to enter two secrets?

There was the idea to put both, the PW and the client secret into the existing PW field. However, that has quite some implications:
It would need some kind of separator, but if the user's PW contains this separator, the user would either have to change the PW or use some form of escaping (which we would then have to define). Same applies for the client secret, only that the user cannot change it: it is generated by Netcup and we don't know which characters might appear in it.

The new field solves all of this and I believe the mapping in the GUI is then quite intuitive.

So to sum up:
In order to use the Netcup DNS API the user has to specify three values, two of them being secret values: username, PW, 2nd secret (in netcup terminology: customer number, PW, API-Key). To which fields should those be mapped? In my opinion to username + PW and the new field. What do you think?

[fichtner]

In Netcup terminology the username is the customer number.

Let's try and cut to the chase here for the third time:

Fine, but no.

[Curly060]

Yeah, let's. Please just answer precisely the following question:

In which fields shall I put

• the customer number
• the password
• the API key

so that this pull request becomes acceptable?

Thanks.

[AdSchellevis]

Below a copy from the code in the PR:

             'param': {
                    'customernumber': self.netcupCustomerNr,
                    'apikey': self.netcupAPIKey,
                    'apipassword': self.netcupAPIPassword
             }

Which matches:

• apikey --> username
• apipassword --> password
• customernumber --> (new)customernumber

simple, easy, just like the rest.

[Sumpfdotter]

Hi Ad!

Thank you for this idea. It indeed is quite simple, but not feasible. With your idea, the value of the field „API Key“ becomes visible to the user:

NetCup’s API Key is not a key in the sense of an identifier. It is a key in the sense of a secret. NetCup’s API documentation makes this clear (https://helpcenter.netcup.com/de/wiki/general/unsere-api/):

„API Key“ is a secret that is different per client, i.e. it is a client secret. „API Key“ must go into a password style edit field. We need another proposal from your side that takes this into regards.
In contrast, NetCup’s so called field „API Password“ instead is a secret which it is not individual per client. It is shared between all clients of a NetCup account.
Let’s analyse how the credentials currently are normalised for dynamic DNS updates over the existing plug-ins:

Currently two main concepts are supported: First is credentials for human accounts, which until now all comes as basic authentication with a username and a password. Some providers call it „Username/Password“, CloudFlare e.g. calls it „Email/Key“. The second concept we support is basic authentication for machine clients with client ID and client secret. One provider calls it „Client ID/Client Secret“, another one „Access Key ID/Secret Access Key“. Some providers work with a token only, but because this token is unique to the client and very long living we can see it for sure matching the client secret concept as well.
The right most column shows the current normalisation: For human account credentials, username and password goes into username and password field. And similarly, for machine account credentials, client ID (if available) and client secret go into username and password as well. Only azure needs a third concept which is the provider specific field „resource ID“. For reasons I don’t understand from outside, your currently do not hide this custom field for all other providers - it’s visible for all providers even though it is not functioning for those. Maybe you consider hiding it not to confuse users, but maybe you have good reason for that.
NetCup comes with three credentials: Customer Number, API Password and API Key.
Customer Number: For human login, some providers use a username/password combination. Others like CloudFlare use more specifically Email/Key for human account login. And similarly, NetCup is using Customer Number/Password for human account login:

The NetCup API login requires to provide this customer number as account identifier (username), and this is exactly comparable to CloudFlare asking for the user’s Email address (also as username). In all such cases it’s what the provider uses as primary key for the user. For debate I listed customer number twice, as explained as username for human account as well as a special concept „Customer Number“.
API Key: As explained above, this field matches the concept of a client secret.
API Password: As explained above, this field matches the concept of a shared secret. „Shared secret“ is a concept currently missing in the normalisation. It could be named e.g. shared secret or shared key. Because „key“ was already causing misunderstandings (key as identifier versus key as secret), I opted for calling it „shared secret“. Although it might be uncommon for the dynamic DNS providers we supported so far, we can see this concept of an additional shared secret out in the world. E.g. in VPN solutions a shared secret might be used as a first line of defence and the user or client specific credentials come afterwards on top of it.
Having all that said, two things are to be decided:
First, it is to decide whether customer number goes into username or a special field
Second, it is to decide whether to introduce a new field for the shared secret concept, or for the client secret concept or for both.
The following table compares the possible normalisation variants depending on the aspect of normalisation:

Each column shows a different aspect of normalisation. When normalising in direction of the implementations with machine credentials that are already present, API Key as a client secret concept, would go into the password filed - like it does for AWS, Azure, CloudFlare and DuckDNS. These are proposals 3 and 4. I want to encourage everybody not only to look from implementation’s perspective but also from user’s perspective. In both variants (proposals 3 and 4), the NetCup user would have to enter what he knows as „API Password“ into the „Shared Secret“ field and what he knows as „API Key“ into the „Password“ field. So it turns out that this is exactly counter intuitive for the user, although implementations are normalised. So question arises if it is really necessary that one plugin maps exactly like the other. We have no possibility to add a context sensitive help dedicated to the NetCup plug-in. For sure documentation can offered on the wiki page. Users maybe will consult it, but maybe not because the field „Password“ by name is perfectly matching. It would be even worse when we call the new field „Shared Key“ instead of „Shared Secret“. They the user had „API Password“ and „API Key“ in their hand, but they would have to enter the password into the „Shared Key“ and the key into the „Password“ labeled field. So again the name „Shared Secret“ turns out to perform better than „Shared Key“.
Proposal 1 normalises in direction of the concepts: Each concept get’s it’s own field. This sounds quite reasonable at first glance. And it turns out that this proposal is equal to normalisation with the existing implementations that use user credentials (CloudFlare, Domeneshop and DynDNS2) which is Proposal 2. But again, when looking at the UX, the user now has to know that „API Password“ must not go into the password field. Instead, the password field has to be left blank. The API password goes into the Shared Secret field instead. Also quite errorprone, but at least we now have three fields for two credentials, so users will start thinking about it and maybe more often consult the documentation. Again, context sensitive help is not possible.
Personally, and for sure debatable I prefer the normalisation in direction of UX labels, because that gives customer centricity and the best usability: API password goes to „Password“, API key goes to „Client Secret“ and Customer Number either goes to „Username“ (Proposal 5) or to „Customer Number“ (Proposal 6). Again personally I prefer proposal 5 because it matches the concept of the customer number as username (like Email for CloudFlare), it does not result in a visible field „Username“ that the user needs to know to leave blank, and in addition, it has less impact to the overall architecture because no second additional field is required.
As introduced, the current idea is not working out. We need another one. Select the variants you can work with so that Curly060 can have a look into your list if there is one he could also agree on.

[fichtner]

I think we should discontinue this effort to integrate into os-ddclient then. I said before I’m not against adding a custom plugin for a single provider with all of its ups and downs and idiosyncrasies.

[Sumpfdotter]

I think we should discontinue this effort to integrate into os-ddclient then. I said before I’m not against adding a custom plugin for a single provider with all of its ups and downs and idiosyncrasies.

And I explained that dynamic DNS has so much in common (determining current IP, comparing IP and triggering) that it'an anti-pattern to double that up. You never commented on that.

[Sumpfdotter]

Would be very interested in using this PR implementation. Is this still under active review after the discussion?

Dear OPNSense organisation. You won: I spent hours and hours to support you in that question with tests, reviews, researches, explanations and moderations. This post ist my last contribution to your organisation.

You are basically rejecting a community pull request which adds a new functionality in a way that no other existing functionality is impacted - I reviewed it and I tested it. There is no change to all other plugin-ins. At the same time, your own solution for Azure as shown impacts all other plugins. First you rejected the committer because he in your opinion does not fulfil your high standards - which was never explained where exactly. And now you are even rejecting the whole problem as incompatible with your solution, ignoring that the committer is just about requesting a pull which proves that it's possible to add that function without any impact.

Translation for this is: You're accepting your own contributions even if they are not mature, and you reject the community contributions even if it adds new requested features without impact to your existing parts. We're here on GitHub which requires by statutes organisations to be open for community contributions. I hope that it's only one user and maybe your organisation changes it's mind some day. Till that day, you will be lucky that you're losing my support.

[AdSchellevis]

The fix is simple, all other providers use the same semantics, username (key) + password (secret) in combination with whatever is vendor specific (same is the case for Azure by the way, where resourceId is specific).

We can debate semantics forever, but I wasn't planning on joining. At the end of the day, the settings being entered are only visible by the admin. A very simple fix would be to follow the reasoning which was used for the rest of this plugin, Alternatively creating a specific plugin is also acceptable. A lot of vendors ship their own client tools.

@Sumpfdotter I understand you are disappointed, but am going to make a final request to communicate normally and let the contributor handle feedback instead of trying to hijack the PR with more noise than needed. @Curly060 is free to change the PR as discussed, I am still willing to spend some time to cleanse the code where needed. The way you are to interacting on these tickets increases the chances that people like myself just give up and walk away.....

[Curly060]

Which matches:

* apikey --> username
* apipassword --> password
* customernumber --> (new)customernumber

simple, easy, just like the rest.

Hi Ad,

thanks. This was the first very clear answer of you guys to me in this whole discussion that finally made me understand what the problem is:
You believe that API-Key is a form of username. In that case your mapping makes perfect sense indeed.

Well, it's not. But all arguments are exchanged and it is clear to me now that you prefer to break semantics in this case as long as it fits to what's already there. I will not argue with that. Your product, your choice.

But please also understand that I will not create a plugin that will show that secret already in the DynDNS Accounts overview (and it would: the username (aka API key) is shown in the Accounts tab).

So the only way forward I see is to go back to a proposal that in the beginning of this discussion Franco had described as "That would be my preference as well". It's certainly not my preference, but it's a compromise and it does work:

I have put API-Password and -secret into one field, the password field, separated by a colon. If the colon is part of the PW or API key, the user will have to escape it with a backslash.

That's awkward for the user and it does require some explanation in the Wiki, but again, Franco had said that's fine. And hey, it does fulfill all requirements:

• It maps quite nicely to username+password, just like all other plugins
• all secrets are in a password style field

No new fields, just one new self contained class in this PR (and a visibility toggle for the TTL field).

That's the current state and it is going to be my last attempt on this. I have jumped through quite some hoops here trying to get this right for everyone (you, me and the users of the plugin). I do this in my spare time in the hope to help others and as a way to say "thank you" to the project but at one point even my patience runs out.

Awaiting your decision.

Regards, Curly060

P.S.: Even though I shouldn't, please allow me one remark: To me it did not feel that Sumpfdotter "hijacked" anything here. Albeit sometimes really lengthy, I found his explanations very detailed and useful.

[AdSchellevis]

@Curly060

I've cleaned up the code and put it in a new branch on our end https://github.com/opnsense/plugins/tree/ddclient_netcup

If you can validate the workings of f087d6e , we can merge it if it still works. As I don't have an account, I can't try this myself.

[Curly060]

That's great. Thanks! I did find a couple of small bugs. You can find the fixes here:
f5edae4

With those patches applied it works.

About your additional check in line 178:
... and resp.get('responsedata', None):

That broke the logout method, because for the logout method the responsedata field is empty and thus this expression will evaluate to False and then an exception was thrown, even though the logout was successful. It is definitely enough to just check for this:
if resp.get('status', '') == 'success'

Why? The API also has a SOAP endpoint and from the WSDL it is clear that there is always exactly one responsedata field. So if the API returns "success" you can rely on this field to be present (but possibly be empty).

Then there are a couple of things that may or may be not problematic, so I have not fixed those yet:

creating hostRecord

if matchingRecords:
            hostRecord = matchingRecords[0]

Now hostRecord is no longer a copy of matchingRecords[0]! This has side effects:

• later on hostRecord is modified, so that will now modify the matchingRecords[0] (which in turn is part of the dnsRecordsInfo which was passed as method parameter)
• while probably unlikely, matchingRecords[0] may have more fields than those that I had manually added. That could break the _updateDnsRecords call

In practice probably not a problem, but I find making an explicit copy more robust.

About the escaping
Do I get this right: One needs to escape the separator? I will eventually make some tests with edge cases and check if this always works. In practice, right now this will not be an issue. Both, API key and PW never seem to contain colons (but only Netcup knows this...)

[AdSchellevis]

@Curly060 I'll add the fixes to the branch, let me provide feedback to the open issues in your comments first.

That broke the logout method, because for the logout method the responsedata field is empty and thus this expression will evaluate to False and then an exception was thrown, even though the logout was successful. It is definitely enough to just check for this:

Ah, clear, missed the logout spot. This also means return resp['responsedata'] needs a small adjustment as the key doesn't exist. I'll add that in my next commit.

... Now hostRecord is no longer a copy of matchingRecords[0]! This has side effects:....

Missed the intention here, but we coulc easily fix that with copy.deepcopy() (https://docs.python.org/3/library/copy.html), I'll put it on the list. Looking further at the code matchingRecords is unused within its scope.

... while probably unlikely, matchingRecords[0] may have more fields than those that I had manually added. That could break the _updateDnsRecords call.

There are pro's and cons there indeed, usually, if the intention is to only modify a selection of data, we would only send the changed ones, but that also depends on the other end accepting partial data. When partial data is not accepted and new fields are added later, you might want to return them as is anyway.

Do I get this right: One needs to escape the separator? I will eventually make some tests with edge cases and check if this always works. In practice, right now this will not be an issue. Both, API key and PW never seem to contain colons (but only Netcup knows this...)..

If a separator is used, you need to escape it (as your code did as well). When there is a character never used in their api key+secret combination (for example |) I would prefer to use that, but I don't know their api.

imported 620737f including the changes discussed.

[Curly060]

About escaping:
We both have a bug in our escape code. Mine didn't work if the password ended with backslash, yours doesn't work if any of PW or key contains the following sequence of characters: \:
While both are quite unlikely edge cases, I'd prefer a much more robust solution which always works no matter what.
How about using CSV style escaping? There is a ready made standard python module for it which handles everything perfectly fine and it is very easy for the end user:
APIPassword,APIKey
Should any of them ever contain a comma, quote them, e.g.:
"APIPassword,withcomma":APIKey
And should the PW ever contain a ", well, then use Excel/Calc, put PW and KEY in separate columns and save as CSV with comma as delimiter and " as escape character and copy&paste from the CSV file.

To my knowledge currently Netcup only generates PWs and Keys with the following characters: 0-9, a-z, A-Z
So right now entering the credentials is a easy putting them in the PW field separated by a comma. But what do we know. Maybe Netcup will change this one day.

With the CSV solution we'd be prepared. I have made a diff against your latest changes which uses the csv module:
13e295a
This also fixes a KeyError in line 75 which occured if neither of line 71 and 73 of f087d6e were executed.
So what do you think?

In any case, I have successfully tested both:

• f087d6e (except for KeyError in edge cases)
• 13e295a

Edit: Corrected what I said about KeyError. There is no KeyError.

[AdSchellevis]

I would go for a pipe sign | and be done with it, csv type input in fields not visible for the user are even harder to explain. If it turns out Netcup uses pipes later on, you can always open a PR then.... adding this to the documentation is likely more important than fine tuning it further.

A key error sounds a bit weird as both APIPassword and APIKey are initialized to None, which input did you use to trigger that?

[Curly060]

A pipe is also fine. And ignore me about the KeyError, wrong test setup, you are right, it will not happen (I had extracted the code to a separate standalone file to test more easily all edge cases and forgot the init part.)

[AdSchellevis]

Ok, I'll change the code, merge it and leave a note in the documentation about the custom input.