CLOUDFLAREAPI: Adds CF_WORKER_ROUTE. #1243

fdcastel · 2021-08-30T00:11:16Z

Initial implementation of CF_WORKER_ROUTE.

Adds Cloudflare Worker routes support via new CF_WORKER_ROUTE function.

No tests, as I have yet to understand the tests infrastructure. But the code is working as I did some manual tests with a personal CF account (Create, Modify and Delete routes are working as expected).

Thanks to @tlimoncelli for guidance and incentive.

fdcastel · 2021-08-30T01:01:46Z

Added initial tests, similar to CF_REDIRECT ones.

Guidance required for the integration tests (I didn't find where the expected result should be).

docs/_functions/domain/CF_WORKER_ROUTE.md

models/record.go

providers/cloudflare/cloudflareProvider.go

providers/cloudflare/rest.go

tlimoncelli

Just a few more comments.

Please rebase
Please add a test to integrationTest/integration_test.go

docs/_functions/domain/CF_WORKER_ROUTE.md

docs/_providers/cloudflare.md

providers/cloudflare/rest.go

fdcastel · 2021-09-02T19:33:47Z

About the integration tests... I am somewhat lost. 😢

How do we test this? How could I check if the test is passing or not (won't I need some API credentials to run it locally?).

I'm surely missing something here 😅

tlimoncelli · 2021-09-02T19:56:06Z

Here is how to add a simple integration test: #1250

To run the integration tests you'll need a test domain:

export CLOUDFLAREAPI_DOMAIN='dnscontroltest-cf.com'      <<< CHANGE TO YOUR DOMAIN
export CLOUDFLAREAPI_TOKEN='REDACTED'
go test -v -verbose -provider CLOUDFLAREAPI

Even without that PR, I've found a small bug. It looks like the code assumes that there will be a worker route list.

tlimwork5:~/git/dnscontrol/integrationTest$ go test -v -verbose -provider CLOUDFLAREAPI
=== RUN   TestDNSProviders
=== RUN   TestDNSProviders/dnscontroltest-cf.com
=== RUN   TestDNSProviders/dnscontroltest-cf.com/Clean_Slate:Empty
{"success":false,"errors":[{"code":10000,"message":"Authentication error"}]}

    integration_test.go:193: runTests: failed fetching worker route list from cloudflare: bad status code from cloudflare: 403 not 200
=== RUN   TestDNSProviders/dnscontroltest-cf.com/00:GeneralACD:Create_an_A_record
{"success":false,"errors":[{"code":10000,"message":"Authentication error"}]}

    integration_test.go:193: runTests: failed fetching worker route list from cloudflare: bad status code from cloudflare: 403 not 200
=== RUN   TestDNSProviders/dnscontroltest-cf.com/Post_cleanup:Empty
{"success":false,"errors":[{"code":10000,"message":"Authentication error"}]}

tlimoncelli · 2021-09-02T19:58:40Z

FYI: The automated tests (https://github.com/StackExchange/dnscontrol/pull/1243/checks) are skipping CloudFlare because it can't find any credentials. Github Actions uses the credentials from the fdcastel/dnscontrol repo and there are none. Once it is merged to StackExchange/dnscontrol it will have access to the credentials in StackExchange/dnscontrol and the tests will run. (this is to protect someone from writing a PR that steals the creds.)

fdcastel · 2021-09-02T21:43:36Z

I'm still missing something. Even after pulling your sample code and settings my env vars like:

$env:CLOUDFLAREAPI_DOMAIN='my-cf-domain.com'
$env:CLOUDFLAREAPI_KEY='my-api-key'
$env:CLOUDFLAREAPI_USER='my-api-user'

A call to

go test -v -verbose -provider CLOUDFLAREAPI

gives me

?   	github.com/StackExchange/dnscontrol/v3	[no test files]

Running on Windows 10 / Powershell

fdcastel · 2021-09-02T21:48:19Z

Just to be clear: I already read https://stackexchange.github.io/dnscontrol/byo-secrets and it seems clear to me.

But I'm trying to run the tests locally on my machine before I put them into the CI.

Is this possible or I misunderstood something? (i.e., the tests would only run inside Github CI engine?)

fdcastel · 2021-09-02T22:07:26Z

Got it!

go test -v ./integrationTest -verbose -provider CLOUDFLAREAPI

Tests are running now 🙌 . Working on it.

fdcastel · 2021-09-02T23:05:13Z

There is some way -- in the current test infrastructure -- to run additional code before some testgroup starts to run?

Rationale: I have the following test:

  testgroup("CF_WORKER_ROUTE",
    only("CLOUDFLAREAPI"),
    tc("simple", cfWorkerRoute("cnn.**current-domain-no-trailing**/*", "cnn-worker")),
    tc("changeScript", cfWorkerRoute("cnn.**current-domain-no-trailing**/*", "msnbc-worker")),

But before it, I need to call an additional Cloudflare api to create cnn-worker and msnbc-worker workers.

Without this, the API calls under test will fail with a

[{"code":10019,"message":"workers.api.error.invalid_route_script_missing"}]

fdcastel · 2021-09-02T23:06:10Z

✔️ Rebase done.
✔️ Integration tests done.
✔️ Requested changes done.
❗ Integration tests need two cloudflare workers (named cnn-worker and msnbc-worker) on cloudflare account to pass.

tlimoncelli · 2021-09-03T14:14:23Z

There is some way -- in the current test infrastructure -- to run additional code before some testgroup starts to run?

Rationale: I have the following test:
  testgroup("CF_WORKER_ROUTE",
    only("CLOUDFLAREAPI"),
    tc("simple", cfWorkerRoute("cnn.**current-domain-no-trailing**/*", "cnn-worker")),
    tc("changeScript", cfWorkerRoute("cnn.**current-domain-no-trailing**/*", "msnbc-worker")),
But before it, I need to call an additional Cloudflare api to create cnn-worker and msnbc-worker workers.

Without this, the API calls under test will fail with a
[{"code":10019,"message":"workers.api.error.invalid_route_script_missing"}]

There isn't a uniform way to do setup code. However....

Around line 55 of integration_test.go we do some "just for CloudFlare" actions. Maybe that could call a function that creates the workers if they don't already exist? Please create them with names that won't overlap real workers... maybe dnsctrl_integrationtest_foo and dnsctrl_integrationtest_bar ?

Tom

fdcastel · 2021-09-06T16:09:04Z

Minor update: Yet working on it. I should update this PR in a few days. Thanks, @tlimoncelli !

fdcastel · 2021-09-11T16:42:09Z

Sorry for the delay. It has been a long week around here.

It's almost done. However, I'm having a hard time making Cloudflare-specific API calls from integration_test.go. It seems wrong to call provider-specific code here. I don't even have the context for Cloudflare account, credentials, etc.

One solution would be to add a PrepareTests() in DNSProvider interface. However this would add a lot of empty boilerplate code for every other provider which doesn't need this. Not good, IMHO.

Could we use preprocess_test.go for the test workers creation? There is any guarantee that this code would run before the tests from integration_test.go?

fdcastel · 2021-09-11T17:04:21Z

Pushed what I got so far. Looking for advice.

I added TestPreprocess_CreateTestWorkers in preprocess_test.go but it is not being called before the integration tests.

I also added two new env vars CLOUDFLAREAPI_ACCOUNTID and CLOUDFLAREAPI_ACCOUNTNAME which were already present in creds.json but without any way to set them via environment.

AccountName is not used, actually. However, there is a test in cloudflareProvider.go which enforces both vars to be set.

tlimoncelli · 2021-09-12T18:13:08Z

Hi there!

Sorry for the delay. It's been a crazy week!

A few things:

I haven't been able to get this running since I can't figure out the permissions. Do you know what perms to give this API key to enable it to use workers?
I've noticed that my API key doesn't have access to Workers at all (maybe because its a free account?). I tried to update the API's permissions but I get:

    integration_test.go:193: runTests: failed fetching worker route list from cloudflare: bad status code from cloudflare: 403 not 200

The code either needs to automagically detect if Workers is disabled and be silent or there needs to be a feature-flag to enable Workers. Otherwise this will be a breaking change... people that don't use workers can't break and can't be asked to modify permissions on the API key just because of a new feature.

I don't know how to detect if workers is enabled, but I'm making a PR that adds the feature flag in case you go that route: https://github.com/fdcastel/dnscontrol/pull/1/files

preprocess_test.go is a unit test and isn't expected to run as part of the integration tests. Instead, it runs when you do "go test -count=1 ./..." from providers/cloudflare.

The right place for TestPreprocess_CreateTestWorkers is in integrationTest/. Yes, that's a layering violation but... that's life. I would move it to integrationTest/cloudflare.go and call it cloudflareSetupWorkers(). You can call it from integration_test.go around line 53 where we do some cloudflare-specific things already.

At least I think that will work. I'm not entirely sure as I can't test it myself until I fix my own permission problem.

I'm in the US/Eastern timezone if you would like to screenshare and work on this in real time.

Tom

fdcastel · 2021-09-12T19:39:20Z

Thanks, Tom, for the insights and the pragmatism 😉 - I will return to this in a few hours.

About your points:

I'm using Cloudflare API Keys (which, as I understand, gives me sudo-like access to the account) instead of API Tokens (which allows you more granular control about what can be accessed). That said, after a quick glimpse at the options available in Cloudflare dashboard, I believe these are what you will need:

You definitely can use Workers on a Free account (my own test domain is on this very same plan). So, the problem is probably permission-related. I will investigate a bit more later to know what specific permissions are needed for token-related access (but I would guess that the above image is very close).
Perfect! Got it!

fdcastel · 2021-09-20T15:28:56Z

Rebase to latest release.

fdcastel · 2021-09-20T15:55:44Z

Added feature flag for CF Workers per @tlimoncelli suggestion.

Porting tests back to integration_tests/ now -- https://www.youtube.com/watch?v=TnlPtaPxXfc

fdcastel · 2021-09-20T17:27:39Z

All done! Integration tests are creating test workers now.

Two considerations:

I added 'testing' module in cloudflare module. Surely I'm not proud of this and would welcome any suggestions to improve (if deemed necessary).
Tom: I believe we now have a better understanding about that comment of integration_test.go 😉 (needed to avoid breaking changes?). We could improve it explaining the rationale behind the feature flags.

fdcastel · 2021-09-20T17:32:26Z

I ran all tests twice (first without test workers and then with them) in a test domain and everything looks good to me.

Save for further coding improvements (see last comment) I would consider the work now done.

Also, if agreed, I would like to rewrite the commit history to put the commits in a better shape before merge.

tlimoncelli · 2021-09-20T22:58:31Z

Looks very good so far!

I do want to find a way to make legacy domains "just work". Here's the one I use for tests:

$ go test -v -verbose -provider CLOUDFLAREAPI
=== RUN   TestDNSProviders
    cloudflareProvider.go:699: [{"code":7003,"message":"Could not route to /accounts/workers/scripts/dnscontrol_integrationtest_cnn, perhaps your object identifier is invalid?"},{"code":7000,"message":"No route for that URI"}]
--- FAIL: TestDNSProviders (0.92s)
=== RUN   TestDualProviders
    cloudflareProvider.go:699: [{"code":7003,"message":"Could not route to /accounts/workers/scripts/dnscontrol_integrationtest_cnn, perhaps your object identifier is invalid?"},{"code":7000,"message":"No route for that URI"}]
--- FAIL: TestDualProviders (0.87s)
FAIL
exit status 1
FAIL	github.com/StackExchange/dnscontrol/v3/integrationTest	2.041s

fdcastel · 2021-09-21T00:53:04Z

Sorry, Tom. I should have stressed this :

To make the tests work you must now set CLOUDFLAREAPI_ACCOUNTID and CLOUDFLAREAPI_ACCOUNTNAME. These are required to the API calls which create the workers.

That /accounts/workers/scripts/dnscontrol_integrationtest_cnn path on your error message should be /accounts/<accoundid>/workers/scripts/dnscontrol_integrationtest_cnn.

Maybe I should add a test and emit a Fail() in this case (missing account id and account name) to warn the user?

P.S: CLOUDFLAREAPI_ACCOUNTNAME is not used at all. It is only needed because of another check. Please see this comment.

tlimoncelli · 2021-09-21T14:58:42Z

Hi there!

I'm sorry but I'm having trouble getting this to run on a legacy domain.

It is very important that we make this change in an upwards compatible manner. Currently we get errors like:

$ go test -v -verbose -provider CLOUDFLAREAPI
=== RUN   TestDNSProviders
    cloudflareProvider.go:699: [{"code":7003,"message":"Could not route to /accounts/workers/scripts/dnscontrol_integrationtest_cnn, perhaps your object identifier is invalid?"},{"code":7000,"message":"No route for that URI"}]
--- FAIL: TestDNSProviders (0.96s)
=== RUN   TestDualProviders
    cloudflareProvider.go:699: [{"code":7003,"message":"Could not route to /accounts/workers/scripts/dnscontrol_integrationtest_cnn, perhaps your object identifier is invalid?"},{"code":7000,"message":"No route for that URI"}]
--- FAIL: TestDualProviders (0.80s)
FAIL
exit status 1
FAIL	github.com/StackExchange/dnscontrol/v3/integrationTest	2.087s

How about we add a feature-flag? For example, with "CLOUDFLARE_WORKERS=1" the new code runs. If it isn't set, the new code silently disables itself.

Tom

fdcastel · 2021-09-21T23:21:11Z

Sorry, Tom. I don't understand. Did you see my last message?

According to your error message:

 [{"code":7003,"message":"Could not route to /accounts/workers/scripts/dnscontrol_integrationtest_cnn, perhaps your object identifier is invalid?"},{"code":7000,"message":"No route for that URI"}]

It seems that the CLOUDFLAREAPI_ACCOUNTID wasn't set during your test. I can only presume this since the path above should be /accounts/<ACCOUNT-ID>/workers/....

To make the things clearer, this is how I ran the tests:

go build 

$env:CLOUDFLAREAPI_DOMAIN='my-test-domain.com'
$env:CLOUDFLAREAPI_KEY='my-api-key'
$env:CLOUDFLAREAPI_USER='my@email.com'

$env:CLOUDFLAREAPI_ACCOUNTID='my-account-id'
$env:CLOUDFLAREAPI_ACCOUNTNAME='MyAccountName'

go test -v ./integrationTest -verbose -provider CLOUDFLAREAPI

The AccountId can be seen on the browser url after the first logon to https://dash.cloudflare.com. You could also see it on "Workers" section from Cloudflare administration panel:

Please tell me how this has worked for you. I don't see the need for an additional feature flag given that we already added one (manage_workers) exactly in the name of backward compatibility.

tresni · 2021-09-30T01:45:38Z

#1267 and this will conflict. Happy to help build this on top of #1267 if that makes sense.

tresni · 2021-10-04T00:32:44Z

@tlimoncelli this PR seems to work fine for me. I don't use cloudflare workers normally and everything runs as expected (I don't set accountId and "manage_workers" defaults to false.) This is fully backwards compatible with existing code/deployments as is.

@fdcastel Here are the changes I made to work with latest master and get testing out of the main code. Still don't like that there is test setup code in here, but I tried everything I could think of, no easy way to move it out completely unless we make cloudflareProvider public (i.e. CloudflareProvider) as that would allow us to move all the setup code into intergration_test.go.

Also, this doesn't cleanup the workers after-the-fact which may not be great, but I don't think it's really hurting anything.

diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go
index a8250c51..28e749e4 100644
--- a/integrationTest/integration_test.go
+++ b/integrationTest/integration_test.go
@@ -70,8 +70,12 @@ func getProvider(t *testing.T) (providers.DNSServiceProvider, string, map[int]bo
 			}
 		}
 
-		// Cloudflare only. Will do nothing if provider != *cloudflareProvider.
-		cloudflare.PrepareCloudflareTestWorkers(t, provider)
+		if name == "CLOUDFLAREAPI" {
+			// Cloudflare only. Will do nothing if provider != *cloudflareProvider.
+			if err := cloudflare.PrepareCloudflareTestWorkers(provider); err != nil {
+				t.Fatal(err)
+			}
+		}
 
 		return provider, cfg["domain"], fails, cfg
 	}
diff --git a/providers/cloudflare/cloudflareProvider.go b/providers/cloudflare/cloudflareProvider.go
index 9ceb4418..e1e4851e 100644
--- a/providers/cloudflare/cloudflareProvider.go
+++ b/providers/cloudflare/cloudflareProvider.go
@@ -6,7 +6,6 @@ import (
 	"log"
 	"net"
 	"strings"
-	"testing"
 
 	"github.com/cloudflare/cloudflare-go"
 	"github.com/miekg/dns/dnsutil"
@@ -506,8 +505,6 @@ func newCloudflare(m map[string]string, metadata json.RawMessage) (providers.DNS
 		api.cfClient.AccountID = m["accountid"]
 	}
 
-	api.cfClient.AccountID = api.AccountID
-
 	if len(metadata) > 0 {
 		parsedMeta := &struct {
 			IPConversions   string   `json:"ip_conversions"`
@@ -674,17 +671,18 @@ func (c *cloudflareProvider) EnsureDomainExists(domain string) error {
 }
 
 // PrepareCloudflareWorkers creates Cloudflare Workers required for CF_WORKER_ROUTE tests.
-func PrepareCloudflareTestWorkers(t *testing.T, prv providers.DNSServiceProvider) {
+func PrepareCloudflareTestWorkers(prv providers.DNSServiceProvider) error {
 	cf, ok := prv.(*cloudflareProvider)
 	if ok {
 		err := cf.createTestWorker("dnscontrol_integrationtest_cnn")
 		if err != nil {
-			t.Fatal(err)
+			return err
 		}
 
 		err = cf.createTestWorker("dnscontrol_integrationtest_msnbc")
 		if err != nil {
-			t.Fatal(err)
+			return err
 		}
 	}
+	return nil
 }

tresni · 2021-10-04T00:40:20Z

@tlimoncelli to be clear, I ran the integration tests as follows. I've sanitized the domain I use as it's not really a testing domain :-P

$ cd integrationTest
$ export CLOUDFLAREAPI_ACCOUNTID=[ACCOUNT_ID]
$ export CLOUDFLAREAPI_DOMAIN=[DOMAIN]
$ export CLOUDFLAREAPI_TOKEN=[TOKEN]
$ go test -v -verbose -provider CLOUDFLAREAPI --start 41
=== RUN   TestDNSProviders
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/Clean_Slate:Empty
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:simple
    integration_test.go:208: CREATE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} cnn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_cnn ttl=1
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:changeScript
    integration_test.go:208: MODIFY WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN}: (cnn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_cnn ttl=1) -> (cnn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1)
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:changePattern
    integration_test.go:208: MODIFY WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN}: (cnn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1) -> (cable.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1)
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:Empty
    integration_test.go:208: DELETE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} cable.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:createMultiple
    integration_test.go:208: CREATE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} cnn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_cnn ttl=1
    integration_test.go:208: CREATE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} msnbc.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:addOne
    integration_test.go:208: CREATE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} api.${CLOUDFLAREAPI_DOMAIN}/cnn/*,dnscontrol_integrationtest_cnn ttl=1
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:changeOne
    integration_test.go:208: MODIFY WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN}: (msnbc.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1) -> (msn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1)
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:deleteOne
    integration_test.go:208: DELETE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} cnn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_cnn ttl=1
=== RUN   TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/Post_cleanup:Empty
    integration_test.go:208: DELETE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} api.${CLOUDFLAREAPI_DOMAIN}/cnn/*,dnscontrol_integrationtest_cnn ttl=1
    integration_test.go:208: DELETE WORKER_ROUTE ${CLOUDFLAREAPI_DOMAIN} msn.${CLOUDFLAREAPI_DOMAIN}/*,dnscontrol_integrationtest_msnbc ttl=1
--- PASS: TestDNSProviders (31.58s)
    --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN} (24.06s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/Clean_Slate:Empty (1.49s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:simple (2.36s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:changeScript (2.69s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:changePattern (2.78s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:Empty (1.06s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:createMultiple (3.17s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:addOne (2.31s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:changeOne (3.45s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/41:CF_WORKER_ROUTE:deleteOne (2.65s)
        --- PASS: TestDNSProviders/${CLOUDFLAREAPI_DOMAIN}/Post_cleanup:Empty (1.99s)
=== RUN   TestDualProviders
    integration_test.go:313: Clearing everything
    integration_test.go:319: Adding nameservers from another provider
WARNING: cloudflare does not support modifying NS records on base domain. ns1.example.com. will not be added.
WARNING: cloudflare does not support modifying NS records on base domain. ns2.example.com. will not be added.
    integration_test.go:322: Running again to ensure stability
WARNING: cloudflare does not support modifying NS records on base domain. ns1.example.com. will not be added.
WARNING: cloudflare does not support modifying NS records on base domain. ns2.example.com. will not be added.
--- PASS: TestDualProviders (9.31s)
PASS
ok  	github.com/StackExchange/dnscontrol/v3/integrationTest	41.469s
$

tlimoncelli · 2021-10-04T16:32:38Z

I'm getting a build error:

$ go install
# github.com/StackExchange/dnscontrol/v3/providers/cloudflare
providers/cloudflare/cloudflareProvider.go:509:30: api.AccountID undefined (type *cloudflareProvider has no field or method AccountID)

tresni · 2021-10-04T16:36:29Z

@tlimoncelli Yeah, need the patch I included above to make it work with latest master.

fdcastel · 2021-10-05T21:23:15Z

Sorry for the delay. Working on it now!

- CLOUDFLAREAPI: Initial support for CF_WORKER_ROUTE. - Put CF_WORKER_ROUTE behind a per-domain feature-flag. - Adds Integration Test. - Create Cloudflare workers for tests. - Updates documentation.

fdcastel · 2021-10-05T22:12:02Z

Rebase done with latest master. @tresni patch applied. All tests passing. 🎉

fdcastel · 2021-10-05T22:21:55Z

@tresni

Still don't like that there is test setup code in here, but I tried everything I could think of, no easy way to move it out completely unless we make cloudflareProvider public (i.e. CloudflareProvider) as that would allow us to move all the setup code into intergration_test.go.

Fully agreed. I don't like this either. We just took the pragmatic approach, here.

Also, this doesn't cleanup the workers after-the-fact which may not be great, but I don't think it's really hurting anything.

Yep. Not ideal. But, also agreed, not a big problem.

Thanks for the patch for the latest master. 👍

fdcastel · 2021-10-05T22:22:59Z

@tlimoncelli please let me know if you have any problem with the integration tests.

tresni · 2021-10-06T00:50:34Z

.github/workflows/build.yml

@@ -94,6 +94,8 @@ jobs:
      CLOUDFLAREAPI_KEY: ${{ secrets.CLOUDFLAREAPI_KEY }}
      CLOUDFLAREAPI_TOKEN: ${{ secrets.CLOUDFLAREAPI_TOKEN }}
      CLOUDFLAREAPI_USER: ${{ secrets.CLOUDFLAREAPI_USER }}
+      CLOUDFLAREAPI_ACCOUNTID: ${{ secrets.CLOUDFLAREAPI_ACCOUNTID }}
+      CLOUDFLAREAPI_ACCOUNTNAME: ${{ secrets.CLOUDFLAREAPI_ACCOUNTNAME }}


You can remove this, it's not needed anymore :)

Nice catch! 😅

tlimoncelli · 2021-10-06T13:02:48Z

Here's what I get:

tlimwork5:~/git/dnscontrol/integrationTest$ export CLOUDFLAREAPI_DOMAIN='dnscontroltest-cf.com'
tlimwork5:~/git/dnscontrol/integrationTest$ export CLOUDFLAREAPI_TOKEN='redacted'
tlimwork5:~/git/dnscontrol/integrationTest$ echo go test -v -verbose -provider CLOUDFLAREAPI
tlimwork5:~/git/dnscontrol/integrationTest$ go test -v -verbose -provider CLOUDFLAREAPI
=== RUN   TestDNSProviders
    integration_test.go:76: account ID required
--- FAIL: TestDNSProviders (0.00s)
=== RUN   TestDualProviders
    integration_test.go:76: account ID required
--- FAIL: TestDualProviders (0.00s)
FAIL
exit status 1
FAIL	github.com/StackExchange/dnscontrol/v3/integrationTest	0.250s

Since that failed, I tried setting the account id:

tlimwork5:~/git/dnscontrol/integrationTest$ export CLOUDFLAREAPI_ACCOUNTID=a5d8f5aa76a5b5518741f059b5189f9d
tlimwork5:~/git/dnscontrol/integrationTest$ go test -v -verbose -provider CLOUDFLAREAPI
=== RUN   TestDNSProviders
=== RUN   TestDNSProviders/dnscontroltest-cf.com
=== RUN   TestDNSProviders/dnscontroltest-cf.com/Clean_Slate:Empty
    integration_test.go:201: runTests: failed fetching worker route list cloudflare: HTTP status 403: Authentication error (10000)
=== RUN   TestDNSProviders/dnscontroltest-cf.com/00:GeneralACD:Create_an_A_record
    integration_test.go:201: runTests: failed fetching worker route list cloudflare: HTTP status 403: Authentication error (10000)
=== RUN   TestDNSProviders/dnscontroltest-cf.com/Post_cleanup:Empty
    integration_test.go:201: runTests: failed fetching worker route list cloudflare: HTTP status 403: Authentication error (10000)
=== RUN   TestDNSProviders/dnscontroltest-cf.com/01:WildcardACD:Create_wildcard

However if I run "dnscontrol preview" on Stackoverflow's dnsconfig.js or my personal dnsconfig.js it works just fine. So, that tells me that it is only the integration tests that are still poking at workers when they shouldn't.

tresni · 2021-10-07T00:24:20Z

@tlimoncelli that sounds like the API key doesn't have the right privileges to access workers. If you update the permissions to the following for your testing API key, it should go off with out a hitch.

I think this should be the expected behavior if we want integration tests to be able to test the worker routes. Another possibility would be to try and bail in the test group if CLOUDFLAREAPI_ACCOUNTID isn't set, but there's no existing support for that in the integration test framework. This leads to a default behavior of "if you want to test the Cloudflare integration, you need to test all of it." (You could replace Cloudflare with any other provider.) That seems to be how page rules are treated today (they just don't require an account id ;-) ).

fdcastel · 2021-10-08T11:06:09Z

@tlimoncelli Could you ask the person responsible for the Cloudflare account used in DNSControl tests to fix the permissions above?

tlimoncelli · 2021-10-10T14:17:30Z

@tlimoncelli Could you ask the person responsible for the Cloudflare account used in DNSControl tests to fix the permissions above?

Sadly I can't change that domain because I need it to test legacy configurations. That said, I'm providing fdcastel#2 which adds a flag to the integration tests for legacy domains and updates the docs.

If you merge those changes into your PR, we should be done.

I appreciate your patience. It's very busy at Stack right now as I'm in the middle of some big system upgrades. That said, I'm super excited to see this functionality come to DNSControl and I appreciate all your effort to get this working. Thanks!

fdcastel · 2021-10-11T19:00:59Z

Great! Working on it...

* alltrue() should be implemented like other filters. * Update documentation.

fdcastel · 2021-10-11T19:21:57Z

All done! Clean merge. Thanks @tlimoncelli !! 🙌

tlimoncelli · 2021-10-11T20:05:14Z

Thanks to everyone involved in getting this done!

fdcastel changed the title ~~CLOUDFLAREAPI: Initial support for CF_WORKER_ROUTE (without tests).~~ CLOUDFLAREAPI: Initial support for CF_WORKER_ROUTE (partial tests). Aug 30, 2021

tlimoncelli reviewed Aug 30, 2021

View reviewed changes

tlimoncelli reviewed Sep 2, 2021

View reviewed changes

docs/_functions/domain/CF_WORKER_ROUTE.md Outdated Show resolved Hide resolved

docs/_providers/cloudflare.md Show resolved Hide resolved

providers/cloudflare/rest.go Outdated Show resolved Hide resolved

fdcastel force-pushed the cf-worker-routes branch from 3eb0e13 to 56085e6 Compare September 2, 2021 23:01

fdcastel changed the title ~~CLOUDFLAREAPI: Initial support for CF_WORKER_ROUTE (partial tests).~~ CLOUDFLAREAPI: Initial support for CF_WORKER_ROUTE (with integration tests). Sep 2, 2021

fdcastel force-pushed the cf-worker-routes branch from 7a6f2cf to e272171 Compare September 20, 2021 15:28

fdcastel added 4 commits October 5, 2021 18:45

CLOUDFLAREAPI: Adds CF_WORKER_ROUTE.

a7e2fa4

- CLOUDFLAREAPI: Initial support for CF_WORKER_ROUTE. - Put CF_WORKER_ROUTE behind a per-domain feature-flag. - Adds Integration Test. - Create Cloudflare workers for tests. - Updates documentation.

Workaround for updateWorkerRoute causing stack overflow.

187edd4

Sets AccountID.

417647f

Updates to latest master (Thanks @tresni!).

f750a6a

fdcastel force-pushed the cf-worker-routes branch from 5a25b14 to f750a6a Compare October 5, 2021 22:10

tresni reviewed Oct 6, 2021

View reviewed changes

Removes $CLOUDFLAREAPI_ACCOUNTNAME.

25f8427

tlimoncelli added 2 commits October 11, 2021 16:17

Add alltrue() filter and -cfworkers flag

4480025

Final workers changes

f4ad8b6

* alltrue() should be implemented like other filters. * Update documentation.

fdcastel force-pushed the cf-worker-routes branch from 997c37f to f4ad8b6 Compare October 11, 2021 19:20

fdcastel mentioned this pull request Oct 11, 2021

Support legacy domains fdcastel/dnscontrol#2

Merged

tlimoncelli approved these changes Oct 11, 2021

View reviewed changes

tlimoncelli merged commit 0d12eb7 into StackExchange:master Oct 11, 2021

tlimoncelli mentioned this pull request Nov 27, 2021

Release v3.13.0 #1310

Merged

ashkulz mentioned this pull request Dec 28, 2021

CLOUDFLARE: Fix documentation for CF_WORKER_ROUTE #1351

Merged

CLOUDFLAREAPI: Adds CF_WORKER_ROUTE. #1243

CLOUDFLAREAPI: Adds CF_WORKER_ROUTE. #1243

Conversation

fdcastel commented Aug 30, 2021

fdcastel commented Aug 30, 2021

tlimoncelli left a comment

Choose a reason for hiding this comment

fdcastel commented Sep 2, 2021

tlimoncelli commented Sep 2, 2021

tlimoncelli commented Sep 2, 2021

fdcastel commented Sep 2, 2021

fdcastel commented Sep 2, 2021

fdcastel commented Sep 2, 2021

fdcastel commented Sep 2, 2021

fdcastel commented Sep 2, 2021

tlimoncelli commented Sep 3, 2021

fdcastel commented Sep 6, 2021

fdcastel commented Sep 11, 2021 • edited

fdcastel commented Sep 11, 2021

tlimoncelli commented Sep 12, 2021

fdcastel commented Sep 12, 2021

fdcastel commented Sep 20, 2021

fdcastel commented Sep 20, 2021

fdcastel commented Sep 20, 2021

fdcastel commented Sep 20, 2021

tlimoncelli commented Sep 20, 2021

fdcastel commented Sep 21, 2021 • edited

tlimoncelli commented Sep 21, 2021

fdcastel commented Sep 21, 2021

tresni commented Sep 30, 2021

tresni commented Oct 4, 2021

tresni commented Oct 4, 2021

tlimoncelli commented Oct 4, 2021

tresni commented Oct 4, 2021

fdcastel commented Oct 5, 2021

fdcastel commented Oct 5, 2021

fdcastel commented Oct 5, 2021

fdcastel commented Oct 5, 2021

tresni Oct 6, 2021

Choose a reason for hiding this comment

fdcastel Oct 8, 2021

Choose a reason for hiding this comment

tlimoncelli commented Oct 6, 2021

tresni commented Oct 7, 2021 • edited

fdcastel commented Oct 8, 2021

tlimoncelli commented Oct 10, 2021

fdcastel commented Oct 11, 2021

fdcastel commented Oct 11, 2021

tlimoncelli commented Oct 11, 2021

fdcastel commented Sep 11, 2021 •

edited

fdcastel commented Sep 21, 2021 •

edited

tresni commented Oct 7, 2021 •

edited