-
Notifications
You must be signed in to change notification settings - Fork 334
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
initial updates to simplify lambda packaging logic #1232
Conversation
f21059f
to
c49050e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point you mention We try to do too much and it's prone to bugs.
My only concern is the lambda package will be bigger and slows down the deployment after this PR. But it may be a good tradeoff. I have few comments, nothing major.
], | ||
"third_party_libraries": [ | ||
"pathlib2==2.3.5", | ||
"policyuniverse==1.3.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about other third party libs 'cbapi', 'netaddr', 'pymsteams', 'requests'
?
Aha, I see they are defined here
streamalert/streamalert_cli/manage_lambda/package.py
Lines 44 to 55 in c49050e
# Define a package dict to support pinning versions across all subclasses | |
REQUIRED_LIBS = { | |
'backoff==1.8.1', | |
'boto3==1.10.6', | |
'cbapi==1.5.4', | |
'google-api-python-client==1.7.11', | |
'jmespath==0.9.4', | |
'jsonlines==1.2.0', | |
'netaddr==0.7.19', | |
'requests==2.22.0', | |
'pymsteams==0.1.12', | |
} |
What's the different adding third party libs in conf/global.json
and streamalert_cli/manage_lambda/package.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the third_party_libraries
is meant to allow users to inject dependencies that are NOT part of the core streamalert
package.. meaning, for all of the user-configurable things like rules
, publishers
, matchers
, schedule_queries
, etc - dependencies not included as part of the core package should be included in third_party_libraries
for installation at deploy time
does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cbapi, pymsteams, etc are all dependencies that are needed by the streamalert
package, so they are hard-coded here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not right now, but we should revisit these dependencies eventually. Like I don't think cbapi
should be deployed as part of the base package, since it's only used by the alert processor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disagree :) that is what we were doing and it's garbage. but a different discussion for another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only agree with you if we adopt the concept of "subpackages" (https://packaging.python.org/guides/packaging-namespace-packages/), which could be very likely in the next iteration of this
streamalert_cli/_infrastructure/modules/tf_scheduled_queries/variables.tf
Outdated
Show resolved
Hide resolved
streamalert_cli/_infrastructure/modules/tf_scheduled_queries/variables.tf
Outdated
Show resolved
Hide resolved
c49050e
to
137ae9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
], | ||
"third_party_libraries": [ | ||
"pathlib2==2.3.5", | ||
"policyuniverse==1.3.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not right now, but we should revisit these dependencies eventually. Like I don't think cbapi
should be deployed as part of the base package, since it's only used by the alert processor.
137ae9a
to
b92fd56
Compare
* bumping version to 3.2.0 * migrating Athena function to use tf_lambda module (#1217) * rename of athena function * updating terraform generation code to use tf_lambda module * updating tf_athena module to remove lambda code * updates for packaging, rollback, and deploy * misc updates related to config path renaming, etc * removing no-longer-used method (athena is default) * addressing PR feedback * adding more granular time prefix to athena client * fixing duplicate resource issues (#1218) * fixing duplicate resource issues * fixing some other bugs in #1217 * fixing tf targets for athena deploy (#1220) * adding "--config-dir" flag to CLI to support specifying path for config files (#1224) * adding support for supplying path to config via CLI flag * misc touchups * updating publishers to accept configurable paths (#1223) * moving matchers outside of rules directory * updating rules for new matcher path * updating unit test for consistency * making publisher locations configurable * fixing typo * updating tf_lambda module to remove extra resources (#1225) * fixing rollback for all functions, removing 'all' flag for function deploys (#1222) * updating rollback functionality to include all funcs * updating tests to check for rollback of all funcs * updating docs * fixing tf cycle and index issue (#1226) * Add missing dependency (#1228) * Implements a v2 Lambda Output with AssumeRole (#1227) * First draft of aws-lambda-v2 * Tests * Fixup * Fixup * Fioxup * Fixup * fixup * adding terraform references for some buckets (#1229) * adding athena terraform references instead of literals * fixing tests * GitHub Actions (#1231) * port to github actions * remove travis * cover the 3.2 branch for now too * initial updates to simplify lambda packaging logic (#1232) * moving some precompiled files * initial revamp to packaging to remove multiple pacakges * taking out more trash * update scheduled queries module * updating deploy logic to suck garbage slightly less * updates to unit tests * addressing pr feedback * addressing PR feedback * small update to docs (#1233) Co-authored-by: Ryxias <[email protected]> Co-authored-by: Paul Kehrer <[email protected]>
to: @airbnb/streamalert-maintainers
Background
Our packaging logic is garbage. We try to do too much and it's prone to bugs.
Changes
streamalert
code, theconf
and any user defined folders needed, specified in theconf/globals.json
file.Testing