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

[2.x] Fix issue with running integ tests with security #1022

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Sep 7, 2023

Description

The security test workflow is currently skipping running tests with security quietly. See the example from the latest commit on main here where Run AD Test is skipped.

The expressions to set values in the github workflow are not getting executed as expected and everything after the pipes is being ignored so when the test tries to pull the docker image it fails.

Specifically these lines:

plugin=`basename $(ls build/distributions/*.zip)` // This is getting set as expected, but not the following three lines utilizing piped commands
version=`echo $plugin|awk -F- '{print $4}'| cut -d. -f 1-3`
plugin_version=`echo $plugin|awk -F- '{print $4}'| cut -d. -f 1-4`
qualifier=`echo $plugin|awk -F- '{print $5}'| cut -d. -f 1-1`
if $qualifier!=SNAPSHOT
then
  docker_version=$version-$qualifier
else
  docker_version=$version
fi

i.e. the value for docker_version comes out to alerting instead of 2.10.0 so this pull:

docker pull opensearchstaging/opensearch:$docker_version

Becomes docker pull opensearchstaging/opensearch:alerting instead of docker pull opensearchstaging/opensearch:2.10.0


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: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
8KuslwkeixpzLDNISSbkeLpXz4xJI1ETMN/VG8ZZP1bjzlHziHHDu0JNZ6TnNzKr
XzCGMCohFfem8vnKNnKUneMQMvXd3rzUaAgvtf7Hc2LTBlf4fZzZF1EkwdSXhaMA
1lkfHiqOBxtgeDLxCHESZ2fqgVqsWX+t3qHQfivcPW6txtDyrFPRdJOGhiMGzT/t
e/9kkAtQRgpTb3skYdIOOUOV0WGQ60kJlFhAzIs=
Copy link
Member

@owaiskazi19 owaiskazi19 Sep 7, 2023

Choose a reason for hiding this comment

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

Certificates are updated in this PR: #1020. What's left to open the PR?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like security tests are passing in #1020

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at the output here from the latest run on main: https://github.com/opensearch-project/anomaly-detection/actions/runs/6103610427/job/16564313234. Expand on Pull and Run Docker.

The output shows:

plugin version plugin_version qualifier docker_version
(opensearch-time-series-analytics-3.0.0.0-SNAPSHOT.zip) (analytics) (analytics) (3) (analytics)

which is showing the wrong values.

Copy link
Member

Choose a reason for hiding this comment

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

Aah.. I see. This should be updated then.

The integTest doesn't pick up the demo certs. They are picked up by integTestRemote and since that is not run, this issue probly didn't surface early

Copy link
Member Author

@cwperks cwperks Sep 7, 2023

Choose a reason for hiding this comment

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

This PR still needs some updates. The backticks are fine, but the logic in the piped commands is not robustly handling the case where plugin=opensearch-time-series-analytics-3.0.0.0-SNAPSHOT.zip

plugin version plugin_version qualifier docker_version
(opensearch-time-series-analytics-3.0.0.0-SNAPSHOT.zip) (analytics) (analytics) (3) (analytics)

The commands to extract version, plugin_version and qualifier need revisiting to handle opensearch-time-series-analytics-3.0.0.0-SNAPSHOT.zip

Copy link
Member Author

Choose a reason for hiding this comment

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

So security tests are being skipped on main, but were being run on 2.x. The cert issue should have been spotted on CI in the 2.x branch within the last week after security merged the change to update the demo certs.

Copy link
Member Author

@cwperks cwperks Sep 7, 2023

Choose a reason for hiding this comment

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

@owaiskazi19 Was anomaly-detection renamed to time-series-analytics? If so, the cut awk commands in the test-security workflow on main need to be updated to account for the extra hyphen.

Instead of:

version=`echo $plugin|awk -F- '{print $4}'| cut -d. -f 1-3`
plugin_version=`echo $plugin|awk -F- '{print $4}'| cut -d. -f 1-4`
qualifier=`echo $plugin|awk -F- '{print $5}'| cut -d. -f 1-1`

it should become:

version=`echo $plugin|awk -F- '{print $5}'| cut -d. -f 1-3`
plugin_version=`echo $plugin|awk -F- '{print $5}'| cut -d. -f 1-4`
qualifier=`echo $plugin|awk -F- '{print $6}'| cut -d. -f 1-1`

on main, or find a more robust way of getting the values that can be run on both branches.

Copy link
Member Author

@cwperks cwperks Sep 7, 2023

Choose a reason for hiding this comment

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

Opened a PR to address the issue on main: #1023

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #1022 (e02b38c) into 2.x (4b21068) will increase coverage by 0.01%.
Report is 8 commits behind head on 2.x.
The diff coverage is 92.30%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #1022      +/-   ##
============================================
+ Coverage     79.42%   79.43%   +0.01%     
  Complexity     4321     4321              
============================================
  Files           307      307              
  Lines         18150    18159       +9     
  Branches       1909     1909              
============================================
+ Hits          14415    14425      +10     
+ Misses         2817     2816       -1     
  Partials        918      918              
Flag Coverage Δ
plugin 79.43% <92.30%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...va/org/opensearch/ad/AnomalyDetectorJobRunner.java 80.50% <ø> (ø)
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java 96.68% <ø> (ø)
...rg/opensearch/ad/AnomalyDetectorProfileRunner.java 67.95% <ø> (ø)
.../java/org/opensearch/ad/AnomalyDetectorRunner.java 43.52% <ø> (ø)
...in/java/org/opensearch/ad/EntityProfileRunner.java 75.40% <ø> (ø)
...opensearch/ad/ExecuteADResultResponseRecorder.java 75.17% <ø> (ø)
.../main/java/org/opensearch/ad/NodeStateManager.java 74.50% <ø> (ø)
src/main/java/org/opensearch/ad/ProfileUtil.java 94.44% <ø> (ø)
.../java/org/opensearch/ad/caching/PriorityCache.java 68.95% <ø> (ø)
.../opensearch/ad/cluster/ADClusterEventListener.java 86.11% <ø> (ø)
... and 164 more

@cwperks
Copy link
Member Author

cwperks commented Sep 7, 2023

Closing this PR. The security integ tests being skipped on main are related to anomaly-detection being renamed to time-series-analytics. New PR: #1023

@cwperks cwperks closed this Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants