Add support for deploy service agent auto updates by bernardjkim · Pull Request #31982 · gravitational/teleport

bernardjkim · 2023-09-15T19:56:42Z

Contributes to #28780, https://github.com/gravitational/cloud/issues/4880

This PR adds support for auto updates of Deploy Service Agents. More info in RFD.
Some changes from the RFD:

The RFD described the upgrade process by first calling ecs:CreateService and then ecs:DeleteService, to update a service, but the implementation just uses ecs:UpdateService to update the service.

r0mant

First quick pass, I'll do another more in-depth one later.

marcoandredinis · 2023-09-25T11:17:23Z

 			"ecs:DescribeClusters", "ecs:CreateCluster", "ecs:PutClusterCapacityProviders",
 			"ecs:DescribeServices", "ecs:CreateService", "ecs:UpdateService",
-			"ecs:RegisterTaskDefinition",
+			"ecs:RegisterTaskDefinition", "ecs:DescribeTaskDefinition", "ecs:DeregisterTaskDefinition",


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?
Cluster Alert might also work

Are we doing anything here?

For new deployments, it should be good (we add all the permissions).
For current deployments, it will fail.
Do we have any kind of alerting?

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?

The log messages are stored internally (as in Teleport Cloud infra).
They will not be visible to the customer.

Maybe we can get away with looking for those logs ourselves and letting the affected customers know.
But honestly, that feels hacky, error-prone and not scalable.

With a ClusterAlert the customer would know.
I'm not very familiar with those as well, but I believe it would work better (even if we do this, let's keep the warning as well).
@r0mant What do you think?

As for method to fix:

Re-run deploy service configuration script to update permissions.

We don't have an easy way for the user to re-run the script.
It is only generated at that step when enrolling an RDS database.

Can we generate the script URL?
It should look something like this:

curl 'https://<tenant>.teleport.sh/webapi/scripts/integrations/configure/deployservice-iam.sh?integrationName=<integration-name>&awsRegion=<aws-region>&taskRole=<taskRole>&role=<integrationRole>'

I think all the variables are present in the current context.

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.

Yeah, I don't think we can.
I thought it was under Service, but it is actually under Task Definition 😭

In that case, we can only ask them to fill it with the same role that was used previously.

Okay the alert might look something like the following:

Open Amazon CloudShell and copy/paste the following command to reconfigure integration. Replace TASK_ROLE with deploy service task role name. bash -c "$(curl 'https://bernard-dev.cloud.gravitational.io/webapi/scripts/integrations/configure/deployservice-iam.sh?awsRegion=us-west-2&integrationName=database-access&role=bernard-dev-database-access&taskRole=TASK_ROLE')"

It is pretty verbose tho. We might want to consider adding a docs page later and linking that instead.

Let's leave cluster alerts out of scope for now and revisit at a later time.

r0mant

I think the overall approach makes sense but I left a few suggestions about code structure/organization.

marcoandredinis

Just tested this (very basic test) and it worked as expected 👍

Left some comments

- Perform updates in parallel - Add additional logging - Add additional documentation

public-teleport-github-review-bot · 2023-10-10T22:19:55Z

@bernardjkim See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed

* Add support for ecs agent auto updates * fix unit test * Remove unused var * Addres feedback * Use list of available AWS database regions * Run update task on proxy instances * Revert GenerateAWSOIDCToken * Move const to start of file * Address feedback * Create separate DeployServiceUpdater struct * Address feedback - Perform updates in parallel - Add additional logging - Add additional documentation * debug * Address feedback - Check OwnershipTags - Use semaphore pkg - Release semaphore lease on success * Make OwnershipTags explicitly required * Add cluster alert * Fix typo and update message * Revert cluster alert * Update err messages * Check minimum compatible server version * Update log msg

) * Add support for deploy service agent auto updates (#31982) * Add support for ecs agent auto updates * fix unit test * Remove unused var * Addres feedback * Use list of available AWS database regions * Run update task on proxy instances * Revert GenerateAWSOIDCToken * Move const to start of file * Address feedback * Create separate DeployServiceUpdater struct * Address feedback - Perform updates in parallel - Add additional logging - Add additional documentation * debug * Address feedback - Check OwnershipTags - Use semaphore pkg - Release semaphore lease on success * Make OwnershipTags explicitly required * Add cluster alert * Fix typo and update message * Revert cluster alert * Update err messages * Check minimum compatible server version * Update log msg * Check for nil auth client

Add support for ecs agent auto updates

ef88209

github-actions Bot requested review from camscale and smallinsky September 15, 2023 19:57

github-actions Bot added the size/lg label Sep 15, 2023

fix unit test

d263297

bernardjkim requested review from marcoandredinis and r0mant September 15, 2023 20:06

bernardjkim commented Sep 15, 2023

View reviewed changes

Comment thread lib/integrations/awsoidc/deployservice_update.go Outdated

Remove unused var

241e070

marcoandredinis reviewed Sep 19, 2023

View reviewed changes

Comment thread lib/automaticupgrades/config.go

Comment thread lib/integrations/awsoidc/deployservice_update.go Outdated

Comment thread lib/integrations/awsoidc/deployservice_update.go Outdated

Comment thread lib/automaticupgrades/version.go Outdated

Comment thread lib/service/awsoidc.go Outdated

r0mant reviewed Sep 19, 2023

View reviewed changes

Comment thread lib/automaticupgrades/version.go Outdated

Comment thread lib/integrations/awsoidc/deployservice_update.go Outdated

Comment thread lib/service/awsoidc.go Outdated

smallinsky reviewed Sep 20, 2023

View reviewed changes

Comment thread lib/auth/integration/integrationv1/awsoidc.go Outdated

Comment thread lib/automaticupgrades/version.go Outdated

Comment thread lib/automaticupgrades/version.go Outdated

Comment thread lib/service/awsoidc.go Outdated

smallinsky self-requested a review September 20, 2023 13:19

bernardjkim added 6 commits September 20, 2023 20:52

Merge branch 'master' into bernard/ecs-auto-update

2ccebeb

Addres feedback

c8d0ddc

Use list of available AWS database regions

ece42b3

Run update task on proxy instances

b4690c2

Revert GenerateAWSOIDCToken

d5c57c9

Move const to start of file

f028a24

bernardjkim requested review from marcoandredinis and r0mant September 24, 2023 01:40

marcoandredinis reviewed Sep 25, 2023

View reviewed changes

marcoandredinis self-requested a review September 25, 2023 11:26

r0mant reviewed Sep 26, 2023

View reviewed changes

bernardjkim added 2 commits September 26, 2023 16:08

Address feedback

d801171

Create separate DeployServiceUpdater struct

a31d614

marcoandredinis reviewed Sep 28, 2023

View reviewed changes

Comment thread lib/integrations/awsoidc/deployservice_update.go Outdated

Comment thread lib/integrations/awsoidc/deployservice_update.go Outdated

Comment thread lib/service/awsoidc.go Outdated

Comment thread lib/service/awsoidc.go Outdated

Comment thread lib/service/awsoidc.go Outdated

Address feedback

aeee42d

- Perform updates in parallel - Add additional logging - Add additional documentation

bernardjkim requested review from marcoandredinis and r0mant September 29, 2023 05:07

marcoandredinis reviewed Oct 4, 2023

View reviewed changes

Comment thread lib/service/awsoidc.go Outdated

Fix typo and update message

3978c82

r0mant reviewed Oct 4, 2023

View reviewed changes

bernardjkim added 2 commits October 4, 2023 16:47

Revert cluster alert

cf4c36e

Update err messages

ffde7f2

bernardjkim requested a review from r0mant October 5, 2023 00:18

r0mant approved these changes Oct 6, 2023

View reviewed changes

marcoandredinis approved these changes Oct 6, 2023

View reviewed changes

bernardjkim added 7 commits October 9, 2023 12:19

Merge branch 'master' into bernard/ecs-auto-update

33e8a5e

Merge branch 'master' into bernard/ecs-auto-update

23a020a

Merge branch 'master' into bernard/ecs-auto-update

a5a07c5

Merge branch 'master' into bernard/ecs-auto-update

7ede4f8

Merge branch 'master' into bernard/ecs-auto-update

5444138

Check minimum compatible server version

e381d26

Update log msg

d27caf2

bernardjkim enabled auto-merge October 10, 2023 21:55

bernardjkim added backport/branch/v13 labels Oct 10, 2023

bernardjkim added this pull request to the merge queue Oct 10, 2023

Merged via the queue into master with commit 27fae1a Oct 10, 2023

bernardjkim deleted the bernard/ecs-auto-update branch October 10, 2023 22:18

bernardjkim mentioned this pull request Oct 11, 2023

[v13] Add support for deploy service agent auto updates (#31982) #33311

Merged

bernardjkim mentioned this pull request Oct 11, 2023

[v14] Add support for deploy service agent auto updates (#31982) #33313

Merged

camscale mentioned this pull request Oct 16, 2023

Release 14.1.0 #33507

Merged

Conversation

bernardjkim commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

r0mant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

r0mant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcoandredinis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bernardjkim commented Sep 15, 2023 •

edited

Loading