Run acceptance tests against dev control #11
Conversation
mcoulombe
changed the base branch from
main
to
gesa/cockroach-acceptance-test
| MustInjectFilterRules(t, control, clientNodeKey, connectorNodeKey) | ||
| } | ||
|
|
||
| const tsDBDatabaseCapability = "tailscale.test/cap/databases" |
There was a problem hiding this comment.
Do you know if there'd be a good place to define this so it's publicly available to other packages? Maybe a /pkg/constants.go for some load-bearing values other projects might want to import?
There was a problem hiding this comment.
Other packages within the ts-db-connector project? Or more along the lines of contributing this back into testcontrol?
There was a problem hiding this comment.
Hang on I missed that this was about the constant. I'd prefer this to be an importable constant too.
There was a problem hiding this comment.
I'll probably get rid of this local constant for the fake control test when I restructure the code that grants the app cap to something closer to "set this ACL".
| }) | ||
| } | ||
|
|
||
| func devControlReadAPIKey(t *testing.T) string { |
There was a problem hiding this comment.
This is just to illustrate the approach, if we choose to go with an approach like this, we should do something a bit more robust than hardcoding assumptions about the control URL and the location of the "stolen" terraform-acceptance-test API key.
| ) | ||
|
|
||
| var accMode = flag.Bool("acc", false, "enable acceptance tests") | ||
| var accDevControlMode = flag.Bool("accdevcontrol", false, "enable acceptance tests against dev control") |
There was a problem hiding this comment.
What do you think of calling this tier of test e2e and it'll also include databases we cannot deploy locally such as Snowflake or the cloud-managed variants such as AWS RDS? Basically this tier of tests would be a superset of the acc tests but executed against real 3rd party systems.
There was a problem hiding this comment.
Good question about the naming... we're running the same test, just exchanging the external dependencies for more realistic ones. I'm not against e2e in principle, after all, we need a short and sweet name for this. And I don't have a better suggestion either! So let's go with that.
| func devControlGrantAppCap(t *testing.T, appCaps string, apiKey string) { | ||
| url := "http://localhost:31544/api/v2/tailnet/-/acl" | ||
|
|
||
| acl := fmt.Sprintf(`{ |
There was a problem hiding this comment.
In addition to the above, we would probably make additional provisions to make the ACL posting easier to troubleshoot, apart from just printing the payloads posted and received .
internal/relay_pgwire_test.go
Outdated
| ], | ||
| "grants": [ | ||
| {"src": ["*"], "dst": ["*"], "app": {"tailscale.test/cap/databases": [%s]}} | ||
| ]}`, appCaps) |
There was a problem hiding this comment.
An advantage of having the small abstraction mentioned in a previous PR to be able to generate filtering rules from ACLs is that we could use the same fixtures and configs for both acc and e2e tests. The acc test would just need to handle the ACL->filter conversion but it'd save the trouble of having to define different configs and test cases for the 2 variants.
There was a problem hiding this comment.
I'll check how far we can get (without having to reinvent testcontrol or creating something that incurs too much maintenance overhead) with the limited means of the fake control server in testcontrol. I'd prefer to create a separate ticket if it's non-trivial as it will likely involve a bunch of trade-off decisions that deserve a dedicated discussion.
| if !*accDevControlMode { | ||
| t.Skip("skipping: -accdevcontrol flag not provided") | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think it'd be possible to hide the difference between acc and e2e tests only into their setup steps but keep the test cases otherwise equal since the only thing that changes is how we hook extrnal dependencies?
This helper ideally should only be used for systems we cannot test with containers or testcontrol such as cloud-managed DBs.
There was a problem hiding this comment.
Not sure I misunderstand your comment, but wouldn't we want to provide a flag that allows running tests dev control separately from other ones, given that they take longer, require being able to start devcontrol, and at the moment also require clicking on the auth links as they pop up?
| t.Run("test CockroachDB relay", func(t *testing.T) { | ||
| testTailnet.TestCockroachDBRelay(t) | ||
| }) | ||
| } |
There was a problem hiding this comment.
We should have methods to create test fixtures such as setupControlServer(t) and based on the environment it should return the information for a test or real control server. I think this is where you are getting at and just put the setup logic in the pgwire_test file for exploration?
The test cases themselves should be oblivious to whether they run with fake or real dependencies. They should not have a concept of fake control.
There was a problem hiding this comment.
Yes, I would keep the shared test scenarios in a separate file, away from the code that defines the different flavours of test harnesses.
There was a problem hiding this comment.
Just to be sure - the test cases (as in TestPostgresRelay, TestCockroachDBRelay) already don't have a concept of which kind of control they're running on. They just mindlessly call the interface (TestTailnet).
internal/relay_pgwire_test.go
Outdated
| acl := fmt.Sprintf(`{ | ||
| "acls": [ | ||
| {"action":"accept","ports":["*:*"],"users":["*"]} | ||
| ], |
There was a problem hiding this comment.
ACLs section is not necessary if we have grants, it's just the old syntax.
There was a problem hiding this comment.
Converted to grant, for now just a similarly permissive one granting "ip": ["tcp:*"].
| "grants": [ | ||
| {"src": ["*"], "dst": ["*"], "app": {"tailscale.test/cap/databases": [%s]}} | ||
| ]}`, appCaps) | ||
| t.Logf("Overwriting ACL with: %s", acl) |
There was a problem hiding this comment.
Since grants are additive I wonder if we should merge the existing ACLs with the grants we need for the tests, and only delete the test grants during teardown.
It'd make running the tests less destructive than replacing the entire policy file.
There was a problem hiding this comment.
Yes, I didn't do anything like that because I hadn't arrived at a good heuristic for the merging logic but simply appending to the grants is probably sensible.
neinkeinkaffee
force-pushed
the
gesa/cockroach-acceptance-test
branch
2 times, most recently
from
85eb50a to
e0b41cb
Compare
Signed-off-by: Gesa Stupperich <gesa@tailscale.com>
neinkeinkaffee
force-pushed
the
gesa/dev-control-acceptance-tests
branch
from
7cd182d to
897114d
Compare
Signed-off-by: Gesa Stupperich <gesa@tailscale.com>
neinkeinkaffee
force-pushed
the
gesa/dev-control-acceptance-tests
branch
from
897114d to
79d183c
Compare
neinkeinkaffee
changed the base branch from
gesa/cockroach-acceptance-test
to
main
This illustrates one possible direction we could go down for ensuring that we can run the same acceptance tests against fake control (aka testcontrol) and real control (aka devcontrol).
This PR builds upon the PR that introduces the CockroachDB acceptance test. This is the diff commit that adds the dev control tests.
Relay tests against fake control are run when the
-accflag is passed togo test.Relay tests against dev control are run when the
-accdevcontrolflag is passed togo test. The assume that devcontrol is started with./tool/go run ./cmd/devcontrol --generate-test-devices=terraform-acceptance-testing.The test against dev control currently requires someone to actively sit there and click on the auth URL to connect the connector and client tsnet servers to the example.com tailnet. If we buy into the approach, we might want to create auth keys via the API instead, so as to avoid that interactive part.