Skip to content
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

Handle disconnect route of Websocket #548

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Conversation

kdnakt
Copy link
Contributor

@kdnakt kdnakt commented Jan 20, 2024

Issue #, if available:

#547

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.30%. Comparing base (de51f68) to head (a8fbffb).

❗ Current head a8fbffb differs from pull request most recent head 9acd9d1. Consider uploading reports for the commit 9acd9d1 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #548   +/-   ##
=======================================
  Coverage   73.30%   73.30%           
=======================================
  Files          26       26           
  Lines        1487     1487           
=======================================
  Hits         1090     1090           
  Misses        324      324           
  Partials       73       73           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@tinhda tinhda left a comment

Choose a reason for hiding this comment

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

thank you, just what I need!

events/apigw.go Outdated
Comment on lines 136 to 186
CognitoIdentityPoolID string `json:"cognitoIdentityPoolId,omitempty"`
AccountID string `json:"accountId,omitempty"`
CognitoIdentityID string `json:"cognitoIdentityId,omitempty"`
Caller string `json:"caller,omitempty"`
APIKey string `json:"apiKey,omitempty"`
APIKeyID string `json:"apiKeyId,omitempty"`
AccessKey string `json:"accessKey,omitempty"`
SourceIP string `json:"sourceIp"`
CognitoAuthenticationType string `json:"cognitoAuthenticationType"`
CognitoAuthenticationProvider string `json:"cognitoAuthenticationProvider"`
UserArn string `json:"userArn"` //nolint: stylecheck
CognitoAuthenticationType string `json:"cognitoAuthenticationType,omitempty"`
CognitoAuthenticationProvider string `json:"cognitoAuthenticationProvider,omitempty"`
UserArn string `json:"userArn,omitempty"` //nolint: stylecheck
UserAgent string `json:"userAgent"`
User string `json:"user"`
User string `json:"user,omitempty"`
}

// APIGatewayWebsocketProxyRequest contains data coming from the API Gateway proxy
type APIGatewayWebsocketProxyRequest struct {
Resource string `json:"resource"` // The resource path defined in API Gateway
Path string `json:"path"` // The url path for the caller
Resource string `json:"resource,omitempty"` // The resource path defined in API Gateway
Path string `json:"path,omitempty"` // The url path for the caller
HTTPMethod string `json:"httpMethod,omitempty"`
Headers map[string]string `json:"headers"`
MultiValueHeaders map[string][]string `json:"multiValueHeaders"`
QueryStringParameters map[string]string `json:"queryStringParameters"`
MultiValueQueryStringParameters map[string][]string `json:"multiValueQueryStringParameters"`
PathParameters map[string]string `json:"pathParameters"`
StageVariables map[string]string `json:"stageVariables"`
Headers map[string]string `json:"headers,omitempty"`
MultiValueHeaders map[string][]string `json:"multiValueHeaders,omitempty"`
QueryStringParameters map[string]string `json:"queryStringParameters,omitempty"`
MultiValueQueryStringParameters map[string][]string `json:"multiValueQueryStringParameters,omitempty"`
PathParameters map[string]string `json:"pathParameters,omitempty"`
StageVariables map[string]string `json:"stageVariables,omitempty"`
RequestContext APIGatewayWebsocketProxyRequestContext `json:"requestContext"`
Body string `json:"body"`
IsBase64Encoded bool `json:"isBase64Encoded,omitempty"`
Body string `json:"body,omitempty"`
IsBase64Encoded *bool `json:"isBase64Encoded,omitempty"`
}

// APIGatewayWebsocketProxyRequestContext contains the information to identify
// the AWS account and resources invoking the Lambda function. It also includes
// Cognito identity information for the caller.
type APIGatewayWebsocketProxyRequestContext struct {
AccountID string `json:"accountId"`
ResourceID string `json:"resourceId"`
Stage string `json:"stage"`
RequestID string `json:"requestId"`
Identity APIGatewayRequestIdentity `json:"identity"`
ResourcePath string `json:"resourcePath"`
Authorizer interface{} `json:"authorizer"`
HTTPMethod string `json:"httpMethod"`
APIID string `json:"apiId"` // The API Gateway rest API Id
ConnectedAt int64 `json:"connectedAt"`
ConnectionID string `json:"connectionId"`
DomainName string `json:"domainName"`
Error string `json:"error"`
EventType string `json:"eventType"`
ExtendedRequestID string `json:"extendedRequestId"`
IntegrationLatency string `json:"integrationLatency"`
MessageDirection string `json:"messageDirection"`
MessageID interface{} `json:"messageId"`
RequestTime string `json:"requestTime"`
RequestTimeEpoch int64 `json:"requestTimeEpoch"`
RouteKey string `json:"routeKey"`
Status string `json:"status"`
AccountID string `json:"accountId,omitempty"`
ResourceID string `json:"resourceId,omitempty"`
Stage string `json:"stage"`
RequestID string `json:"requestId"`
Identity APIGatewayRequestIdentity `json:"identity"`
ResourcePath string `json:"resourcePath,omitempty"`
Authorizer interface{} `json:"authorizer,omitempty"`
HTTPMethod string `json:"httpMethod,omitempty"`
APIID string `json:"apiId"` // The API Gateway rest API Id
ConnectedAt int64 `json:"connectedAt"`
ConnectionID string `json:"connectionId"`
DomainName string `json:"domainName"`
Error string `json:"error,omitempty"`
EventType string `json:"eventType"`
ExtendedRequestID string `json:"extendedRequestId"`
IntegrationLatency string `json:"integrationLatency,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

undo the json tag changes, they're not related the the feature you're trying to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment, but it's related to the feature because integrationLatency field is not in the WebSocket message sent on the $disconnect route as @tinhda wrote .
If I remove .omitempty json tag, TestApiGatewayWebsocketRequestDisconnectMarshaling would fail.

events/apigw.go Outdated
ExtendedRequestID string `json:"extendedRequestId"`
IntegrationLatency string `json:"integrationLatency,omitempty"`
MessageDirection string `json:"messageDirection"`
MessageID *string `json:"messageId,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the type of a struct field is a breaking change, revert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it in 5cdae93.

Comment on lines +193 to +194
DisconnectStatusCode int64 `json:"disconnectStatusCode,omitempty"`
DisconnectReason *string `json:"disconnectReason,omitempty"`
Copy link
Collaborator

@bmoffatt bmoffatt Jan 24, 2024

Choose a reason for hiding this comment

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

I couldn't find a reference to these fields in the AWS Docs, nor in the event libraries for .NET or Java.

Do you happen to have a reference handy? My search skills are failing me right now.

Copy link

@tinhda tinhda Jan 24, 2024

Choose a reason for hiding this comment

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

Hi, I found this sample online, you can also recreate this by looking into the websocket api gateway log in cloudwatch. I couldn't find anything in the aws documents neither.

    Host: '3xcls223mg.execute-api.eu-west-1.amazonaws.com',
    'x-api-key': '',
    'X-Forwarded-For': '',
    'x-restapi': ''
  },
  multiValueHeaders: {
    Host: [ '3xcls223mg.execute-api.eu-west-1.amazonaws.com' ],
    'x-api-key': [ '' ],
    'X-Forwarded-For': [ '' ],
    'x-restapi': [ '' ]
  },
  requestContext: {
    routeKey: '$disconnect',
    disconnectStatusCode: 1006,
    eventType: 'DISCONNECT',
    extendedRequestId: 'fvLJ9EMQDoEFXSw=',
    requestTime: '22/May/2021:15:38:26 +0000',
    messageDirection: 'IN',
    disconnectReason: 'Connection Closed Abnormally',
    stage: 'updatedevice',
    connectedAt: 1621697605538,
    requestTimeEpoch: 1621697906882,
    identity: {
      userAgent: 'arduino-WebSocket-Client',
      sourceIp: '137.97.106.23'
    },
    requestId: 'fvLJ9EMQDoEFXSw=',
    domainName: '3xcls223mg.execute-api.eu-west-1.amazonaws.com',
    connectionId: 'fvKa4d88joECFDw=',
    apiId: '3xcls223mg'
  },
  isBase64Encoded: false
}

Link to the sample: Links2004/arduinoWebSockets#667

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also found here: boto/boto3#3789

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I'll need a reference that comes from docs.aws.amazon.com

I'll otherwise make time to walk myself through the developer guide so that I can reproduce the test payloads in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything else I can do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's perfect! Thanks for following up!

@bmoffatt bmoffatt added enhancement type/events issue or feature request related to the events package labels Jan 24, 2024
events/apigw.go Outdated
Body string `json:"body"`
IsBase64Encoded bool `json:"isBase64Encoded,omitempty"`
Body string `json:"body,omitempty"`
IsBase64Encoded *bool `json:"isBase64Encoded,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

breaks back-compat, revert back to bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the change. 🙇

Copy link
Collaborator

@bmoffatt bmoffatt left a comment

Choose a reason for hiding this comment

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

Another field type back-compat issue snuck in - revert it and the accomanying test data json, and this PR should be good to go

events/apigw.go Outdated
ExtendedRequestID string `json:"extendedRequestId"`
IntegrationLatency string `json:"integrationLatency,omitempty"`
MessageDirection string `json:"messageDirection"`
MessageID string `json:"messageId,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

MessageID needs to stay as interface{} for back-compat

@@ -97,11 +97,11 @@
"extendedRequestId": "TWegAcC4EowCHnA=",
"integrationLatency": "123",
"messageDirection": "IN",
"messageId": null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"messageId": null should remain in the testdata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"messageId" field is not documented for CONNECT eventType.

https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-integration-requests.html#api-gateway-simple-proxy-for-lambda-input-format-websocket

And I couldn't find out a way to keep this field here in the testdata and also to keep the MessageID field of the APIGatewayWebsocketProxyRequestContext struct as interface{}.
If I can remove this field from the testdata, the MessageID field can stay as interface{}.

What should I do?

@kdnakt
Copy link
Contributor Author

kdnakt commented Mar 29, 2024

Thanks for your feedback, I'll fix the field type back-compat issue 🙇

@bmoffatt bmoffatt merged commit 90a3af7 into aws:main Apr 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement type/events issue or feature request related to the events package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants