Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions session/sql_migration.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ func migrateSessionsToSQLAndValidate(ctx context.Context,
overrideSessionTimeZone(migratedSession)
overrideMacaroonRecipe(kvSession, migratedSession)
overrideRemovedAccount(kvSession, migratedSession)
overrideFeatureConfig(kvSession, migratedSession)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another approach would be to convert nil configs to empty ones in unmarshalFeatureConfigs. Wouldn't that be better because that way we ensure that both session (kvdb/sql) retrievals are consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this offline, and we decided to continue with the override approach in favour of changing the unmarshal function. While that approach will likely work, we'll keep the same approach we've used to tackle similar issues previously, and consider to in the future instead change to the unmarshal approach. But when doing so, we'll change for all current overrides, and not just this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a call-site perspective I think it's fine to keep the nil here

clientConfig := make(map[string]string)
if sess.FeatureConfig != nil {
for k, v := range *sess.FeatureConfig {
clientConfig[k] = string(v)
}
}

This just leads to an empty string.


if !reflect.DeepEqual(kvSession, migratedSession) {
diff := difflib.UnifiedDiff{
Expand Down Expand Up @@ -530,3 +531,28 @@ func overrideRemovedAccount(kvSession *Session, migratedSession *Session) {
},
)
}

// overrideFeatureConfig overrides a specific feature's config for the SQL
// session in a certain scenario:
//
// In the bbolt store, an empty config for a feature is represented as an empty
// array, while in for the SQL store, the same config is represented as nil.
// Therefore, in the scenario where a specific feature has an empty config, we
// override the SQL FeatureConfig for that feature to also be set to an empty
// array. This is needed to ensure that the deep equals check in the migration
// validation does not fail in this scenario.
func overrideFeatureConfig(kvSession *Session, mSession *Session) {
// If FeatureConfig is not set for both sessions, we return early.
if kvSession.FeatureConfig == nil || mSession.FeatureConfig == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an &&? Otherwise it could be that mSession.FeatureConfig is nil, which would panic below.

return
}

migratedConf := *mSession.FeatureConfig
for featureName, config := range *kvSession.FeatureConfig {
// If the config is empty for the feature, we override
// the SQL version.
if len(config) == 0 {
migratedConf[featureName] = make([]byte, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we erroneously migrated an empty feature config to a non-empty one, then we'd silently override the migrated one, right? So maybe we could just convert the migrated config if it is nil to an empty one?

}
}
}
61 changes: 58 additions & 3 deletions session/sql_migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ func TestSessionsStoreMigration(t *testing.T) {
// session doesn't.
overrideRemovedAccount(kvSession, sqlSession)

// Finally, we also need to override specific feature's
// config if they are empty.
overrideFeatureConfig(kvSession, sqlSession)

assertEqualSessions(t, kvSession, sqlSession)
}

Expand Down Expand Up @@ -200,6 +204,48 @@ func TestSessionsStoreMigration(t *testing.T) {
return getBoltStoreSessions(t, store)
},
},
{
name: "one session with a feature config with empty",
populateDB: func(t *testing.T, store *BoltStore,
_ accounts.Store) []*Session {

// Set a feature config, which has an empty
// entry for a specific feature.
featureConfig := map[string][]byte{
"AutoFees": {},
}

_, err := store.NewSession(
ctx, "test", TypeMacaroonAdmin,
time.Unix(1000, 0), "",
WithFeatureConfig(featureConfig),
)
require.NoError(t, err)

return getBoltStoreSessions(t, store)
},
},
{
name: "one session with a feature config with nil",
populateDB: func(t *testing.T, store *BoltStore,
_ accounts.Store) []*Session {

// Set a feature config, which has a nil entry
// for a specific feature.
featureConfig := map[string][]byte{
"AutoFees": nil,
}

_, err := store.NewSession(
ctx, "test", TypeMacaroonAdmin,
time.Unix(1000, 0), "",
WithFeatureConfig(featureConfig),
)
require.NoError(t, err)

return getBoltStoreSessions(t, store)
},
},
{
name: "one session with dev server",
populateDB: func(t *testing.T, store *BoltStore,
Expand Down Expand Up @@ -801,9 +847,18 @@ func randomPrivacyFlags() PrivacyFlags {
func randomFeatureConfig() FeaturesConfig {
featureConfig := make(FeaturesConfig)
for i := range rand.Intn(10) {
featureName := fmt.Sprintf("feature%d", i)
featureValue := []byte{byte(rand.Int31())}
featureConfig[featureName] = featureValue
var (
featureName = fmt.Sprintf("feature%d", i)
configValue []byte
)

// For the vast majority of the features, we set a value for the
// feature's config. But in 1/5 of the cases, we set an empty
// config.
if rand.Intn(5) != 0 {
configValue = []byte{byte(rand.Int31())}
}
featureConfig[featureName] = configValue
}

return featureConfig
Expand Down
Loading