add AWS SQS event source by rootfs · Pull Request #328 · knative/eventing

rootfs · 2018-08-07T19:56:17Z

Fixes #

Proposed Changes

rootfs · 2018-08-07T20:01:24Z

/assign @scothis

evankanderson · 2018-08-08T21:40:58Z

/assign @n3wscott

n3wscott

Will look again when not WIP

n3wscott · 2018-08-08T21:53:21Z

+}
+
+func NewAWSSQSEventSource(kubeclientset kubernetes.Interface, feedNamespace string, feedServiceAccountName string, image string) sources.EventSource {
+	glog.Infof("Image: %q", image)


Note we have been moving away from glog. We are moving to either zap for controller logging and log for jobs logging.

n3wscott · 2018-08-08T21:53:56Z

+
+	err := a.deleteAWSSQSDeployment(deploymentName)
+	if err != nil {
+		glog.Warningf("Failed to delete deployment: %s", err)


%s should be %v

n3wscott · 2018-08-08T21:54:38Z

+	glog.Infof("creating awssqs feed context")
+	// create aws sqs deployment
+	awsToken := ""
+	awsId := trigger.Parameters["AWS_ACCESS_KEY_ID"].(string)


magic keys should be moved to a string const block at top.

n3wscott · 2018-08-08T21:59:43Z

+	awsToken := ""
+	awsId := trigger.Parameters["AWS_ACCESS_KEY_ID"].(string)
+	awsKey := trigger.Parameters["AWS_SECRET_ACCESS_KEY"].(string)
+	if token, ok := trigger.Parameters["AWS_SESSION_TOKEN"].(string); ok {


I would assign awsToken directly and then ...; !ok || len(awsToken) == 0 {

I have a few examples of pulling params out of contexts and objects here: https://github.com/knative/eventing/pull/276/files#diff-15c4ba004c85a3abc13efbe47a731b58R391

n3wscott · 2018-08-08T22:00:14Z

+	if len(resource) != 2 {
+		return nil, fmt.Errorf("invalid resource: must be region/queue-name")
+	}
+	region := resource[0]


again, I updated: https://github.com/knative/eventing/pull/276/files#diff-15c4ba004c85a3abc13efbe47a731b58R391

n3wscott · 2018-08-08T22:02:11Z

+	if err != nil {
+		return nil, err
+	}
+	deploymentName := "awssqs-" + queueName + "-" + uuid.String()


uuid is a very large number. Paul had a better method by using k8s generate name. @pmorie can you link to that api to get the short random name?

n3wscott · 2018-08-08T22:05:50Z

+	var p parameters
+	err := json.Unmarshal(decodedParameters, &p)
+	if err != nil {
+		panic(fmt.Sprintf("can not unmarshal %q : %v", decodedParameters, err))


I moved from panic to os.Exit(0) with a log above

whynowy · 2018-09-21T21:37:06Z

+    resource: us-east-1/knative-demo
+    service: aws-sqs
+    parametersFrom:
+      - secretKeyRef:


Should consider the case that cluster is running on AWS, and use KIAM to do IAM role delegation. This is how people access AWS resources from a k8s cluster running on EC2 instances without using any credentials.

The implementation can support both cases, with credential, or KIAM delegated Role. For the latter case, the IAM role configuration would be like:

Kubernetes cluster master nodes IAM role (This should already be existing after the cluster is created), this role should have the policy to assume-role

An IAM role which is assigned to a namespace (and will be used by the POD), this role is set to trust cluster master node IAM role, with KIAM, your pod can get temp credential to this IAM role by calling AWS metadata API, without using any credential. This Role also needs the policy to assume-role.

A third role to be able to operate SQS, and it trusts the 2nd IAM role.

In flow configuration, the 2nd and 3rd IAM role ARNs are needed, so that in your receive_adaptor you can use the 2nd role credential to assume 3rd role, and then read messages.

I didn't see this PR until this afternoon. Before that, I implemented a SQS event source about 1 week ago, which was very similar to this. The major difference is, my k8s cluster is running on EC2 instances, and I use KIAM, so I don't use any explicit credentials to access AWS SQS. So I gave above comments, it would be better if this case can be considered in the PR.

rootfs · 2018-09-24T12:19:20Z

@whynowy thanks, i ran my tests locally, IAM is a good idea, I'll get this here after eventing core is settled.

Signed-off-by: Huamin Chen <hchen@redhat.com>

knative-prow-robot · 2018-10-04T13:38:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rootfs
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: scothis

If they are not already assigned, you can assign the PR to them by writing /assign @scothis in a comment when ready.

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

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

knative-prow-robot · 2018-10-04T13:41:27Z

@rootfs: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-eventing-unit-tests	`099adfc`	link	`/test pull-knative-eventing-unit-tests`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

matzew · 2018-11-07T11:18:15Z

@rootfs @n3wscott @scothis IMO we could close this... since this should be PR'd on the new repo (eventing-sources), following the new model

Co-authored-by: pierDipi <pierDipi@users.noreply.github.com>

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2018

knative-prow-robot requested review from inlined and vaikas August 7, 2018 19:56

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 7, 2018

knative-prow-robot assigned scothis Aug 7, 2018

rootfs force-pushed the sqs-src branch 2 times, most recently from ba2e056 to 636b2e4 Compare August 8, 2018 18:38

knative-prow-robot assigned n3wscott Aug 8, 2018

n3wscott reviewed Aug 8, 2018

View reviewed changes

rootfs force-pushed the sqs-src branch from 4243d3a to efb1324 Compare August 14, 2018 18:10

rootfs changed the title ~~[WIP] add AWS SQS event source~~ add AWS SQS event source Aug 14, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2018

whynowy reviewed Sep 21, 2018

View reviewed changes

n3wscott mentioned this pull request Sep 25, 2018

adding n3wscott to approvers. #465

Merged

Huamin Chen added 2 commits October 4, 2018 13:38

support aws sqs as event source

cff54c7

Signed-off-by: Huamin Chen <hchen@redhat.com>

add vendor files

099adfc

Signed-off-by: Huamin Chen <hchen@redhat.com>

rootfs force-pushed the sqs-src branch from efb1324 to 099adfc Compare October 4, 2018 13:38

rootfs closed this Nov 7, 2018

srvaroa mentioned this pull request Nov 20, 2018

AWS SQS source knative/eventing-contrib#124

Merged

matzew pushed a commit to matzew/eventing that referenced this pull request Oct 12, 2023

Run make generate-release (knative#328)

91a7436

Co-authored-by: pierDipi <pierDipi@users.noreply.github.com>

Conversation

rootfs commented Aug 7, 2018

Proposed Changes

Uh oh!

rootfs commented Aug 7, 2018

Uh oh!

evankanderson commented Aug 8, 2018

Uh oh!

n3wscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rootfs commented Sep 24, 2018

Uh oh!

knative-prow-robot commented Oct 4, 2018

Uh oh!

knative-prow-robot commented Oct 4, 2018

Uh oh!

matzew commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants