Skip to content

Commit 279622e

Browse files
WindzCUHKmlajkim
WindzCUHK
and
mlajkim
authored
fix error & fatal in normal shutdown (#6)
* draft Signed-off-by: wfan <wfan@yahoo-corp.jp> * bug fix: server error not appened Signed-off-by: wfan <wfan@yahoo-corp.jp> * add main unit test Signed-off-by: wfan <wfan@yahoo-corp.jp> * remove - Signed-off-by: wfan <wfan@yahoo-corp.jp> * refactor: handle err for shared functionalities Signed-off-by: Jeongwoo Kim - jekim <jekim@yahoo-corp.jp> * fix comment Signed-off-by: wfan <wfan@yahoo-corp.jp> Signed-off-by: wfan <wfan@yahoo-corp.jp> Signed-off-by: Jeongwoo Kim - jekim <jekim@yahoo-corp.jp> Co-authored-by: Jeongwoo Kim - jekim <jekim@yahoo-corp.jp>
1 parent 466a0a0 commit 279622e

File tree

9 files changed

+191
-63
lines changed

9 files changed

+191
-63
lines changed

‎config/config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ type Server struct {
6363
// ShutdownTimeout represents the duration before force shutdown.
6464
ShutdownTimeout string `yaml:"shutdownTimeout"`
6565

66-
// ShutdownDelay represents the delay duration between the health check server shutdown and the client sidecar server shutdown.
66+
// ShutdownDelay represents the delay duration between the health check server shutdown and the api server shutdown.
6767
ShutdownDelay string `yaml:"shutdownDelay"`
6868

6969
// TLS represents the TLS configuration of the authorization proxy.

‎config/config_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ func TestNew(t *testing.T) {
4343
{
4444
name: "Test file content not valid",
4545
args: args{
46-
path: "../test/data/not_valid_config.yaml",
46+
path: "../test/data/invalid_config.yaml",
4747
},
4848
wantErr: fmt.Errorf("decode file failed: yaml: line 11: could not find expected ':'"),
4949
},

‎main.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,19 @@ func run(cfg config.Config) []error {
118118

119119
signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT)
120120

121+
isSignal := false
121122
for {
122123
select {
123-
case <-sigCh:
124+
case sig := <-sigCh:
125+
glg.Infof("authorization proxy received signal: %v", sig)
126+
isSignal = true
124127
cancel()
125-
glg.Warn("Got authorization-proxy server shutdown signal...")
128+
glg.Warn("authorization proxy main process shutdown...")
126129
case errs := <-ech:
127-
return errs
130+
if !isSignal || len(errs) != 1 || errs[0] != ctx.Err() {
131+
return errs
132+
}
133+
return nil
128134
}
129135
}
130136
}
@@ -172,6 +178,8 @@ func main() {
172178
glg.Fatal(emsg)
173179
return
174180
}
181+
glg.Info("authorization proxy main process shutdown success")
182+
os.Exit(1)
175183
}
176184

177185
func getVersion() string {

‎main_test.go‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,12 @@ package main
22

33
import (
44
"os"
5+
"os/exec"
56
"reflect"
7+
"strconv"
8+
"strings"
69
"testing"
10+
"time"
711

812
"github.com/AthenZ/authorization-proxy/v4/config"
913
"github.com/kpango/glg"
@@ -70,6 +74,10 @@ func TestParseParams(t *testing.T) {
7074
}
7175
for _, tt := range tests {
7276
t.Run(tt.name, func(t *testing.T) {
77+
defer func(oldArgs []string) {
78+
// restore os.Args
79+
os.Args = oldArgs
80+
}(os.Args)
7381
if tt.beforeFunc != nil {
7482
tt.beforeFunc()
7583
}
@@ -461,3 +469,135 @@ func Test_getVersion(t *testing.T) {
461469
})
462470
}
463471
}
472+
473+
func Test_main(t *testing.T) {
474+
type test struct {
475+
name string
476+
beforeFunc func()
477+
afterFunc func()
478+
}
479+
tests := []test{
480+
func() test {
481+
var oldArgs []string
482+
return test{
483+
name: "show version",
484+
beforeFunc: func() {
485+
oldArgs = os.Args
486+
os.Args = []string{"authorization-proxy", "-version"}
487+
},
488+
afterFunc: func() {
489+
os.Args = oldArgs
490+
},
491+
}
492+
}(),
493+
}
494+
for _, tt := range tests {
495+
t.Run(tt.name, func(t *testing.T) {
496+
defer tt.afterFunc()
497+
tt.beforeFunc()
498+
main()
499+
})
500+
}
501+
}
502+
503+
func Test_mainExitCode(t *testing.T) {
504+
tests := []struct {
505+
name string
506+
args []string
507+
signal os.Signal
508+
wantExitCode int
509+
}{
510+
{
511+
name: "normal exit",
512+
args: []string{
513+
"-version",
514+
},
515+
signal: nil,
516+
wantExitCode: 0,
517+
},
518+
{
519+
name: "undefined flag",
520+
args: []string{
521+
"-undefined_flag",
522+
},
523+
signal: nil,
524+
wantExitCode: 1,
525+
},
526+
{
527+
name: "run with log error",
528+
args: []string{
529+
"-f",
530+
"./test/data/invalid_log_config.yaml",
531+
},
532+
signal: nil,
533+
wantExitCode: 1,
534+
},
535+
// TODO: need Athenz public key endpoint mock
536+
/*
537+
{
538+
name: "run till termination SIGINT",
539+
args: []string{
540+
"-f",
541+
"./test/data/valid_config.yaml",
542+
},
543+
signal: syscall.SIGINT,
544+
wantExitCode: 1,
545+
},
546+
{
547+
name: "run till termination SIGTERM",
548+
args: []string{
549+
"-f",
550+
"./test/data/valid_config.yaml",
551+
},
552+
signal: syscall.SIGTERM,
553+
wantExitCode: 1,
554+
},
555+
*/
556+
}
557+
558+
rc := os.Getenv("RUN_CASE")
559+
if rc != "" {
560+
c, err := strconv.Atoi(rc)
561+
if err != nil {
562+
panic(err)
563+
}
564+
tt := tests[c]
565+
566+
oldArgs := os.Args
567+
defer func() { os.Args = oldArgs }()
568+
os.Args = append([]string{"authorization-proxy"}, tt.args...)
569+
570+
if tt.signal != nil {
571+
// send signal
572+
go func() {
573+
proc, err := os.FindProcess(os.Getpid())
574+
if err != nil {
575+
panic(err)
576+
}
577+
578+
time.Sleep(200 * time.Millisecond)
579+
proc.Signal(tt.signal)
580+
}()
581+
}
582+
583+
// run main
584+
main()
585+
return
586+
}
587+
588+
for i, tt := range tests {
589+
t.Run(tt.name, func(t *testing.T) {
590+
var outbuf, errbuf strings.Builder
591+
592+
cmd := exec.Command(os.Args[0], "-test.run=Test_mainExitCode")
593+
cmd.Stdout = &outbuf
594+
cmd.Stderr = &errbuf
595+
cmd.Env = append(os.Environ(), "RUN_CASE="+strconv.Itoa(i))
596+
err := cmd.Run()
597+
exitCode := cmd.ProcessState.ExitCode()
598+
if exitCode != tt.wantExitCode {
599+
t.Errorf("main() err = %v, stdout = %s, stderr = %s, exit code = %v, wantExitCode %v", err, outbuf.String(), errbuf.String(), exitCode, tt.wantExitCode)
600+
}
601+
})
602+
}
603+
}

‎service/server.go‎

Lines changed: 24 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ func (s *server) ListenAndServe(ctx context.Context) <-chan []error {
271271
return errs
272272
}
273273

274-
shutdownSrvs := func(errs []error) {
274+
shutdownSrvs := func(errs []error) []error {
275275
if s.hcRunning {
276276
glg.Info("authorization proxy health check server will shutdown...")
277277
errs = appendErr(errs, s.hcShutdown(context.Background()))
@@ -286,65 +286,50 @@ func (s *server) ListenAndServe(ctx context.Context) <-chan []error {
286286
}
287287
if s.dRunning {
288288
glg.Info("authorization proxy debug server will shutdown...")
289-
appendErr(errs, s.dShutdown(context.Background()))
289+
errs = appendErr(errs, s.dShutdown(context.Background()))
290290
}
291-
glg.Info("authorization proxy has already shutdown gracefully")
291+
if len(errs) == 0 {
292+
glg.Info("authorization proxy has already shutdown gracefully")
293+
}
294+
return errs
292295
}
293296

294297
errs := make([]error, 0, 3)
298+
299+
handleErr := func(err error) {
300+
if err != nil {
301+
errs = append(errs, errors.Wrap(err, "close running servers and return any error"))
302+
}
303+
s.mu.RLock()
304+
errs = shutdownSrvs(errs)
305+
s.mu.RUnlock()
306+
echan <- errs
307+
}
308+
295309
for {
296310
select {
297311
case <-ctx.Done(): // when context receive done signal, close running servers and return any error
298312
s.mu.RLock()
299-
shutdownSrvs(errs)
313+
errs = shutdownSrvs(errs)
300314
s.mu.RUnlock()
301315
echan <- appendErr(errs, ctx.Err())
302316
return
303317

304318
case err := <-sech: // when authorization proxy server returns, close running servers and return any error
305-
if err != nil {
306-
errs = append(errs, errors.Wrap(err, "close running servers and return any error"))
307-
}
308-
309-
s.mu.RLock()
310-
shutdownSrvs(errs)
311-
s.mu.RUnlock()
312-
echan <- errs
319+
handleErr(err)
313320
return
314321

315322
case err := <-gsech: // when authorization proxy grpc server returns, close running servers and return any error
316-
if err != nil {
317-
errs = append(errs, errors.Wrap(err, "close running servers and return any error"))
318-
}
319-
320-
s.mu.RLock()
321-
shutdownSrvs(errs)
322-
s.mu.RUnlock()
323-
echan <- errs
323+
handleErr(err)
324324
return
325325

326326
case err := <-hech: // when health check server returns, close running servers and return any error
327-
if err != nil {
328-
errs = append(errs, errors.Wrap(err, "close running servers and return any error"))
329-
}
330-
331-
s.mu.RLock()
332-
shutdownSrvs(errs)
333-
s.mu.RUnlock()
334-
echan <- errs
327+
handleErr(err)
335328
return
336329

337330
case err := <-dech: // when debug server returns, close running servers and return any error
338-
if err != nil {
339-
errs = append(errs, errors.Wrap(err, "close running servers and return any error"))
340-
}
341-
342-
s.mu.RLock()
343-
shutdownSrvs(errs)
344-
s.mu.RUnlock()
345-
echan <- errs
331+
handleErr(err)
346332
return
347-
348333
}
349334
}
350335
}()
@@ -364,7 +349,7 @@ func (s *server) dShutdown(ctx context.Context) error {
364349
return s.dsrv.Shutdown(dctx)
365350
}
366351

367-
// apiShutdown returns any error when shutdown the authorization proxy server.
352+
// apiShutdown returns any error when shutdown the authorization proxy API server.
368353
// Before shutdown the authorization proxy server, it will sleep config.ShutdownDelay to prevent any issue from K8s
369354
func (s *server) apiShutdown(ctx context.Context) error {
370355
time.Sleep(s.sdd)
@@ -373,7 +358,7 @@ func (s *server) apiShutdown(ctx context.Context) error {
373358
return s.srv.Shutdown(sctx)
374359
}
375360

376-
// apiShutdown returns any error when shutdown the authorization proxy server.
361+
// grpcShutdown returns any error when shutdown the authorization proxy gPRC server.
377362
// Before shutdown the authorization proxy server, it will sleep config.ShutdownDelay to prevent any issue from K8s
378363
func (s *server) grpcShutdown() {
379364
time.Sleep(s.sdd)

test/data/not_valid_config.yaml renamed to test/data/invalid_config.yaml

File renamed without changes.

‎test/data/invalid_log_config.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
version: v2.0.0
3+
log:
4+
level: invalid

‎usecase/authz_proxyd.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ func (g *authzProxyDaemon) Start(ctx context.Context) <-chan []error {
101101
pch := g.athenz.Start(ctx)
102102

103103
for err := range pch {
104-
if err != nil {
104+
if err != nil && err != ctx.Err() {
105105
glg.Errorf("pch: %v", err)
106106
// count errors by cause
107107
cause := errors.Cause(err).Error()
@@ -120,12 +120,11 @@ func (g *authzProxyDaemon) Start(ctx context.Context) <-chan []error {
120120
// handle proxy server error, return on server shutdown done
121121
eg.Go(func() error {
122122
errs := <-g.server.ListenAndServe(ctx)
123+
if len(errs) == 0 || len(errs) == 1 && errors.Cause(errs[0]) == ctx.Err() {
124+
return nil
125+
}
123126
glg.Errorf("sch: %v", errs)
124127

125-
if len(errs) == 0 {
126-
// cannot be nil so that the context can cancel
127-
return errors.New("")
128-
}
129128
var baseErr error
130129
for i, err := range errs {
131130
if i == 0 {
@@ -156,8 +155,10 @@ func (g *authzProxyDaemon) Start(ctx context.Context) <-chan []error {
156155
perrs = append(perrs, errors.WithMessagef(errors.New(errMsg), "authorizerd: %d times appeared", count))
157156
}
158157

159-
// proxy server go func, should always return not nil error
160-
ech <- append(perrs, err)
158+
if err != nil {
159+
ech <- append(perrs, err)
160+
}
161+
ech <- perrs
161162
}()
162163

163164
return ech

0 commit comments

Comments
 (0)