-
Notifications
You must be signed in to change notification settings - Fork 57
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.
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
Add Connect function #11
Changes from 1 commit
6ee8a5f
e7386db
635b0f9
8147e17
fad9258
23d286e
e2d2c14
155f487
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
package connection | ||
|
||
import ( | ||
"context" | ||
"net" | ||
"strings" | ||
"time" | ||
|
||
"github.com/golang/glog" | ||
"github.com/kubernetes-csi/csi-lib-utils/protosanitizer" | ||
"google.golang.org/grpc" | ||
) | ||
|
||
const ( | ||
// Interval of logging connection errors | ||
connectionLoggingInterval = 10 * time.Second | ||
) | ||
|
||
// Connect opens insecure gRPC connection to a CSI driver. Address must be either absolute path to a socket file | ||
// or have format '<protocol>://', following gRPC name resolution mechanism at https://github.com/grpc/grpc/blob/master/doc/naming.md. | ||
// The function tries to connect indefinitely every second until it connects. The function automatically adds | ||
// interceptor for gRPC message logging. | ||
func Connect(address string, dialOptions ...grpc.DialOption) (*grpc.ClientConn, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worthwhile to pass in a context? Then a caller can decide to abort after a certain time ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, the point of this function is to have the same behavior in all sidecar containers. Adding optional parameters works against this unification. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can have the same behavior in all sidecars by calling the function the same way, while still writing the underlying function in a more flexible way. But I get your point. Let me add another reason for passing in a context: when discussing glog vs. klog, it was mentioned that it would be good to pass in a logger via the context instead of using the same global instance. If we had that now, a unit test could select a mock logger to test that the warning messages are printed. But we don't have that now, so I'm fine with leaving out the context and adding it when really needed. We can deal with the API change in the sidecars on a case-by-case basis, should we do it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we move to klog, a test can add its own logging target to check that the right things are logged. IMO, the less options we offer to callers the better its usage is going to be. I removed |
||
dialOptions = append(dialOptions, | ||
grpc.WithInsecure(), // Don't use TLS, it's usually local Unix domain socket in a container. | ||
grpc.WithBackoffMaxDelay(time.Second), // Retry every second after failure. | ||
grpc.WithBlock(), // Block until connection succeeds. | ||
grpc.WithUnaryInterceptor(LogGRPC), // Log all messages. | ||
) | ||
if strings.HasPrefix(address, "/") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't handle an explicit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure? grpc/grpc-go#1741 was asking for this and then the feature request moved to grpc/grpc-go#1911, which is still open. I was under the impression that the custom dialer was used specifically to work around that shortcoming in gRPC. Perhaps I misunderstood. 🤷♂️ Looks like a good test case, don't you think? 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does indeed work. Then I don't understand what those open gRPC issues are about. It looks like the custom dialer is needed because an absolute path without the I'm still a bit wary about not always using that dialer, though. It creates two different code paths in our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added code that removes custom dialer and uses unix:// for all sockets. This changes behavior of existing sidecar containers that used custom dialer in old releases, IMO it should be fine. |
||
// It looks like filesystem path. | ||
dialOptions = append(dialOptions, grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) { | ||
return net.DialTimeout("unix", addr, timeout) | ||
})) | ||
} | ||
glog.Infof("Connecting to %s", address) | ||
|
||
// Connect in background. | ||
var conn *grpc.ClientConn | ||
var err error | ||
ready := make(chan bool) | ||
go func() { | ||
conn, err = grpc.Dial(address, dialOptions...) | ||
close(ready) | ||
}() | ||
|
||
// Log error every connectionLoggingInterval | ||
ticker := time.NewTicker(connectionLoggingInterval) | ||
defer ticker.Stop() | ||
|
||
// Wait until Dial() succeeds. | ||
for { | ||
select { | ||
case <-ticker.C: | ||
glog.Warningf("Still connecting to %s", address) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should move all repos to klog in one (short) burst. Is it the right time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but we should run it through CSI WG meeting. And add common logging initialization function to this library (as separate PR), so all the sidecar containers have the same cmdline arguments for logging. |
||
|
||
case <-ready: | ||
return conn, err | ||
} | ||
} | ||
} | ||
|
||
// LogGRPC is gPRC unary interceptor for logging of CSI messages at level 5. It removes any secrets from the message. | ||
func LogGRPC(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { | ||
glog.V(5).Infof("GRPC call: %s", method) | ||
glog.V(5).Infof("GRPC request: %s", protosanitizer.StripSecrets(req)) | ||
err := invoker(ctx, method, req, reply, cc, opts...) | ||
glog.V(5).Infof("GRPC response: %s", protosanitizer.StripSecrets(reply)) | ||
glog.V(5).Infof("GRPC error: %v", err) | ||
return err | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need the usual copyright and license boiler plate code at the beginning of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed