Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add support for running CI with security #263

Merged
merged 5 commits into from
Jul 30, 2020

Conversation

ohltyler
Copy link
Contributor

Issue #, if available:

Description of changes:

This PR adds a workflow job to run all integration tests with security enabled.

Specific changes:

  1. Changes all Cypress visit() commands to custom visitWithAuth() commands to pass a username/password in request. This allows tests to run on a security-enabled domain, and will still work for non-security-enabled domains.
  2. Adds job in e2e workflow to run tests on security-enabled domain by setting up container using ODFE ES and Kibana images.
  3. Minor changes to other workflows to make it easier to bump versions during upgrades
  4. Increase default Cypress timeouts to avoid flaky failures

Latest run: https://github.com/ohltyler/anomaly-detection-kibana-plugin/actions/runs/187512306

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ohltyler ohltyler added the test fix/enhance testing label Jul 29, 2020
@ohltyler ohltyler linked an issue Jul 29, 2020 that may be closed by this pull request
@yizheliu-amazon
Copy link
Contributor

Looks like we are using cy.visitWithAuth() no matter if security is enabled or not. I am thinking maybe you can add one more env variable like IS_SECURITY_ENABLED for cypress tests, if true, use cy.visitWithAuth() if false, use cy.visit(). This can make sure cy.visit() is working for security disabled case.

And also, add 1 more case if possible: if with security, run test without auth, it should get expected error.

@ohltyler
Copy link
Contributor Author

ohltyler commented Jul 29, 2020

Looks like we are using cy.visitWithAuth() no matter if security is enabled or not. I am thinking maybe you can add one more env variable like IS_SECURITY_ENABLED for cypress tests, if true, use cy.visitWithAuth() if false, use cy.visit(). This can make sure cy.visit() is working for security disabled case.

And also, add 1 more case if possible: if with security, run test without auth, it should get expected error.

So like I mentioned it will still work if security is disabled, basically just passes unnecessary auth in request, but doesn't cause any failures and the security disabled server will just ignore it. I was thinking of adding a flag to call either visit() or visitWithAuth() in tests, but thought that would add a lot more lines since we call visit() so much.

I could look into overriding the visit() and then dynamically adding the auth or not, but it will work on security disabled either way.

Adding a test case that tests for no-auth on a security enabled server would only be possible if we had a dynamically set env variable like you mention. I've tested this case locally, if I remember right it just times out.

@ohltyler
Copy link
Contributor Author

@yizheliu-amazon looks like passing in env variables and overriding visit() should be pretty straightforward, thoughts on me adding that?

@yizheliu-amazon
Copy link
Contributor

@yizheliu-amazon looks like passing in env variables and overriding visit() should be pretty straightforward, thoughts on me adding that?

I would prefer not overriding visit(), which is cypress native method. We can have env variable indicating if security is enabled or not, and pass it to visitWithAuth(isSecurityEnabled).

Meanwhile, we can have 2 yarn target, one is 'cy:run', the other is 'cy:run-with-security', and for that security target, we add cypress run --env isSecurityEnabled=true. Env variable can be used like this.

We can let infra team use yarn cy:run-with-security for security case.

@ohltyler
Copy link
Contributor Author

ohltyler commented Jul 29, 2020

Please see last 2 commits, which mimics the exact behavior of cy.visit() but adds in the optional auth options if the env var SECURITY_ENABLED is set to true. I feel like this is better where we can run visit() like normal and handle the auth behind the scenes. Let me know if you still think I should create a separate command.

I definitely agree about adding a yarn script, will add.

@yizheliu-amazon
Copy link
Contributor

Please see last 2 commits, which mimics the exact behavior of cy.visit() but adds in the optional auth options if the env var SECURITY_ENABLED is set to true. I feel like this is better where we can run visit() like normal and handle the auth behind the scenes. Let me know if you still think I should create a separate command.

I definitely agree about adding a yarn script, will add.

It looks good to me. Thanks for the change.

@ohltyler ohltyler merged commit 455e3f8 into opendistro-for-elasticsearch:master Jul 30, 2020
@ohltyler ohltyler deleted the securityCI branch July 30, 2020 03:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test fix/enhance testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E test with security plugin enabled
3 participants