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

fix(eventsource): fix GCP Pub/Sub behavior #845

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

tmshn
Copy link
Contributor

@tmshn tmshn commented Aug 24, 2020

Fixed several problems of GCP Pub/Sub event source:

  • poor documents on how subscriptions/topics are used and how to use Workload Identity
    • add description and fixed examples. also tweaked other nits
  • subscription project ID is required even Argo Events are running on GKE
  • even if topic project ID is different from subscription's one, there's a bug that both use same one (the latter one is ignored)
    • fixed such logic, and use another client to handle topic
  • more than minimal permissions are required (violates least privilege principle!)
    • changed used APIs such as subscription.getsubscription.testPermission for existing check, topic.getsubscription.get for topic verification
  • redelivery of failed messages are delayed until ackDeadline
    • call m.Nack() immediately on error to allow faster redelivery
  • first API call fails when using Workload Identity, due to delay of token synchronization in metadata server
    • update cloud.google.com/go to v0.52.0 where read timeout to metadata server is removed (ref: googleapis/google-cloud-go@fbf2f51)
    • Note: v0.52 is smallest version which fix the problem. I want to update to latest version (v0.64), but it involves version up of many other libraries used in other places, which is too hard for me to verify every parts work correctly

Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR! I don't have more comments than some doc related (obviously you have more knowledge on Pub/Sub than me), please make sure it is well tested. Also I wonder if @apurvchandra has any comments on this as I know he uses Pub/Sub as well, or @tmshn would you like to provide a RC image for him to do cross verification?

kubectl apply -n argo-events -f https://raw.githubusercontent.com/argoproj/argo-events/stable/examples/event-sources/gcp-pubsub.yaml
```

If you use Workload Identity, omit `credentialSecret` field. Instead don't forget to configure appropriate service account (see [example](https://github.com/argoproj/argo-events/blob/stable/examples/event-sources/gcp-pubsub.yaml)).
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't need a service account with privileged access any more, see https://github.com/argoproj/argo-events/blob/master/docs/service-accounts.md

Copy link
Contributor Author

@tmshn tmshn Aug 27, 2020

Choose a reason for hiding this comment

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

Workload Identity works by connecting Google Service Account (GSA) and Kubernetes Service Account (KSA) so that KSA can obtain GSA's token.

This connection is done by putting an annotation to KSA like below:
(NB: in addition to this, appropriate IAM should be set on GCP side, but no extra RoleBinding is needed on k8s side)

apiVersion: v1
kind: ServiceAccount
metadata:
  namespace: argo-events
  name: tmshn-tester
  annotations:
    iam.gke.io/gcp-service-account: argo-events-tmshn-tester@my-gcp-project.iam.gserviceaccount.com

ref: https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity

This setup is too basic on using Workload Identity to write on Argo Events' document, but anyway we need to know which KSA runs the event source pod.
So at least I want to document that even if we don't encourage users to create new KSA.

Is that default or argo-events-sa ?

Copy link
Member

Choose a reason for hiding this comment

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

You are right, at my first sight, I thought the doc was something about "configure proper service account to access the secret...", please ignore that comment. However, I think the link should be pointing to https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.

@tmshn
Copy link
Contributor Author

tmshn commented Aug 27, 2020

I've tested to cover all cases, but I'm happy if anyone can double-check this!

would you like to provide a RC image for him to do cross verification?

I pushed the event source docker image built on HEAD.

You can try by fixing EVENTSOURCE_IMAGE environmental variable on event source controller to following value:

tmshn/argoproj-eventsource:v1.0.0-rc4-9a0d274

Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@whynowy whynowy merged commit 52bec98 into argoproj:master Aug 28, 2020
@whynowy
Copy link
Member

whynowy commented Aug 28, 2020

@tmshn - would you like to ping me on Slack @whynowy ?

@tmshn tmshn deleted the fix-gcp-pubsub branch August 29, 2020 09:38
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically determine GCP project for Pub/Sub
2 participants