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: pass correct protocol to functions server #4604

Merged
merged 2 commits into from
May 12, 2022

Conversation

eduardoboucas
Copy link
Member

Summary

When using Netlify Dev with HTTPS, the event.rawUrl property in serverless functions incorrectly references the HTTP URL. This PR fixes that.

Closes https://github.com/netlify/pod-compute/issues/86

@eduardoboucas eduardoboucas added the type: bug code to address defects in shipped code label May 12, 2022
@github-actions
Copy link

github-actions bot commented May 12, 2022

📊 Benchmark results

Comparing with b6d76e8

Package size: 286 MB

(no change)

^  285 MB                                                                                          286 MB 
│   ┌──┐   274 MB  274 MB  274 MB  274 MB  274 MB  273 MB  273 MB  273 MB  273 MB  273 MB  273 MB   ┌──┐  
│ ──┼──┼────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────|▒▒|──
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

Comment on lines 92 to 93
const protocol = options.config.dev.https ? 'https' : 'http'
const url = new URL(requestPath, `${protocol}://${request.get('host') || 'localhost'}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

so the server doesn't listen on 80 anymore when you enable https?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have multiple servers running: a main server that then forwards requests to a functions server. The main server is listening on 443, the functions server is listening on 80.

This will continue to be the case, all we're changing is the event data that is sent to functions. Rather than using the protocol from the functions server (which will always be 80), we're using the protocol of the main server.

Copy link
Contributor

Choose a reason for hiding this comment

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

but if you have two servers, you can't really use the config setting right?
since you need to set the protocol based on which server the request came in.
i'd recommend using a HTTP header for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the main/outer server is exposed to customers. The functions server is an internal implementation detail. So the rawUrl value of functions should always match the URL of the outer server.

Copy link
Contributor

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

lgtm

@eduardoboucas eduardoboucas added the automerge Add to Kodiak auto merge queue label May 12, 2022
@kodiakhq kodiakhq bot merged commit a4c9dbb into main May 12, 2022
@kodiakhq kodiakhq bot deleted the fix/functions-https-rawurl branch May 12, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants