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(hooks): Fix github webhooks with secrets #840

Closed
wants to merge 1 commit into from
Closed

fix(hooks): Fix github webhooks with secrets #840

wants to merge 1 commit into from

Conversation

ferwguerra
Copy link
Contributor

@ferwguerra ferwguerra commented Apr 7, 2020

Issue:
The pipelines are no longer being triggered by Github webhooks if there is a secret required. Removing the secret allows them to function normally.

This issue was reported here.

Problem:
The headings are all lowercase, so they are now not found.
This previously worked because the org.springframework.http.HttpHeaders class was used but was replaced by java.util.Map, which is case sensitive. That change was made here.

image

@gregjones
Copy link

Is it possible they're lowercase when they arrive in our setup because of the load-balancer doing normalization? Maybe it makes sense to go back to using a case-insensitive lookup, or else normalize the case when the headers are copied?

@ferwguerra
Copy link
Contributor Author

Is it possible they're lowercase when they arrive in our setup because of the load-balancer doing normalization? Maybe it makes sense to go back to using a case-insensitive lookup, or else normalize the case when the headers are copied?

The headers are mapped using org.springframework.http.HttpHeaders and that's the reason they are lowercase.
You can check running echo and sending this request:

curl --location --request POST 'http://localhost:8089/webhooks/git/github' \ --header 'User-Agent: GitHub-Hookshot/XXXXX' \ --header 'X-GitHub-Enterprise-Host: ${HOST}' \ --header 'X-GitHub-Enterprise-Version: 2.20.1' \ --header 'X-GitHub-Event: push' \ --header 'X-Hub-Signature: sha1=XXXXXXXXX' \ --header 'Content-Type: application/json' \ --data-raw '{ ... } }'

@xavileon
Copy link
Contributor

xavileon commented Apr 8, 2020

@spinnaker/oss-approvers PTAL

@xavileon
Copy link
Contributor

@spinnaker/oss-approvers any comment on this? thanks!

@ezimanyi
Copy link
Contributor

@jonsie : Mind taking a look at this as it might be tangentially related to your PR #790. Thanks!

@ezimanyi ezimanyi requested a review from jonsie April 14, 2020 14:16
@wli66666
Copy link

any estimated time for merging this ? since this is a live bug.....

@sofam
Copy link

sofam commented Apr 20, 2020

We are also hit by this.

@iiro
Copy link

iiro commented Apr 20, 2020

We as well.

@jonsie
Copy link
Contributor

jonsie commented Apr 20, 2020

@ferwguerra Thanks for the contribution, and sorry for introducing this bug (and also sorry for the delay here - missed this notification). I'm proposing an alternative fix here that addresses the underlying issue:

#863

@jonsie
Copy link
Contributor

jonsie commented Apr 20, 2020

Addressed in #863

@jonsie jonsie closed this Apr 20, 2020
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.

9 participants