Don't exit tablet server on reloading invalid ACL

This fixes potentially bringing down a tablet with an innocuous SIGHUP. It also logs the fact it's read the ACL file, to fix not getting any feedback on SIGHUP. #17139 Signed-off-by: Wiebe Cazemier <[email protected]>
vitessio · Jan 14, 2025 · 9155ce8 · 9155ce8
1 parent 6ac8998
commit 9155ce8
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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) {