diff --git a/go/cmd/vttablet/cli/cli.go b/go/cmd/vttablet/cli/cli.go index 3b77b43d9cb..e48a11c79dc 100644 --- a/go/cmd/vttablet/cli/cli.go +++ b/go/cmd/vttablet/cli/cli.go @@ -248,7 +248,10 @@ func createTabletServer(ctx context.Context, env *vtenv.Environment, config *tab addStatusParts(qsc) }) servenv.OnClose(qsc.StopService) - qsc.InitACL(tableACLConfig, enforceTableACLConfig, tableACLConfigReloadInterval) + err := qsc.InitACL(tableACLConfig, tableACLConfigReloadInterval) + if err != nil && enforceTableACLConfig { + return nil, fmt.Errorf("failed to initialize table acl: %w", err) + } return qsc, nil } diff --git a/go/vt/tableacl/tableacl.go b/go/vt/tableacl/tableacl.go index 1b236cb1812..9d6762b4adb 100644 --- a/go/vt/tableacl/tableacl.go +++ b/go/vt/tableacl/tableacl.go @@ -107,14 +107,14 @@ func (tacl *tableACL) init(configFile string, aclCB func()) error { } data, err := os.ReadFile(configFile) if err != nil { - log.Infof("unable to read tableACL config file: %v Error: %v", configFile, err) + log.Errorf("unable to read tableACL config file: %v Error: %v", configFile, err) return err } config := &tableaclpb.Config{} if err := config.UnmarshalVT(data); err != nil { // try to parse tableacl as json file if jsonErr := json2.UnmarshalPB(data, config); jsonErr != nil { - log.Infof("unable to parse tableACL config file as a protobuf or json file. protobuf err: %v json err: %v", err, jsonErr) + log.Errorf("unable to parse tableACL config file as a protobuf or json file. protobuf err: %v json err: %v", err, jsonErr) return fmt.Errorf("unable to unmarshal Table ACL data: %s", data) } } diff --git a/go/vt/vttablet/tabletserver/tabletserver.go b/go/vt/vttablet/tabletserver/tabletserver.go index deeac10bd05..ba0d3778eb5 100644 --- a/go/vt/vttablet/tabletserver/tabletserver.go +++ b/go/vt/vttablet/tabletserver/tabletserver.go @@ -359,31 +359,32 @@ func (tsv *TabletServer) SetQueryRules(ruleSource string, qrs *rules.Rules) erro return nil } -func (tsv *TabletServer) initACL(tableACLConfigFile string, enforceTableACLConfig bool) { +func (tsv *TabletServer) initACL(tableACLConfigFile string) error { // tabletacl.Init loads ACL from file if *tableACLConfig is not empty - err := tableacl.Init( + return tableacl.Init( tableACLConfigFile, func() { tsv.ClearQueryPlanCache() }, ) - if err != nil { - log.Errorf("Fail to initialize Table ACL: %v", err) - if enforceTableACLConfig { - log.Exit("Need a valid initial Table ACL when enforce-tableacl-config is set, exiting.") - } - } } // InitACL loads the table ACL and sets up a SIGHUP handler for reloading it. -func (tsv *TabletServer) InitACL(tableACLConfigFile string, enforceTableACLConfig bool, reloadACLConfigFileInterval time.Duration) { - tsv.initACL(tableACLConfigFile, enforceTableACLConfig) +func (tsv *TabletServer) InitACL(tableACLConfigFile string, reloadACLConfigFileInterval time.Duration) error { + if err := tsv.initACL(tableACLConfigFile); err != nil { + return err + } sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, syscall.SIGHUP) go func() { for range sigChan { - tsv.initACL(tableACLConfigFile, enforceTableACLConfig) + err := tsv.initACL(tableACLConfigFile) + if err != nil { + log.Errorf("Error reloading ACL config file %s in SIGHUP handler: %v", tableACLConfigFile, err) + } else { + log.Info("Successfully reloaded ACL file %s in SIGHUP handler", tableACLConfigFile) + } } }() @@ -395,6 +396,7 @@ func (tsv *TabletServer) InitACL(tableACLConfigFile string, enforceTableACLConfi } }() } + return nil } // SetServingType changes the serving type of the tabletserver. It starts or diff --git a/go/vt/vttablet/tabletserver/tabletserver_test.go b/go/vt/vttablet/tabletserver/tabletserver_test.go index 621941224a9..9472168f113 100644 --- a/go/vt/vttablet/tabletserver/tabletserver_test.go +++ b/go/vt/vttablet/tabletserver/tabletserver_test.go @@ -2154,6 +2154,16 @@ var aclJSON2 = `{ } ] }` +var aclJSONOverlapError = `{ + "table_groups": [ + { + "name": "group02", + "table_names_or_prefixes": ["test_table2", "test_table2"], + "readers": ["vt2"], + "admins": ["vt2"] + } + ] + }` func TestACLHUP(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) @@ -2167,12 +2177,23 @@ func TestACLHUP(t *testing.T) { require.NoError(t, err) defer os.Remove(f.Name()) + // We need to confirm this ACL JSON is broken first, for later use. + _, err = io.WriteString(f, aclJSONOverlapError) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) + err = tsv.InitACL(f.Name(), 0) + require.Error(t, err) + + f, err = os.Create(f.Name()) + require.NoError(t, err) _, err = io.WriteString(f, aclJSON1) require.NoError(t, err) err = f.Close() require.NoError(t, err) - tsv.InitACL(f.Name(), true, 0) + err = tsv.InitACL(f.Name(), 0) + require.NoError(t, err) groups1 := tableacl.GetCurrentConfig().TableGroups if name1 := groups1[0].GetName(); name1 != "group01" { @@ -2187,20 +2208,37 @@ func TestACLHUP(t *testing.T) { syscall.Kill(syscall.Getpid(), syscall.SIGHUP) time.Sleep(100 * time.Millisecond) // wait for signal handler - groups2 := tableacl.GetCurrentConfig().TableGroups - if len(groups2) != 1 { - t.Fatalf("Expected only one table group") - } - group2 := groups2[0] - if name2 := group2.GetName(); name2 != "group02" { - t.Fatalf("Expected name 'group02', got '%s'", name2) - } - if group2.GetAdmins() == nil { - t.Fatalf("Expected 'admins' to exist, but it didn't") - } - if group2.GetWriters() != nil { - t.Fatalf("Expected 'writers' to not exist, got '%s'", group2.GetWriters()) + test_loaded_acl := func() { + groups2 := tableacl.GetCurrentConfig().TableGroups + if len(groups2) != 1 { + t.Fatalf("Expected only one table group") + } + group2 := groups2[0] + if name2 := group2.GetName(); name2 != "group02" { + t.Fatalf("Expected name 'group02', got '%s'", name2) + } + if group2.GetAdmins() == nil { + t.Fatalf("Expected 'admins' to exist, but it didn't") + } + if group2.GetWriters() != nil { + t.Fatalf("Expected 'writers' to not exist, got '%s'", group2.GetWriters()) + } } + + test_loaded_acl() + + // Now try to reload an invalid ACL and see if we are still operating with the same ACL config as before. + + f, err = os.Create(f.Name()) + require.NoError(t, err) + _, err = io.WriteString(f, aclJSONOverlapError) + require.NoError(t, err) + + syscall.Kill(syscall.Getpid(), syscall.SIGHUP) + time.Sleep(100 * time.Millisecond) // wait for signal handler + + test_loaded_acl() + } func TestConfigChanges(t *testing.T) {