Skip to content

DESEC: Adopt diff2 in compatibility mode#1876

Merged
tlimoncelli merged 5 commits intomasterfrom
tlim_diff2_desec
Feb 2, 2023
Merged

DESEC: Adopt diff2 in compatibility mode#1876
tlimoncelli merged 5 commits intomasterfrom
tlim_diff2_desec

Conversation

@tlimoncelli
Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli commented Dec 31, 2022

Hi @D3luxee:

In reference to replacing pkg/diff with the new pkg/diff2 (#1854)

I've introduced a compatibility mode to pkg/diff2 to make it easier for providers to adopt the new system. If you do a diff with "hide whitespace", you'll see the changes are minimal.

I've taken the liberty of providing this PR as a starting point.

Please run the integration tests and let me know the result. (If you're getting this, I probably don't have access to an account on this provider)

Please run:

    go test -v -verbose -provider DESEC

and

    go test -v -verbose -provider DESEC -diff2

They should both complete with no failures. Please fix any failures or loop me in for help.

If you are interested in porting to the new pkg/diff2/By*() functions, I'd be glad to accept a PR. However using the compatibility mode is sufficient if all tests pass.

Thanks and happy new years!!!

Tom
@tlimoncelli

@tlimoncelli
Copy link
Copy Markdown
Contributor Author

Friendly ping? @D3luxee

@D3luxee
Copy link
Copy Markdown
Contributor

D3luxee commented Jan 21, 2023

Hi,
i was pretty busy recently.
I will have a look at it but i'm not sure if i can run all tests.
The last time i ran into rate limits on DESEC which caused the tests to fail.

Edit:
I did run the first test " go test -v -verbose -provider DESEC" and it looks "okay" but the last couple tests failed due to rate limit exceeded

There are two rate limts, one that asks you to wait before the next request which is something my provider handles.
But it seems there is some global hard limit on requests in a specific period which i exceed with the tests.

I will run the second set of tests tomorrow.

@tlimoncelli
Copy link
Copy Markdown
Contributor Author

If the rate limit kicks in I have two suggestions:

  1. Please file an issue to implement a back-off when rate limits are hit. (its easier to implement than you'd expect). We don't have to fix this bug right away, but its good to record that it exists.
  2. Run the tests in two groups using the -start and -end flags. For example, if the rate limit is usually hit on test 60, try running -end 50 to run the first 51 tests, then -start 51 to run the remainder.

Thanks!
Tom

@D3luxee
Copy link
Copy Markdown
Contributor

D3luxee commented Jan 26, 2023

Hi Tom,
i've already implemented back-off for rate limits, the desec api exposes the expected backoff time if we hit the rate limit.
But there seems to be another rate limit that can't be handled.
According to https://desec.readthedocs.io/en/latest/rate-limits.html there is a 100/h and 300/day limit on RRset creation/deletion/modification (per domain). Which is what i hit.

Because the normal rate limit hits already 60s back-off times every 10th test or something.

But the hint with -start and -end flags is helpful, will do it that way.

@D3luxee
Copy link
Copy Markdown
Contributor

D3luxee commented Jan 26, 2023

@peterthomassen would it be possible to raise those hourly and daily limits for my d3luxee.de zone that i use for the tests with my account?

@peterthomassen
Copy link
Copy Markdown
Contributor

Unfortunately, we currently don't have any account-specific knobs for tuning rate limits.

the last couple tests failed due to rate limit exceeded

Would you be able to run those with the -start flag?

@D3luxee
Copy link
Copy Markdown
Contributor

D3luxee commented Jan 27, 2023

Yes i will do it via -start. I just thought it might be possible on your side to resolve this, if thats not possible its totally fine for me :D

@tlimoncelli
Copy link
Copy Markdown
Contributor Author

If the rate limit issue is the same with and without -diff2 then I'm ready to merge this. We can deal with any limits later. Am I understanding the situation correctly?

@D3luxee
Copy link
Copy Markdown
Contributor

D3luxee commented Jan 30, 2023

I've run the first set and will continue with the -diff2 with the start things it works but takes some time

@D3luxee
Copy link
Copy Markdown
Contributor

D3luxee commented Feb 2, 2023

@tlimoncelli all tests passed, no issues

feel free to merge

@tlimoncelli
Copy link
Copy Markdown
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants