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

Added code to track tainting of a tunnel. #155

Closed
wants to merge 2 commits into from
Closed

Added code to track tainting of a tunnel. #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c82ef4
Added code to track tainting of a tunnel.
cheftako Oct 24, 2020
67b05e9
Fixing test failures.
cheftako Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ bin:
.PHONY: build
build: bin/proxy-agent bin/proxy-server bin/proxy-test-client bin/http-test-server

bin/proxy-agent: proto/agent/agent.pb.go konnectivity-client/proto/client/client.pb.go bin cmd/agent/main.go
agent_targets := $(wildcard cmd/agent/*.go pkg/agent/*.go pkg/util/*.go)
bin/proxy-agent: proto/agent/agent.pb.go konnectivity-client/proto/client/client.pb.go bin $(agent_targets)
GO111MODULE=on go build -o bin/proxy-agent cmd/agent/main.go

bin/proxy-test-client: konnectivity-client/proto/client/client.pb.go bin cmd/client/main.go
Expand All @@ -69,7 +70,8 @@ bin/proxy-test-client: konnectivity-client/proto/client/client.pb.go bin cmd/cli
bin/http-test-server: bin cmd/test-server/main.go
GO111MODULE=on go build -o bin/http-test-server cmd/test-server/main.go

bin/proxy-server: proto/agent/agent.pb.go konnectivity-client/proto/client/client.pb.go bin cmd/server/main.go
server_targets := $(wildcard cmd/server/*.go pkg/server/*.go pkg/util/*.go)
bin/proxy-server: proto/agent/agent.pb.go konnectivity-client/proto/client/client.pb.go bin $(server_targets)
GO111MODULE=on go build -o bin/proxy-server cmd/server/main.go

## --------------------------------------
Expand Down
1 change: 1 addition & 0 deletions cmd/client/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ func (c *Client) run(o *GrpcProxyClientOptions) error {
time.Sleep(wait)
}
}
client.CloseIdleConnections()
Copy link
Member

Choose a reason for hiding this comment

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

What happens if client idle connections are not closed?

Copy link
Contributor Author

@cheftako cheftako Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In my tests it causes us to incorrectly taint tunnels. We then systematically work our way through the list of tunnels/backends until they are all tainted. At that point the code works the way it does today. So its not worse than today but it does mean your not getting any benefit out of the tainting tunnels/backends code.

Copy link
Member

Choose a reason for hiding this comment

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

The default IdleConnTimeout for golang's http library is 90 seconds, and almost all clients in k/k keep that parameter untouched.

I'm surprised that we get incorrectly tainted tunnels rather than just a really delayed CLOSE_REQ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

90 seconds is a really long time. With our test client (and no CloseIdleConnections call) I was seeing a context closed error on the front end connection after <10 seconds. That may just be the OS cleaning up the connection once the process went away.

Copy link
Member

Choose a reason for hiding this comment

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

That may just be the OS cleaning up the connection once the process went away.

Ahh I think that's exactly it. I remember adding a sleep interval at the end of of the client previously and it gracefully closed the connection after 90s was hit.

This line addition makes sense but we shouldn't make this assumption for all our clients. It's unlikely for the kube-apiserver process to be terminated so we probably wouldn't run into these context closed errors in k/k too often. However with the 90s idle time, that means the earliest time we'd be able to detect a problem on a connection is also 90s, which is already in the magnitude of minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be interested in seeing that happens with webhook calls which have <10 second http timeouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kubernetes/kubernetes#95981 would reduce that in general to 45s. (Still not great). I am also curious to see the behavior with webhook calls as they generally had a sub 10s http timeout.


return nil
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ func (p *Proxy) run(o *ProxyRunOptions) error {
KubernetesClient: k8sClient,
AuthenticationAudience: o.authenticationAudience,
}
server := server.NewProxyServer(o.serverID, int(o.serverCount), authOpt)
server := server.NewProxyServer(o.serverID, int(o.serverCount), authOpt, o.keepaliveTime)
klog.V(1).Infoln("Starting master server for client connections.")
masterStop, err := p.runMasterServer(ctx, o, server)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion cmd/test-server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func SetupSignalHandler() (stopCh <-chan struct{}) {
}

func returnSuccess(w http.ResponseWriter, req *http.Request) {
fmt.Fprintf(w, "<!DOCTYPE html>\n<html>\n <head>\n <title>Success</title>\n </head>\n <body>\n <p>The success test page!</p>\n </body>\n</html>")
fmt.Fprintf(w, "<!DOCTYPE html>\n<html>\n <head>\n <title>Success on Jura</title>\n </head>\n <body>\n <p>The success test page!</p>\n </body>\n</html>")
}

func returnError(w http.ResponseWriter, req *http.Request) {
Expand Down
7 changes: 3 additions & 4 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,20 @@ module sigs.k8s.io/apiserver-network-proxy
go 1.12

require (
github.com/golang/mock v1.4.0
github.com/golang/mock v1.4.4
github.com/golang/protobuf v1.4.2
github.com/google/uuid v1.1.1
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/prometheus/client_golang v1.7.1
github.com/spf13/cobra v0.0.3
github.com/spf13/pflag v1.0.5
golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3 // indirect
golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135 // indirect
golang.org/x/net v0.0.0-20191004110552-13f9640d40b9
google.golang.org/grpc v1.27.0
honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc // indirect
k8s.io/api v0.17.1
k8s.io/apimachinery v0.17.1
k8s.io/client-go v0.17.1
k8s.io/klog/v2 v2.0.0
rsc.io/quote/v3 v3.1.0 // indirect
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.0
)

Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ github.com/golang/mock v1.2.0 h1:28o5sBqPkBsMGnC6b4MvE2TzSr5/AT4c/1fLqVGIwlk=
github.com/golang/mock v1.2.0/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A=
github.com/golang/mock v1.4.0 h1:Rd1kQnQu0Hq3qvJppYSG0HtP+f5LPPUiDswTLiEegLg=
github.com/golang/mock v1.4.0/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw=
github.com/golang/mock v1.4.4 h1:l75CXGRSwbaYNpl/Z2X1XIIAMSCquvXgpVZDhwEIJsc=
github.com/golang/mock v1.4.4/go.mod h1:l3mdAwkq5BuhzHwde/uurv3sEJeZMXNpwsxVWU71h+4=
github.com/golang/protobuf v0.0.0-20161109072736-4bd1920723d7/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.2.0 h1:P3YflyNX/ehuJFLhxviNdFxQPkGK5cDcApsge1SqnvM=
github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
Expand Down
3 changes: 3 additions & 0 deletions konnectivity-client/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ go 1.13

require (
github.com/golang/protobuf v1.4.0
golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3 // indirect
golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135 // indirect
google.golang.org/grpc v1.27.0
honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc // indirect
k8s.io/klog/v2 v2.0.0
)
Loading