-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
New options to configure roles and VPC #11779
New options to configure roles and VPC #11779
Conversation
Concurrency: 5, | ||
} | ||
|
||
arnRolePattern = "arn:(aws[a-zA-Z-]*)?:iam::\\d{12}:role/?[a-zA-Z_0-9+=,.@\\-_/]+" |
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.
We are using the arn reference in a few places in the code, we might want to create a custom type for it and have a single place to validate it.
As a note, I've decided not to add a validation for arn, since I was a bit worried that we would get it wrong. Can you add a link concerning the rules for an ARN.
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.
Note: I don't like the name virtual_private_cloud as it's too long. But naming the option vpc seems wrong. Do you have any other suggestions?
Well, I don't like long names, but in this case, we use it for configuration and it clearly describes the intent.
|
||
role := lambdaConfig.Role | ||
dependsOn := make([]string, 0) | ||
if lambdaConfig.Role == "" { |
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 think we should log that we are using a custom role, I presume that we will get a few questions concerning the policies required if they use their role. So I think we will need to create a followup issue doc issue to describe them. Can you coordinate with @dedemorton for that?
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.
Sure.
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.
Created issue with the required policies: #11787
@kvch we will also need a changelog entry. |
Change LGTM, I will do a quick human check asap. |
From now on it is possible to configure permissions in
functionbeat.yml
for the deployed lambda function. Two new options are added:role
andvirtual_private_cloud
.Note: I don't really like the name
virtual_private_cloud
as it's too long. But naming the optionvpc
seems wrong. Do you have any other suggestions?Closes #9425