test,pkg: add e2e tests #30
Conversation
Based on github.com/coreos/vault-operator e2e tests
This is more idiomatic use of the test framework
|
LGTM |
vault-operator/Gopkg.toml
Outdated
| @@ -19,4 +19,4 @@ | |||
| name = "github.com/operator-framework/operator-sdk" | |||
| # The version rule is used for a specific release and the master branch for in between releases. | |||
| branch = "master" | |||
| # version ="v0.0.5" | |||
| # version ="v0.0.5" | |||
| @@ -62,13 +62,13 @@ func deployEtcdCluster(v *api.VaultService) (*eopapi.EtcdCluster, error) { | |||
| } | |||
|
|
|||
| // etcdNameForVault returns the etcd cluster's name for the given vault's name | |||
There was a problem hiding this comment.
capitalize \\ etcdNameForVault?
| @@ -0,0 +1,238 @@ | |||
| // Copyright 2018 The vault-operator Authors | |||
There was a problem hiding this comment.
I don't see the point to include the backup_restore_test.go. We need to have S3 for this to work. I am hoping that the test example we have shown here can be run locally. I believe that's what user expect as well.
cc/ @hasbro17
There was a problem hiding this comment.
@AlexNPavel We can remove backup_restore_test.go. The other basic and scale tests are good enough for example purposes. This test is just getting into the specifics of the vault-operator's reliance on the etcd-operator.
|
Also I am not sure if shaving all the tests from valut-operator e2e to here is a good idea. It is really hard to reason on what happened? Maybe we can just pick one test and port it to using operator-sdk test framework to illustrate how to properly write a production grade e2e test? |
|
@fanminshi The reason we have to run the test within a cluster is that we need the vault api to communicate with the vault pods. To do this outside of kubernetes, we need port forwarding. There used to be port forwarding, but it was really messy and was eventually changed. Now the vault-operator ci tests build an image that gets run from within the cluster to do the testing: https://github.com/coreos/vault-operator/blob/master/hack/ci/run_e2e If we want to allow these tests to run locally, we could port the port-forwarding functions that used to exist in the vault-operator into this repo. It would be a very large amount of work, and it is a very specific use case for this operator, so it may not make sense to include all of that in a sample project. In reference to your second comment, we would still need port-forwarding. I agree that removing the backup tests makes sense, but all the other tests are quite simple, standard tests that I think we should keep for example purposes. Maybe we could clean them up a bit, but they are good to keep as examples. |
|
@AlexNPavel Yeah, now i remember we use port forwarding to do unseal the vault pod. Just remove the backup and restore test then. Still I think the test example might be too much for reader to digest. |
This test is quite complicated and also requires S3 storage. It's quite specific as well. The other tests are better for example purposes.
| @@ -0,0 +1,56 @@ | |||
| // Copyright 2018 The vault-operator Authors | |||
There was a problem hiding this comment.
And probably remove this license carried over from the vault-operator.
|
@fanminshi I agree that these tests are a bit much for an example, but I feel like that's mostly due to how complicated vault itself is. We have to worry about 2 operators, we need to create the cluster, set the up, unseal them, wait for them to become active after that, look at standby nodes, etc. I feel like it would be better to add a different operator as a sample that is in between the complexity of the memcached-operator and vault-operator that would allow us to show how to use the operator-sdk in more complicated scenarios than the memcached operator, but without being as complex and as large as the vault-operator. It looks like the Dex operator being added (#24) may help with this. |
|
I agree with this. |
vault-operator/README.md
Outdated
| @@ -106,6 +106,19 @@ For example: | |||
|
|
|||
| `kubectl -n default get vault example ...` -> `kubectl -n default get vaultservice example ...` | |||
|
|
|||
| ## Tests | |||
| This repo contains some tests that use the operator-sdk's test framework. These tests are based directly on the original vault-operator | |||
| tests, and thus cannot fully complete when run on a local machine and must be run inside a kubernetes cluster instead. This is a very | |||
There was a problem hiding this comment.
Can you bold this sentence thus cannot fully complete when run on a local machine and must be run inside a kubernetes cluster instead so that the user knows that i can't be run locally.
|
lgtm after nits. |
This is a port of the tests from github.com/coreos/vault-operator that uses the operator-sdk test framework. The original tests are meant to run from within a kubernetes cluster since they need to talk with the vault cluster. Older versions used a port forwarder, but as this is just an example project that will not be actively used/tested, we will not be porting all of that over as that would be require an excessive amount of work. These tests are a good example of how to use the global and namespaced manifests and the tests will correctly create and teardown the vault instances, which is what this example is intended to demonstrate.
Based on github.com/coreos/vault-operator e2e tests