Skip to content

AWS OIDC Integration: Deploy DB Service in a single click#27035

Merged
marcoandredinis merged 11 commits intomasterfrom
marco/agent1clickinstall
Jun 20, 2023
Merged

AWS OIDC Integration: Deploy DB Service in a single click#27035
marcoandredinis merged 11 commits intomasterfrom
marco/agent1clickinstall

Conversation

@marcoandredinis
Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis commented May 26, 2023

This PR adds a new AWS OIDC Integration action: deploy DB and Discovery service in a single click.

This uses Amazon ECS to deploy a Database and a Discovery Service in a single click.
Please read lib/integrations/awsoidc.DeployDBService for more information.

Demo:

OneAgentDeploy.mp4

image

To use this new endpoint, the client (webapp) should send the following:
POST .../integrations/aws-oidc/<integrationname>/deployservice

{
    "deploymentMode": "database-service",
    "region": "<db region>",
    "subnetIds": ["<list>", "<of>", "<subnets>"],
    "taskRoleARN": "<aws role arn created in the flow>",
    "databaseAgentMatcherLabels": [{"name":"*", "value":"*"}]
}
  • region and subnetIds comes from the response of listing databases
    subnetIds is from database[].aws.rds.subnets
  • taskRoleArn should have a default value of TeleportDatabaseAccess (we should probably let the user change this, but not sure the design team is accounting for that
    This is the Role that they will be asked to create when executing the aws commands in aws cli or Amazon CloudShell
  • databaseAgentMatcherLabels is the only required input from the user and can be defaulted to '*':'*' like we do for manual installation.

Fixes #25521

@marcoandredinis marcoandredinis marked this pull request as ready for review May 26, 2023 18:05
@public-teleport-github-review-bot
Copy link
Copy Markdown

@marcoandredinis - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

@github-actions github-actions Bot requested review from fspmarshall and lxea May 26, 2023 18:05
@marcoandredinis marcoandredinis requested a review from r0mant May 26, 2023 18:07
Comment thread lib/integrations/awsoidc/deploydbservice_vcr_test.go Outdated
@marcoandredinis marcoandredinis force-pushed the marco/agent1clickinstall branch from 52d39db to ffb6351 Compare May 29, 2023 11:17
@marcoandredinis marcoandredinis marked this pull request as draft May 30, 2023 17:30
@marcoandredinis marcoandredinis force-pushed the marco/agent1clickinstall branch 4 times, most recently from 1f07fd6 to 363d223 Compare May 31, 2023 10:58
@marcoandredinis marcoandredinis marked this pull request as ready for review May 31, 2023 11:20
@marcoandredinis marcoandredinis requested a review from kimlisa May 31, 2023 11:21
@github-actions github-actions Bot requested review from avatus and probakowski May 31, 2023 11:22
Copy link
Copy Markdown
Contributor

@fspmarshall fspmarshall left a comment

Choose a reason for hiding this comment

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

General suggestion: I think it would be good to decide on a standardized tag to apply to teleport-created aws resources. Ideally one that included the cluster name. That way a teleport cluster could determine which resources originate from itself. This would be very useful if we decide to implement cleanup helpers, or have configuration parameters that should only be applied to existing resources if the resource is "owned" by teleport (similar to how we currently update the builtin editor/auditor/etc roles only if they have a label indicating that they are teleport-generated).

Comment thread lib/integrations/awsoidc/deploydbservice.go Outdated
Comment on lines 455 to 460
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're going to establish a pattern of deploying ECS clusters to inject teleport agents, I think we should have a common configuration for all such clusters if possible. I.e. if I deploy 3 teleport agents to the same region/account I'd want them to all end up in the same cluster with one common set of configuration parameters that don't conflict with one another. That'll be easier to do if the cluster setup logic is designed to be generic from the beginning.

Would it be possible to make this its own standalone helper rather than being tightly coupled with DB service creation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's creating a fairly generic Cluster: name, tags, and FARGATE as a capacity provider
So, I don't think we are being coupled with the current creation.

I think we generalize the creation code-wise when we get a new use-case, doing it now, imo, feels premature.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's only premature if it's a lot of work. If it's relatively straightforward to break out, then it's just good planning!

Nbd either way, but if u leave as-is, maybe add a TODO suggesting reuse in the event we take up this pattern elsewhere. Don't want future folks overlooking this and leaving us with multiple parallel implementations.

Comment thread lib/integrations/awsoidc/deploydbservice.go Outdated
Comment thread lib/integrations/awsoidc/deploydbservice.go Outdated
@marcoandredinis
Copy link
Copy Markdown
Contributor Author

General suggestion: I think it would be good to decide on a standardized tag to apply to teleport-created aws resources. Ideally one that included the cluster name. That way a teleport cluster could determine which resources originate from itself. This would be very useful if we decide to implement cleanup helpers, or have configuration parameters that should only be applied to existing resources if the resource is "owned" by teleport (similar to how we currently update the builtin editor/auditor/etc roles only if they have a label indicating that they are teleport-generated).

Yes, I was thinking about having deterministic names based on the cluster name.
But having a proper tag sounds better

What do you think about:

teleport-cluster: <cluster-name>

@fspmarshall
Copy link
Copy Markdown
Contributor

What do you think about:

teleport-cluster: <cluster-name>

@marcoandredinis I don't know much about aws resource tagging best-practices, would it be appropriate to use a host-like namespace the way we do with kubernetes and teleport-internal labels? Ex:

teleport.dev/cluster: <cluster-name>

@marcoandredinis marcoandredinis force-pushed the marco/agent1clickinstall branch 2 times, most recently from 29f2f69 to 2aa412c Compare June 2, 2023 16:17
@marcoandredinis
Copy link
Copy Markdown
Contributor Author

@fspmarshall Thank you for your review.

If you want to test this feature yourself, I can help you out 👍

@marcoandredinis
Copy link
Copy Markdown
Contributor Author

@r0mant We talked about changing the ephemeral storage to use
However, it might get pricey to use the max available.
The current configuration is:
0.25 vCPU, 512MB of RAM and 20GB ("free" storage) disk = ~9$/month

Setting the disk space to 200GB, it will increase the total to ~23$/month
Technically, the change is simple:

	taskDefOut, err := clt.RegisterTaskDefinition(ctx, &ecs.RegisterTaskDefinitionInput{
		Cpu:    &taskCPU,
		Memory: &taskMem,
		EphemeralStorage: &ecsTypes.EphemeralStorage{
			// AWS charges ~ $0.000111 per GB per hour
			// https://aws.amazon.com/fargate/pricing/
			SizeInGiB: 200,
		},
       		// ...

Let me know if you still think we should increase.

@marcoandredinis marcoandredinis force-pushed the marco/agent1clickinstall branch from 2aa412c to 8071fa7 Compare June 6, 2023 08:17
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

My main comment so far is I would like to design the "deploy service" API to be more generic where it just accepts the agent config it needs to run instead of being database agent specific.

Comment thread lib/web/apiserver.go Outdated
Comment thread lib/integrations/awsoidc/deploydbservice.go Outdated
Comment thread lib/integrations/awsoidc/deploydbservice.go Outdated
@wadells
Copy link
Copy Markdown
Contributor

wadells commented Jun 9, 2023

What do you think about:

teleport-cluster: <cluster-name>

@marcoandredinis I don't know much about aws resource tagging best-practices, would it be appropriate to use a host-like namespace the way we do with kubernetes and teleport-internal labels? Ex:

teleport.dev/cluster: <cluster-name>

Chiming in from afar, we just had an RFD to standardize on some tags for internal use:

https://github.com/gravitational/cloud/blob/af5e8a829814f304df8705ecf4608c01dfe12e68/rfd/0071-Cloud-Resource-Tagging.md

Taking those ideas, they'd suggest:

teleport.dev/creator_type: teleport
teleport.dev/creator: <cluster-name>

cc @jof

@marcoandredinis marcoandredinis force-pushed the marco/agent1clickinstall branch from 71b966a to a19f8a8 Compare June 19, 2023 22:47
@marcoandredinis marcoandredinis disabled auto-merge June 20, 2023 06:36
@marcoandredinis marcoandredinis added this pull request to the merge queue Jun 20, 2023
Merged via the queue into master with commit 782c385 Jun 20, 2023
@marcoandredinis marcoandredinis deleted the marco/agent1clickinstall branch June 20, 2023 11:25
@public-teleport-github-review-bot
Copy link
Copy Markdown

@marcoandredinis See the table below for backport results.

Branch Result
branch/v13 Failed

marcoandredinis added a commit that referenced this pull request Jun 20, 2023
* AWS OIDC Integration: Deploy DB Service in a single click

This PR adds a new AWS OIDC Integration action: deploy database service

This uses Amazon ECS to deploy a Database and a Discovery Service in a
single click.
Please read `lib/integrations/awsoidc.DeployDBService` for more
information.

* set discovery group to uuid

* add agent matcher labels

* add tags to indicate ownership

* create deployment mode

* allow for dot named clusters

* rename service and taskdefinition to include deployment mode

* add ECS service dashboard url to the response

* change ownership tags

* remove delete service api call

* fix json indent in comment and iam token name
github-merge-queue Bot pushed a commit that referenced this pull request Jun 21, 2023
…28051)

* AWS OIDC Integration: Deploy DB Service in a single click

This PR adds a new AWS OIDC Integration action: deploy database service

This uses Amazon ECS to deploy a Database and a Discovery Service in a
single click.
Please read `lib/integrations/awsoidc.DeployDBService` for more
information.

* set discovery group to uuid

* add agent matcher labels

* add tags to indicate ownership

* create deployment mode

* allow for dot named clusters

* rename service and taskdefinition to include deployment mode

* add ECS service dashboard url to the response

* change ownership tags

* remove delete service api call

* fix json indent in comment and iam token name
@r0mant r0mant mentioned this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatic agent installation for Teleport Discover

5 participants