kubernetes-client-php
kubernetes-client-php copied to clipboard
travisghansen
Default to not escaping forward slashes in JSON
Hi me again,
This patch updates the default JSON encode flags to disable escaping forward slashes. (By default, json_encode("my/string") -> my\/string.)
This appears to be mainly a PHP oddity to reduce the risk when embedding the JSON in a web page. It prevents a string containing something like </script> from interacting with the DOM. This doesn't really apply at all anywhere in this library.
However, in trying to make use of server-side apply I did discover that while my\/string is valid JSON, it is not valid YAML. So, contrary to the k8s docs' statement that "All JSON documents are valid YAML." I was unable to apply a patch with typical URI-style tags. my.domain/tag: value would be encoded to "my.domain\/tag": "value", and the patch would fail with:
error decoding YAML: error converting YAML to JSON: yaml: found unknown escape character
The JSON generated is valid YAML per RFC8259, however this seems to be an open and longstanding bug in the go-yaml parser used by kubernetes (and shared by yq which exhibits similar behaviour). Probably easier to work around it here rather than waiting for them to merge the PRs that have been sitting for a couple years.
There's no case for escaping forward-slashes outside of a document sent to a browser, and there are definitely cases where it causes issues talking to k8s. So while the caller can specify these options, it seems that making this one a default could make things less surprising.
Thank you!
Adam
Good call, that's probably the smarter path. I'm sure I'd just be back here later after finding some other edge case. May as well skip the end of that process and just send YAML.
Should be able to get something over for that later or tomorrow.
Any objections to me widening up the version constraint on symfony/yaml if no issues show up while doing this? (E.g., ^6.3|^7.0.) These days a fresh Laravel install ends up with 7.x installed. It can be downgraded without conflict, but looking at the commit history on that package it doesn't look like they've changed anything that would impact this library.
EDIT: Sent as PR #13. I'll leave it to you to decide whether or not to close this one. From my end, the other PR solves all of my issues.
Been thinking about this one further and would actually like to do this deeper and force that option. In either scenario I think we revert the specific change you made in this PR. After reverting the change we rework that same function to force the bitwise value with the relevant option before returning (vs setting it as a default as you have implemented)…ie: introduce some forced value(s) for that specific option. Either we duplicate the same logic 3 times (before each return statement) or change the flow control logic to only have a single return and implement the logic once.
https://github.com/travisghansen/kubernetes-client-php/commit/2c14daab48d20a4b96455cc3ef12d62e7ee08ea9