fix: resemble Netlify Production logic for base64 encoding more closely #3631

netlify-team-account-1 · 2021-11-12T08:49:34Z

Summary

Updates the CLI's decision logic for encoding incoming requests as base64 or not, to better match what Netlify production infrastructure is doing.

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

see https://github.com/netlify/proxy/blob/main/pkg/functions/request.go#L119

github-actions · 2021-11-12T08:50:10Z

📊 Benchmark results

Comparing with 72927da

Package size: 365 MB

⬇️ 0.00% decrease vs. 72927da

^  365 MB  365 MB  365 MB  365 MB  365 MB  365 MB  365 MB  364 MB  364 MB  364 MB  364 MB  364 MB  365 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

T-30 (72927da): 365 MB
T-29 (5d07d8a): 365 MB
T-28 (7058451): 365 MB
T-27 (f5f53c5): 365 MB
T-26 (e192b7c): 365 MB
T-25 (ebf295f): 365 MB
T-24 (bd3e265): 364 MB
T-23 (581078c): 364 MB
T-22 (c639926): 364 MB
T-21 (98d4e97): 364 MB
T-20 (f6c10f8): 364 MB
T-19 (406f6f6): 364 MB
T-18 (868868f): 364 MB
T-17 (01861ce): 364 MB
T-16 (19e755e): 364 MB
T-15 (42e03b9): 364 MB
T-14 (57ff1a7): 364 MB
T-13 (a420d3c): 364 MB
T-12 (f71904e): 365 MB
T-11 (51e7b69): 365 MB
T-10 (1622898): 365 MB
T-9 (1147a47): 365 MB
T-8 (f6317a0): 365 MB
T-7 (fc02c27): 365 MB
T-6 (13d5996): 365 MB
T-5 (0a0d897): 364 MB
T-4 (e6e7f6f): 364 MB
T-3 (de12838): 364 MB
T-2 (90de7ed): 364 MB
T-1 (2e055d1): 364 MB
T (current commit): 365 MB

requests without a content-type will be base64-encoded this also applies to HTTP GET

src/lib/functions/utils.js

eduardoboucas · 2021-11-12T10:45:40Z

src/lib/functions/utils.js

+
+  contentType = contentType.toLowerCase()
+
+  if (contentType.startsWith('text/')) {


Any reason why we can't move these to a single condition?

I like how each of the conditions is its own case, à la "if it's a text, don't encode; if it's another content-type encoded as json or xml, don't encode; here's a set of exceptions we don't encode". Moving them into a single condition would make that grouping harder to see. Can you elaborate on why we should move them into a single condition?

Personally, I think having 3 termination points when you can have 1 makes it harder to trace and debug. I also don't see how having a single condition makes the grouping harder to see. It's not a big deal though, and I respect the personal preference if you think it adds more clarity.

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>

Xenonym · 2021-11-15T14:38:54Z

Turns out I inadvertently tried to fix this in #3564 😅, this PR should supersede that I think.

netlify-team-account-1 added 2 commits November 12, 2021 09:44

chore: add reproduction test for multipart/form-data encoding

e356278

fix: resemble proxy's base64 encoding logic

e63caa3

see https://github.com/netlify/proxy/blob/main/pkg/functions/request.go#L119

netlify-team-account-1 requested a review from eduardoboucas November 12, 2021 08:49

github-actions bot added the type: bug code to address defects in shipped code label Nov 12, 2021

chore: update tests

f764ce4

requests without a content-type will be base64-encoded this also applies to HTTP GET

eduardoboucas reviewed Nov 12, 2021

View reviewed changes

netlify-team-account-1 and others added 2 commits November 15, 2021 07:46

Add comment to production code

3b404d9

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>

chore: fix test

29fdf42

eduardoboucas approved these changes Nov 15, 2021

View reviewed changes

netlify-team-account-1 added the automerge Add to Kodiak auto merge queue label Nov 15, 2021

Merge branch 'main' into resemble-base64-logic-of-proxy

e05c16a

kodiakhq bot merged commit efc0b48 into main Nov 15, 2021

kodiakhq bot deleted the resemble-base64-logic-of-proxy branch November 15, 2021 15:46

Xenonym mentioned this pull request Nov 17, 2021

fix: correct regex for binary types #3564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resemble Netlify Production logic for base64 encoding more closely #3631

fix: resemble Netlify Production logic for base64 encoding more closely #3631

netlify-team-account-1 commented Nov 12, 2021

github-actions bot commented Nov 12, 2021 •

edited

Loading

eduardoboucas Nov 12, 2021

netlify-team-account-1 Nov 15, 2021

eduardoboucas Nov 15, 2021

Xenonym commented Nov 15, 2021


		contentType = contentType.toLowerCase()

		if (contentType.startsWith('text/')) {

fix: resemble Netlify Production logic for base64 encoding more closely #3631

fix: resemble Netlify Production logic for base64 encoding more closely #3631

Conversation

netlify-team-account-1 commented Nov 12, 2021

Summary

github-actions bot commented Nov 12, 2021 • edited Loading

📊 Benchmark results

Package size: 365 MB

eduardoboucas Nov 12, 2021

Choose a reason for hiding this comment

netlify-team-account-1 Nov 15, 2021

Choose a reason for hiding this comment

eduardoboucas Nov 15, 2021

Choose a reason for hiding this comment

Xenonym commented Nov 15, 2021

github-actions bot commented Nov 12, 2021 •

edited

Loading