decode add a replace option

[saberuster]

as mentioned above #88 .

we can determine whether data is utf8 encode in another way，instead of raise a exception on reading stream.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[codecov-io]

Codecov Report

Merging #104 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #104   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files          13       13           
  Lines        1182     1182           
=======================================
  Hits         1088     1088           
  Misses         94       94

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 879ab01...15474ef. Read the comment docs.

[dbazhal]

can anyone approve this?

[saberuster]

/assign @roycaihw

[dbazhal]

Btw, don't know if it is correct to discuss here - i know that 'replace' is kind of solution, but it seems to me that it is incorrect to decode chunks of data without preliminary concatenation. For me it looks like chunk can really be broken - if it's bytes it's not necessary complete information about encoded symbol, but it may be just the first byte of two bytes total. In that case adding 'replace' could just corrupt data received by client.

[saberuster]

i am agree with your. but in my scene：
if i exec kubectl exec mypod cat /bin/my_binary i can recieve some data like this:

\����̋���܍��,  

if i use python-base , i get an exception when reading data. i want to do same things with kubectl ...
What do you think about this?
@dbazhal

[dbazhal]

if i exec kubectl exec mypod cat /bin/my_binary i can recieve some data like this:

\����̋���܍��,  

What do you think about this?
@dbazhal

I beleive kubectl throws raw binary data to output, but I'm not sure it tries to decode data partially. I assume correct solution to this wold be to merge chunks, then try to decode it with 'replace'. But I'm not sure, that's just guess, need to dig deeper -)

[roycaihw]

from the rfc I think we should decode utf8 if opcode is Text and return raw data if opcode is Binary

the previous implementation seems to be modeled from WebSocketApp but I don't see why we assumed utf8 for OPCODE_BINARY kubernetes-client/python#125 (comment). cc @mbohlool

[dbazhal]

from the rfc I think we should decode utf8 if opcode is Text and return raw data if opcode is Binary

the previous implementation seems to be modeled from WebSocketApp but I don't see why we assumed utf8 for OPCODE_BINARY kubernetes-client/python#125 (comment). cc @mbohlool

ok. as I understand, we should mark whole channel by the first frame opcode to treat all it's parts the same way? opcode isn't supposed to change fron frame to frame, right?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[dbazhal]

/remove-lifecycle rotten

[roycaihw]

I checked the websocket-client module that we depend on. It looks like the module has handled fragment concatenation for us already

our ws client invokes recv_data_frame

python-base/stream/ws_client.py

Line 177 in 1d5231c

op_code, frame = self.sock.recv_data_frame(True)

the websocket-client extracts the data type (text or binary) from the first frame, and accumulates following continuation frames into the payload. The client only returns the ws message when the last fragment (FIN) is received. Note that we've disabled fire_cont_frame and skip_utf8_validation already.

I don't think we need one more layer to concatenate multiple ws messages. websocket-client guarantees each text-type ws message can be decoded as utf8. It's just we shouldn't assume binary-type ws messages can be decoded in the same way

[dbazhal]

@roycaihw ok, that's great! Seems like there is no trouble with ws anymore.
But I think there is still trouble with watch. I tried to switch from my fork to kubernetes-9.0.0 and again got that same error

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xd0 in position 2047: unexpected end of data
<...>
  File "kubernetes/watch/watch.py", line 134, in stream
    for line in iter_resp_lines(resp):
  File "kubernetes/watch/watch.py", line 49, in iter_resp_lines
    seg = seg.decode('utf8')

So it looks like this concrete PR is outdated, but could you please take a look at #112 especially https://github.com/kubernetes-client/python-base/pull/112/files#diff-7768487132d1edef2f3cb4e8d4101890 ?

[dbazhal]

@roycaihw I created separate PR on that #138

[roycaihw]

This PR fixes a bug (not honoring opcode when decoding) in our ws client. I haven't convinced myself if we want to decode binary message using utf8, but this change is still better than failing

/lgtm
/approve

@dbazhal Watch doesn't use ws so that error should be unrelated. I will review #138

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roycaihw, saberuster

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [roycaihw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment