Feat/scaleway.com #899

francesco086 · 2025-01-01T19:53:38Z

Solves issue #660

francesco086 · 2025-01-01T20:09:29Z

Regarding the change in the markdown tables: I use the VSCode formatter https://marketplace.visualstudio.com/items?itemName=fcrespo82.markdown-table-formatter

Do you want me to reverse the markdown table back to the original state?

qdm12

Thanks 👍 !

Two important things are to remove the field_type and field_name, unless there is reason I'm not aware of 😉

qdm12 · 2025-01-01T22:35:09Z

README.md

+| Version | Readme link                                                           | Docs link                                                       |
+|---------|-----------------------------------------------------------------------|-----------------------------------------------------------------|


Yeah let's revert this otherwise the git history is going to look all weird; Note you can open the vscode command palette and hit "save without formatting" 😉

I would however be okay with a separate PR to auto-format this + perhaps enforce this in the CI 🤔

done 😄

I am happy to do this in a separate PR. I am not sure of a way to enforce this in CI, but I guess there must be a way.

Have a look at the .github/workflows/markdown.yml there might be rules you can set in the linter 😉

I opened an issue #908 (comment)

Will be happy to contribute 😄

docs/scaleway.md

qdm12 · 2025-01-01T22:38:03Z

docs/scaleway.md

+- `"ip_version"` can be `"ipv4"` or `"ipv6"`. It defaults to `"ipv4"`.
+- `"ipv6_suffix"` is the suffix to append to the IPv6 address. It defaults to `""`.
+- `"field_type"` is the type of DNS record to update. It can be `"A"` or `"AAAA"`. It defaults to `"A"`.
+- `"field_name"` is the name of the DNS record to update. For example, it could be `"www"`, `"@"` or `"*"` for the wildcard. It defaults to to `""` (equivalent to `"@"`).


You should not need this, it should be deducted from the "domain" field

Please help me out here. How should I extract this? The only way I can imagine is to count how many . are in the domain string, and if there are two take the first part of the domain string before the first . as name (according to the scaleway API). Is that correct?

The upper layers of ddns-updater takes care of extracting it:

ddns-updater/internal/params/json.go

Line 231 in 20ac110

func extractFromDomainField(domainField string) (domainRegistered string,

It's a bit more complicated than the number of dots, it uses the "effective TLD + 1" mechanism to do this (i.e. some effective TLDs are com., others are duckdns.org.)

So just use the domain and owner strings from calling layers 😉 - it's already parsed and ready to use for you.

Ok, it took me a while to understand what you mean and especialyl how this maps to the scaleway api, but I think I sorted out! I removed the field_name both in the code and documentation, and tested that everything works correctly.

qdm12 · 2025-01-01T22:38:22Z

docs/scaleway.md

+
+- `"ip_version"` can be `"ipv4"` or `"ipv6"`. It defaults to `"ipv4"`.
+- `"ipv6_suffix"` is the suffix to append to the IPv6 address. It defaults to `""`.
+- `"field_type"` is the type of DNS record to update. It can be `"A"` or `"AAAA"`. It defaults to `"A"`.


You should not need this, it should be deducted from the public IP address obtained and "ip_version"

Uhm... again can you help me here? Do you have already a function that is able to distinguish between the two?

I just found it in one of your comment below. Will change this in a moment!

field_type is still in the documentation 😉 Please remove 🙏

argh sorry... missed the second entry...

Now it is removed for good!

internal/provider/providers/scaleway/provider.go

qdm12 · 2025-01-02T00:05:02Z

internal/provider/providers/scaleway/provider.go

+	s, err := utils.ReadAndCleanBody(response.Body)
+	if err != nil {
+		return netip.Addr{}, fmt.Errorf("reading response: %w", err)
+	}
+
+    if response.StatusCode != http.StatusOK {
+        return netip.Addr{}, fmt.Errorf("%w: %d: %s",
+			errors.ErrHTTPStatusNotValid, response.StatusCode, utils.ToSingleLine(s))
+    }


is there any useful information in the body? if it's just for the error case, let's use instead:

Suggested change

s, err := utils.ReadAndCleanBody(response.Body)

if err != nil {

return netip.Addr{}, fmt.Errorf("reading response: %w", err)

}

if response.StatusCode != http.StatusOK {

return netip.Addr{}, fmt.Errorf("%w: %d: %s",

errors.ErrHTTPStatusNotValid, response.StatusCode, utils.ToSingleLine(s))

}

if response.StatusCode != http.StatusOK {

return netip.Addr{}, fmt.Errorf("%w: %d: %s",

errors.ErrHTTPStatusNotValid, response.StatusCode, utils.BodyToSingleLine(response.Body))

}

the body looks something like

{ "records": [ { "id": "aa051759-72e6-4034-8af5-dafba3eebb79", "data": "127.1.2.3", "name": "", "priority": 0, "ttl": 300, "type": "A", "comment": null } ] }

I think there is nothing useful, let me know if you disagree. Otherwise I will apply your change.

What happens if you send a request with the wrong secret key or an owner that doesn't exist, what's the json body you get in the response? It might be a standardized error format which we could parse and use to build a better error to log to the user 😉 Not compulsory, but nice to have if you have the time

if the secret key is wrong the response is:

{ "message": "authentication is denied", "method": "api_key", "reason": "invalid_argument", "type": "denied_authentication" }

If the owner doesn't exist then the record with the right domain is added. The response is indistinguishable from one where the owner already exists.

So, I changed the code into:

if response.StatusCode != http.StatusOK { var errorResponse struct { Message string `json:"message"` Type string `json:"type"` } if jsonErr := json.Unmarshal([]byte(s), &errorResponse); jsonErr == nil { if errorResponse.Type == "denied_authentication" { return netip.Addr{}, fmt.Errorf("%w: %s", errors.ErrAuth, errorResponse.Message) } } return netip.Addr{}, fmt.Errorf("%w: %d: %s", errors.ErrHTTPStatusNotValid, response.StatusCode, utils.ToSingleLine(s)) }

Is that ok?

francesco086 · 2025-01-10T08:28:01Z

Hi @qdm12 , any chance for a second round of review?

qdm12

Sorry for the delay, I've been quite away from (personal open source) keyboard because of work/life/travels 😄

docs/scaleway.md

qdm12 · 2025-01-11T08:50:28Z

docs/scaleway.md

+
+- `"ip_version"` can be `"ipv4"` or `"ipv6"`. It defaults to `"ipv4"`.
+- `"ipv6_suffix"` is the suffix to append to the IPv6 address. It defaults to `""`.
+- `"field_type"` is the type of DNS record to update. It can be `"A"` or `"AAAA"`. It defaults to `"A"`.


field_type is still in the documentation 😉 Please remove 🙏

internal/provider/providers/scaleway/provider.go

qdm12 · 2025-01-11T08:52:32Z

internal/provider/providers/scaleway/provider.go

+        Path:   fmt.Sprintf("/domain/v2beta1/dns-zones/%s/records", p.domain),
+    }
+
+	field_type := "A"


nit rename to fieldType (Go convention is no snake_case 🤷 Only camelCase)

true! Should have noticed... changed accordingly!

qdm12 · 2025-01-11T08:54:23Z

internal/provider/providers/scaleway/provider.go

+	s, err := utils.ReadAndCleanBody(response.Body)
+	if err != nil {
+		return netip.Addr{}, fmt.Errorf("reading response: %w", err)
+	}
+
+    if response.StatusCode != http.StatusOK {
+        return netip.Addr{}, fmt.Errorf("%w: %d: %s",
+			errors.ErrHTTPStatusNotValid, response.StatusCode, utils.ToSingleLine(s))
+    }


What happens if you send a request with the wrong secret key or an owner that doesn't exist, what's the json body you get in the response? It might be a standardized error format which we could parse and use to build a better error to log to the user 😉 Not compulsory, but nice to have if you have the time

Co-authored-by: Quentin McGaw <[email protected]>

francesco086 · 2025-01-11T15:17:31Z

Hi @qdm12 ! I had a feeling that perhaps you were on holidays, this is why I waited... after a week I tried my luck 😃

Alright, I think I addressed all your concerns and hope that it is ready to merge now. Perhaps let me bring you attention to these changes in particular. It took me a while to figure this out, and this is why I thought it would be useful to document them. But perhaps it's just me, I am not familiar with DNS and networking terminologies (I studied physics), I am trying to learn as a hobby. So, let me know your opinion.

francesco086 · 2025-01-28T05:37:27Z

Hi @qdm12 , time for a third review? I believe the PR is ready to be merged now

Francesco Calcavecchia added 5 commits January 1, 2025 15:11

implement scaleway.com flavor

75d1a15

fix tests

740aa38

troubleshooting

ac47230

add optional settings

1e80577

update documentation

ea37d4a

francesco086 mentioned this pull request Jan 1, 2025

scaleway.com #660

Open

qdm12 requested changes Jan 2, 2025

View reviewed changes

Francesco Calcavecchia added 7 commits January 2, 2025 08:44

revert readme tables

49128bc

remove comments in example json

1ff7a34

remove field_type

5576557

change ttl type to uint16 and shorten comment

0398397

remove unnecessary comments

fd3ee27

remove RawQuery in url

6f66a08

structure the request body following qdm12 suggestion

0b9af33

francesco086 requested a review from qdm12 January 2, 2025 08:18

qdm12 reviewed Jan 11, 2025

View reviewed changes

Francesco Calcavecchia and others added 6 commits January 11, 2025 14:02

remove last trace of field_type from doc

d13729d

Apply suggestions from code review

4ab47f7

Co-authored-by: Quentin McGaw <[email protected]>

Apply suggestions from code review

d21fd33

Co-authored-by: Quentin McGaw <[email protected]>

use camel case for variable names

f67e76f

remove type_name in favor of owner

3642186

handle denied_authentication

65684fa

francesco086 mentioned this pull request Jan 11, 2025

Format markdown tables for better readability in raw format #908

Open

francesco086 requested a review from qdm12 January 11, 2025 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/scaleway.com #899

Feat/scaleway.com #899

francesco086 commented Jan 1, 2025

francesco086 commented Jan 1, 2025

qdm12 left a comment

qdm12 Jan 1, 2025

francesco086 Jan 2, 2025

qdm12 Jan 11, 2025

francesco086 Jan 11, 2025

qdm12 Jan 1, 2025

francesco086 Jan 2, 2025

qdm12 Jan 11, 2025

francesco086 Jan 11, 2025

qdm12 Jan 1, 2025

francesco086 Jan 2, 2025

francesco086 Jan 2, 2025

francesco086 Jan 2, 2025

qdm12 Jan 11, 2025

francesco086 Jan 11, 2025

qdm12 Jan 2, 2025

francesco086 Jan 2, 2025

qdm12 Jan 11, 2025

francesco086 Jan 11, 2025

francesco086 commented Jan 10, 2025

qdm12 left a comment

qdm12 Jan 11, 2025

qdm12 Jan 11, 2025

francesco086 Jan 11, 2025

qdm12 Jan 11, 2025

francesco086 commented Jan 11, 2025

francesco086 commented Jan 28, 2025

		\| Version \| Readme link \| Docs link \|
		\|---------\|-----------------------------------------------------------------------\|-----------------------------------------------------------------\|

Feat/scaleway.com #899

Are you sure you want to change the base?

Feat/scaleway.com #899

Conversation

francesco086 commented Jan 1, 2025

francesco086 commented Jan 1, 2025

qdm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francesco086 commented Jan 10, 2025

qdm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francesco086 commented Jan 11, 2025

francesco086 commented Jan 28, 2025