Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

feat(images): gcr pubsub push notification for image updates #20

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

sjk07
Copy link

@sjk07 sjk07 commented Jan 22, 2020

Add ability to listen to Google Pubsub push notifications from the google container registry.

This is still a WIP; fighting with my own cluster to setup things end-to-end and also add authentication token validation.
closes #18

@squaremo
Copy link
Member

@sjk07 Do you need some help pushing this further? No worries if you are just busy doing other stuff :-)

@sjk07
Copy link
Author

sjk07 commented Feb 11, 2020

@squaremo I got everything working with Google auth stuff but got stuck when trying to write tests. Could use some help with that.

the Google docs tell you to pass the bearer token via API and Google will authenitcate for you and Slsince the handler doesn't have a client param im not able to mock out the call / return.

I also wanted to add some options for enabling authentication, expected audience, etc.
Happy to push up the code I have now and discuss

@squaremo
Copy link
Member

Yes please, push any more WIP -- we can tidy up later :-)

So far I see you have a handler, and a test which goes as far as the other tests; is there extra work that must be done for it to operate in a real environment?

@sjk07
Copy link
Author

sjk07 commented Feb 11, 2020

The current code works as is in an environment but without authentication. Authentication changes a few things. I'll push up what I have with the auth implementation and we can work though the issues.

@sjk07
Copy link
Author

sjk07 commented Feb 19, 2020

@squaremo sorry for the delay, was on vacation 🍻 ; I have pushed up my code for the authentication piece, If you can provide some guidance with extending the config and testing would be much appreciated.

@squaremo
Copy link
Member

Thanks Scott, I'll have a look.

was on vacation 🍻

Sounds like a good vacation :-P

@squaremo
Copy link
Member

OK I think I have got my head around the authentication bit: each hook POST has an Authorization header with a token which can be checked against their OAuth2 service (there's some mention in the docs of caching responses, but: optimisation, for later, perhaps).

So the things to resolve are:

  • Can authorisation be switched on in config, and how does that get wired through to the hander?

The change you made to the config struct is reasonable, I reckon, even if it only gets used for GCR. The endpoint could be given to the sourceHandler so it gets to see whether it should do authentication -- or some similar formulation.

  • how to test that the authentication works

The simplest thing might be to swap out the URL for OAuth2 verification, and start an httptest.Server to do it for you. It's a bit of a juggling act! But even if you reassign a var just for testing, that's not too bad.

@sjk07 sjk07 changed the title [WIP] feat(images): gcr pubsub push notification for image updates feat(images): gcr pubsub push notification for image updates Mar 6, 2020
@sjk07
Copy link
Author

sjk07 commented Mar 6, 2020

@squaremo added the configuration stuff you talked about, let me know if this is what you envisioned. I didn't quite follow the testing of the auth, I just tested the authentication function directly and mocked the response back to show that it works as intended.

This is currently running on our development clusters with auth enabled. Ill have to go back and double check if no-auth works in a real env again but this should be pretty much good to go!

@tun0
Copy link

tun0 commented Mar 10, 2020

Hi @sjk07, what does it take to test this? Specifically stuff like configuration, on both the side of flux-recv and GCP/GCR.

@chainlink
Copy link

It might be a little easier to use this library https://github.com/golang/oauth2/blob/master/google/default.go

Specifically, you can pass it a service account json blob, and it handles translating that into a bearer token

@hiddeco hiddeco mentioned this pull request Apr 3, 2020
@dmccaffery
Copy link

I think this all looks good, personally.

A short write up / example of setting up a GCR pubsub push subscription would be helpful for those who are not familiar.

Re: testing with a real token -- I don't think that's necessary.

@chainlink are you suggesting using a real token purely for testing purposes? The token is contained within the webhook post and is issued by GCP so you can prove the request came from them. There is no need to supply one to flux-recv in normal operation.

@hiddeco hiddeco added the enhancement New feature or request label Apr 3, 2020
@dmccaffery
Copy link

@squaremo any other requirements on this PR before it can be merged? :)

Copy link
Member

@squaremo squaremo 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 🔝, thank you! I made a couple of suggestions; the main thing to do is rebase on master and update the new handler (Harbor) with the endpoint argument, I think.

gcr.go Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
gcr.go Outdated
var d data
json.Unmarshal(raw, &d)

if strings.ToUpper(d.Action) != insert {
Copy link
Member

Choose a reason for hiding this comment

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

I did a double-take at .ToUpper vs. insert here. Perhaps either make the const value "insert" and compare it to .ToLower, or make the const name INSERT (though go lint might take offence at that, not sure).

gcr.go Outdated
}

token := bearer[tokenIndex:]
url := fmt.Sprintf("https://oauth2.googleapis.com/tokeninfo?id_token=%v", token)
Copy link
Member

Choose a reason for hiding this comment

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

Can this be more explicit than %v? A minor thing.

@chainlink
Copy link

chainlink commented Apr 5, 2020

@chainlink are you suggesting using a real token purely for testing purposes? The token is contained within the webhook post and is issued by GCP so you can prove the request came from them. There is no need to supply one to flux-recv in normal operation.

Apologies, I thought this was reading the topic directly and thus would be need a service account.

@hiddeco
Copy link
Member

hiddeco commented Apr 14, 2020

@sjk07 I am happy to pick this up and address Michael's comments for you if this PR no longer has your attention.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

There's a stray file, I think, but looks good -- thanks @sjk07, @hiddeco and commenters 🍍

test/fixtures/gcr_auth_headers Outdated Show resolved Hide resolved
@hiddeco hiddeco merged commit 2557181 into fluxcd:master Apr 16, 2020
@sjk07
Copy link
Author

sjk07 commented Apr 23, 2020

@squaremo sorry for going MIA on this; Thanks @hiddeco for seeing this through!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(images): gcr pubsub push notification for image updates
6 participants