From a77165f48adb0e2c87aa18eb2fbbbd48788580cb Mon Sep 17 00:00:00 2001 From: Anna Fang Date: Fri, 1 Sep 2023 15:32:34 +0000 Subject: [PATCH 1/4] fix: let proxy continue to run with inactive instance(s) (#1854) Replaced fmt.Errorf with logger.Errorf for instance failure Fail the inactive instance with unix path Fixed lint error Added inactive instance test with unix path Replace getNetworkType with networkType Fixed the error path indentation Make the unix path failure test a seperate func Fail the proxy binary if the tcp socket argument is wrong --- internal/proxy/proxy.go | 32 ++++++++---- internal/proxy/proxy_test.go | 97 ++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 9 deletions(-) diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index fd5a68fbc..bd1b9eebd 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -510,19 +510,25 @@ func NewClient(ctx context.Context, d cloudsql.Dialer, l cloudsql.Logger, conf * for _, inst := range conf.Instances { m, err := c.newSocketMount(ctx, conf, pc, inst) if err != nil { - for _, m := range mnts { - mErr := m.Close() - if mErr != nil { - l.Errorf("failed to close mount: %v", mErr) - } + c.logger.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) + if networkType(conf, inst) == "unix" { + continue + } + switch err.(type) { + // this is not the inactive instance case. Fail the proxy + // so that the user could fix the proxy command error. + case *net.OpError: + return nil, fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) } - return nil, fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) } l.Infof("[%s] Listening on %s", inst.Name, m.Addr()) mnts = append(mnts, m) } c.mnts = mnts + if len(mnts) == 0 { + return nil, fmt.Errorf("No active instance to mount") + } return c, nil } @@ -747,6 +753,14 @@ type socketMount struct { dialOpts []cloudsqlconn.DialOption } +func networkType(conf *Config, inst InstanceConnConfig) string { + if (conf.UnixSocket == "" && inst.UnixSocket == "" && inst.UnixSocketPath == "") || + (inst.Addr != "" || inst.Port != 0) { + return "tcp" + } + return "unix" +} + func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfig, inst InstanceConnConfig) (*socketMount, error) { var ( // network is one of "tcp" or "unix" @@ -765,8 +779,7 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi // instance) // use a TCP listener. // Otherwise, use a Unix socket. - if (conf.UnixSocket == "" && inst.UnixSocket == "" && inst.UnixSocketPath == "") || - (inst.Addr != "" || inst.Port != 0) { + if networkType(conf, inst) == "tcp" { network = "tcp" a := conf.Addr @@ -784,7 +797,6 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi version, err := c.dialer.EngineVersion(ctx, inst.Name) if err != nil { c.logger.Errorf("could not resolve version for %q: %v", inst.Name, err) - return nil, err } np = pc.nextDBPort(version) } @@ -801,6 +813,8 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi address, err = newUnixSocketMount(inst, conf.UnixSocket, strings.HasPrefix(version, "POSTGRES")) if err != nil { + c.logger.Errorf("could not mount unix socket %q for %q: %v", + conf.UnixSocket, inst.Name, err) return nil, err } } diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index 8d509295e..db0b3b504 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -17,6 +17,7 @@ package proxy_test import ( "context" "errors" + "fmt" "io" "net" "os" @@ -83,6 +84,8 @@ func (f *fakeDialer) EngineVersion(_ context.Context, inst string) (string, erro return "MYSQL_8_0", nil case strings.Contains(inst, "sqlserver"): return "SQLSERVER_2019_STANDARD", nil + case strings.Contains(inst, "fakeserver"): + return "", fmt.Errorf("non existing server") default: return "POSTGRES_14", nil } @@ -306,6 +309,25 @@ func TestClientInitialization(t *testing.T) { filepath.Join(testUnixSocketPathPg), }, }, + { + desc: "without TCP port or unix socket for non functional instance", + in: &proxy.Config{ + Instances: []proxy.InstanceConnConfig{ + {Name: "proj:region:fakeserver"}, + }, + }, + }, + { + desc: "with TCP port for non functional instance", + in: &proxy.Config{ + Instances: []proxy.InstanceConnConfig{ + {Name: "proj:region:fakeserver", Port: 50000}, + }, + }, + wantTCPAddrs: []string{ + "127.0.0.1:50000", + }, + }, } for _, tc := range tcs { @@ -711,3 +733,78 @@ func TestRunConnectionCheck(t *testing.T) { } } + +func TestProxyInitializationWithFailedUnixSocket(t *testing.T) { + ctx := context.Background() + testDir, _ := createTempDir(t) + testUnixSocketPath := path.Join(testDir, "db") + + tcs := []struct { + desc string + in *proxy.Config + }{ + { + desc: "with unix socket for non functional instance", + in: &proxy.Config{ + Instances: []proxy.InstanceConnConfig{ + { + Name: "proj:region:fakeserver", + UnixSocketPath: testUnixSocketPath, + }, + }, + }, + }, + } + for _, tc := range tcs { + t.Run(tc.desc, func(t *testing.T) { + _, err := proxy.NewClient(ctx, &fakeDialer{}, testLogger, tc.in) + if err == nil { + t.Fatalf("want non nil error, got = %v", err) + } + }) + } +} + +func TestProxyMultiInstances(t *testing.T) { + ctx := context.Background() + testDir, _ := createTempDir(t) + testUnixSocketPath := path.Join(testDir, "db") + + tcs := []struct { + desc string + in *proxy.Config + wantSuccess bool + }{ + { + desc: "with tcp socket and unix for non functional instance", + in: &proxy.Config{ + Instances: []proxy.InstanceConnConfig{ + { + Name: "proj:region:fakeserver", + UnixSocketPath: testUnixSocketPath, + }, + {Name: mysql, Port: 3306}, + }, + }, + wantSuccess: true, + }, + { + desc: "with two tcp socket instances", + in: &proxy.Config{ + Instances: []proxy.InstanceConnConfig{ + {Name: "proj:region:fakeserver", Port: 60000}, + {Name: mysql, Port: 60000}, + }, + }, + wantSuccess: false, + }, + } + for _, tc := range tcs { + t.Run(tc.desc, func(t *testing.T) { + _, err := proxy.NewClient(ctx, &fakeDialer{}, testLogger, tc.in) + if tc.wantSuccess != (err == nil) { + t.Fatalf("want return = %v, got = %v", tc.wantSuccess, err == nil) + } + }) + } +} From 2c7bb3fc6e7a89d546dfbc3423b1debdce0293a2 Mon Sep 17 00:00:00 2001 From: Anna Fang Date: Tue, 19 Sep 2023 01:40:33 +0000 Subject: [PATCH 2/4] Make consistent error logging and fail the inactive instance without port config --- internal/proxy/proxy.go | 26 +++++++++++++++----------- internal/proxy/proxy_test.go | 16 ++++++++-------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index bd1b9eebd..f20589009 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -510,20 +510,22 @@ func NewClient(ctx context.Context, d cloudsql.Dialer, l cloudsql.Logger, conf * for _, inst := range conf.Instances { m, err := c.newSocketMount(ctx, conf, pc, inst) if err != nil { - c.logger.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) - if networkType(conf, inst) == "unix" { - continue - } switch err.(type) { // this is not the inactive instance case. Fail the proxy // so that the user could fix the proxy command error. case *net.OpError: + for _, m := range mnts { + mErr := m.Close() + if mErr != nil { + l.Errorf("[%v] failed to close mount: %v", m.inst, mErr) + } + } return nil, fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) } + } else { + l.Infof("[%s] Listening on %s", inst.Name, m.Addr()) + mnts = append(mnts, m) } - - l.Infof("[%s] Listening on %s", inst.Name, m.Addr()) - mnts = append(mnts, m) } c.mnts = mnts if len(mnts) == 0 { @@ -795,8 +797,10 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi np = pc.nextPort() default: version, err := c.dialer.EngineVersion(ctx, inst.Name) + // Exit if the port is not specified for inactive instance if err != nil { - c.logger.Errorf("could not resolve version for %q: %v", inst.Name, err) + c.logger.Errorf("[%v] could not resolve instance version: %v", inst.Name, err) + return nil, err } np = pc.nextDBPort(version) } @@ -807,14 +811,13 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi version, err := c.dialer.EngineVersion(ctx, inst.Name) if err != nil { - c.logger.Errorf("could not resolve version for %q: %v", inst.Name, err) + c.logger.Errorf("[%v] could not resolve instance version: %v", inst.Name, err) return nil, err } address, err = newUnixSocketMount(inst, conf.UnixSocket, strings.HasPrefix(version, "POSTGRES")) if err != nil { - c.logger.Errorf("could not mount unix socket %q for %q: %v", - conf.UnixSocket, inst.Name, err) + c.logger.Errorf("[%v] could not mount unix socket %q: %v", inst.Name, conf.UnixSocket, err) return nil, err } } @@ -822,6 +825,7 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi lc := net.ListenConfig{KeepAlive: 30 * time.Second} ln, err := lc.Listen(ctx, network, address) if err != nil { + c.logger.Errorf("[%v] could not listen to address %v: %v", inst.Name, address, err) return nil, err } // Change file permissions to allow access for user, group, and other. diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index db0b3b504..9efa86212 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -309,14 +309,6 @@ func TestClientInitialization(t *testing.T) { filepath.Join(testUnixSocketPathPg), }, }, - { - desc: "without TCP port or unix socket for non functional instance", - in: &proxy.Config{ - Instances: []proxy.InstanceConnConfig{ - {Name: "proj:region:fakeserver"}, - }, - }, - }, { desc: "with TCP port for non functional instance", in: &proxy.Config{ @@ -754,6 +746,14 @@ func TestProxyInitializationWithFailedUnixSocket(t *testing.T) { }, }, }, + { + desc: "without TCP port or unix socket for non functional instance", + in: &proxy.Config{ + Instances: []proxy.InstanceConnConfig{ + {Name: "proj:region:fakeserver"}, + }, + }, + }, } for _, tc := range tcs { t.Run(tc.desc, func(t *testing.T) { From 31e096ccd6dd9f6d4c78dfe5f9d20bb0979d02b0 Mon Sep 17 00:00:00 2001 From: Anna Fang Date: Tue, 19 Sep 2023 01:47:14 +0000 Subject: [PATCH 3/4] Added an example of the case where listen call fails --- internal/proxy/proxy.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index f20589009..ed6e454cf 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -513,6 +513,8 @@ func NewClient(ctx context.Context, d cloudsql.Dialer, l cloudsql.Logger, conf * switch err.(type) { // this is not the inactive instance case. Fail the proxy // so that the user could fix the proxy command error. + // for example: two instances with the same port will fail + // in the listen call. case *net.OpError: for _, m := range mnts { mErr := m.Close() From 0d280127a6fc79a9eb4aca17b7bde2831d19c6c2 Mon Sep 17 00:00:00 2001 From: Anna Fang Date: Fri, 22 Sep 2023 20:41:05 +0000 Subject: [PATCH 4/4] Revert the logic change for failing the inactive instances --- internal/proxy/proxy.go | 26 ++++++++------------------ internal/proxy/proxy_test.go | 4 ++-- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index ed6e454cf..89c471910 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -510,29 +510,19 @@ func NewClient(ctx context.Context, d cloudsql.Dialer, l cloudsql.Logger, conf * for _, inst := range conf.Instances { m, err := c.newSocketMount(ctx, conf, pc, inst) if err != nil { - switch err.(type) { - // this is not the inactive instance case. Fail the proxy - // so that the user could fix the proxy command error. - // for example: two instances with the same port will fail - // in the listen call. - case *net.OpError: - for _, m := range mnts { - mErr := m.Close() - if mErr != nil { - l.Errorf("[%v] failed to close mount: %v", m.inst, mErr) - } + for _, m := range mnts { + mErr := m.Close() + if mErr != nil { + l.Errorf("failed to close mount: %v", mErr) } - return nil, fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) } - } else { - l.Infof("[%s] Listening on %s", inst.Name, m.Addr()) - mnts = append(mnts, m) + return nil, fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err) } + + l.Infof("[%s] Listening on %s", inst.Name, m.Addr()) + mnts = append(mnts, m) } c.mnts = mnts - if len(mnts) == 0 { - return nil, fmt.Errorf("No active instance to mount") - } return c, nil } diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index 9efa86212..807a27264 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -786,10 +786,10 @@ func TestProxyMultiInstances(t *testing.T) { {Name: mysql, Port: 3306}, }, }, - wantSuccess: true, + wantSuccess: false, }, { - desc: "with two tcp socket instances", + desc: "with two tcp socket instances and conflicting ports", in: &proxy.Config{ Instances: []proxy.InstanceConnConfig{ {Name: "proj:region:fakeserver", Port: 60000},