jpoehnelt / secrets-sync-action

A Github Action that can sync secrets from one repository to many others.
https://github.com/marketplace/actions/secrets-sync-action
Apache License 2.0
314 stars 92 forks source link

Abuse Detection Limits #11

Closed chiefjester closed 4 years ago

chiefjester commented 4 years ago

Hi @jpoehnelt 👋

I just want to report that I triggered an abuse detection mechanism for setting up secrets:

image

I have 128 113 (did a :%sort u) entries in my list, and I sync about 2 secrets per repo.

chiefjester commented 4 years ago

by the way, @jpoehnelt dryrun is super helpful! 🙌 and I just saw you added a hardcoded version of repositories now. 🎉 Thanks!

I'm so glad this exists! It made GitHub Actions useable for me. 😊

jpoehnelt commented 4 years ago

Thanks for the feedback!

Regarding this issue:

  1. Does it matter how often the action runs?
  2. What is the acceptable rate to not trigger this?
  3. Are you running multiple actions simultaneously? (jobs vs steps)

For 2, I could see adding adding a flag or just changing the behavior to run in serial. https://github.com/google/secrets-sync-action/blob/master/src/main.ts#L84-L88

chiefjester commented 4 years ago

Hey @jpoehnelt 👋 Sorry I missed your reply, so here goes:

  1. Does it matter how often the action runs?

I'm not sure what you meant, but I just ran it on every push. And it's just one job and one step.

  1. What is the acceptable rate to not trigger this?

It was fine when there are about 200 secrets being updated. It triggered the detection when I added 16, (8 repositories x 2 secrets). I wouldn't mind if we made it batch 100~150 secrets in serial? Updating 200 secrets take about 40 seconds for me.

  1. Are you running multiple actions simultaneously? (jobs vs steps)

Just one job and one step.

jpoehnelt commented 4 years ago

@thisguychris

The action should retry on abuse limits now. I'm not sure if that will be suitable as there could still be a thundering herd problem and only Github knows how abuse limits are trigered. I haven't look too closely into the internals of https://github.com/octokit/plugin-throttling.js.

If this doesn't work, I'll change https://github.com/google/secrets-sync-action/blob/master/src/main.ts#L84-L88 to be serial.

chiefjester commented 4 years ago

hey @jpoehnelt thanks for replying so fast! 👋

So I tried to use v1.3.1 It still gives me an exit 1 status code. By the way, Just to give you more context on where the abuse errors get logged out. It's about pass 100th line (this seems to be variable, 117th for one test, 124th for another). However, it still prints the rest of the 100+ lines.

While this is true, I've been syncing above 100 for quite a while, just hovering 200 secrets, it triggered when I went beyond it.

I haven't checked the code but if you print 2020-05-01T04:09:38.3449589Z Set FOO= *** in the line does it mean that the API returned a 200 success for that operation?

That throttling plugin by octokit seems cool, but I'm not sure if it's the right throttling we need since we are updating secrets, does it count as writing?

If you do opt for serial, I'd gladly test it, I hope you add a parameter I can play with on how many "chunks" of secrets I can update.

vinigfer commented 4 years ago

Hi there. We are also having this issue. Adding a flag/option to run it in serial would be awesome! Or maybe even a configurable wait time between each request. Don't mind if it takes a bit longer, as long as it's done.

jpoehnelt commented 4 years ago

Please try v1.3.2 which await's on each repo serially.

chiefjester commented 4 years ago

@jpoehnelt I'm happy to report making it post serially doesn't trigger abuse detection limits. Thank you 🙌

Just a suggestion though, any chance I can configure chunks 50~100 request at a time instead of 1 request per time?

jpoehnelt commented 4 years ago

v1.4.0 is using p-limit and a corresponding concurrency setting that defaults to 10.

with:
  CONCURRENCY: 50 # 10 is default
chiefjester commented 4 years ago

@jpoehnelt success! Thanks so much for adding this! 🙌 I learned so much (about the existence of those libraries) p-limit seems to be a gem too.

Concurrency settings I've tried: image

No concurrency: image

The difference is night and day!

I guess you can close this issue now 🙂

jpoehnelt commented 4 years ago

Glad it worked out! Thanks for all the feedback.