feat(mTLS): adds mTLS support to dataplane api-server

[EandrewJones]

Closes #50.

Questions

I am not sure how best to add tests for this. Unit testing the config trivially tests the behavior of Opt which we know works. Unit testing setup_tls forces me to mock read_to_string in the absence of an actual file to read in and all the ways I've read about doing that in rust feel really kludgie / dirty up the code with dependency injections, etc.

1. Is this all we need to add mTLS support for the api-server?
2. Is there some sort of API contract we need to maintain for this such that I can write a conformance or integration test?
3. What documentation would we like to see for this?

[aryan9600]

Unit testing setup_tls forces me to mock read_to_string in the absence of an actual file to read in

how about generating certs during the test, writing them to temp files and then passing those to setup_tls?

1. Is this all we need to add mTLS support for the api-server?

this looks good. we can test it using https://github.com/islishude/grpc-mtls-example/blob/main/cmd/client/main.go.

2. Is there some sort of API contract we need to maintain for this such that I can write a conformance or integration test?

we don't have an integration test for the current grpc server. i guess an integration test for the api server would involve spinning up the server and calling each method from an independent client? for testing mTLS specifically, i think we can just get away with only testing one method, since the thing we want to test is actually the TLS connection. something, like the answer to your first question, but coded in a script.

3. What documentation would we like to see for this?

we don't really have any docs apart from code comments. i think code comments for now is fine. documentation is an entirely separate conversation that we need to have soon.

[EandrewJones]

@aryan9600 All comments addressed apart from the one issue flagged below.

1. Is this all we need to add mTLS support for the api-server?

this looks good. we can test it using https://github.com/islishude/grpc-mtls-example/blob/main/cmd/client/main.go.

If we're not treating this as an integration test, where should we add this so it runs in our pipeline? We need the dataplane server running so we can mimic the client, so it seems like it should go into the integration test pipeline (tests/integration). However, the makefile states these tests are deprecated, so should I really be adding new tests to the suite?

[EandrewJones]

I pushed up a go test that creates certs, spins up the dataplane, and dials the grpc w/ mTLS from a client. The test passes, but I suspect it's a false positive because the dataplane exits almost immediately on my machine due to an error:

Error: map error: failed to create map `AYA_LOGS` with code -1

Caused by:
    0: failed to create map `AYA_LOGS` with code -1
    1: Operation not permitted (os error 1)
Error: map error: failed to create map `GATEWAY_INDEXES` with code -1

Caused by:
    0: failed to create map `GATEWAY_INDEXES` with code -1
    1: Operation not permitted (os error 1)

I only care about having a valid test at this point and adding it to our CI.

My guess is I need to deploy the dataplane into KTF to get a proper environment for testing. Is that correct @shaneutt ?

[EandrewJones]

Does the golang controlplane archival have any impact on this? Or do I just need to stand up KTF before I run these tests. Best Evan Jones Website: www.ea-jones.com

…

On Wed, Oct 23, 2024 at 4:50 PM Shane Utt ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/integration/dataplane_mtls_test.go <#280 (comment)> : > +// runDockerImage starts the Docker container with the specified name. +func runDockerImage(containerName, imageName string) error { + cmd := exec.Command( + "docker", + "run", + "--name", containerName, + "-d", + "-p", "9874:9874", + "-v", os.Getenv("PWD")+"/certs:/app/certs", + imageName, + "mutual-tls", + "--server-certificate-path", "/app/certs/server.pem", + "--server-private-key-path", "/app/certs/server-key.pem", + "--client-certificate-authority-root-path", "/app/certs/root.pem", + ) + return cmd.Run() +} Right, we use KTF specifically for MetalLB which we use for IPAM for our (bpf hijacked) services right now. So for a complete testing environment we're still a bit bound to KTF (or you can manually reproduce the same environment with your own metallb configmap). — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ2T6AMKOYZPALI3TE4QDBTZ5AY2ZAVCNFSM6AAAAABN3I65QGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJQHA3TSMJUGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[shaneutt]

Does the golang controlplane archival have any impact on this? Or do I just need to stand up KTF before I run these tests?

The archival shouldn't have an impact: there's a make build.cluster target that sends the right flags (and will automatically install ktf for you). The Golang integration tests actually were left in main despite removing all the other Go code (since the tests in still have value and help us validate our Rust changes until we replace them with Rust tests).

[EandrewJones]

@shaneutt @aryan9600 This should be ready for final review. I finally have passing tests. Upon lgtm, I'll squash my commits.

[EandrewJones]

Rebased. Best Evan Jones Website: www.ea-jones.com

…

On Mon, Dec 2, 2024 at 11:26 AM Shane Utt ***@***.***> wrote: Woops I still owe you a review! If you wouldn't mind doing a rebase to main I'll have some time this week to start a review (I was out the last couple weeks sorry for the lag ^_^) — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ2T6AK7S2D5FHCE5F4WF432DSX7TAVCNFSM6AAAAABN3I65QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJSGU2TQNJTGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[EandrewJones]

Changes to the docker-build on main break this branch's docker-build step now. Looks like it's due to the branch name violating docker syntax (can't include '#' in a docker image tag is my guess).

I won't have time to touch this this week. :(

[shaneutt]

Ok! No worries, no rush.

[EandrewJones]

You still can review. I don't recall what I've done since you last reviewed. Maybe the enum variants and the test?

…

On Tue, Dec 3, 2024, 08:31 Shane Utt ***@***.***> wrote: Ok! No worries, no rush. — Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ2T6AN4WBTFQXZM47FBMVD2DXMG3AVCNFSM6AAAAABN3I65QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJVGAZTQNJVGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[shaneutt]

Thank you so much for all your hard work making this happen.

I approve in that I think we're just about ready to go and I don't see any major blocking issues, however I do have some comments I would like to see resolved before we merge please. Also we gotta figure out the CI issue, but in any case, I think we're really close. Thank you!

[EandrewJones]

@shaneutt I addressed all your comments and fixed the docker-build naming issue by switching the workflow to use the PR number instead of branch name since it's guaranteed to comply with docker's naming rules.

The last thing I need to do is squash. Is there a way for me to squash and re-push without triggering the docker-build action? It's extremely expensive/long-running.

[shaneutt]

/approve
/unhold

[shaneutt]

Oh wow yeah 70m build time... holy cow.

[shaneutt]

We could always spend more time here, but given the project is still nascent and we do not have end-users for breakages to impact, let's merge this and start using it!

Thanks for all your hard work @EandrewJones 🙇

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EandrewJones, shaneutt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [shaneutt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment