Support for updating instances #27

nvnyale · 2022-05-09T15:02:45Z

No description provided.

PUT "/{account}/instances/{id}" PUT "/{account}/instances/{id}/power" PUT "/{account}/instances/{id}/ssm/association" PUT "/{account}/instances/{id}/tags" PUT "/{account}/instances/{id}/attribute"

…UTSSMAssociation

tenyo

Overall looks good, I just think we should combine some of those routes to use the same InstanceUpdateHandler.

tenyo · 2022-05-10T18:36:12Z

api/routes.go

-	api.HandleFunc("/{account}/instances/{id}/ssm/association", s.ProxyRequestHandler).Methods(http.MethodPut)
-	api.HandleFunc("/{account}/instances/{id}/tags", s.ProxyRequestHandler).Methods(http.MethodPut)
-	api.HandleFunc("/{account}/instances/{id}/attribute", s.ProxyRequestHandler).Methods(http.MethodPut)
+	api.HandleFunc("/{account}/instances/{id}/ssm/association", s.CreateAssociationHandler).Methods(http.MethodPut)


InstanceSSMAssociationHandler

tenyo · 2022-05-10T18:37:39Z

api/routes.go

@@ -65,12 +65,12 @@ func (s *server) routes() {
 	api.HandleFunc("/{account}/images", s.ProxyRequestHandler).Methods(http.MethodPost)

 	api.HandleFunc("/{account}/images/{id}/tags", s.ImageUpdateHandler).Methods(http.MethodPut)
-	api.HandleFunc("/{account}/instances/{id}", s.ProxyRequestHandler).Methods(http.MethodPut)
+	api.HandleFunc("/{account}/instances/{id}", s.InstanceIDHandler).Methods(http.MethodPut)


InstanceUpdateHandler

For ID's route, handler is not implemented, so I didn't generalize this handler.

ok, then let's just call it NotImplementedHandler, so it's clearer and it can be used by other routes that are not implemented

tenyo · 2022-05-10T18:57:47Z

api/types.go

@@ -728,6 +732,10 @@ type Ec2InstanceStateChangeRequest struct {
 	State string
 }

+type SSMCreateRequest struct {


SSMAssociationRequest

tenyo · 2022-05-10T19:00:21Z

api/handlers_instances.go

+		s.session.ExternalID,
+		role,
+		"",
+		"arn:aws:iam::aws:policy/AmazonSSMReadOnlyAccess",


hmm, does this work? you're not passing any IAM policy besides the read-only access

That's a good point, I missed it. I am not familiar with the AWS policies and can you please help me to choose the correct policy.

In this case you're working with SSM (Systems Manager) so you can take a look here first: https://docs.aws.amazon.com/systems-manager/latest/userguide/security_iam_service-with-iam.html
Try to find which IAM permissions are required to do the work (in this case CreateAssociation).
For example, here you can see all the different IAM permissions: https://aws.permissions.cloud/iam/ssm
In some cases it may be a bit of trial-and-error until you get all the permissions right, but in this case should be pretty straightforward.

tenyo · 2022-05-10T19:01:23Z

api/types.go

@@ -728,6 +732,10 @@ type Ec2InstanceStateChangeRequest struct {
 	State string
 }

+type SSMCreateRequest struct {
+	Document string


I think you need `json:"document"`

tenyo · 2022-05-10T19:06:11Z

api/routes.go

-	api.HandleFunc("/{account}/instances/{id}/attribute", s.ProxyRequestHandler).Methods(http.MethodPut)
+	api.HandleFunc("/{account}/instances/{id}/ssm/association", s.CreateAssociationHandler).Methods(http.MethodPut)
+	api.HandleFunc("/{account}/instances/{id}/tags", s.InstancesTagsHandler).Methods(http.MethodPut)
+	api.HandleFunc("/{account}/instances/{id}/attribute", s.InstancesAttributeHandler).Methods(http.MethodPut)


Let's try to use the same InstanceUpdateHandler for both tags and attributes (and {id})
In that handler you can check the request and if they passed Tags - update the tags, if they passed InstanceType - update the attributes.

I have generalized the Instance Tags and Instance Attributes update handlers, with the assumption that the input payload doesn't have both (tags & attributes) sent at the same time. Let me know if there are any changes required.

tenyo · 2022-05-10T19:08:40Z

api/handlers_instances.go

+	account := s.mapAccountNumber(vars["account"])
+	instanceId := vars["id"]
+
+	req := &Ec2ImageUpdateRequest{}


When we move this to the InstanceUpdateHandler, this can become just Ec2InstanceUpdateRequest

tenyo · 2022-05-10T19:09:56Z

api/handlers_instances.go

+	account := s.mapAccountNumber(vars["account"])
+	instanceId := vars["id"]
+
+	req := &Ec2InstancesAttributeUpdateRequest{}


Ec2InstanceUpdateRequest

tenyo · 2022-05-11T18:07:50Z

api/handlers_instances.go

-	if len(req.Tags) == 0 {
-		handleError(w, apierror.New(apierror.ErrBadRequest, "missing required field: tags", nil))
+	if len(req.Tags) == 0 && len(req.InstanceType) == 0 {
+		handleError(w, apierror.New(apierror.ErrBadRequest, "missing required fields", nil))


missing required fields: tags or instance_type

tenyo · 2022-05-11T18:13:29Z

api/handlers_instances.go

-	if len(req.Tags) == 0 {
-		handleError(w, apierror.New(apierror.ErrBadRequest, "missing required field: tags", nil))
+	if len(req.Tags) == 0 && len(req.InstanceType) == 0 {
+		handleError(w, apierror.New(apierror.ErrBadRequest, "missing required fields", nil))
 		return
 	}



I think you should create and use a separate instanceUpdatePolicy here (since you need more permissions than just tags)

updated code.

tenyo · 2022-05-11T18:15:40Z

api/routes.go

@@ -65,12 +65,12 @@ func (s *server) routes() {
 	api.HandleFunc("/{account}/images", s.ProxyRequestHandler).Methods(http.MethodPost)

 	api.HandleFunc("/{account}/images/{id}/tags", s.ImageUpdateHandler).Methods(http.MethodPut)
-	api.HandleFunc("/{account}/instances/{id}", s.ProxyRequestHandler).Methods(http.MethodPut)
+	api.HandleFunc("/{account}/instances/{id}", s.InstanceIDHandler).Methods(http.MethodPut)


ok, then let's just call it NotImplementedHandler, so it's clearer and it can be used by other routes that are not implemented

tenyo · 2022-05-11T18:32:16Z

api/handlers_instances.go

 	if err := json.NewDecoder(r.Body).Decode(req); err != nil {
 		msg := fmt.Sprintf("cannot decode body into update image input: %s", err)
 		handleError(w, apierror.New(apierror.ErrBadRequest, msg, err))
 		return
 	}

-	if len(req.Tags) == 0 {
-		handleError(w, apierror.New(apierror.ErrBadRequest, "missing required field: tags", nil))
+	if len(req.Tags) == 0 && len(req.InstanceType) == 0 {


Maybe we should also check that both are not > 0 and return an error only one of these is expected: tags or instance_type

tenyo · 2022-05-18T19:23:45Z

api/types.go

@@ -693,27 +693,37 @@ type AssociationTarget struct {

 func toSSMAssociationDescription(rawDesc *ssm.DescribeAssociationOutput) *AssociationDescription {
 	const dateLayout = "2006-01-02 15:04:05 +0000"
+	formatDate := func(t *time.Time) string {


We already have timeFormat and tzTimeFormat functions defined, you should use one of those

In ruby response, for GET SSMAssociation, we are getting the time format as above ex: "2022-03-14 16:19:34 +0000". Please let me know if you want me to use timeFormat or tzTimeFormat

tzTimeFormat should be fine

tenyo · 2022-05-18T19:29:46Z

api/handlers_instances.go

@@ -498,7 +498,7 @@ func (s *server) InstanceSSMAssociationHandler(w http.ResponseWriter, r *http.Re
 		return
 	}

-	handleResponseOk(w, struct{ AssociationId string }{AssociationId: *out.AssociationDescription.AssociationId})
+	handleResponseOk(w, aws.StringValue(out.AssociationDescription.AssociationId))


I think your function should only return the AssociationId in out so we don't need to parse this in the handler.
That's what we've done for all the other functions.

nvnyale added 3 commits May 7, 2022 12:24

PUT SSM Association

f223778

SPIN-2936: Implemented below update instances endpoints

2409e78

PUT "/{account}/instances/{id}" PUT "/{account}/instances/{id}/power" PUT "/{account}/instances/{id}/ssm/association" PUT "/{account}/instances/{id}/tags" PUT "/{account}/instances/{id}/attribute"

Merge branch 'master' of https://github.com/YaleSpinup/ec2-api into P…

a17b0ef

…UTSSMAssociation

nvnyale requested a review from tenyo May 10, 2022 15:19

tenyo reviewed May 10, 2022

View reviewed changes

Fixed comments

c48bb62

tenyo reviewed May 11, 2022

View reviewed changes

nvnyale added 5 commits May 12, 2022 11:14

Added policies

11782be

Role updated

3427772

updated PUT SSM

a683732

Print stack trace when encountering panic

f8c75ee

Updated SSM Association

f67007e

tenyo reviewed May 18, 2022

View reviewed changes

nvnyale added 3 commits May 19, 2022 13:22

Fixed AssociationID output

9cfb7a9

Fixed unit testcase

e049389

Updated time format

d944c1b

tenyo approved these changes May 19, 2022

View reviewed changes

nvnyale merged commit 1a3d9d3 into master May 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for updating instances #27

Support for updating instances #27

nvnyale commented May 9, 2022

tenyo left a comment

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 11, 2022

nvnyale May 12, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 11, 2022

nvnyale May 12, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 10, 2022

nvnyale May 11, 2022

tenyo May 11, 2022

nvnyale May 12, 2022

tenyo May 11, 2022

nvnyale May 12, 2022

tenyo May 11, 2022

tenyo May 11, 2022

nvnyale May 12, 2022

tenyo May 18, 2022

nvnyale May 19, 2022

tenyo May 19, 2022

tenyo May 18, 2022

nvnyale May 19, 2022

Support for updating instances #27

Support for updating instances #27

Conversation

nvnyale commented May 9, 2022

tenyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment