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

tftp: make file filename mandatory and anchor the regex to the end of line too #16

Open
rgl opened this issue Dec 20, 2021 · 2 comments

Comments

@rgl
Copy link

rgl commented Dec 20, 2021

I think the following regex should be modified to ^(.+)-[[:xdigit:]]{2}-([[:xdigit:]]{32})-([[:xdigit:]]{16})-([[:xdigit:]]{2})$:

https://github.com/tinkerbell/boots-ipxe/blob/e9e36bd85356785cfeaf9e2f340d5a751f5b1708/tftp/tftp.go#L123

I'm also curious, is there a reason for not using a query string alike syntax? for example, filename?traceparent=xxx?

@jacobweinstock
Copy link
Member

Hey @rgl, thanks for the feedback. I'm curious about "make file filename mandatory". Would you mind expanding on this idea?

In regard to "anchor the regex to the end of line", what do you think we gain by capping the name in this way?

"is there a reason for not using a query string alike syntax?"
The reason is that this code was taken from the work already done to enable otel in Boots proper. https://github.com/tinkerbell/boots/blob/129caee6916a2ef9a5cb4af216141654b0bc6e77/cmd/boots/tftp.go#L120

@rgl
Copy link
Author

rgl commented Dec 21, 2021

Hey @rgl, thanks for the feedback. I'm curious about "make file filename mandatory". Would you mind expanding on this idea?

the regex .* means its optional, that is, it matches zero or more characters.

In regard to "anchor the regex to the end of line", what do you think we gain by capping the name in this way?

prevent it from being interpreted as something else? that way, the id would be always at the end of the string. not at the middle.

"is there a reason for not using a query string alike syntax?" The reason is that this code was taken from the work already done to enable otel in Boots proper. https://github.com/tinkerbell/boots/blob/129caee6916a2ef9a5cb4af216141654b0bc6e77/cmd/boots/tftp.go#L120

I'm not sure how picky the tftp clients are, but stashing an id in a filename using a regular char (-) is quite odd for me; I would prefer a more structured way (like a query string).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants