[sql-25] firewalldb: start cleaning up RulesDB/KVStores code #1023
Conversation
ellemouton
added
the
no-changelog
ellemouton
force-pushed
the
sql25
branch
2 times, most recently
from
f5d3615 to
b6946ce
Compare
| firewall *firewalldb.DB | ||
| firewallBolt *firewalldb.BoltDB |
There was a problem hiding this comment.
The naming is a bit confusing here, but I guess this is temporary, looking at the commit message
|
Note to reviewers: since there will be another round anyways, i've added one more commit on: all it does is move code from one file to another |
In preparation for a db-backend agnostic DB struct along with a SQL implementation of the various stores in the package which will be housed under a struct named `SQLDB`.
In preparation for adding a DB struct in the db.go file which will be db-backend agnostic.
In this commit, we add a `DB` struct in the `firewalldb` package. This struct will be responsible for housing abstract implementations of the various stores in the `firewalldb`. For now, we start with just the RulesDB. We also add Start&Stop methods for the struct in preparation for future additions here - for now, these do nothing. In the main LiT setup, we move the firewall.BoltDB and the new firewalldb.DB to the `stores` struct and implement them in the two `config_` files. For now, both varients create the Bbolt version of the firewallDB and this is used to init the `firewalldb.DB` struct. This will be changed in future commits where we will add a sql implementation.
| firewallDB, err := firewalldb.NewBoltDB( | ||
| networkDir, firewalldb.DBFilename, sessStore, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating firewall DB: %v", err) | ||
| } |
There was a problem hiding this comment.
Hmmmm this is pre-existing, so I'll let you decide if you'd want to address this or not:
One issue that I realized we have here, is that if this firewalldb.NewBoltDB call, or the session.NewDB function call above returns an err, we will never Close the previously initialized dbs. That is because we'll return nil for the *stores return param, and therefore not run the close function in terminal.go.
Not sure if that's something we'd want to address or not, so will let you decide on that :)!
There was a problem hiding this comment.
great catch! yes this absolutely should be addressed. But I will do it in a follow up since these commits have already been approved and since the issue was introduced before (my bad!).
In this commit, a new `DeleteTempKVStores` method is added to the `RulesDB` interface and this is called in the `DB`'s `Start` method. This is done because regardless of the backend that we use for the RulesDB, we want this clean-up code to run at start time. It also makes more sense to have this call in a Start method rather than in a constructor.
In preparation for testing each of the existing unit tests against various types of DB backends (ie, different implementations of the RulesDB interface), we clean-up the test code such that the RulesDB is always constructed through the same `NewTestDB` function. Eventually, we will add different implementations of this function which will build under different build flags.
Here, we move the abstract kvstore interfaces to the `interfaces.go` file and then rename the `kvstores.go` file to `kvstores_kvdb.go` to indicate that it houses the kvdb implementation of the kvstore interfaces. This is in preparation for adding a `kvstores_sql.go` file.
In preparation for adding a SQL version of the
RulesDBinterface, we need to do a bit of clean-up in thefirewalldbpackage. See each commits description for detail.Part of #917