diff --git a/changelog/22.0/22.0.0/summary.md b/changelog/22.0/22.0.0/summary.md index f773df2b10c..c8ba10f995e 100644 --- a/changelog/22.0/22.0.0/summary.md +++ b/changelog/22.0/22.0.0/summary.md @@ -19,6 +19,7 @@ - **[Support for Filtering Query logs on Error](#query-logs)** - **[Minor Changes](#minor-changes)** - **[VTTablet Flags](#flags-vttablet)** + - **[VTTablet ACL enforcement and reloading](#reloading-vttablet-acl)** - **[Topology read concurrency behaviour changes](#topo-read-concurrency-changes)** - **[VTAdmin](#vtadmin)** - [Updated to node v22.13.1](#updated-node) @@ -155,6 +156,10 @@ While the flag will continue to accept float values (interpreted as seconds) for - `--consolidator-query-waiter-cap` flag to set the maximum number of clients allowed to wait on the consolidator. The default value is set to 0 for unlimited wait. Users can adjust this value based on the performance of VTTablet to avoid excessive memory usage and the risk of being OOMKilled, particularly in Kubernetes deployments. +#### VTTablet ACL enforcement and reloading + +When a tablet is started with `--enforce-tableacl-config` it will exit with an error if the contents of the file are not valid. After the changes made in https://github.com/vitessio/vitess/pull/17485 the tablet will no longer exit when reloading the contents of the file after receiving a SIGHUP. When the file contents are invalid on reload the tablet will now log an error and the active in-memory ACLs remain in effect. + ### `--topo_read_concurrency` behaviour changes The `--topo_read_concurrency` flag was added to all components that access the topology and the provided limit is now applied separately for each global or local cell _(default `32`)_. 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 b65b3949354..53322384bdc 100644 --- a/go/vt/vttablet/tabletserver/tabletserver.go +++ b/go/vt/vttablet/tabletserver/tabletserver.go @@ -361,31 +361,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) + } } }() @@ -397,6 +398,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 e4faf4ee6c9..ef79560ca8f 100644 --- a/go/vt/vttablet/tabletserver/tabletserver_test.go +++ b/go/vt/vttablet/tabletserver/tabletserver_test.go @@ -2155,6 +2155,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()) @@ -2168,12 +2178,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" { @@ -2188,20 +2209,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) {