Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Druid e2e tests (I) #296

Merged
merged 11 commits into from
Aug 18, 2022
Merged

Add Druid e2e tests (I) #296

merged 11 commits into from
Aug 18, 2022

Conversation

timuthy
Copy link
Member

@timuthy timuthy commented Feb 15, 2022

How to categorize this PR?

/area testing
/kind enhancement

What this PR does / why we need it:
This PR adds e2e tests for Etcd-Druid. It uses the foundation of #195 with additional automation.

Please see docs/development/local-e2e-tests.md for more information about how to run those tests.

Which issue(s) this PR fixes:
Fixes #194
Fixes parts of #286

Special notes for your reviewer:
Co-authored-by: Shreyas Rao shreyas.sriganesh.rao@sap.com

ℹ️ Test machinery integration is not part of this PR and will follow as soon as the credential rotation feature is implemented in TM.

Release note:

A Helm chart for deploying Etcd-Druid is now available in `charts/druid`.
Developers can now run Druid e2e tests via `make test-e2e`. Please see `docs/development/local-e2e-tests.md` for detailed information.

@timuthy timuthy requested a review from a team as a code owner February 15, 2022 14:47
@gardener-robot gardener-robot added area/testing Testing related kind/enhancement Enhancement, improvement, extension needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Feb 15, 2022
@gitguardian
Copy link

gitguardian bot commented Feb 15, 2022

⚠️ GitGuardian has uncovered 8 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- RSA Private Key fdccd30 test/e2e/resources/certs/ca.key View secret
- RSA Private Key fdccd30 test/e2e/resources/certs/client.key View secret
- RSA Private Key fdccd30 test/e2e/resources/certs/server.key View secret
- RSA Private Key ed35a92 test/e2e/resources/certs/client.key View secret
- RSA Private Key ed35a92 test/e2e/resources/certs/server.key View secret
- Generic Private Key ed35a92 test/e2e/resources/tls/client.key View secret
- Generic Private Key ed35a92 test/e2e/resources/tls/server.key View secret
- Generic Private Key f1f6ae2 test/e2e/resources/tls/server.key View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@timuthy timuthy changed the title Feature.e2e tests Add Druid e2e tests Feb 15, 2022
@timuthy timuthy mentioned this pull request Feb 15, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 15, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 16, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 16, 2022
@timuthy timuthy changed the title Add Druid e2e tests Add Druid e2e tests (I) Feb 16, 2022
@timuthy
Copy link
Member Author

timuthy commented May 30, 2022

/ping @gardener/etcd-druid-maintainers

@gardener-robot
Copy link

@gardener/etcd-druid-maintainers

Message

/ping @gardener/etcd-druid-maintainers

@timuthy timuthy force-pushed the feature.e2e-tests branch from 0560f0e to 508c05d Compare June 15, 2022 14:12
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022
@ishan16696
Copy link
Member

Hi @timuthy ,
When e2e-tests fails they don't do the cleanup of resources like secrets, config-map etc .... Manually doing the cleanup of resources locally won't be an issue but it might be an issue if we gonna use this as TM test in ci pipeline ... right ?
Can we try to do the cleanup of resources irrespective of e2e tests fails or succeeds?

@ishan16696
Copy link
Member

Hi @timuthy ,
I have retested this PR after you rebased on master.
The e2e tests fails with this error:

# github.com/gardener/etcd-druid/test/e2e [github.com/gardener/etcd-druid/test/e2e.test]
test/e2e/utils.go:127:3: cannot use "k8s.io/api/core/v1".SecretReference{...} (type "k8s.io/api/core/v1".SecretReference) as type "github.com/gardener/etcd-druid/api/v1alpha1".SecretReference in field value
test/e2e/utils.go:208:3: unknown field 'TLS' in struct literal of type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig
test/e2e/druid_test.go:59:19: etcd.Spec.Etcd.TLS undefined (type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig has no field or method TLS)
test/e2e/druid_test.go:60:19: etcd.Spec.Etcd.TLS undefined (type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig has no field or method TLS)
test/e2e/druid_test.go:61:19: etcd.Spec.Etcd.TLS undefined (type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig has no field or method TLS)
test/e2e/druid_test.go:698:19: etcd.Spec.Etcd.TLS undefined (type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig has no field or method TLS)
test/e2e/druid_test.go:699:19: etcd.Spec.Etcd.TLS undefined (type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig has no field or method TLS)
test/e2e/druid_test.go:700:19: etcd.Spec.Etcd.TLS undefined (type "github.com/gardener/etcd-druid/api/v1alpha1".EtcdConfig has no field or method TLS)
FAIL	github.com/gardener/etcd-druid/test/e2e [build failed]
FAIL
make: *** [test-e2e] Error 2

@timuthy
Copy link
Member Author

timuthy commented Jun 20, 2022

Thanks for the feedback @ishan16696. I'll have a 👀

@timuthy timuthy force-pushed the feature.e2e-tests branch from 508c05d to 350cc47 Compare July 22, 2022 15:16
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 22, 2022
@timuthy
Copy link
Member Author

timuthy commented Jul 22, 2022

The errors have been fixed with 350cc47. PTAL @ishan16696.

@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 22, 2022
@timuthy timuthy force-pushed the feature.e2e-tests branch from 350cc47 to 87ccb8b Compare July 25, 2022 09:06
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 16, 2022
@ishan16696
Copy link
Member

GitGuardian Security Checks is not having a green tick .... Is this expected ?

@ishan16696
Copy link
Member

ishan16696 commented Aug 17, 2022

Is this #296 (comment) has been addressed now or should we create a follow-up issue to track this just to delete the orphan resources if this has been enable it in a CI pipeline?

@timuthy
Copy link
Member Author

timuthy commented Aug 17, 2022

GitGuardian Security Checks is not having a green tick .... Is this expected ?

Yes, here it's because of the test TLS certificates and keys.

Is this #296 (comment) has been addressed now or should we create a follow-up issue to track this just to delete the orphan resources if this has been enable it in a CI pipeline?

I think it's quite helpful that the cleanup does not happen when tests fail because it gives developers the opportunity to troubleshoot the system, e.g. checking the logs of etcd-druid. For the pipeline this shouldn't be a problem either because we can define a separate make steps="cleanup" test-e2e step that is executed in any case.

@ishan16696
Copy link
Member

For the pipeline this shouldn't be a problem either because we can define a separate make steps="cleanup" test-e2e step that is executed in any case.

ok, make sense

@timuthy timuthy force-pushed the feature.e2e-tests branch from 9558d52 to 6b68058 Compare August 18, 2022 08:37
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 18, 2022
@timuthy timuthy force-pushed the feature.e2e-tests branch from 6b68058 to f1f6ae2 Compare August 18, 2022 08:49
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 18, 2022
@timuthy
Copy link
Member Author

timuthy commented Aug 18, 2022

As discussed @ishan16696, some improvements were required through f1f6ae2. PTAL.

Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overrall, looks good to me. Couple of nitpicks.
And Can you also mention in docs/development/local-e2e-tests.md about cleaning up your existing resources of cluster-scope like CRD,clusterroles before running these e2e-tests.

@ishan16696
Copy link
Member

Hey @timuthy ,
I have run the e2e-tests for Azure and AWS providers, so e2e-tests are running fine for both of the providers but one thing I noticed is shoot namespace is not getting deleted after cleanup step.
It also left some orphans resources in shoot namespace:

k -n shoot get secrets                                                                                                                 
NAME                  TYPE                DATA   AGE
ca-etcd-az            Opaque              2      2m36s
etcd-backup-az        Opaque              3      2m33s
etcd-client-tls-az    kubernetes.io/tls   3      2m36s
etcd-server-cert-az   kubernetes.io/tls   3      2m36s

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 18, 2022
@timuthy
Copy link
Member Author

timuthy commented Aug 18, 2022

Thanks for noticing @ishan16696. Unfortunately, that was due to a left-over I forgot. It's fixed now 🙂

@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 18, 2022
@ishan16696
Copy link
Member

yes, now it's getting cleanup, thanks for quick fix.
Would you like to address this as well #296 (review) ?

@timuthy
Copy link
Member Author

timuthy commented Aug 18, 2022

Thanks for the feedback @ishan16696 🚀 I addressed all comments.

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 18, 2022
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@timuthy timuthy merged commit f56dce4 into gardener:master Aug 18, 2022
@timuthy timuthy deleted the feature.e2e-tests branch August 18, 2022 13:50
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Testing related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Standalone Helm Chart for Etcd Druid
6 participants