Test verify subcommand in Go end-to-end #185

burgerdev · 2024-02-29T15:03:04Z

No description provided.

katexochen

Otherwise looks good to me. 🦭

katexochen · 2024-02-29T17:02:38Z

e2e/openssl/openssl_test.go

+	output, err := os.MkdirTemp("", "nunki-verify.*")
+	require.NoError(err)
+	t.Cleanup(func() {
+		_ = os.RemoveAll(output)
+	})
+
+	coordinator, cancelPortforward, err := c.PortForwardPod(ctx, namespace, "port-forwarder-coordinator", "1313")
+	require.NoError(err)
+	t.Cleanup(cancelPortforward)
+
+	verify := cmd.NewVerifyCmd()
+	verify.SetArgs([]string{
+		"--output", output,
+		"--coordinator-policy-hash=", // TODO(burgerdev): enable policy checking
+		"--coordinator", coordinator,
+	})
+	verify.SetOut(io.Discard) // TODO: do we need it?
+	errBuf := &bytes.Buffer{}
+	verify.SetErr(errBuf)
+
+	if err := verify.Execute(); err != nil {
+		t.Log(string(errBuf.Bytes()))
+		t.Fatalf("could not verify coordinator: %v", err)
+	}
+
+	for _, expected := range []string{"manifest.0.json", "coordinator-root.pem", "mesh-root.pem"} {
+		_, err := os.Stat(path.Join(output, expected))
+		assert.NoError(err, "expected verify output to contain file %q", expected)
+	}
+


I'd say this shouldn't be part of the OpenSSL test. We should rather create a test suite in TestMain or at least run subtests in this within this test.

I moved this into a TestMain, but then realized that there's a bunch of duplication between that and the OpenSSL test proper, so I decided to restructure the Kubeclient a bit.

It now owns the k8s namespace (in anticipation of being responsible for creating it later).

It exposes a Setup/Teardown pair, to be called by the TestMain.

Nunki verify (and later apply and set) are part of Setup (and cleaned up in Teardown).

A bit ugly is how the kubeclient is passed to TestOpenssl (via global var).

3u13r

Good Job! the overall architecture looks very usable.

3u13r · 2024-03-04T12:14:05Z

e2e/internal/kubeclient/kubeclient.go

@@ -30,6 +30,13 @@ type Kubeclient struct {
 	client *kubernetes.Clientset
 	// config is the "Kubeconfig" for the client
 	config *rest.Config
+
+	// Below fields are only populated by Setup().


I think that this makes the Kubeclient a bit too stateful. At least we should think about separating the Kubernetes parts from the Contrast/testing parts.
Generally, I think it's fine to hold the namespace state at e.g. the testing level.

Thanks for the suggestion, it's probably for the better to split these up.

3u13r · 2024-03-04T12:17:53Z

e2e/internal/kubeclient/setup.go

+//
+// If any setup step fails, Setup returns an error but does not clean up any resources. Call
+// Teardown for that.
+func (k *Kubeclient) Setup() error {


Since we want to assert the behavior of the Nunki steps contained in here, this should likely be part of the test code. Also this makes more sense, since this code is purpose built for the e2e tests.

On the other hand, this is the same for all test variants (simple, openssl, enojivoto), and at least set is a prerequisite for running the tests at all. Maybe my idea of putting all of just default into Setup is wrong, and it should rather be Setup and a dedicated test for verify. Let me try to rework that.

e2e/openssl/openssl_test.go

e2e/internal/kubeclient/setup.go

3u13r · 2024-03-04T12:54:16Z

e2e/openssl/openssl_test.go

-// namespace the tests are executed in.
-const namespaceEnv = "K8S_NAMESPACE"
+// filled by TestMain
+var k *kubeclient.Kubeclient


I don't think that we need global state in the test. Sure, there is sort of global state since we are re-using on AKS, but by using different e.g. namespaces we can keep the tests independent (and also execute them conurrently if we were to scale up the AKS)

We have global state, in the sense that the coordinator is shared between tests and set up/torn down in the TestMain (see also @katexochen's requested changes). Sharing the kubeclient just seemed like a convenient way of sharing that state, but I agree that it's less obvious what state is shared.

Talked to Paul and convinced him that it's not a good idea to generally share the coordinator between tests. The reasoning is mostly that the emojivoto test should be completely independent of the openssl test.

Of course we want to re-use the coordinator for (sub-)tests of the same functionality, i.e. 1. Test is calling set and asserting the certs and the 2. test calls set again and also checks that the new mesh certs are not singed by the same Mesh/IntermRoot.

This creates some overhead, mostly in the memory the coordinator is using so we likely want to call those test with --parallel=1.

This creates some overhead, mostly in the memory the coordinator is using so we likely want to call those test with --parallel=1.

Test and subtests in the same package should run sequentially if they don't call t.Parallel(). The rest can be done using the sync server (soon™).

True for subtests, but I don't think execution order is specified for top-level tests.

No order isn't specified, and tests should not rely on order. But still won't run in parallel.

Yeah, according to what I found top-level test are run in separate go go routines which execution can overlap. I wasn't sure if we put all e2e tests in the same package, therefore the hint.

I was reading @3u13r's proposal as having set, verify and specific tests in separate Test functions on the same level, which only works if execution order is guaranteed. Maybe I got that wrong. But we can have ordered subtests to that:

func TestOpenSSL(t *testing.T) { c := kubeclient.NewForTest(t) // Generate and apply YAML cli := nunkitest.New(t) t.Run("set", cli.set) t.Run("verify", cli.verify) t.Run("openssl-specifics", func(t *testing.T) { // exec into backend, etc. }) }

Maybe even:

func TestBasicCommands(t *testing.T) { c := kubeclient.NewForTest(t) // Generate and apply YAML cli := nunkitest.New(t) t.Run("set", cli.set) t.Run("verify", cli.verify) // Teardown } func TestOpenSSL(t *testing.T) { c := kubeclient.NewForTest(t) // Generate and apply YAML cli := nunkitest.New(t) t.Run("set", cli.set) t.Run("openssl test a", func(t *testing.T) { // exec into backend, etc. }) t.Run("openssl test b", func(t *testing.T) { // exec into backend, etc. }) // Teardown }

The most important part here is that we get individual feedback from different tests, even if some of them fail. I'm not that sure about the grouping in this case, but we will for sure have tests that should be executed against a separate coordinator, so it might be a good idea to do that for the supported tests now so it is part of our test structure already.

Sounds good, thanks for the discussion everybody. While I revisit that, I would like to advertise a spin off at #191 that should be less contentious and would help me keep things focused here.

burgerdev · 2024-03-05T17:28:41Z

I rewrote this PR to focus on the use of verify for the OpenSSL test case. Next steps should be:

Add set to the Go test code, similarly to how verify is added here.
Factor verify (and set, ...) into a test helper lib to cater the common use cases (e.g. get mesh CA cert). I decided against doing that here because the concrete API is not obvious to me without having seen additional use cases, and I'd like to agree on the overall approach first.

e2e/openssl/openssl_test.go

This commit implements a first use case for calling the CLI commands from e2e test code. It is deliberately kept inline to focus on the test mechanics first. Once we need to call verify from more places, we can factor it into a test helper library. Co-authored-by: Paul Meyer <[email protected]>

stale

burgerdev requested a review from 3u13r February 29, 2024 15:03

burgerdev force-pushed the burgerdev/cmd-package branch from f0591ab to f51edc1 Compare February 29, 2024 15:08

burgerdev force-pushed the burgerdev/portforward branch from ce7aaa8 to 0779720 Compare February 29, 2024 15:08

Base automatically changed from burgerdev/cmd-package to main February 29, 2024 15:42

burgerdev force-pushed the burgerdev/portforward branch from 0779720 to e24c192 Compare February 29, 2024 15:43

burgerdev marked this pull request as ready for review February 29, 2024 15:44

burgerdev requested a review from katexochen as a code owner February 29, 2024 15:44

katexochen requested changes Feb 29, 2024

View reviewed changes

burgerdev force-pushed the burgerdev/portforward branch from e24c192 to 4928d38 Compare March 1, 2024 14:22

burgerdev requested a review from katexochen March 1, 2024 14:29

burgerdev force-pushed the burgerdev/portforward branch from 4928d38 to 9a5693d Compare March 1, 2024 15:45

3u13r previously requested changes Mar 4, 2024

View reviewed changes

This was referenced Mar 4, 2024

e2e: generate deployment yaml #189

Merged

e2e: add port forwarding and a simple use-case #191

Merged

burgerdev force-pushed the burgerdev/portforward branch from 1e1a190 to 8937588 Compare March 5, 2024 17:17

burgerdev requested a review from 3u13r March 5, 2024 17:28

burgerdev force-pushed the burgerdev/portforward branch from 8937588 to 72dfea9 Compare March 6, 2024 15:25

katexochen approved these changes Mar 7, 2024

View reviewed changes

e2e/openssl/openssl_test.go Outdated Show resolved Hide resolved

e2e/openssl/openssl_test.go Outdated Show resolved Hide resolved

burgerdev force-pushed the burgerdev/portforward branch from 01a39d5 to ae08e4d Compare March 7, 2024 09:41

burgerdev merged commit 5a3ae1e into main Mar 7, 2024
6 checks passed

burgerdev deleted the burgerdev/portforward branch March 7, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test verify subcommand in Go end-to-end #185

Test verify subcommand in Go end-to-end #185

burgerdev commented Feb 29, 2024

katexochen left a comment

katexochen Feb 29, 2024

burgerdev Mar 1, 2024

3u13r left a comment

3u13r Mar 4, 2024

burgerdev Mar 4, 2024

3u13r Mar 4, 2024

burgerdev Mar 4, 2024

3u13r Mar 4, 2024

burgerdev Mar 4, 2024

3u13r Mar 4, 2024

katexochen Mar 5, 2024

burgerdev Mar 5, 2024

katexochen Mar 5, 2024

3u13r Mar 5, 2024

burgerdev Mar 5, 2024

katexochen Mar 5, 2024

burgerdev Mar 5, 2024

burgerdev commented Mar 5, 2024

Test verify subcommand in Go end-to-end #185

Test verify subcommand in Go end-to-end #185

Conversation

burgerdev commented Feb 29, 2024

katexochen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3u13r 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

burgerdev commented Mar 5, 2024