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.

Sign up for GitHub

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

Jump to bottom

user-data Support #171

Merged
merged 6 commits into from
Jun 6, 2022
Merged

user-data Support #171

merged 6 commits into from
Jun 6, 2022

Conversation

ihamburglar
Copy link
Contributor

Issue #170

Description of changes:
This PR is a work in progress. The idea is to be able to have the "/latest/user-data" path supported. As discussed in the issue it is a new content-type. I believe I have taken care of that.

The main issue currently is that I'm not an expert on interfaces. The parsing of the JSON is done with an interface, and the functions assume that there is something served in a file or sub-folder of the first file level, rather than a first level returning a response directly as in the case with user-data. This means that "/latest/user-data/a" is how you can access the contents of the user-data response that is stored in the JSON config, rather than "/latest/user-data". Any tips to help me figure out how to serve it as the path with just "latest" being the only folder in the path, would be appreciated.

The other thing is that the JSON config section for user-data is still formatted with the path/value trees as with the other paths. Would it make more sense for this section to be 'userdata : "/bin/bash aws sts get-caller-identity"' rather than the trees?

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

@ihamburglar ihamburglar requested a review from a team as a code owner May 28, 2022 03:25
@ihamburglar
Copy link
Contributor Author

I can also take a stab at local tests too, once I have it in a working state.

@brycahta brycahta self-requested a review May 31, 2022 15:22
@brycahta
Copy link
Contributor

brycahta commented Jun 1, 2022

@ihamburglar great start on the PR; appreciate it.

This means that "/latest/user-data/a" is how you can access the contents of the user-data response that is stored in the JSON config, rather than "/latest/user-data". Any tips to help me figure out how to serve it as the path with just "latest" being the only folder in the path, would be appreciated.

This is happening because of the userdata.ServicePath entry added to getHandlePairs. The ListRoutesHandler takes precedence over the Handler defined in pkg/mock/userdata/userdata.go and therefore never returns the data. The fix is to remove the userdata.ServicePath entry and update the config defaults to remove the trailing /a. I was able to get it working locally:

[1/06/22 11:07:24] ➜  ~ curl localhost:1338/latest/
dynamic
meta-data
user-data

[1/06/22 11:10:12] ➜  ~ curl localhost:1338/latest/user-data
/bin/bash aws sts get-caller-identity%

[1/06/22 11:10:14] ➜  ~ curl localhost:1338/latest/user-data/a
<?xml version="1.0" encoding="iso-8859-1"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
	"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
 <head>
  <title>404 - Not Found</title>
 </head>
 <body>
  <h1>404 - Not Found</h1>
 </body>
</html>
[1/06/22 11:10:29] ➜  ~

The other thing is that the JSON config section for user-data is still formatted with the path/value trees as with the other paths. Would it make more sense for this section to be 'userdata : "/bin/bash aws sts get-caller-identity"' rather than the trees?

It probably would make more sense, but not sure if that makes implementation any easier (without some refactoring). With that said, I'm okay keeping it consistent with meta-data and dynamic trees for now and handling the nuances for user data in code (assuming there aren't many).

brycahta
brycahta suggested changes Jun 1, 2022
View reviewed changes
pkg/cmd/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
pkg/config/types.go Outdated Show resolved Hide resolved
pkg/config/userdata.go Outdated Show resolved Hide resolved
pkg/config/userdata.go Outdated Show resolved Hide resolved
pkg/mock/userdata/types/types.go Outdated Show resolved Hide resolved
pkg/mock/userdata/userdata.go Outdated Show resolved Hide resolved
pkg/server/httpserver.go Outdated Show resolved Hide resolved
@ihamburglar
Copy link
Contributor Author

I am going to take a crack at writing some tests for this tonight.

I think the only other thing I thought of in terms of questions was what the default placeholder should be and how the userdata should be formatted in the JSON config. At first I was going to leave it empty, but because userdata is the only thing in the config file that is likely to be multi-line I left something in there that is multi-line to convey to the user how to add multiple lines with something innocuous. Currently I'm using new line characters like below. I think the better thing to do would be to base64 encode the data, and then decode at runtime or to have a JSON array with each item being a line. Let me know if I should do something better than new line characters.
"userdata": "#!/bin/bash \n #"

@ihamburglar
Copy link
Contributor Author

I have tried to add tests. Don't think I got it right, think I'm missing something.

@brycahta
Copy link
Contributor

brycahta commented Jun 2, 2022

I think the better thing to do would be to base64 encode the data, and then decode at runtime

+1. This would also align better with the way IMDS works because user data must be base64-encoded. However, this should be addressed in a separate PR-- let's focus on getting this path working+tested with a less complex value (i.e. no new lines).

One suggestion is the default value used in the documentation:

  • 1234,john,reboot,true | 4512,richard, | 173,,,

Would you mind opening the issue for base-64 encoding user data? I'd like to add some details in that thread such as making this value configurable via flag.

brycahta
brycahta reviewed Jun 2, 2022
View reviewed changes
pkg/mock/userdata/userdata.go Outdated Show resolved Hide resolved
pkg/server/httpserver.go Outdated Show resolved Hide resolved
@brycahta
Copy link
Contributor

brycahta commented Jun 2, 2022

I have tried to add tests. Don't think I got it right, think I'm missing something.

Let me try it locally with a simpler userdata value

brycahta
brycahta suggested changes Jun 2, 2022
View reviewed changes
pkg/mock/userdata/userdata.go Outdated Show resolved Hide resolved
test/e2e/cmd/userdata-test Outdated Show resolved Hide resolved
test/e2e/cmd/userdata-test Outdated Show resolved Hide resolved
@ihamburglar ihamburglar mentioned this pull request Jun 4, 2022
Closed
@ihamburglar
Copy link
Contributor Author

Made some changes. Let me know if I missed anything.

@brycahta
Copy link
Contributor

brycahta commented Jun 6, 2022

Made some changes. Let me know if I missed anything.

Looks like shellcheck tests are failing. At a glance, I think it's because the default value you're using, #!/bin/bash, is triggering the script in non-shell files. We can either add these files to an ignore list for shellcheck or change default value. I'm leaning towards the latter since the intent is to support base64 in the near future, which shouldn't contain this string.

After removing that line from RegisterHandlers, can you run make e2e-test locally? If all tests pass, then I'm good to ship+merge. I can own fixing the other ci/cd jobs failing prior to cutting a release.

@ihamburglar
Copy link
Contributor Author

I think tests are passing.

brycahta
brycahta reviewed Jun 6, 2022
View reviewed changes
pkg/config/userdata.go Outdated Show resolved Hide resolved
brycahta
brycahta approved these changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@brycahta brycahta left a comment

Choose a reason for hiding this comment

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

/lgtm , thanks!

@brycahta brycahta merged commit 0965dfc into aws:main Jun 6, 2022
@ihamburglar ihamburglar deleted the fr-user-data branch June 6, 2022 20:07
@brycahta brycahta mentioned this pull request Jun 13, 2022
Closed
This was referenced Jun 16, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brycahta brycahta brycahta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ihamburglar @brycahta