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

Added additional cypress tests for Security Analytics Dashboards plugin. #486

Merged

Conversation

AWSHurneyt
Copy link
Contributor

Description

  1. Fixed the exiting cypress tests to accommodate new functionality.
  2. Added additional cypress tests and related commands.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
@AWSHurneyt AWSHurneyt changed the title Added additional Security Analytics cypress tests Added additional cypress tests for Security Analytics Dashboards plugin. Feb 8, 2023
… for PRs to all branches.

Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
@AWSHurneyt
Copy link
Contributor Author

AWSHurneyt commented Feb 8, 2023

Still investigating the security analytics cypress test failures. They pass when run in a local functional test repo package, but are failing when run as part of a github action.

I'm unsure why the ISM cypress tests are failing, as the security analytics tests shouldn't have impacted those. I'll investigate after I've resolved the security analytics tests, and will possibly reach out to the ISM team for additional context.

…om waitForPageLoad command.

Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
@AWSHurneyt
Copy link
Contributor Author

The security analytics cypress tests are succeeding now. There is 1 failing ISM cypress test, but that shouldn't be related to the security analytics tests. Will look into resolving that test.

Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
@AWSHurneyt
Copy link
Contributor Author

I was able to reproduce the failing ISM test locally, and it does not seem to be an issue with the tests added by the addition of the security analytics tests.

The test is failing because the admin user doesn't have the permissions necessary to delete aliases. That permissions issue is causing the test to stall and fail because a sample alias needs to be deleted for the test to proceed. It appears the cypress tests were failing in PR #446 which merged these ISM tests to the 2.5 branch. @SuZhou-Joe, did you notice if the test failed for a similar reason in your initial PR?

I manually tested this permissions issue using the docker-compose.yml procedure described here to bring up a security-enabled domain. I logged into the domain using the admin/admin credentials, created an alias using the ISM UI, and attempted to delete that alias using the UI. The error popup below was thrown.

Screen Shot 2023-02-09 at 4 56 25 PM

I then attempted to delete the alias using devtools, and received the same error.

{
  "error": {
    "root_cause": [
      {
        "type": "security_exception",
        "reason": "no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=__user__]"
      }
    ],
    "type": "security_exception",
    "reason": "no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=__user__]"
  },
  "status": 403
}

To reiterate, the failing ISM cypress test is not related to the additional security analytics tests added in this PR.

@AWSHurneyt AWSHurneyt marked this pull request as ready for review February 10, 2023 01:33
@AWSHurneyt AWSHurneyt requested a review from a team as a code owner February 10, 2023 01:33
@CCongWang
Copy link
Collaborator

hi @AWSHurneyt , I see destination branch is 2.5, any specific reason that you don't want to merge to main? If it's a change that can be used in all versions >= 2.5, usually people merge their change to main, and backport to 2.x and 2.5

@AWSHurneyt
Copy link
Contributor Author

hi @AWSHurneyt , I see destination branch is 2.5, any specific reason that you don't want to merge to main? If it's a change that can be used in all versions >= 2.5, usually people merge their change to main, and backport to 2.x and 2.5

@CCongWang
The security analytics dashboards main branch isn't currently set to version 3.0 as the backend plugin is still on version 2.5. I believe the plan is to investigate bumping the plugin to 3.0 after the 2.5 release as the alerting plugin ran into issues when attempting to bump to 3.0. Because there are no 3.0 artifacts for security analytics, I was planning to merge these new tests to the 2.5 branch, and then create a separate PR to add them to the 3.0 branch once the plugin is successfully bumped.

Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
@tianleh tianleh merged commit 46e1937 into opensearch-project:2.5 Feb 13, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-486-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 46e193719d2aa2659f8e4cf6cd1a134ec5752359
# Push it to GitHub
git push --set-upstream origin backport/backport-486-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-486-to-2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants