feat(sql): Add an SQL permission repository #838
Conversation
This PR maintains backwards compatibility and does not modify existing
controllers.
Implements a permission repository backed by SQL. No migration is
provided as the data is recreated on the next role sync or at startup.
Enabling SQL as the permission repository
```yaml
sql:
enabled: true
connectionPools:
default:
jdbcUrl: jdbc:mysql://.....
user: ...
password: ...
default: true
migration:
jdbcUrl: jdbc:mysql://.....
user: ...
password: ...
permissionRepository:
redis:
enabled: false
sql:
enabled: true
```
Both MySQL and Postgres are supported.
Integration tests have been updated to enable testing with the SQL
provider (using testcontainers), but still default to Redis only
as that's faster.
deverton
requested review from
cfieber,
jonsie and
robzienert
as code owners
Attempting to optimise the delete results in dead locks so just wipe the table.
| extensionResources.add(resource); | ||
| } | ||
| }); | ||
| resources.forEach(this::addResource); |
|
It's been subjected to some load testing in our staging environment. For the workload there we see performance is roughly the same. Results with the current set of patches and with one ElasticacheAmazon ElastiCache running
SQLAmazon RDS
|
|
Once I get the data migration logic in place we'll try rolling this in our production environment soonish. |
|
Just ran in to an issue with how the dual repository looks up the beans from the context which I'm hoping to fix today. |
|
@deverton Fiat should rebuild all permissions on boot (and on a schedule) so I'm not sure the dual repository is necessary anyways. |
|
It might be how we've got things configured, but new instances of Fiat don't seem to role sync immediately. There's typically roughly 10 minutes before the first scheduled sync runs and there's only a single instance in this case. {"@timestamp":"2021-03-03T04:45:02.602+00:00","@version":1,"message":"Server is now HEALTHY. Hooray!","logger_name":"com.netflix.spinnaker.fiat.config.ResourceProvidersHealthIndicator","thread_name":"http-nio-0.0.0.0-7003-exec-2","level":"INFO","level_value":20000}
...
{"@timestamp":"2021-03-03T04:53:27.007+00:00","@version":1,"message":"Acquired Lock Lock{name='fiat.userrolessyncer', ownerName='spin-fiat-6954658dd9-nrhmj', leaseDurationMillis=10000, successIntervalMillis=600000, failureIntervalMillis=600000, version=1614747207004, ownerSystemTimestamp=1614747207004, attributes=''}.","logger_name":"com.netflix.spinnaker.kork.jedis.lock.RedisLockManager","thread_name":"scheduler-3","level":"INFO","level_value":20000} |
|
I've dropped the dual repository code out of this PR for now. I'll do some more testing locally to see if the issues on cutover persist for us. Would you like me to squash these commits up before merge? |
|
Yeah, 10 minutes is the default of No need to squash commits, I'll squash them on merge. |
|
@jonsie so rolled this out in production and it seems to be working fine. Performance for bulk operations like Overall, this means that the user role sync process is still faster than when we were on Redis but is still too slow. The majority of our sync time was spent in The |
|
@deverton Yeah I was wondering about that delete/insert in the put operation but I decided not to say anything since it's +/- the same behavior in the Redis permission repository. I think all in all this is a good sign though. We're running this in our test environment right now and things look good, I will promote this to our staging environment soon. |
This PR maintains backwards compatibility and does not modify existing controllers.
Implements a permission repository backed by SQL with both MySQL and Postgres supported.
Enabling SQL as the permission repository is done as follows:
Integration tests have been updated to enable testing with the SQL provider (using testcontainers), but still default to Redis only as that's faster.
Depends on #835 a bit for testcontainer usage.