firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Published 20 hours ago • verified publisher icon firecracker-microvm

MMDS: Make header parsing case-insensitive

Open CompuIves opened this issue 3 years ago • 6 comments

Reason for This PR

The MMDS API requires the X to be uppercased in header names (e.g. X-metadata-token). According to the HTTP1 spec, headers should be ignorant of capitalisation, and because of this libraries like hyper automatically lowercase all header names. Because of this, some HTTP clients cannot send requests to the MMDS API.

Description of Changes

Make the header parsing for MMDS ignorant to casing.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.] [Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • [x] All commits in this PR are signed (git commit -s).
  • [ ] The issue which led to this PR has a clear conclusion.
  • [ ] This PR follows the solution outlined in the related issue.
  • [x] The description of changes is clear and encompassing.
  • [x] Any required documentation changes (code and docs) are included in this PR.
  • [x] Any newly added unsafe code is properly documented.
  • [x] Any API changes follow the Runbook for Firecracker API changes.
  • [ ] Any user-facing changes are mentioned in CHANGELOG.md.
  • [x] All added/changed functionality is tested.

CompuIves avatar May 26 '22 19:05 CompuIves

@luminitavoicu Saw that one test failed, I created a commit now which fixes that test!

CompuIves avatar May 27 '22 09:05 CompuIves

I've updated the PR again to fix the tests, now there should be 0 change in behaviour and error logging.

CompuIves avatar May 31 '22 09:05 CompuIves

Ah, there was still a test that verified an error message that I've changed and I hadn't signed all commits. Apologies for failing the tests each time, now it should work 😅

CompuIves avatar Jun 01 '22 10:06 CompuIves

Updated PR! 👍

CompuIves avatar Jun 15 '22 12:06 CompuIves

Squashed and signed! 👍

CompuIves avatar Jun 21 '22 10:06 CompuIves

@CompuIves you need to rebase your PR so that we can go ahead with the merge

dianpopa avatar Jul 07 '22 08:07 dianpopa

@CompuIves do you still want to merge this functionality? Thanks!

dianpopa avatar Sep 27 '22 13:09 dianpopa

I would really like to see this PR merged - @CompuIves do you plan to handle this in the near future or should I open a separate PR/somehow get permission to edit this one if possible? I think there is just the rebase missing.

ValentaTomas avatar Oct 11 '22 10:10 ValentaTomas

I would really like to see this PR merged - @CompuIves do you plan to handle this in the near future or should I open a separate PR/somehow get permission to edit this one if possible? I think there is just the rebase missing.

@ValentaTomas one way of getting this merged would be to cherry-pick this commit into a fork of yours and open a new PR with it. Make sure to add "Co-authored by" in the commit so that we make sure @CompuIves is also mentioned there. WDYT?

dianpopa avatar Oct 11 '22 15:10 dianpopa

Moved to #3180.

dianpopa avatar Oct 12 '22 15:10 dianpopa