-
Notifications
You must be signed in to change notification settings - Fork 0
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
support optional CODE
environment/stage via new optional action input codeDomain
#19
base: main
Are you sure you want to change the base?
Conversation
BLOCKED: because the ... will give this some thought? Any ideas @nicl (aside from a riff-raff PR to support varying S3 path by Stage, like |
IIUC this will use the same bucket for CODE and PROD stages, this goes against the recommendations. I've a strong suspicion that the |
@akash1810 currently
I think the fact we don't expect one application to affect an other, I personally think that recommendation doesn't warrants a whole extra stack for
Unfortunately we need set the key to the domain name, as per the comment in the file |
@akash1810 yeah looking at it, the Go code would need to look across before two buckets before serving, I don't know Go so might struggle... |
e816c1d
to
4bf2179
Compare
actually I've taken a stab at it, in d772b86 |
1533bc8
to
e24eb84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, think this is really worthwhile - so thank you for doing the work!
My only concern atm is around the fallback store behaviour. It feels like quite surprising behaviour and doesn't sit quite right - both because of the interesting security implications (though low risk, we are still allowing 'code' stuff to potentially leak into prod in strange ways) and because it adds redundant calls.
The alternatives I can think of are:
- detecting if the request domain is dev-gutools or contains
code.
in the domain and then using the CODE bucket (downside is that only certain CODE domains would work) - adding a separate CODE ALB in the infra stack and registering certs etc against that when stage is CODE (downside: extra $$$)
Should we just go for (2)? Keen to get thoughts anyway.
index.ts
Outdated
@@ -25,6 +26,19 @@ export const main = (): void => { | |||
|
|||
const cfn = Template.fromStack(cdkStack).toJSON(); | |||
fs.writeFileSync("cfn.json", JSON.stringify(cfn, undefined, 2)); | |||
|
|||
if(codeDomain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Minor - whitespace between if
and opening (
.)
e24eb84
to
28d841a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How important is this feature to you @twrichards ? If absolutely required, we might be better off creating a v3 of this action that has separate buckets and stacks for the core infrastructure. I.e. at the moment we have a single INFRA stack for the ALB, EC2 instance, and bucket. But we could switch to proper CODE and PROD infra stacks here and then update the action to act on the appropriate ALB/bucket as required based on stage (with your codeDomain input useful here). We'd have to migrate existing users to v3, but that shouldn't be too difficult.
Alternatively, the separate stack deployed to 'PROD' (which you mentioned) is an option here - it's not pretty I realise but it keeps the code here simple. At the moment I think galaxies is the only request for a CODE stage, so this might be the pragmatic option until we get more demand?
My concern with the approach here is that we are reading from both buckets even for a CODE stage request, which feels sus. It is also a bit more complicated in the details too - more conditionals.
service/main.go
Outdated
|
||
http.HandleFunc("/healthcheck", middleware.WithRequestLog(http.HandlerFunc(ok))) | ||
|
||
if config.RequireAuth { | ||
http.Handle("/", middleware.WithRequestLog(middleware.WithAuth(middleware.WithDomainPrefix(storeServer(store))))) | ||
http.Handle("/", middleware.WithRequestLog(middleware.WithAuth(middleware.WithDomainPrefix(storeServer(store, codeStore))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bit that makes me somewhat uneasy: we are potentially serving the prod bucket via a code domain. The prefixe match should prevent this from happening but fundamentally the CODE path is still attempting to hit PROD data and this doesn't quite sit right.
bd55f4a
to
2d7a0a1
Compare
f4ddd2e
to
512ccee
Compare
…proach at the point of read. `codeDomain` passed in the action is validated against this list. Co-authored-by: Frederick O'Brien <frederick.obrien@guardian.co.uk>
512ccee
to
a4c7b97
Compare
TO BE TESTED VIA https://github.com/guardian/galaxies/pull/155
For most applications we have a
CODE
environment to allow testing amongst devs and select other users before releasing to PROD, and although for static sites this is less relevant, its still useful for certain projects but currently not easily possible withactions-static-site
with out ugly workarounds like...... in order to specify the different domains. The
Stage
option is then counter intuitive (when CODE variant is selected)...... lastly the workaround requires the same build of the static side to have two instances of
actions-static-site
in its CI definition which is wasteful (and no guarantees they're the same).This PR aims to address that by offering an optional action input
codeDomain
which if specfied, generates another CFN stack and differingriff-raff.yaml
(making use ofprefixStagePaths
added in guardian/riff-raff#1015).