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

Removes driver downloads and driver candidates #1770

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

Stringy
Copy link
Collaborator

@Stringy Stringy commented Aug 2, 2024

Description

Final (?) removal of driver downloads and collection method switching. This removes the file downloader, the kernel object downloader, driver candidates, and thus simplifies various places where they were used.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.

For more details, ref the Confluence page about this section.

@Stringy Stringy marked this pull request as ready for review August 2, 2024 10:24
@Stringy Stringy requested a review from a team as a code owner August 2, 2024 10:24
@Stringy Stringy requested a review from Molter73 August 2, 2024 12:34
Comment on lines +22 to +23
CLOG(WARNING) << "Unexpected CollectionMethod: " << static_cast<uint8_t>(method);
return "unknown";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would argue ERROR is a more appropriate severity, since we are not going to be able to continue working with an invalid collection method. I would go as far as to go into FATAL, but I'm an extremist.

Copy link
Collaborator Author

@Stringy Stringy Aug 2, 2024

Choose a reason for hiding this comment

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

Yeah I went for a warning just due to the function's purpose -- other parts of the codebase can be a bit more strict about validating the collection method (in fact in CollectorConfig.cpp it's switching to CORE_BPF on an invalid method, so I think we're safe)

Copy link
Collaborator

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

This is the kind of PR all devs dream of, just remove a bunch of unneeded stuff!

Just a small comment left on log severity, but I wouldn't block merging on it. LGTM!

@Stringy Stringy merged commit 4a983b1 into master Aug 2, 2024
46 checks passed
@Stringy Stringy deleted the giles/cleanup-driver-downloads branch August 2, 2024 13:41
Molter73 added a commit that referenced this pull request Aug 5, 2024
This commit started as an attempt to remove the MODULE_VERSION file, but
is quickly devolved into removing a lot of unused code. Namely:

- Removed the MODULE_VERSION file and all direct and indirect references
  to it in collector code, ansible playbooks, docker files, GHA
  workflows, etc..
- Removed support-package scripts that were unused and would be broken
  after removing the MODULE_VERSION file.
- Removed code and references to the /module directory that would be
  unused after #1770. This also triggered some additional cleanup on
  constants and environment variables associated to driver downloads
  that were leftover.
- Fixed a CMake warning due to cmake_minimum_required directive being
  called after the project directive.
- Removed the WITH_CHISEL configuration since chisels have been removed
  from upstream and it is no longer being used after the latest falco
  update.
- Updated docs as best as possible.
Molter73 added a commit that referenced this pull request Aug 5, 2024
This commit started as an attempt to remove the MODULE_VERSION file, but
is quickly devolved into removing a lot of unused code. Namely:

- Removed the MODULE_VERSION file and all direct and indirect references
  to it in collector code, ansible playbooks, docker files, GHA
  workflows, etc..
- Removed support-package scripts that were unused and would be broken
  after removing the MODULE_VERSION file.
- Removed code and references to the /module directory that would be
  unused after #1770. This also triggered some additional cleanup on
  constants and environment variables associated to driver downloads
  that were leftover.
- Fixed a CMake warning due to cmake_minimum_required directive being
  called after the project directive.
- Removed the WITH_CHISEL configuration since chisels have been removed
  from upstream and it is no longer being used after the latest falco
  update.
- Updated docs as best as possible.
Molter73 added a commit to stackrox/stackrox that referenced this pull request Aug 5, 2024
After stackrox/collector#1770, collector is no longer able to download
drivers, so the /module directory is now effectively unused and can
safely be removed from the helm charts.
Molter73 added a commit that referenced this pull request Aug 6, 2024
This commit started as an attempt to remove the MODULE_VERSION file, but
is quickly devolved into removing a lot of unused code. Namely:

- Removed the MODULE_VERSION file and all direct and indirect references
  to it in collector code, ansible playbooks, docker files, GHA
  workflows, etc..
- Removed support-package scripts that were unused and would be broken
  after removing the MODULE_VERSION file.
- Removed code and references to the /module directory that would be
  unused after #1770. This also triggered some additional cleanup on
  constants and environment variables associated to driver downloads
  that were leftover.
- Fixed a CMake warning due to cmake_minimum_required directive being
  called after the project directive.
- Removed the WITH_CHISEL configuration since chisels have been removed
  from upstream and it is no longer being used after the latest falco
  update.
- Updated docs as best as possible.
Molter73 added a commit to stackrox/stackrox that referenced this pull request Aug 7, 2024
After stackrox/collector#1770, collector is no longer able to download
drivers, so the /module directory is now effectively unused and can
safely be removed from the helm charts.
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.

2 participants