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

Rework Axiom Cloudwatch Forwarder #69

Closed
wants to merge 79 commits into from

Conversation

dasfmi
Copy link
Collaborator

@dasfmi dasfmi commented Jul 16, 2024

This PR revises the naming of CloudFormation stack elements to make their purpose clearer, and allows greater flexibility in filtering log groups that are applied as subscription filters to the (newly named) Lambda Forwarder, now using a combination of names, prefix and regular expression. It also introduces versioning to the CloudFormation stacks so breaking changes do not impact users, and introduces a new Unsubscriber stack for convenience.

TODO:

  • Renames the Ingester to Forwarder, Backfiller to Subscriber and the LogsSubscriber to LogGroupsListener.
  • Moves the stack templates to their own folder to improve readability with shorter file names
  • Moves out the responsibility of creating log group subscription filters and gives it to the Subscriber component
  • Subscriber component to handle different inputs for log groups selection (names list, prefix or regex pattern)
  • Create Unsubscriber component
  • Versioning the released stack templates
  • Removes the ambiguous * from permissions and creates a statement per log group
  • Provides a terraform example
  • Prefix the created log groups names with /aws/axiom to easily discard them from being forwarded

@dasfmi dasfmi requested a review from a-khaledf July 16, 2024 15:59
@dasfmi dasfmi force-pushed the islam/axm-4385-update-axiom-cloudwatch-forwarder branch from 568e66f to 8592ebc Compare July 17, 2024 11:44
@dasfmi dasfmi force-pushed the islam/axm-4385-update-axiom-cloudwatch-forwarder branch from 9fce1e8 to 96f358a Compare July 17, 2024 15:19
@dasfmi dasfmi marked this pull request as ready for review July 18, 2024 08:13
Copy link
Member

@dominicchapman dominicchapman left a comment

Choose a reason for hiding this comment

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

Some copy nits. Hopefully easy to merge suggestions ⚡

Copy link
Member

@dominicchapman dominicchapman left a comment

Choose a reason for hiding this comment

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

Refinements to README

dasfmi and others added 2 commits July 18, 2024 13:44
Co-authored-by: Dominic Chapman <[email protected]>
Co-authored-by: Dominic Chapman <[email protected]>

capabilities = ["CAPABILITY_IAM", "CAPABILITY_NAMED_IAM", "CAPABILITY_AUTO_EXPAND"]

template_body = file("forwarder_stack_url")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
template_body = file("forwarder_stack_url")
template_url = file("forwarder_stack_url")


capabilities = ["CAPABILITY_IAM", "CAPABILITY_NAMED_IAM", "CAPABILITY_AUTO_EXPAND"]

template_body = file("subscriber-stack-url")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
template_body = file("subscriber-stack-url")
template_url = file("subscriber-stack-url")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are you sure? this is how we used it in our testing.

prefix: Optional[str] = None,
):
# ensure filter params have correct values
if not names:
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think you need these if statements, the default args should work fine, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it didn't because the environment variables are empty string not an empty list

logGroupName=group["name"]
)
# TODO: improve the Subscription filters check
if len(response["subscriptionFilters"]) > 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a better check would be the destinationArn == functionArn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would check if the log group is subscribed to the same Forwarder, which is not the case. Here, we need to check if the log group has subscription for another/previous Forwarder and avoid overriding it. Not sure what is a good solution here.

@dominicchapman This is part of the mechanism of the Subscriber that tries to help the users overcome the limit size of permissions. Currently, when a Subscriber is checking which groups to add subscription on, it checks if there is any subscription filters on it, if there is already, it will skip it. This might not be what the users want since they might have subscriptions that send to other places, or maybe be they intend to have that log group forwarded to 2 different datasets, which they won't be able to do here. What's your thoughts?

Type: AWS::CloudTrail::Trail
Condition: ShouldEnableCloudTrail
DependsOn: AxiomCloudWatchLogsSubscriberS3BucketPolicy
DependsOn: AxiomCloudWatchLogGroupsListenerS3BucketPolicy
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should do something like

      AdvancedEventSelectors:
        - Name: "LogGroupCreationEvents"
          FieldSelectors:
            - Field: "eventSource"
              Equals:
                - "logs.amazonaws.com"
            - Field: "eventName"
              Equals:
                - "CreateLogGroup"

to decrease the risk of $$$

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added it, but still needs testing.

@dasfmi
Copy link
Collaborator Author

dasfmi commented Jul 26, 2024

archiving this to keep it as a reference, moved work to #74

@dasfmi dasfmi closed this Jul 26, 2024
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