docs: proposed set of bindings for AWS - SNS and SQS #84
Conversation
iancooper
changed the title
|
@iancooper tbh there are no comments as it is in |
feat: Introduce name property for SNS channel binding object
|
Hi, any progress done on this? Seems an interesting approach to me. |
|
@iancooper can you please remove @jorgebo10 if you know these protocols, please do review proposed bindings from @iancooper as I'm definitely not an expert, and we do not know many in the community @boyney123 do you know anyone who could help with review @iancooper as currently these bindings are empty, will you also maintain these definitions long term? do you want to become the maintainer of this repo? I'm really sorry for keeping it waiting, for just such a short comment for 4 months 😞 |
Will do, I had left the schema to get discussion of the proposal, before doing that work. It was originally why I put it into draft. It might be worth thinking about process here.
Love to. I have a team at Just Eat Takeaway who I can rope in to help make sure we are on top of PRs etc. It could be a lot for one person (and we get the bus problem).
|
ok, that makes sense, for initial review markdown is critical
Perfect ❤️ if you know other folks outside your company and use cases using SNS/SQS that you can reach for giving feedback in this PR, please contact them, also on social media. I will also ping community on AsyncAPI social accounts |
There was a problem hiding this comment.
Couple nits here and there at the first pass.
As previously mentionned the .DS_Store file needs to be removed and the json schemas generated.
I also mentionned this PR to @Kong folks as we are inclined to incorporate those specs in our platforms.
sns/README.md
Outdated
| |Field Name | Type | Description| | ||
| |---|:---:|---| | ||
| | <a name="channelBindingPolicyStatementObjectEffect"></a>`effect` | string |**Required.** Either "Allow" or "Deny"| | ||
| | <a name="channelBindingPolicyStatementObjectPrincipal"></a>`principal` | string or array of string |**Required.** The AWS account or IAM user to whom this statement applies| |
There was a problem hiding this comment.
Shouldn't this be ARN or Aws Account ID?
eg:
"Principal": { "AWS": "arn:aws:iam::123456789012:root" }
"Principal": { "AWS": "123456789012" }ref: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html
sqs/README.md
Outdated
| |---|:---:|---| | ||
| |<a name="queueObjectRef"></a>$ref | `string` | Allows for an external definition of a queue. The referenced structure MUST be in the format of a [Queue](#queue). If there are conflicts between the referenced definition and this Queue's definition, the behavior is *undefined*.| | ||
| | <a name="queueObjectName"></a>`name` | string | **Required.** The name of the queue. When an [SNS Operation Binding Object]() references an SQS queue by name, the identifier should be the one in this field.| | ||
| | <a name="queueObjectType"></a>`type` | string one of: fifo, standard | **Required.** Is this a FIFO queue or a standard queue? | |
There was a problem hiding this comment.
Any reason to use a string field in here instead of a Boolean for FifoQueue ? Aws defines it as one: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-sqs-queue.html
sqs/README.md
Outdated
| |Field Name | Type | Description| | ||
| |---|:---:|---| | ||
| | <a name="channelBindingPolicyStatementObjectEffect"></a>`effect` | string |**Required.** Either "Allow" or "Deny"| | ||
| | <a name="channelBindingPolicyStatementObjectPrincipal"></a>`principal` | string or array of string |**Required.** The AWS account or IAM user to whom this statement applies| |
sns/README.md
Outdated
| | <a name="operationBindingObjectEndpoint"></a>`endpoint` |[identifier](#identifier)| **Required.** Where are messages being delivered to?| | ||
| | <a name="operationBindingObjectFilerPolicy"></a>`filterPolicy` | [filterPolicy](#filter-policy) | **Optional.** Only receive a subset of messages from the channel, determined by this policy.| | ||
| | <a name="operationBindingObjectRawMessageDelivery"></a>`rawMessageDelivery` | boolean | **Required.** If *true* AWS SNS attributes are removed from the body, and for SQS, SNS message attributes are copied to SQS message attributes. If *false* the SNS attributes are included in the body. | | ||
| | <a name="operationBindingObjectRedrivePolicy"></a>`reDrivePolicy` | [redrivePolicy](#redrive-policy) | **Optional.** Prevent poison pill messages by moving un-processable messages to an SQS dead letter queue. | |
There was a problem hiding this comment.
| | <a name="operationBindingObjectRedrivePolicy"></a>`reDrivePolicy` | [redrivePolicy](#redrive-policy) | **Optional.** Prevent poison pill messages by moving un-processable messages to an SQS dead letter queue. | | |
| | <a name="operationBindingObjectRedrivePolicy"></a>`redrivePolicy` | [redrivePolicy](#redrive-policy) | **Optional.** Prevent poison pill messages by moving un-processable messages to an SQS dead letter queue. | |
There is the casing issue over the spec. I think redrivePolicy is the good format.
|
Hey, I see there's not been traction in this PR in a while. When can we expect these bindings (particularly SNS as that's what I need 😁) to be included in the Async spec/what else needs to be done? |
|
Kamran,
I’ll try to pick this up with my team, we need to add the schema, but really we were waiting for comments from others before we went ahead with that
…
On 18 Nov 2022, at 12:51, Kamran ***@***.***> wrote:
Hey, I see there's not been traction in this PR in a while. When can we expect these bindings (particularly SNS as that's what I need 😁) to be included in the Async spec/what else needs to be done?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
|
@iancooper - Thanks for the update, who do you need reviews from? This Async team directly or people in the community who have worked with SNS before? If the latter I can see if I can loop some people in to share their thoughts. |
There was a problem hiding this comment.
Does this design allow for a one-to-many relationship between SNS and SQS?
Something like: SNS message --> [SQS 0, SQS 1, SQS 2, ..., SQS n]
And also associated dead letter queues where needed for each SQS queue?
It is not clear to me from the examples that this is a possibility.
Thanks
| ### Fields | ||
| |Field Name | Type | Description| | ||
| |---|:---:|---| | ||
| | <a name="channelBindingObjectQueue"></a>`queue` | [Queue](#queue)| **Required.** A definition of the queue that will be used as the channel. | |
There was a problem hiding this comment.
Should'n the name here be queues? This is an array, so why is it singular? Was that a typo?
There was a problem hiding this comment.
@jeff9finger I don't think the type is an array. It's of type Queue which I think is consistent with source bindings living at this level of the specification.
Something like: SNS message --> [SQS 0, SQS 1, SQS 2, ..., SQS n]
The queues property does exist on the proposed binding but its part of the binding that sits at the publish level for facilitating things like an sns configured at the channel level and then multiple consumer queues receiving messages. Example from the proposal below as you cannot link to specific MD lines.
sqs:
queues:
- name: user-signedup-queue
type: standard
receiveMessageWaitTime: 4
policy:
statements:
- effect : Allow
principal: *
action: Sqs:SendMessage
- effect : Allow
principal: *
action: Sqs:ReceiveMessage | | <a name="queueObjectPolicy"></a>`policy` |[Policy](#policy) | **Optional.** The security policy for the SQS Queue | | ||
| | <a name="queueObjectTags"></a>`tags` |Object | **Optional.** Key-value pairs that represent AWS tags on the queue. | | ||
|
|
||
| #### Identifier |
There was a problem hiding this comment.
Can an Identifier contain more than one of these? Or does that table have more of a oneOf semantic?
| user-signedup: | ||
| bindings: | ||
| sqs: | ||
| queue: |
There was a problem hiding this comment.
From comment above, shouldn't this be queues?
| deadLetterQueue: | ||
| name: user-signedup-queue-dlq | ||
| sqs: | ||
| queues: |
There was a problem hiding this comment.
I noticed that this item uses queues. This seems correct, but queue is used elsewhere, so am a little confused.
There was a problem hiding this comment.
Hope my comment above addressed this. At the publish level you can configure multiple queues that receive data, but at the channel level you should only configure a single queue because this is the primary logical pipe through which data flows. For the example a point to point SQS model (in the single channel level queue example) as opposed to SNS -> SQS pub sub model (possible one to many messaging).
sqs/README.md
Outdated
| #### Redrive Policy | ||
| |Field Name | Type | Description| | ||
| |---|:---:|---| | ||
| | <a name="redrivePolicyObjectDeadLetterQueue"></a>`deadLetterQueue` |[Identifier](#identifier)| **Required.** The SQS queue to use as a dead letter queue (DLQ) | |
There was a problem hiding this comment.
@iancooper why DeadLetterQueue is required? Don't you think that the Redrive policy should be a decision of the application and the spec shouldn't enforce it?
|
How can I help to get this PR forward? I'm about to spec messages for SNS/SQS and would love to make use of this |
reword descriptions to be declarative
|
@dpwdec thanks for all the hard work!!! I see no objection from other participants in this PR to make you a maintainer of these bindings Just please remove There is one concern I have, the conventions file, it is not related to these new bindings only but sets a conventions on all the bindings, which should be done in a separate PR imho, to not mix contexts. Conventions are something that need to be respected, thus part of contribution guide and review process. So to make sure that happens, separate discussion and 👍🏼 from other bindings owners is a must imho |
Delete .DS_Store
feat: migrate conventions to PR asyncapi#211
|
Hi @derberg! Thanks for taking the time to look through it all :D I've removed |
|
Is this likely to get merged soon? |
|
@JaredAAT As far as I am aware everything is complete. It's just waiting to be merged. |
|
@dpwdec sorry for another question but I just want to be 100% sure before we merge
Basically trying to figure what will happen the next day we merge this 😅 |
Hi @derberg, thanks for asking. I work with @dpwdec. He is part of a team at Just Eat Takeaway I provide advice to, who are implementing AsyncAPI for us. I ran out of time to pursue this PR, so I delegated it to the team instead. I am happy to keep being informed and help out, but the team has more time for the detail here than I do right now.
Happy to keep being a maintainer with @dpwdec to avoid a single-point of failure. We might be able to find some other folks in the team too. |
|
@derberg Seconding what's said above, more than happy to provide support for SNS/SQS bindings in tandem with @iancooper or any others who feel they have the expertise or time to provide the support as well. I'm also on the slack now and can provide more contact details to other AsyncAPI maintainers if required. 👍 |
|
@dpwdec sorry if I sounded like you are not welcome or something like that. It’s just that the problem with bindings we have that they are basically “micro specifications for AsyncAPI” so they do not change often, very rare, so there is no proper path for potential maintainer to feel the AsyncAPI community spirit before we invite to the org. So basically since last few months where AnypointMQ spec maintainers had to change we were afraid we just invite someone who we will not be really able to contact with in case of problems, someone that is not known. I hope you know what I mean. So in JMS we luckily had existing codeowner volunteer to help the other “fresh” maintainer. Governance of AsyncAPI Initiative works in the way that any maintainer can become a TSC member without any secret invitation to secret club 😄 so good to know that the person we appoint for maintainer role is easy to reach on different channels in case of emergency. anyway, super happy to see @iancooper already commenting on this. He is a great guy that was very active some time ago so if you folks work together and are both fine to be co-maintainers that is a perfect situation 🍺 @dpwdec please update the CODEOWNERS and add Ian After we merge this PR, feel free to add your names to https://github.com/asyncapi/community/blob/master/MAINTAINERS.yaml through a PR. Do it if you wish to be more involved in the community and help it to grow. Also, you might be interested in applying for one of our AsyncAPI events this year. We have London CFP opened https://conference.asyncapi.com/ and I bet SQS/SNS talk would be super interesting |
|
@derberg No problem at all, I didn't feel it was unwelcoming! As you've said above there are can and will be issues and you need to make sure there are some contingencies in place for when that happens. I'll update CODEOWNERS tomorrow morning and take a look at the conference as well! 🙌 |
feat: update CODEOWNERS to include iancooper
|
@derberg |
There was a problem hiding this comment.
LGTM, thanks for all the hard work
so this binding will work with AsyncAPI prior 3.0
once this is merged, please look into #182. We need you to inspect if there will be any changes needed to make binding work with 3.0
|
@KhudaDad414 you need to approve again as JSON Schemas guardian 😄 |
|
/rtm |
|
Great effort @iancooper @dpwdec 🙇 |
Description
This is currently a draft, once we are through feedback, I need to add the JSON schema that matches what we agree too, and then I'll convert to ready