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 job for ESCU Tests #358

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Add job for ESCU Tests #358

wants to merge 3 commits into from

Conversation

spanchal-crest
Copy link

No description provided.

@spanchal-crest spanchal-crest force-pushed the test/escu_testing branch 25 times, most recently from 1981dfe to 4a71719 Compare February 7, 2025 07:10
@spanchal-crest spanchal-crest force-pushed the test/escu_testing branch 5 times, most recently from 72f8f82 to 78ef048 Compare February 7, 2025 09:56
@spanchal-crest spanchal-crest force-pushed the test/escu_testing branch 8 times, most recently from 4de44b9 to 993aede Compare February 24, 2025 09:50
@spanchal-crest spanchal-crest force-pushed the test/escu_testing branch 2 times, most recently from 0698fba to bc87701 Compare February 27, 2025 11:26
- name: Install Python Dependencies and ContentCTL
run: |
python -m pip install --upgrade pip
pip install contentctl
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should discuss with Security Content team to have releases matching somehow the repo, as downloading develop and newest tool may be problematic at some point?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, we can

I asked this in our ESCU channel - https://splunk.slack.com/archives/C088U396U11/p1739876980416399?thread_ts=1739268536.325259&cid=C088U396U11

But Eric mentioned Unfortunately we don’t have a specific version of contentctl called out for building security_content

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes yes, they do not have, but can have :)

Comment on lines 455 to 482
# Load the YAML content
with open("security_content/contentctl.yml", "r") as file:
data = yaml.safe_load(file)
for app in data["apps"]:
if app['appid'] == APP_ID or GITHUB_REPOSITORY in app['hardcoded_path'] or app["title"] == APP_LABEL:
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}"
elif app['appid'] == "PALO_ALTO_NETWORKS_ADD_ON_FOR_SPLUNK" and APP_ID == "Splunk_TA_paloalto_networks":
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Load the YAML content
with open("security_content/contentctl.yml", "r") as file:
data = yaml.safe_load(file)
for app in data["apps"]:
if app['appid'] == APP_ID or GITHUB_REPOSITORY in app['hardcoded_path'] or app["title"] == APP_LABEL:
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}"
elif app['appid'] == "PALO_ALTO_NETWORKS_ADD_ON_FOR_SPLUNK" and APP_ID == "Splunk_TA_paloalto_networks":
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}"
# Load the YAML content
with open("security_content/contentctl.yml", "r") as file:
data = yaml.safe_load(file)
found = False
for app in data["apps"]:
if app['appid'] == APP_ID or GITHUB_REPOSITORY in app['hardcoded_path'] or APP_LABEL in app['title']:
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}"
found = True
if not found:
exit(127)

Copy link
Author

Choose a reason for hiding this comment

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

Don't we need this condition as well?
elif app['appid'] == "PALO_ALTO_NETWORKS_ADD_ON_FOR_SPLUNK" and APP_ID == "Splunk_TA_paloalto_networks":

Because the name in contentctl is I guess of the vendor TA and we have Splunk TA for palo alto.
So, unless the contentctl is updated with the proper name, we will need this.

name: escu_test_summary_results
path: |
security_content/test_results/summary.yml
security_content/dist/DA-ESS-ContentUpdate-latest.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Dow we need ths additonal file?

Copy link
Author

Choose a reason for hiding this comment

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

I think we can remove that.

echo "contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }}"
contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is failure of test making build red? - it should.

@mgazda-splunk mgazda-splunk marked this pull request as ready for review February 27, 2025 15:56
@mgazda-splunk mgazda-splunk requested a review from a team as a code owner February 27, 2025 15:56
@mgazda-splunk
Copy link
Contributor

Please also rebase first on newest develop @spanchal-crest

- name: Run Python Script
id: filter-detection-files
shell: python
run: |
Copy link
Member

Choose a reason for hiding this comment

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

Can we move it out to a separate file / script?

Copy link
Author

Choose a reason for hiding this comment

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

Can I know what would be the appropriate place to put that file?

echo "contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }}"
contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }}
Copy link
Member

Choose a reason for hiding this comment

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

How did you solve the performance aspect of running for big add-ons?

Copy link
Author

Choose a reason for hiding this comment

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

We have run the detections using a larger runner and it is very fast.

ESCU Tests in Windows TA using large runner (32 cores)
Using 4 containers - https://github.com/splunk/splunk-add-on-for-microsoft-windows/actions/runs/13492512521/job/37693050535 took ~52min
Using 8 containers - https://github.com/splunk/splunk-add-on-for-microsoft-windows/actions/runs/13492512521/job/37701127717 took ~35min
Using 12 containers - https://github.com/splunk/splunk-add-on-for-microsoft-windows/actions/runs/13492512521/job/37703683659 took ~35min

We have also tried other approach of splitting the tests into multiple jobs.

Copy link
Member

Choose a reason for hiding this comment

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

We have also tried other approach of splitting the tests into multiple jobs.

Can you please share the results for it?

Can I run the tests locally? What would be the performance of it? I expect folks to run tests (if possible) locally before pushing code to GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

@mgazda-splunk we run it completely in GitHub while other types of tests we do run in AWS, right? Did you consider running it in AWS instead? This PR brings testing logic into the reusable workflow - we use https://github.com/splunk/ta-automation-k8s-manifests/blob/main/test-addon.sh for testing logic.

@spanchal-crest spanchal-crest force-pushed the test/escu_testing branch 9 times, most recently from 859cc31 to 9f47022 Compare March 3, 2025 08:32
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