AWS SQS source

[srvaroa]

Proposed Changes

• Provide basic suport for an AWS SQS source.

[srvaroa]

I have been able to run this successfully. I still want to give a pass to the READMEs and cleanup code but it should be in a decent state to get some feedback.

cc @rootfs, I used some of your earlier work at knative/eventing#328 as reference, if you have some time to take a look that'd be great.

[srvaroa]

This is actually not working and I had to use the FQDN (sqs.eu-west-1.amazonaws.com in my case) to make it work.

[rootfs]

what do you see in the istio sidecar container?

[srvaroa]

Thanks @rootfs. It looks like a false alarm. The istio-proxy container takes longer to start than the receive-adapter so you can see this in the logs:

default/awssqs-devhose-source-vzrsc-765c547b68-gmp7m[receive-adapter]: {"level":"info","ts":1542735771.9919236,"caller":"awssqs_receive_adapter/main.go:68","msg":"Starting AWS SQS Receive Adapter. %v","adapter":{"Region":"eu-west-1","QueueUrl":"...","SinkURI":"http://qux-1-channel.default.svc.cluster.local/","CredsFile":"/var/secrets/aws/credentials"}}
default/awssqs-test-source-vzrsc-765c547b68-gmp7m[receive-adapter]: {"level":"info","ts":1542735771.9922512,"logger":"fallback","caller":"awssqs/adapter.go:63","msg":"Starting with config: {adapter 22 0 0xc4200cedc0}"}
default/awssqs-test-source-vzrsc-765c547b68-gmp7m[receive-adapter]: {"level":"warn","ts":1542735772.3487105,"logger":"fallback","caller":"awssqs/adapter.go:77","msg":"Failed to poll from SQS queue{error 25 0 RequestError: send request failed\ncaused by: Post https://sqs.eu-west-1.amazonaws.com/: dial tcp 52.95.114.128:443: connect: connection refused}"}

But the istio-proxy does initialize correctly after a while

default/awssqs-devhose-source-vzrsc-765c547b68-gmp7m[istio-proxy]: [2018-11-20 17:42:53.579][19][info][upstream] external/envoy/source/common/upstream/cluster_manager_impl.cc:494] add/update cluster outbound|80||message-dumper-00001-service.default.svc.cluster.local during init
default/awssqs-devhose-source-vzrsc-765c547b68-gmp7m[istio-proxy]: [2018-11-20 17:42:53.579][19][info][upstream] external/envoy/source/common/upstream/cluster_manager_impl.cc:494] add/update cluster outbound|443||controller-manager-service.knative-sources.svc.cluster.local during init
default/awssqs-devhose-source-vzrsc-765c547b68-gmp7m[istio-proxy]: [2018-11-20 17:42:53.580][19][info][upstream] external/envoy/source/common/upstream/cluster_manager_impl.cc:494] add/update cluster outbound|443||*.amazonaws.com during init

At this point, the receive adapter starts to poll successfully.

In my tests I had a plain exit if there was a poll error (I'm reading from a queue with a lot of messages so it was hard to read the logs while debugging.) I didn't realize that the sleep I added on error actually masks this behaviour.

[rootfs]

@srvaroa thanks for mentioning :)

I am adopting container source for my new event source here. It is quite similar to SQS. Except secret volume (which is to be addressed in #55, container source is ready for its prime time.

cc @grantr @n3wscott

[srvaroa]

@Harwayne I applied your other suggestion re. having stopCh parametrized to the start method in the adapter (see 6cec8fc) - I couldn't reply on that thread

[whynowy]

@srvaroa - Since it's AWS SQS event source, I would assume people use SQS are running their K8s clusters on AWS EC2 instances. From within the cluster to access AWS resources (like SQS), there's another way to do it with kiam (https://github.com/uswitch/kiam) or kube2iam instead of using a long lived credential stored in k8s secret, I implemented the SQS event source by using kiam (https://github.com/whynowy/eventing-sources/commits/develop), I think it would be better to support both ways, what do you think?

[whynowy]

@srvaroa - Since it's AWS SQS event source, I would assume people use SQS are running their K8s clusters on AWS EC2 instances. From within the cluster to access AWS resources (like SQS), there's another way to do it with kiam (https://github.com/uswitch/kiam) or kube2iam instead of using a long lived credential stored in k8s secret, I implemented the SQS event source by using kiam (https://github.com/whynowy/eventing-sources/commits/develop), I think it would be better to support both ways, what do you think?

As I know, a lot of people are using kiam to do delegated access to AWS resources, which is much more secure than using a stored credential. The approach of using secret is not going to be approved by the security team in my company.

[srvaroa]

Hi @whynowy thanks for the review. I agree that also having support for kiam would be good. Since it doesn't replace the secret-based approach (which covers also non AWS environments) I would add that support in a separate PR after this one is in (happy to do it myself or help out). WDYT? (cc @Harwayne @vaikas-google)

[Harwayne]

Hi @whynowy thanks for the review. I agree that also having support for kiam would be good. Since it doesn't replace the secret-based approach (which covers also non AWS environments) I would add that support in a separate PR after this one is in (happy to do it myself or help out). WDYT? (cc @Harwayne @vaikas-google)

I don't know enough about SQS to have an informed opinion. In general, I think this PR is looking pretty good and can get in soon, so my preference is to add the kiam support in a subsequent PR, rather than slow this one down. But, as I said, I don't know how important kiam support is.

[srvaroa]

@Harwayne IAM is a better solution, but valid only if you're running your k8s cluster in AWS. The solution proposed here based on secrets would work regardless of where the k8s cluster runs (AWS included - I went down this route because I'm using GKE + and minikube for dev). The downside is that it's less secure (you're spreading secrets around, if an attacker gets access to the k8s secrets that store the values they could use them to consume from the queue.)

I think kiam support is important for the reasons @whynowy points out so I think it's a very desirable addition. I can work on this shortly after.

[srvaroa]

@Harwayne there was a small change to the type tests required after the bump to /pkg which I forgot to add to the type. The change is simply applying the same fix to the newly added type and this should make tests pass now (needs the lgtm though)

[Harwayne]

/lgtm

And thanks for not rebasing! It made it much easier to see what changed 😄

[Harwayne]

/ok-to-test

[srvaroa]

@Harwayne sorry, looks like it also needed an update-manifests (tests were passing locally so I didn't spot this)

[Harwayne]

/lgtm

[Harwayne]

/ok-to-test

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/adapter/awssqs/adapter.go	Do not exist	54.1%
pkg/apis/sources/v1alpha1/aws_sqs_types.go	Do not exist	100.0%

[grantr]

Thanks @srvaroa!

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, srvaroa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [grantr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[grantr]

and sorry for the approval delay :(

[srvaroa]

Thanks @grantr, no worries! Let me know if there's anything else I should fix to enable merging (cc @Harwayne)

[grantr]

/lgtm

[whynowy]

@srvaroa - one last request, please continue on the KIAM support :)