athena audit logs - publisher#23987
Conversation
| retry, err := retryutils.NewLinear(retryutils.LinearConfig{ | ||
| Step: 100 * time.Millisecond, | ||
| Max: 1 * time.Minute, | ||
| }) |
There was a problem hiding this comment.
@rosstimothy what do you think about those values? i think we can accept waiting for 1minute, but maybe I am wrong.
There was a problem hiding this comment.
Doesn't the aws sdk have builtin retry and backoff?
There was a problem hiding this comment.
good idea, I can use the one from AWS, what do you think about values there?
DefaultMaxAttempts int = 3
DefaultMaxBackoff time.Duration = 20 * time.Second
Is 1 minute good idea? default for aws is 20s.
There was a problem hiding this comment.
I don't think that any of our other AWS clients modify the default retry behavior at all, including the client used by the current dynamo events backend.
There was a problem hiding this comment.
I don't think it will hurt us to extend those values a little bit. I think that dynamo offers SLA 99.99% while SNS 99.9% so it make sense to retry a little bit more.
| retry, err := retryutils.NewLinear(retryutils.LinearConfig{ | ||
| Step: 100 * time.Millisecond, | ||
| Max: 1 * time.Minute, | ||
| }) |
There was a problem hiding this comment.
Doesn't the aws sdk have builtin retry and backoff?
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
| } | ||
| } | ||
|
|
||
| // EmitAuditEvent emits audit event. |
There was a problem hiding this comment.
Suggestion add some more context to how this works for this particular implementation
| retry, err := retryutils.NewLinear(retryutils.LinearConfig{ | ||
| Step: 100 * time.Millisecond, | ||
| Max: 1 * time.Minute, | ||
| }) |
There was a problem hiding this comment.
I don't think that any of our other AWS clients modify the default retry behavior at all, including the client used by the current dynamo events backend.
|
@rosstimothy @zmb3 @atburke PTAL |
| } | ||
|
|
||
| // newPublisher returns new instance of publisher. | ||
| func newPublisher(cfg Config, awsCfg aws.Config, log *log.Entry) *publisher { |
There was a problem hiding this comment.
Why not include awsCfg aws.Config, log *log.Entry in the Config with the rest of the dependencies?
There was a problem hiding this comment.
I have reworked it in 47b7912, let me know if it's better now
| _, err := p.snsPublisher.Publish(ctx, &sns.PublishInput{ | ||
| TopicArn: aws.String(p.topicARN), | ||
| Message: aws.String(b64Encoded), | ||
| MessageAttributes: map[string]snsTypes.MessageAttributeValue{ |
There was a problem hiding this comment.
Don't these MessageAttributes get discarded when using RawMessageDelivery = true? I haven't tested this but am not sure: https://docs.aws.amazon.com/sns/latest/dg/sns-large-payload-raw-message-delivery.html.
Only noticed since your example integration tests show it enabled.
There was a problem hiding this comment.
When you enable raw message delivery for Amazon Kinesis Data Firehose or Amazon SQS endpoints, any Amazon SNS metadata is stripped from the published message and the message is sent as is.
For SQS, it strips only SNS metadata, TopicArn etc. MessageAttributes are preserved. I am using all time RawMessageDelivery and message attributes are present.
|
@tobiaszheller - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
tobiaszheller
force-pushed
the
tobiaszheller/auditevents-athena-sns
branch
from
d8ac8b9 to
7242c70
Compare
4 participants
Part of https://github.com/gravitational/teleport.e/issues/894
RFD: #23700
This PR adds possibility to publish events to SNS for athena audit log.