Closed Mikaayenson closed 1 month ago
Customer Update
It's forked but not forked, I have to wipe my repo everytime I update since I am not allowed to have externally forked repos in my GH Org.
I delete everything but my custom rules folder and then c/p everything into my repo from detection_rules except the .git...
This seems to not be the problem with origin/main
as anticipated earlier because when we create fork, the main branch is the same origin/main
As for testing, a local fork was created and setup
detection-rules on main is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co
❯ git remote -v
origin git@github.com:shashank-elastic/detection-rules.git (fetch)
origin git@github.com:shashank-elastic/detection-rules.git (push)
detection-rules on main is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co
❯
The failing Unit test was run on the forked setup and it executed as expected without an above mentioned errors.
detection-rules on main is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co took 49s
❯ pytest tests/test_all_rules.py::TestRuleMetadata::test_deprecated_rules
======================================================================================================================================= test session starts =======================================================================================================================================
platform darwin -- Python 3.12.3, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/shashankks/fork_testing/detection-rules
configfile: pyproject.toml
plugins: typeguard-3.0.2
collected 1 item
tests/test_all_rules.py . [100%]
======================================================================================================================================= 1 passed in 47.78s ========================================================================================================================================
The original problem lies, where the user is removing everything apart from custom rules folder which will also remove _deprecated rules folder, which causes this failure, reproduced it locally by removing the _deprecated rules folder, similar testcases where failing [ although i never wiped clean the repo so depending test cases would fail]
# f'Re-add to the deprecated folder and update maturity to "deprecated": \n {missing_rule_strings}'
# self.assertEqual([], missing_rules, err_msg)
for rule_id, entry in deprecations.items():
# if a rule is deprecated and not backported in order to keep the rule active in older branches, then it
# will exist in the deprecated_rules.json file and not be in the _deprecated folder - this is expected.
# However, that should not occur except by exception - the proper way to handle this situation is to
# "fork" the existing rule by adding a new min_stack_version.
if PACKAGE_STACK_VERSION < Version.parse(entry['stack_version'], optional_minor_and_patch=True):
continue
rule_str = f'{rule_id} - {entry["rule_name"]} ->'
> self.assertIn(rule_id, deprecated_rules, f'{rule_str} is logged in "deprecated_rules.json" but is missing')
E AssertionError: '041d4d41-9589-43e2-ba13-5680af75ebc2' not found in {} : 041d4d41-9589-43e2-ba13-5680af75ebc2 - Deprecated - Potential DNS Tunneling via Iodine -> is logged in "deprecated_rules.json" but is missing
tests/test_all_rules.py:645: AssertionError
===================================================================================================================================== short test summary info =====================================================================================================================================
FAILED tests/test_all_rules.py::TestRuleMetadata::test_deprecated_rules - AssertionError: '041d4d41-9589-43e2-ba13-5680af75ebc2' not found in {} : 041d4d41-9589-43e2-ba13-5680af75ebc2 - Deprecated - Potential DNS Tunneling via Iodine -> is logged in "deprecated_rules.json" but is missing
===============================================================================
Since the test specifically looks for those files, am not sure what can be a better fix here other than the one where we could gracefully warn or skip if the rule path is not present!
cc @Mikaayenson
Additional Update
detection-rules on main [$?] is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co
❯ rm -rf .git/*
zsh: sure you want to delete all 11 files in /Users/shashankks/fork_testing/detection-rules/.git [yn]? y
~/fork_testing/detection-rules is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co took 2s
❯ rmdir .git/
~/fork_testing/detection-rules is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co
❯ ls -ltr .git
ls: .git: No such file or directory
origin/main
~/fork_testing/detection-rules is 📦 v0.1.0 via 🐍 v3.12.3 (.venv) on ☁️ shashank.suryanarayana@elastic.co
❯ pytest tests/test_all_rules.py::TestRuleMetadata::test_deprecated_rules
======================================================================================================================================= test session starts =======================================================================================================================================
platform darwin -- Python 3.12.3, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/shashankks/fork_testing/detection-rules
configfile: pyproject.toml
plugins: typeguard-3.0.2
collected 1 item
tests/test_all_rules.py . [100%]
======================================================================================================================================= 1 passed in 45.83s
_deprecated
rules file path exist Awaiting user response for replicating the issue
Customer Updated
Further investigation
Ok looks like something caught my eye in the GH workflow you have. By principle actions/checkout Only a single commit is fetched by default, Refer. But for our test case to work, we would need a remote branch. if you look at the original detection-rules repo workflow. We have a fetch depth of 1 and we fetch the remote branch. Here is a small snippet that you can add to your workflow and enable the test to see if it passes. Am using the latest checkout version which is identical to our codebase.
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 1
- name: Fetch main branch
run: |
git fetch origin main:refs/remotes/origin/main
User Confirmed the suggested fixes on workflow worked for the customer. Refer
Closing this issue
Describe the Bug
As reported on our community slack channel, unit tests like
test_deprecated_rules_modified
break on forked repos.To Reproduce
.git
Expected Behavior
The unit test should work on our PRs and forked implementations of our repo.
Screenshots
Provided Details
``` Jim Antonya 22 hours ago =================================== FAILURES =================================== _______________ TestRuleMetadata.test_deprecated_rules_modified ________________ self =
def test_deprecated_rules_modified(self):
"""Test to ensure deprecated rules are not modified."""
rules_path = get_path("rules", "_deprecated")
# Use git diff to check if the file(s) has been modified in rules/_deprecated directory
detection_rules_git = make_git()
> result = detection_rules_git("diff", "--diff-filter=M", "origin/main", "--name-only", rules_path)
tests/test_all_rules.py:637:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
detection_rules/utils.py:392: in git
return subprocess.check_output(full_args, encoding="utf-8").rstrip()
/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/subprocess.py:466: in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
input = None, capture_output = False, timeout = None, check = True
popenargs = (['/usr/bin/git', '-C', PosixPath('/home/runner/work/SECOPS-Detection-Engineering/SECOPS-Detection-Engineering'), 'diff', '--diff-filter=M', 'origin/main', ...],)
kwargs = {'encoding': 'utf-8', 'stdout': -1}
process =
stdout = '', stderr = None, retcode = 128
def run(*popenargs,
input=None, capture_output=False, timeout=None, check=False, **kwargs):
"""Run command with arguments and return a CompletedProcess instance.
The returned instance will have attributes args, returncode, stdout and
stderr. By default, stdout and stderr are not captured, and those attributes
will be None. Pass stdout=PIPE and/or stderr=PIPE in order to capture them,
or pass capture_output=True to capture both.
If check is True and the exit code was non-zero, it raises a
CalledProcessError. The CalledProcessError object will have the return code
in the returncode attribute, and output & stderr attributes if those streams
were captured.
If timeout is given, and the process takes too long, a TimeoutExpired
exception will be raised.
There is an optional argument "input", allowing you to
pass bytes or a string to the subprocess's stdin. If you use this argument
you may not also use the Popen constructor's "stdin" argument, as
it will be used internally.
By default, all communication is in bytes, and therefore any "input" should
be bytes, and the stdout and stderr will be bytes. If in text mode, any
"input" should be a string, and stdout and stderr will be strings decoded
according to locale encoding, or by "encoding" if set. Text mode is
triggered by setting any of text, encoding, errors or universal_newlines.
The other arguments are the same as for the Popen constructor.
"""
if input is not None:
if kwargs.get('stdin') is not None:
raise ValueError('stdin and input arguments may not both be used.')
kwargs['stdin'] = PIPE
if capture_output:
if kwargs.get('stdout') is not None or kwargs.get('stderr') is not None:
raise ValueError('stdout and stderr arguments may not be used '
'with capture_output.')
kwargs['stdout'] = PIPE
kwargs['stderr'] = PIPE
with Popen(*popenargs, **kwargs) as process:
try:
stdout, stderr = process.communicate(input, timeout=timeout)
except TimeoutExpired as exc:
process.kill()
if _mswindows:
# Windows accumulates the output in a single blocking
# read() call run on child threads, with the timeout
# being done in a join() on those threads. communicate()
# _after_ kill() is required to collect that and add it
# to the exception.
exc.stdout, exc.stderr = process.communicate()
else:
# POSIX _communicate already populated the output so
# far into the TimeoutExpired exception.
process.wait()
raise
except: # Including KeyboardInterrupt, communicate handled that.
process.kill()
# We don't call process.wait() as .__exit__ does that for us.
raise
retcode = process.poll()
if check and retcode:
> raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
E subprocess.CalledProcessError: Command '['/usr/bin/git', '-C', PosixPath('/home/runner/work/SECOPS-Detection-Engineering/SECOPS-Detection-Engineering'), 'diff', '--diff-filter=M', 'origin/main', '--name-only', '/home/runner/work/SECOPS-Detection-Engineering/SECOPS-Detection-Engineering/rules/_deprecated']' returned non-zero exit status 128.
```
Desktop - OS
other - explain
Desktop - Version
CICD
Additional Context
We may be able to get the base branch pseudocode
That way it works for us and forked repos. We need to do something similar to the
test_rule_change_has_updated_date
unit test as well.