cnti-testcatalog / testsuite

📞📱☎️📡🌐 Cloud Native Telecom Initiative (CNTI) Test Catalog is a tool to check for and provide feedback on the use of K8s + cloud native best practices in networking applications and platforms
https://wiki.lfnetworking.org/display/LN/Test+Catalog
Apache License 2.0
169 stars 70 forks source link

[BUG] coredns should pass default_namespace #1952

Closed collivier closed 2 months ago

collivier commented 2 months ago

Describe the bug

example-cnfs/coredns/ fails passing default_namespace (other example should be checked) Please note it fails in gates too: https://github.com/cnti-testcatalog/testsuite/actions/runs/8564278589/job/23493678803#step:11:1170

./cnf-testsuite default_namespace
Deployment coredns-coredns in default namespace failed. validation error: Using 'default' namespace is not allowed for pod controllers. rule validate-podcontroller-namespace failed at path /metadata/namespace/
✖️  FAILED: Resources are created in the default namespace 🏷️
✔️  PASSED: default namespace is not being used 🏷️
kubectl get pods -n default
NAME                               READY   STATUS    RESTARTS   AGE
coredns-coredns-7b6b6c5d6d-cg4qk   1/1     Running   0          5m58s

To Reproduce

./cnf-testsuite setup
./cnf-testsuite cnf_setup  cnf-config=./example-cnfs/coredns/cnf-testsuite.yml
./cnf-testsuite default_namespace

Expected behavior

default_namespace in success

martin-mat commented 2 months ago

My result with with coredns is as expected:

$ ./cnf-testsuite default_namespace Deployment coredns-coredns in default namespace failed. validation error: Using 'default' namespace is not allowed for pod controllers. rule validate-podcontroller-namespace failed at path /metadata/namespace/ ✖️ FAILED: Resources are created in the default namespace 🏷️

The facts:

As for results you are getting, it looks you are getting both passed and failed. This is typically because of some leftovers/mess in "cnfs" directory. I'd suggest to run "./cnf-testsuite cleanup_all" and re-run cnf deployment and test.

collivier commented 2 months ago

@martin-mat I agree the test is correct here. I rather meant here to add helm_install_namespace: cnfspace in example-cnfs/coredns/cnf-testsuite.yml to allow the test passing in gates and by the endusers as it's a main example.

I don't see any reason to fail here as it's not a negative test. Could we basically add helm_install_namespace in all examples ?

collivier commented 2 months ago

@martin-mat It's quite the same case for rollback. Why not adding the following config in example-cnfs/coredns/cnf-testsuite.yml

git diff
diff --git a/example-cnfs/coredns/cnf-testsuite.yml b/example-cnfs/coredns/cnf-testsuite.yml
index c805c460..955a099a 100644
--- a/example-cnfs/coredns/cnf-testsuite.yml
+++ b/example-cnfs/coredns/cnf-testsuite.yml
@@ -7,3 +7,11 @@ helm_repository: # CONFIGURATION OF HELM REPO - ONLY NEEDED WHEN USING helm_char
 #helm_directory: coredns # PATH_TO_CNFS_HELM_CHART ; or
 #manifest_directory: coredns # PATH_TO_DIRECTORY_OF_CNFS_MANIFEST_FILES ; or
 release_name: coredns # DESIRED_HELM_RELEASE_NAME
+helm_install_namespace: cnfspace
+allowlist_helm_chart_container_names: []
+container_names:
+  - name: coredns
+    rolling_update_test_tag: "1.8.0"
+    rolling_downgrade_test_tag: 1.6.7
+    rolling_version_change_test_tag: 1.8.0
+    rollback_from_tag: 1.8.0
martin-mat commented 2 months ago

@martin-mat I agree the test is correct here. I rather meant here to add helm_install_namespace: cnfspace in example-cnfs/coredns/cnf-testsuite.yml to allow the test passing in gates and by the endusers as it's a main example.

I don't see any reason to fail here as it's not a negative test. Could we basically add helm_install_namespace in all examples ?

Oh, ok then. It was not clear to me from the bug description. I agree then.

collivier commented 2 months ago

I will update all example-cnfs' cnf-testsuite.yml in a next PR.

@martin-mat coud we also agree to add the missing rollback data for all coredns examples ?

martin-mat commented 2 months ago

I will update all example-cnfs' cnf-testsuite.yml in a next PR.

@martin-mat coud we also agree to add the missing rollback data for all coredns examples ?

My 5-cent reply is yes, they should be added as well. (unless somebody else sees any reason against this).

collivier commented 2 months ago

@HashNuke thank you for the review

HashNuke commented 2 months ago

Thanks for the issue report and PR. I'll merge it in soon.