Don't exit tablet server on reloading invalid ACL #17485

wiebeytec · 2025-01-08T14:06:01Z

This fixes potentially bringing down a tablet with an innocuous SIGHUP.

It also logs the fact it's reading the ACL file, to fix not getting any feedback on SIGHUP.

Description

Currently, deploying new ACL files is not only very involved because I have to test them against a running Vitess instance, but it's also very stressful to do killall -SIGHUP vttablet knowing that it can bring down the entire system.

Because of the risk of downtime caused by simple ACL definition mistakes, I think this is a candidate for back-porting.

Related Issue(s)

#17139

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2025-01-08T14:06:06Z

dbussink

You'll need to amend your commit with the proper sign off to pass the DCO check as well.

dbussink · 2025-01-09T09:37:50Z

go/vt/tableacl/tableacl.go

@@ -105,6 +105,7 @@ func (tacl *tableACL) init(configFile string, aclCB func()) error {
 	if configFile == "" {
 		return nil
 	}
+	log.Infof("Reading ACL file %v", configFile)


I don't think we should log it here. What's the reason you need it here? Was it not clear that this was happening? I would imagine it was clear from the lines below on errors what the cause of the problem was?

Below here, there's also Info logging on errors which is wrong, those should be raised at least to warning if not error. It's confusing those are set at the info level.

If there's a good reason to add the logging, maybe instead we should log inside the SIGHUP handler to make it specific for the scenario? I don't think this line is something useful to print on each startup then. We should be careful with not adding too much logging noise that loses signal then.

I don't think we should log it here. What's the reason you need it here? Was it not clear that this was happening? I would imagine it was clear from the lines below on errors what the cause of the problem was?

The issue is that when there is no error, sending a SIGHUP doesn't cause any log output to be generated. So, you don't know if it even did anything.

I considered adding the line to the SIGHUP handler. We can do that too I suppose. I'll change it, along with the sign-off.

The issue is that when there is no error, sending a SIGHUP doesn't cause any log output to be generated. So, you don't know if it even did anything.

Let's log it then after it succeeds, so that we only have the line if there was a successful reload due to a SIGHUP. We can then make it more something like "Successfully reloaded ACL".

I had just force-pushed with the log line in the SIGHUP handler, also to make it clear it responds to SIGHUP. It's a convention this is logged by software, so I think it's a good idea.

But about your proposal: I generally prefer to log intentions rather than result. Otherwise you may get log entries like 'error: too many open files' and you have no idea what it was trying to do.

you have no idea what it was trying to do.

I'm not sure what you mean here? This is something that logging can do properly? Looking at the code more I think it would benefit from some additional refactor and remove the exit at a distance kinda behavior and use error returns and let the callers decide.

I'm not sure what you mean here?

I think we're miscommunicating a bit. You said:

What's the reason you need it here? Was it not clear that this was happening? I would imagine it was clear from the lines below on errors what the cause of the problem was?

Indeed, on error. I meant that as a sysop, I would like to see a program respond to something. If I send it a SIGHUP and no log lines are printed, I can't tell whether it did something or not.

Looking at the code more I think it would benefit from some additional refactor and remove the exit at a distance kinda behavior and use error returns and let the callers decide.

OK, I can do that.

@wiebeytec https://github.com/wiebeytec/vitess/compare/main...dbussink:vitess:acl-refactor-suggestion?expand=1 is kinda what I was thinking about here. Dunno what you think of that?

Looks good. In the branch I just force-pushed I still logged in the callees (deep), but functionality it's similar.

You can remove my code in favor of yours then.

I think logging at the higher level would be preferable here.

wiebeytec · 2025-01-13T13:22:23Z

What's the next step here? I think we can go with your code.

dbussink · 2025-01-13T13:42:33Z

What's the next step here? I think we can go with your code.

We can update your PR here with that code? Feel free to use it! We'll also need to add some tests for it as well then to validate the new behavior.

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. vitessio#17139 Signed-off-by: Wiebe Cazemier <[email protected]>

wiebeytec · 2025-01-14T09:39:14Z

Alright, I updated the PR and added a test case that tests loading an invalid ACL does not remove the existing one.

codecov · 2025-01-15T19:22:43Z

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Project coverage is 67.71%. Comparing base (6ac8998) to head (9155ce8).
Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
go/cmd/vttablet/cli/cli.go	50.00%	2 Missing ⚠️
go/vt/tableacl/tableacl.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17485      +/-   ##
==========================================
+ Coverage   67.67%   67.71%   +0.04%     
==========================================
  Files        1584     1584              
  Lines      254466   254513      +47     
==========================================
+ Hits       172215   172351     +136     
+ Misses      82251    82162      -89

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

go/cmd/vttablet/cli/cli.go

mattlord

This makes sense to me. I'm leaning towards not backporting it for two reasons as I'm not confident in how this is used across the Vitess user base:

What does this mean if the operator isn't watching the logs? Couldn't this potentially mean that the vttablet is running along as if everything is fine while having more permissive permissions than intended/expected? We can say that this is the operators concern/fault, but it's still a change.
Some people could be relying on the current behavior (I've made this mistake in the past)

It's also for this reason that I think we should add a small blurb about this here https://github.com/vitessio/vitess/blob/main/changelog/22.0/22.0.0/changelog.md

What do others think?

wiebeytec · 2025-01-16T20:07:06Z

People who are relying on that behavior would have to have a weird setup, because they need to have a running tablet server that they expect to exit after SIGHUP when the file contains errors. One example use is deployment on the replicas as a test, but then they'd be putting their replicas at risk; especially because it calls the OS exit system call, so it's an unclean shutdown (probably?).

Having said that, I do see your point that it's a change.

wiebeytec requested review from harshit-gangal, systay, shlomi-noach, rohit-nayak-ps, timvaillancourt and deepthi as code owners January 8, 2025 14:06

github-actions bot added this to the v22.0.0 milestone Jan 8, 2025

dbussink reviewed Jan 9, 2025

View reviewed changes

wiebeytec force-pushed the main branch 2 times, most recently from 62e220f to 2c30527 Compare January 9, 2025 11:31

Don't exit tablet server on reloading invalid ACL

9155ce8

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. vitessio#17139 Signed-off-by: Wiebe Cazemier <[email protected]>

wiebeytec force-pushed the main branch from 2c30527 to 9155ce8 Compare January 14, 2025 09:38

wiebeytec requested review from ajm188 and mattlord as code owners January 14, 2025 09:38

deepthi removed request for mattlord and shlomi-noach January 15, 2025 19:02

deepthi removed request for timvaillancourt, ajm188 and rohit-nayak-ps January 15, 2025 19:02

dbussink approved these changes Jan 16, 2025

View reviewed changes

mattlord reviewed Jan 16, 2025

View reviewed changes

go/cmd/vttablet/cli/cli.go Show resolved Hide resolved

rohit-nayak-ps added the Type: Bug label Jan 16, 2025

mattlord approved these changes Jan 16, 2025

View reviewed changes

rohit-nayak-ps removed the Type: Enhancement Logical improvement (somewhere between a bug and feature) label Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't exit tablet server on reloading invalid ACL #17485

Don't exit tablet server on reloading invalid ACL #17485

wiebeytec commented Jan 8, 2025

vitess-bot bot commented Jan 8, 2025

dbussink left a comment

dbussink Jan 9, 2025

wiebeytec Jan 9, 2025

dbussink Jan 9, 2025

wiebeytec Jan 9, 2025

dbussink Jan 9, 2025

wiebeytec Jan 9, 2025

dbussink Jan 9, 2025

wiebeytec Jan 9, 2025

dbussink Jan 9, 2025

wiebeytec commented Jan 13, 2025

dbussink commented Jan 13, 2025 •

edited

Loading

wiebeytec commented Jan 14, 2025

codecov bot commented Jan 15, 2025 •

edited

Loading

mattlord left a comment

wiebeytec commented Jan 16, 2025

Don't exit tablet server on reloading invalid ACL #17485

Are you sure you want to change the base?

Don't exit tablet server on reloading invalid ACL #17485

Conversation

wiebeytec commented Jan 8, 2025

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 8, 2025

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

dbussink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiebeytec commented Jan 13, 2025

dbussink commented Jan 13, 2025 • edited Loading

wiebeytec commented Jan 14, 2025

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

mattlord left a comment

Choose a reason for hiding this comment

wiebeytec commented Jan 16, 2025

dbussink commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading