vozlt / nginx-module-vts

Nginx virtual host traffic status module
BSD 2-Clause "Simplified" License
3.24k stars 464 forks source link

Fix escaping filter_key in prometheus output #260

Closed u5surf closed 1 year ago

u5surf commented 1 year ago

https://github.com/vozlt/nginx-module-vts/issues/142#issuecomment-1431427669

u5surf commented 1 year ago

@jongiddy Hi, I heard you implement this function, please you could review on this patch🙏

u5surf commented 1 year ago

@jongiddy

I would suggest a fix to ngx_utf8_decode to detect more invalid UTF-8 characters,

I agree with your suggestion. Right away I send the patch to nginx-devel team. https://mailman.nginx.org/pipermail/nginx-devel/2023-February/34K6WOSI5JSIRESP32TY73EIIUHATOM5.html

After patch merged. to follow backwards compatibility, we suppose to prepare the branches between versions sometime soon.

jongiddy commented 1 year ago

Great. Thanks for creating the nginx patch.

Also, I retract problem 2 in my comment above. My initial reading was that it would replace an invalid character with \xff in the exposition format, which Prometheus does not recognize.

Taking a second look, it sends two backslashes (\\xff) which is valid in the exposition format and the final text shown to users will be \xfftest-api.

Maybe %FFtest-api would have been better. It would allow the code to allocate 3 bytes instead of 5 for the remaining characters. But I'm not sure it is worth changing behavior that someone might be expecting.

u5surf commented 1 year ago

Maybe %FFtest-api would have been better. It would allow the code to allocate 3 bytes instead of 5 for the remaining characters. But I'm not sure it is worth changing behavior that someone might be expecting.

@jongiddy I consider that your opinion is true in this aspect, but @axingblog @topicgit had said that the below behavior is not expecting because it can be occurred the invalid format error in promethes(https://github.com/vozlt/nginx-module-vts/issues/142#issuecomment-1278651670). I don't see wether it is true or not, once I heard to them what is happened but none of response from them(https://github.com/vozlt/nginx-module-vts/issues/142#issuecomment-1430555279). Thus they seem to hope that their character get to be encoding something expression format. So I choice the percent encoding which is adopted generally in RFC 2396 https://www.ietf.org/rfc/rfc2396.txt Also I was hearing in these points, but they had not any response yet(https://github.com/vozlt/nginx-module-vts/issues/142#issuecomment-1431438908).

u5surf commented 1 year ago

@jongiddy The nginx patch has merged and I consider that it may release next versions! https://github.com/nginx/nginx/commit/2c5fccd4693c0a68e1c72d65e016ba83e861120e For compatibility, We consider that it should return as an invalid character which starts with above 0xf8 following modified nginx decode rules. I changed this patch such that. Can you check it?