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.

Sign up for GitHub

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

Jump to bottom

improve remotecommand testing fuzzing the data stream #116191

Merged
merged 1 commit into from Apr 26, 2023
Merged

improve remotecommand testing fuzzing the data stream #116191

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 61 additions & 0 deletions staging/src/k8s.io/client-go/tools/remotecommand/remotecommand_test.go
View file
Open in desktop
Expand Up @@ -17,10 +17,13 @@ limitations under the License.
package remotecommand

import (
"bytes"
"context"
"crypto/rand"
"encoding/json"
"errors"
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"net/url"
Expand Down Expand Up @@ -365,3 +368,61 @@ func TestStreamExitsAfterConnectionIsClosed(t *testing.T) {
return
}
}

func TestStreamRandomData(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
var stdin, stdout bytes.Buffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use https://pkg.go.dev/io#Pipe instead?

Copy link
Member Author

@aojea aojea Mar 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, I started with that approach but the race detector complains , so I need to create these buffers all over the test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's surprising, I think this is what io.Pipe is meant to do. Is it convenient to show me the code that didn't work?

The current code of reading everything and then writing everything probably doesn't exercise as many code paths as interleaving reads and writes would.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, now that I look, these buffers are not used the logic inside createHTTPStreams (copied from the kubelet code IIRC) uses it just to identify the number of streams expected.

only the output streams are used, and if you use io.Copy or a io.Pipe to plumb those together the race detector complains

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, that's extremely bizarre, I read the createHTTPStreams function, it's super weird.

The streams here seem to be from the spdy library, right? Using io.Copy sounds like it should work to me. I'm actually concerned that the race checker is correct and we may have the same problem in production code. Can you post a stack trace from the race checker?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, this works without race, I do not know if there was some change or if I made the wrong test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

tress ./remotecommand.test -test.run TestStreamRandomData
5s: 1410 runs so far, 0 failures
10s: 2838 runs so far, 0 failures
15s: 4273 runs so far, 0 failures
20s: 5695 runs so far, 0 failures
25s: 7136 runs so far, 0 failures
30s: 8564 runs so far, 0 failures
35s: 9980 runs so far, 0 failures
40s: 11390 runs so far, 0 failures
45s: 12816 runs so far, 0 failures

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavalamp please take a look and get this merged to increase the coverage

ctx, err := createHTTPStreams(w, req, &StreamOptions{
Stdin: &stdin,
Stdout: &stdout,
})
if err != nil {
t.Errorf("error on createHTTPStreams: %v", err)
return
}
defer ctx.conn.Close()

io.Copy(ctx.stdoutStream, ctx.stdinStream)
}))

defer server.Close()

uri, _ := url.Parse(server.URL)
exec, err := NewSPDYExecutor(&rest.Config{Host: uri.Host}, "POST", uri)
if err != nil {
t.Fatal(err)
}

randomData := make([]byte, 1024*1024)
if _, err := rand.Read(randomData); err != nil {
t.Errorf("unexpected error reading random data: %v", err)
}
var stdout bytes.Buffer
options := &StreamOptions{
Stdin: bytes.NewReader(randomData),
Stdout: &stdout,
}
errorChan := make(chan error)
go func() {
errorChan <- exec.StreamWithContext(context.Background(), *options)
}()

select {
case <-time.After(wait.ForeverTestTimeout):
t.Fatalf("expect stream to be closed after connection is closed.")
case err := <-errorChan:
if err != nil {
t.Errorf("unexpected error")
}
}

data, err := ioutil.ReadAll(bytes.NewReader(stdout.Bytes()))
if err != nil {
t.Errorf("error reading the stream: %v", err)
return
}
if !bytes.Equal(randomData, data) {
t.Errorf("unexpected data received: %d sent: %d", len(data), len(randomData))
}

}