Implement automated tests. #21

moticless · 2019-10-13T13:49:13Z

The tests have a common initialization flow and set of tests that follows.

Durinig intialization flow

the repo kubernetes-conjur-deploy is being cloned and deployed.
test_app namespace is being created
conjur master is being loaded with relevant policies

Execution of set of tests is being invoked via run_tests.sh that run each test such that

script test_case_setup.sh run before each test
script test_case_teardown.sh run after each test
on error, execution is being terminated immediately, leaving environment untouched.

Notes:

Under time limitation there was some compromise on code refactoring and code convention.
We should consider having proper tool for manipulting yaml files.

The tests have a common initialization flow and set of tests that follows. 1. Durinig intialization flow - the repo kubernetes-conjur-deploy is being cloned and deployed. - test_app namespace is being created - conjur master is being loaded with relevant policies 2. Execution of set of tests is being invoked via run_tests.sh that run each test such that - script test_case_setup.sh run before each test - script test_case_teardown.sh run after each test - on each error, execution is being terminated immediately, leaving environment untouched. Notes: - Under time limitation there was some compromise on code refactoring and code convention. - We should consider having proper tool for manipulting yaml files.

doodlesbykumbi

Left some comments. I think the test cases could really benefit from an abstract (if it's possible). I feel like using bash is having a negative impact on readability and maintainability.

test/k8s-config/secrets-access-role.sh.yml

test/k8s-config/test-env.sh.yml

test/stop

test/test_cases/TEST_ID_7_K8S_SECRETS_env_var_not_exist.sh

test/test_cases/TEST_ID_2_multiple_pods_changing_pwd_inbetween.sh

sgnn7

@moticless / @doodlesbykumbi Finally got some time to review this - let e know if you have any questions.

test/k8s-config/test-env.sh.yml

test/test_cases/TEST_ID_10_SECRETS_DESTINATION_not_exist.sh

test/test_cases/TEST_ID_1_providing_secret_successfully.sh

test/test_with_summon.sh

sgnn7 · 2019-10-21T15:11:38Z

test/utils.sh

+  $cli exec $(get_conjur_cli_pod_name) -- conjur variable values add $SECRET_NAME $SECRET_VALUE
+  set_namespace $TEST_APP_NAMESPACE_NAME
+}
+


Stray newline

- Update conjur version from 5.0-stable to 5.5.0 because tests get failed on known issue : “502 Bad Gateway… nginx”

sgnn7

@moticless Mostly non-blockers but there's a few big things that should probably addressed in this or followup PRs.

test/bootstrap.env

sgnn7 · 2019-11-04T14:36:57Z

test/k8s-config/test-env.sh.yml

 set -euo pipefail
+
+source $TEST_CASES_UTILS


Is there a reason why we source a path from a variable? Should the util script(s) have a stable path?

I didn't want that tests will be aware of the location of utils.sh since it is located outside of folder test_cases.

removed it. and source it only once via run_tests.sh

sgnn7 · 2019-11-04T14:39:46Z

test/k8s-config/test-env.sh.yml

+# related key-value from yaml file
+SECRETS_DESTINATION_KEY_VALUE=${SECRETS_DESTINATION_KEY_VALUE:-"SECRETS_DESTINATION k8s_secrets"}
+CONTAINER_MODE_KEY_VALUE=${CONTAINER_MODE_KEY_VALUE:-"CONTAINER_MODE init"}
+K8S_SECRETS_KEY_VALUE=${K8S_SECRETS_KEY_VALUE:-"K8S_SECRETS test-k8s-secret"}


It's not a blocker for merging but this seems a bit messy of a way to specify overrides vs CLI arguments.

This should be an issue: Improve parameterization of test scripts

test/k8s-config/test-env.sh.yml

sgnn7 · 2019-11-04T14:44:18Z

test/test_cases/TEST_ID_2_multiple_pods_changing_pwd_inbetween.sh

+pod_name1=$(cli_get_pods_test_env | awk '{print $1}')
+
+echo "Verify pod $pod_name1 has environment variable 'TEST_SECRET' with value 'supersecret'"
+verify_secret_value_in_pod $pod_name1 TEST_SECRET supersecret


Not a blocker either but keep in mind all of these unquoted vars are liable to injection or failure when combined with spaces or special chars

sgnn7 · 2019-11-04T14:50:02Z

test/test_cases/test_case_setup.sh

+
+source $TEST_CASES_UTILS
+
+# TODO: replace the following with `oc create secret`


Tiny comment: we shouldn't use any OC-specific CLIs if we can - ideally we should be able to set everything up using vanilla kubectl for portability. oc generally doesn't provide almost any additional functionality other than for account administration.

sgnn7 · 2019-11-04T14:55:43Z

test/test_in_docker.sh

+    git clone --single-branch \
+      --branch $KUBERNETES_CONJUR_DEPLOY_BRANCH \
+      https://github.com/cyberark/kubernetes-conjur-deploy.git \
+      kubernetes-conjur-deploy-$UNIQUE_TEST_ID


I just realized what this code is conceptually trying to do and I missed it in my last review. Using a separate per-test branch from another repo is extremely liable to fail and would be a huge perpetual effort to maintain. I don't have much guidance on what the tests should look like but this approach should be changed asap to something sturdier (even if it means that it uses conjur-intro repo deployments).

We are aware of this issue. This is what we have for now. :(

This needs to be a separate issue for tech debt

sgnn7 · 2019-11-04T14:57:24Z

test/test_local.sh

+    git clone --single-branch \
+       --branch $KUBERNETES_CONJUR_DEPLOY_BRANCH \
+       https://github.com/cyberark/kubernetes-conjur-deploy.git \
+       kubernetes-conjur-deploy-$UNIQUE_TEST_ID


Not a blocker: lots of overlap with test_in_docker.sh

sgnn7 · 2019-11-04T14:58:23Z

test/test_with_summon.sh


-exit $exit_code
+exit 0


This change makes the exit call unnecessary. If you have set -e everywhere and depend on that, that probably means that cleanup isn't properly done on failures.

sgnn7 · 2019-11-04T15:00:23Z

test/utils.sh

+   wait_for_it 600 "oc get dc/test-env -o jsonpath={.status.replicas} | grep '^${expected_num_replicas}$'|| oc rollout latest dc/test-env"
+
+   echo "Expecting for $expected_num_replicas deployed pods"
+   wait_for_it 600 "oc get pods --namespace=$TEST_APP_NAMESPACE_NAME --selector app=test-env --no-headers | wc -l | grep $expected_num_replicas"


these CLIs are hardcoded. Probably worthy to mention here that we should just use kubectl instead of oc everywhere.

modified to $cli

sgnn7

@moticless / @Tovli : Just 3 issues need to be opened and we ca get this approved.

sgnn7 · 2019-11-05T15:00:43Z

test/k8s-config/test-env.sh.yml

+# related key-value from yaml file
+SECRETS_DESTINATION_KEY_VALUE=${SECRETS_DESTINATION_KEY_VALUE:-"SECRETS_DESTINATION k8s_secrets"}
+CONTAINER_MODE_KEY_VALUE=${CONTAINER_MODE_KEY_VALUE:-"CONTAINER_MODE init"}
+K8S_SECRETS_KEY_VALUE=${K8S_SECRETS_KEY_VALUE:-"K8S_SECRETS test-k8s-secret"}


This should be an issue: Improve parameterization of test scripts

test/test_cases/TEST_ID_7_K8S_SECRETS_env_var_not_exist.sh

sgnn7 · 2019-11-05T15:03:29Z

test/test_in_docker.sh

+    git clone --single-branch \
+      --branch $KUBERNETES_CONJUR_DEPLOY_BRANCH \
+      https://github.com/cyberark/kubernetes-conjur-deploy.git \
+      kubernetes-conjur-deploy-$UNIQUE_TEST_ID


This needs to be a separate issue for tech debt

sgnn7

@moticless @Tovli Approved now that we have the following issues filed:

…grpc CNJR-3914: Remove vulnerable versions of grpc and go-sqlite3

moticless requested review from doodlesbykumbi and Tovli October 13, 2019 13:49

Implement automated tests - modify wait_for_it timeout from 30 to 600

c2be905

doodlesbykumbi reviewed Oct 15, 2019

View reviewed changes

sgnn7 suggested changes Oct 21, 2019

View reviewed changes

Moti Cohen added 4 commits October 24, 2019 17:29

- PR fixes of "Implement automated tests"

6cc6261

- Update conjur version from 5.0-stable to 5.5.0 because tests get failed on known issue : “502 Bad Gateway… nginx”

- PR fixes of "Implement automated tests"

87495f8

- Update conjur version from 5.0-stable to 5.5.0 because tests get failed on known issue : “502 Bad Gateway… nginx”

Small fix of environment variable declaration KEY_VALUE_NOT_EXIST

ca5fc1e

PR fixes

546a95c

sgnn7 suggested changes Nov 4, 2019

View reviewed changes

PR fixes2

6c5b6a5

sgnn7 suggested changes Nov 5, 2019

View reviewed changes

sgnn7 approved these changes Nov 5, 2019

View reviewed changes

sgnn7 merged commit fb5635f into master Nov 5, 2019

sgnn7 deleted the adding_integration_tests branch November 5, 2019 15:57

This was referenced Nov 11, 2019

avoid cloning repo kuernetes-conjur-deploy as part of testing #27

Closed

refactor tests #28

Closed

improve parameterization of test scripts #29

Open

conjur-jenkins pushed a commit that referenced this pull request Mar 28, 2024

Merge pull request #21 from Conjur-Enterprise/update-protobuf-remove-…

a1df574

…grpc CNJR-3914: Remove vulnerable versions of grpc and go-sqlite3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement automated tests. #21

Implement automated tests. #21

moticless commented Oct 13, 2019

doodlesbykumbi left a comment

sgnn7 left a comment

sgnn7 Oct 21, 2019

sgnn7 left a comment

sgnn7 Nov 4, 2019

moticless Nov 4, 2019

moticless Nov 4, 2019

sgnn7 Nov 4, 2019

sgnn7 Nov 5, 2019

sgnn7 Nov 4, 2019

sgnn7 Nov 4, 2019

moticless Nov 4, 2019

sgnn7 Nov 4, 2019

moticless Nov 4, 2019

sgnn7 Nov 5, 2019

sgnn7 Nov 4, 2019

sgnn7 Nov 4, 2019

moticless Nov 4, 2019

sgnn7 Nov 4, 2019

moticless Nov 4, 2019

sgnn7 left a comment

sgnn7 Nov 5, 2019

sgnn7 Nov 5, 2019

sgnn7 left a comment


		source $TEST_CASES_UTILS

		# TODO: replace the following with `oc create secret`


		exit $exit_code
		exit 0

Implement automated tests. #21

Implement automated tests. #21

Conversation

moticless commented Oct 13, 2019

doodlesbykumbi left a comment

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgnn7 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment