feat(updater): support custom HTTP headers via updater.conf - #54
feat(updater): support custom HTTP headers via updater.conf#54ranganath42 wants to merge 3 commits into
Conversation
| return nil, err | ||
| } | ||
| req.Header.Set("User-Agent", userAgent()) | ||
| // Legacy updater-profile.conf headers are set first so that |
There was a problem hiding this comment.
Here, the header priority is enforced purely by call order.
Reviewers, do you think we should make it a bit more robust by introducing a merge function instead of relying on order? Not trivial, but not a lot of rewrok either.
e9ab958 to
f19a98f
Compare
| // Go's net/http manages these itself and ignores whatever is set on | ||
| // req.Header for them. Allowing them through would silently do nothing, | ||
| // which is a confusing trap for an operator configuring updater.conf. | ||
| var systemHeaderKeys = map[string]bool{ |
|
|
||
| const ( | ||
| headersFileName = "updater.conf" | ||
| maxHeadersFileBytes = 64 * 1024 |
There was a problem hiding this comment.
I assume these limits are intended to restrict the header size.
If such a restriction is to be implemented, I think it should either limit only the data size of the JSON file, or, instead of the file size limitation, calculate the number of headers, the size of keys and values when decoding the JSON.
If restrictions are applied to both, there is a possibility that a definition cannot be established because it fails to meet one of the restrictions. Wouldn't that cause confusion for users defining custom headers?
There was a problem hiding this comment.
Want me to drop the per-value/per-count limits and keep only the file-size limit?
There was a problem hiding this comment.
Envoy proxy does something similar.
It enforces hard byte-size limits (max_request_headers_kb) and max header counts (max_headers_count).
There was a problem hiding this comment.
If the new configuration file is used for other purposes as well, it may be difficult in the future to manage the size of custom headers using only the configuration file size. If flexibility is required, calculating the custom header size with low-level parsing of JSON data will be required.
| func AddCustomHeaders(req *http.Request) { | ||
| headers, err := loadCustomHeaders() | ||
| if err != nil { | ||
| if os.IsNotExist(err) { |
| // headerFieldNameRE matches a non-empty RFC 9110 token. | ||
| // That's the valid form for an HTTP field-name. | ||
| // See https://www.rfc-editor.org/rfc/rfc9110.html#section-5.6.2 | ||
| var headerFieldNameRE = regexp.MustCompile(`^[A-Za-z0-9!#$%&'*+\-.^_` + "`" + `|~]+$`) |
There was a problem hiding this comment.
It would be better to use "re" as a prefix like ErrSomething.
Also, it would be written using either of the following methods
regexp.MustCompile(`^[\w!#$%&'*+\-.^` + "`" + `|~]+$`)
regexp.MustCompile("^[\\w!#$%&'*+\\-.^`|~]+$")
| // See the project's LICENSE file for more information. | ||
| // | ||
|
|
||
| package update |
There was a problem hiding this comment.
It might be better to split into an internal pacakge, such as update/internal/customheaer.
|
|
||
| const ( | ||
| headersFileName = "updater.conf" | ||
| maxHeadersFileBytes = 64 * 1024 |
There was a problem hiding this comment.
If the new configuration file is used for other purposes as well, it may be difficult in the future to manage the size of custom headers using only the configuration file size. If flexibility is required, calculating the custom header size with low-level parsing of JSON data will be required.
2 participants
Custom HTTP headers via updater.conf
Adds an optional
updater.conffile next to the updater binary, letting operators attach custom HTTP headers (tenant IDs, routing tags, etc.) to update checks and downloads. No code changes needed per deployment.updater-profile.confheaders on collision, but cannot override headers describing the binary/host itself (User-Agent, OS headers, hop-by-hop headers).http.Get, no headers, no error status check).