Change default metadata endpoint handling #49

mmlb · 2020-11-05T18:30:39Z

Description

Change the handling the /metadata endpoint so the data returned is of the same content as the /meta-data EC2 style endpoint.

Why is this needed

Hegel is meant as a replacement for what is used in EM Production today (Kant). The response from /metadata and /meta-data Kant returns is the same content. Its also arguably confusing to have 2 similarly named endpoints return very different data. This confusion is in part due to not having an Instance object yet expecting instance data somewhere. This should be cleaned up/made to make sense by tinkerbell/proposals#25.

How Has This Been Tested?

Unit tests still pass.

How are existing users impacted? What migration steps/scripts do we need?

This is a bc-break. If any users were using the /metadata endpoint in its default configuration their workflow would be broken. I'd like to wait and see if thats the case before prematurely having any workarounds.

codecov · 2020-11-05T18:32:29Z

Codecov Report

Merging #49 (4970c17) into master (0cec86e) will increase coverage by 0.45%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   23.95%   24.40%   +0.45%     
==========================================
  Files           5        5              
  Lines         501      504       +3     
==========================================
+ Hits          120      123       +3     
  Misses        359      359              
  Partials       22       22

Impacted Files	Coverage Δ
http_server.go	`0.00% <0.00%> (ø)`
mock.go	`72.22% <ø> (ø)`
main.go	`9.00% <38.46%> (+2.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cec86e...4970c17. Read the comment docs.

kqdeng · 2020-11-05T20:43:20Z

main.go

-		return errors.Wrap(err, "error in parsing custom endpoints")
+		err = errors.Wrap(err, "error in parsing custom endpoints")
+		logger.Fatal(err)
+		panic(err)


why are both Fatal and panic used here? I thought fatal already exits the program?

yep, thats left over from when I copy/pasted. Will fix.

fixed, PTAL

kqdeng · 2020-11-05T21:23:13Z

http_server_test.go

-	defer os.Setenv("CUSTOM_ENDPOINTS", customEndpoints)
-	os.Setenv("CUSTOM_ENDPOINTS", `{"/metadata":".metadata.instance","/components":".metadata.components","/userdata":".metadata.userdata"}`)
-
+	customEndPoints := map[string]string{


super nitpicky i know, but can you lower case the the p here (customEndpoints)?

fixed, PTAL

kqdeng · 2020-11-05T21:49:55Z

http_server_test.go

-		status:          404,
-		error:           "error in parsing custom endpoints: invalid character ':' after top-level value",
-		json:            tinkerbellDataModel,
-	},


what's the reasoning for removing these two cases?
shouldn't we have a case that tests the default? and the second one is for covering the bases of all the possible invalid envvar formats

I had originally done the CUSTOM_ENDPOINTS parsing in func main() and so removed it. But I didn't want to affect codecoverage too much so put it behind the func we have now. But I forgot to bring this test back in. Will fix.

fixed, PTAL. I left the default one out as thats done in func main() now, as it should be :D.

kqdeng · 2020-11-05T21:53:54Z

With this change, would it make sense that we add in a CUSTOM_ENDPOINTS: '{"/metadata":".metadata"}' in tink's docker-compose file? (I think James @splaspood also mentioned this earlier but I could be wrong.)

I understand the reasoning behind this change but I'm still a little e_e.
But with this^ we can omit the envvar when we bring up hegel, but for the open source community who (I assume) mostly use the docker compose file, they can "default" to "/metadata": ".metadata"? What do you think?

mmlb · 2020-11-05T22:03:23Z

Yeah that is a good option. I kind of would rather merge this as is though and see if there are any problems with it. I'm not sure if any workflows are even using hegel/metadata (https://deploy-preview-162--suspicious-jackson-6c9a73.netlify.app/blog/2020/10/provisioning-flatcar-container-linux-with-tinkerbell/ just embeds the ignition straight into the template). So I'd rather err on the side of /metadata is same as /meta-data unless we need to come up with a work around.

mmlb · 2020-11-06T19:31:16Z

alrighty @kdeng3849 I think everything should all set now.

Signed-off-by: Manuel Mendez <[email protected]>

This is more concise. Signed-off-by: Manuel Mendez <[email protected]>

This is easier to track down failures with. Signed-off-by: Manuel Mendez <[email protected]>

main is the correct place to parse out env vars and command line flags. Signed-off-by: Manuel Mendez <[email protected]>

So we have nicely aligned and sorted json. Signed-off-by: Manuel Mendez <[email protected]>

…nterface{} When the default handling for /metadata changes in an upcoming commit unmarshalling the json to packet.Metadata is not going to work. So we change the unmarshalling destination here instead so the next commit is doing only one thing. Signed-off-by: Manuel Mendez <[email protected]>

This is so we can avoid confusion when taking into account the handling of /meta-data. In EM Production today (Kant) /meta-data and /metadata both have the same output data. Making Hegel match by default is in line with the goal of Hegel as a Kant replacement. This is a bc-break and there's no current way to fetch the full hardware.metadata json field, but I'm ok with that. This should all go away when the Instance Proposal is implemented. Signed-off-by: Manuel Mendez <[email protected]>

mmlb added the breaking-change Denotes a PR that introduces potentially breaking changes that require user action. label Nov 5, 2020

mmlb requested review from kqdeng and a team November 5, 2020 18:30

mmlb force-pushed the change-default-metadata-endpoint-handling branch 2 times, most recently from 41a4be5 to a132fc1 Compare November 5, 2020 18:31

kqdeng reviewed Nov 5, 2020

View reviewed changes

mmlb force-pushed the change-default-metadata-endpoint-handling branch from a132fc1 to a2fe424 Compare November 5, 2020 23:01

mmlb requested a review from kqdeng November 5, 2020 23:02

mmlb force-pushed the change-default-metadata-endpoint-handling branch from a2fe424 to c2383c7 Compare November 6, 2020 19:31

kqdeng previously approved these changes Nov 6, 2020

View reviewed changes

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Nov 6, 2020

mmlb removed the request for review from a team November 6, 2020 21:19

mmlb added 7 commits November 6, 2020 16:19

Tidy up the go.sum file

15b3759

Signed-off-by: Manuel Mendez <[email protected]>

Use logger.Fatal instead of logger.Error + panic

82c1062

This is more concise. Signed-off-by: Manuel Mendez <[email protected]>

Convert http_server_tests to subtests

e084634

This is easier to track down failures with. Signed-off-by: Manuel Mendez <[email protected]>

Move customEndpoint parsing into main.go

b4495d9

main is the correct place to parse out env vars and command line flags. Signed-off-by: Manuel Mendez <[email protected]>

Pass mock json through jq -S

d5640c0

So we have nicely aligned and sorted json. Signed-off-by: Manuel Mendez <[email protected]>

mmlb dismissed kqdeng’s stale review via 4970c17 November 6, 2020 21:19

mmlb force-pushed the change-default-metadata-endpoint-handling branch from c2383c7 to 4970c17 Compare November 6, 2020 21:19

mmlb requested a review from kqdeng November 6, 2020 21:20

kqdeng approved these changes Nov 6, 2020

View reviewed changes

mergify bot merged commit 1583c90 into tinkerbell:master Nov 6, 2020

mmlb deleted the change-default-metadata-endpoint-handling branch November 6, 2020 21:33

qmfrederik mentioned this pull request Dec 9, 2020

React to Tinkerbell metadata changes fransvanberckel/ubuntu-workflow#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default metadata endpoint handling #49

Change default metadata endpoint handling #49

mmlb commented Nov 5, 2020

codecov bot commented Nov 5, 2020 •

edited

Loading

kqdeng Nov 5, 2020

mmlb Nov 5, 2020

mmlb Nov 5, 2020

kqdeng Nov 5, 2020

mmlb Nov 5, 2020

mmlb Nov 5, 2020

kqdeng Nov 5, 2020

mmlb Nov 5, 2020

mmlb Nov 5, 2020

kqdeng commented Nov 5, 2020

mmlb commented Nov 5, 2020

mmlb commented Nov 6, 2020

Change default metadata endpoint handling #49

Change default metadata endpoint handling #49

Conversation

mmlb commented Nov 5, 2020

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

codecov bot commented Nov 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqdeng commented Nov 5, 2020

mmlb commented Nov 5, 2020

mmlb commented Nov 6, 2020

codecov bot commented Nov 5, 2020 •

edited

Loading