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

Clarify documentation of RequestHeaderModifier .set and .add behaviour #3314

Open
FredrikAugust opened this issue Sep 4, 2024 · 4 comments
Labels
kind/documentation Categorizes issue or PR as related to documentation. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@FredrikAugust
Copy link

FredrikAugust commented Sep 4, 2024

What would you like to be added:

The documentation for set currently reads:

To edit an existing header, use the set action and specify the value of the header to be modified and the new header value to be set.

However, according to the implementation by Traefik it actually sets it regardless of it exists or not. Go playground

This also goes for the add functionality, which according to the implementation in Traefik will add values iteratively, i.e. if an incoming request already has header x-header set to value and you add the add filter with value2 the actual header value will be value,value2. This is just a clarification I think could be useful to add.

If this is a wrong implementation by Traefik then let me know and I'll move the issue over there.

Why this is needed:

To avoid confusion when using the filters.

@FredrikAugust FredrikAugust added the kind/documentation Categorizes issue or PR as related to documentation. label Sep 4, 2024
@youngnick
Copy link
Contributor

I think the issue here is that we haven't been clear enough about the expected behavior.

set is supposed to unconditionally set that header to that value and only that value. So, if the header is not present, then the header will be added, set to the specified value. If values are already set, they will be overridden with the specified value.

That's how you can edit a header that's already existing - set will do that if it's already set, with the side effect that it will set it if unset.

add is supposed add to values, setting them if unset, and adding comma-separated values to an existing header if the header already exists.

If you are looking for "edit this value if present and don't do anything if not", I don't think we have a way to do that at the moment.

From what I can see above, Traefik is behaving according to the spec.

@FredrikAugust
Copy link
Author

That's what I was expecting @youngnick, and I'm glad to hear that this is the intended behaviour. Then I think it should just be clarified in the docs, as it now reads:

To edit an existing header, use the set action and specify the value of the header to be modified and the new header value to be set.

Which for me implies that set does not unconditionally set, but rather only sets if it is already present.

@FredrikAugust FredrikAugust changed the title Clarify RequestHeaderModifier .set and .add behaviour Clarify documentation of RequestHeaderModifier .set and .add behaviour Sep 16, 2024
@youngnick
Copy link
Contributor

Yes, I agree that line could be changed to something more like:

To edit an existing header, use the fact that set unconditionally sets a header to override whatever is in that header using the same header name and the new value.

or something.

Does that make more sense to you @FredrikAugust?

@FredrikAugust
Copy link
Author

FredrikAugust commented Sep 20, 2024

It stills appears a little self-contradictory to me as the first sentence reads "existing header" which implies that the header has to be there in the first place — which we have established is not the case.

What about:

To unconditionally set the value of a header, use set. It will override the value of a header with the provided value if the header is already present, or set it to the provided value if the header does not exist.

@shaneutt shaneutt added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Status: Triage
Development

No branches or pull requests

3 participants