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

WIP: add Webhook to reduce no. of registry scans required #284

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tuananh
Copy link

@tuananh tuananh commented Oct 28, 2021

this is just a poc. I'm looking for feedbacks. I'm not sure this is the correct/prefer way to implement.

  • create a webhook endpoint for each registry
  • upon receiving event from Webhook, only compare the current tag and the incoming tag from Webhook. I'm not sure this would work for all cases.
  • with this, we can reduce the CheckInterval to reduce no. of registry scan required.
  • Only nexus is implemented for now.

@CLAassistant
Copy link

CLAassistant commented Oct 28, 2021

CLA assistant check
All committers have signed the CLA.

@tuananh tuananh force-pushed the add-registry-webhook branch 3 times, most recently from ec60e5e to 7626dea Compare October 29, 2021 17:19
Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

This is an awesome PR 🚀 , thanks a lot @tuananh! And sorry for coming back so late to it.

I'm willing to merge the code early on, so we can progress on its development without you having to constantly rebase it to changes in the master branch.

However, I have two concerns:

  • The web hook feature should be marked experimental and not enabled by default unless we are sure it's stable (and safe) enough to enable it by default. So the default value for --registry-webhook-port parameter should be set to 0 so people would have to explicitly enable it. When we had a bit of testing (and possibly refactoring, and bug fixes) we can then enable it by default.

  • The way this was implemented is that it trusts the input from the registry webhook, e.g. uses the tag in the hook's payload. This is sensitive, and we should therefore require webhook secrets and not have it optional. Otherwise, anybody could send arbitrary tags and trigger updates to possibly unwanted images.

WDYT?

@tuananh
Copy link
Author

tuananh commented Nov 11, 2021

@jannfis thanks. i'm ok with merging it early on and have it disable.

i change the default webhook port to 0.

as for webhook secret. i implemented it for sonatype nexus but dockerhub doesn't support secret in webhook (at least in free version i'm using) so that's why i didn't enforce it.

@tuananh tuananh force-pushed the add-registry-webhook branch 3 times, most recently from 34113de to cd7c098 Compare November 11, 2021 04:21
Signed-off-by: Tuan Anh Tran <me@tuananh.org>
Signed-off-by: Tuan Anh Tran <me@tuananh.org>
Signed-off-by: Tuan Anh Tran <me@tuananh.org>
@vr
Copy link

vr commented Apr 25, 2022

@jannfis @tuananh any news since 6 month ago ? 🙏

@manelpb
Copy link

manelpb commented Apr 20, 2023

any updates on this? it would be nice if we could get it merged

@tkaesserfm
Copy link

Please, any update on this? 🙏

@tran4774
Copy link

Does it stop updating?? This feature is valuable

@hugosxm
Copy link

hugosxm commented Nov 17, 2023

Any update on this one please ?

@wangtaotao0524
Copy link

hi guys, any update on this one? it is an amazing feature.

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.

None yet

9 participants