-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
AWS OIDC: List Deployed Database Services HTTP API #49352
base: marco/awsoidc-listdatabaseservices-impl
Are you sure you want to change the base?
AWS OIDC: List Deployed Database Services HTTP API #49352
Conversation
4831b35
to
3dc5a4f
Compare
46744e1
to
f131a9f
Compare
3dc5a4f
to
f69697f
Compare
f131a9f
to
696d356
Compare
f69697f
to
cc2c59d
Compare
696d356
to
b7237e5
Compare
cc2c59d
to
d6b59a8
Compare
b7237e5
to
6cd29f8
Compare
d6b59a8
to
0846c4d
Compare
6cd29f8
to
557241b
Compare
0846c4d
to
5962451
Compare
557241b
to
aef905d
Compare
5962451
to
d2ce837
Compare
aef905d
to
c4f7b14
Compare
d2ce837
to
682d719
Compare
lib/web/apiserver.go
Outdated
@@ -985,6 +985,7 @@ func (h *Handler) bindDefaultEndpoints() { | |||
h.GET("/webapi/scripts/integrations/configure/listdatabases-iam.sh", h.WithLimiter(h.awsOIDCConfigureListDatabasesIAM)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/deployservice", h.WithClusterAuth(h.awsOIDCDeployService)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/deploydatabaseservices", h.WithClusterAuth(h.awsOIDCDeployDatabaseServices)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/listdeployeddatabaseservices", h.WithClusterAuth(h.awsOIDCListDeployedDatabaseService)) |
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.
Should this be GET
?
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.
+1
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 have been using POST
for most of the integration actions, even when listing things.
Line 984 in 04bc1c9
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/databases", h.WithClusterAuth(h.awsOIDCListDatabases)) |
Line 990 in 04bc1c9
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/ec2", h.WithClusterAuth(h.awsOIDCListEC2)) |
Line 991 in 04bc1c9
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/eksclusters", h.WithClusterAuth(h.awsOIDCListEKSClusters)) |
Lines 995 to 997 in 04bc1c9
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/securitygroups", h.WithClusterAuth(h.awsOIDCListSecurityGroups)) | |
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/databasevpcs", h.WithClusterAuth(h.awsOIDCListDatabaseVPCs)) | |
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/subnets", h.WithClusterAuth(h.awsOIDCListSubnets)) |
I see these endpoints as a way to execute some action in the Integration. So, something closer to a RPC over HTTP, in contrast with a more typical REST format.
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 have been using POST for most of the integration actions
Hmm, any background on why we decided to do this? Unless there's strong reason, sticking with established conventions is usually the preferred option.
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 it started like this because we wanted this RPC-like interface where we could register a single endpoint but then use url path variables to call the AWS OIDC gRPC methods directly.
Things evolved in a different manner, and at this point I would argue that following a more REST-y approach would be better (less surprises).
So, we either start converting them to use GETs where is makes sense or we keep it as is.
No strong opinion on which one to follow.
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.
my 2c: keep the existing endpoints as-is for backward compatibility (unless you want to migrate them), but for this PR make the new list deployed db services endpoint GET
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.
Switched to GET
cc @michellescripts
lib/web/apiserver.go
Outdated
@@ -985,6 +985,7 @@ func (h *Handler) bindDefaultEndpoints() { | |||
h.GET("/webapi/scripts/integrations/configure/listdatabases-iam.sh", h.WithLimiter(h.awsOIDCConfigureListDatabasesIAM)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/deployservice", h.WithClusterAuth(h.awsOIDCDeployService)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/deploydatabaseservices", h.WithClusterAuth(h.awsOIDCDeployDatabaseServices)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/listdeployeddatabaseservices", h.WithClusterAuth(h.awsOIDCListDeployedDatabaseService)) |
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.
+1
c4f7b14
to
6da2178
Compare
682d719
to
502e32d
Compare
6da2178
to
0bfa0b8
Compare
502e32d
to
39dfbe7
Compare
lib/web/apiserver.go
Outdated
@@ -985,6 +985,7 @@ func (h *Handler) bindDefaultEndpoints() { | |||
h.GET("/webapi/scripts/integrations/configure/listdatabases-iam.sh", h.WithLimiter(h.awsOIDCConfigureListDatabasesIAM)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/deployservice", h.WithClusterAuth(h.awsOIDCDeployService)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/deploydatabaseservices", h.WithClusterAuth(h.awsOIDCDeployDatabaseServices)) | |||
h.POST("/webapi/sites/:site/integrations/aws-oidc/:name/listdeployeddatabaseservices", h.WithClusterAuth(h.awsOIDCListDeployedDatabaseService)) |
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.
my 2c: keep the existing endpoints as-is for backward compatibility (unless you want to migrate them), but for this PR make the new list deployed db services endpoint GET
0bfa0b8
to
5634a35
Compare
This PR adds a new endpoint which returns the deployed database services. Calling the ECS APIs requires a region, so we had to iterate over the following resources to collect the relevant regions: - databases - database services - discovery configs
39dfbe7
to
be41afa
Compare
@zmb3 Can you please take another look? |
This PR adds a new endpoint which returns the deployed database services.
Calling the ECS APIs requires a region, so we had to iterate over the following resources to collect the relevant regions:
Demo:
curl --silent 'https://.../integrations/aws-oidc/teleportdev/listdeployeddatabaseservices' -XPOST ... | jq