-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add support for deploy service agent auto updates #31982
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
Changes from all commits
ef88209
d263297
241e070
2ccebeb
c8d0ddc
ece42b3
b4690c2
d5c57c9
f028a24
d801171
a31d614
aeee42d
406b0e1
0f8dc75
2cc89a3
fec081d
3978c82
cf4c36e
ffde7f2
33e8a5e
23a020a
a5a07c5
7ede4f8
5444138
e381d26
d27caf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,7 @@ func StatementForECSManageService() *Statement { | |
| Actions: []string{ | ||
| "ecs:DescribeClusters", "ecs:CreateCluster", "ecs:PutClusterCapacityProviders", | ||
| "ecs:DescribeServices", "ecs:CreateService", "ecs:UpdateService", | ||
| "ecs:RegisterTaskDefinition", | ||
| "ecs:RegisterTaskDefinition", "ecs:DescribeTaskDefinition", "ecs:DeregisterTaskDefinition", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have to think of some way to let the users know that their current set of permissions might not be enough, and that they must re-run the script. Maybe a warning in the logs?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we doing anything here? For new deployments, it should be good (we add all the permissions).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will log the following message if permissions are lacking, but I don't know how effective that will be. https://github.com/gravitational/teleport/pull/31982/files#diff-b2b691cbb3f300d6396f5b1ff26ac2569a944859ac8041067d8c97b23037f9c3R321-R325 I'm not familiar with Cluster Alerts, do you think this would be a better approach?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The log messages are stored internally (as in Teleport Cloud infra). Maybe we can get away with looking for those logs ourselves and letting the affected customers know. With a ClusterAlert the customer would know. As for method to fix:
We don't have an easy way for the user to re-run the script. Can we generate the script URL? I think all the variables are present in the current context.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a cluster alert here fec081d Do you know of a way to get the task role in the current context? The only way I'm aware of is extracting the task role from the task definition. But that would require that the instance already has permissions to describe the task definition.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't think we can. In that case, we can only ask them to fill it with the same role that was used previously.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay the alert might look something like the following: It is pretty verbose tho. We might want to consider adding a docs page later and linking that instead.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's leave cluster alerts out of scope for now and revisit at a later time. |
||
|
|
||
| // EC2 DescribeSecurityGroups is required so that the user can list the SG and then pick which ones they want to apply to the ECS Service. | ||
| "ec2:DescribeSecurityGroups", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.