authn: add Redis KV backend for multi-replica AuthN#307
Conversation
Signed-off-by: Rohithmatham12 <rohithmatham@gmail.com>
Rohithmatham12
force-pushed
the
authn-kv-service-config
branch
from
9cd1259 to
5d3799d
Compare
Rohithmatham12
changed the title
aaronnw
left a comment
aaronnw
left a comment
There was a problem hiding this comment.
Thanks for opening! I left some comment on the implementation, will look at the go mod additions and licensing separately.
| defaultPort = 52001 | ||
| defaultTokenExpiration = cos.Duration(24 * time.Hour) | ||
| defaultMaxTokenAge = cos.Duration(90 * 24 * time.Hour) | ||
| defaultKVServiceAddr = "localhost:6379" |
There was a problem hiding this comment.
Let's split port away from address.
| AisAuthKVAddr = "AIS_AUTHN_KV_ADDR" // external KV service address for AuthN DB backends | ||
| AisAuthKVPassword = "AIS_AUTHN_KV_PASSWORD" // optional external KV service password | ||
| AisAuthKVDBIndex = "AIS_AUTHN_KV_DB_INDEX" // optional external KV service database/index | ||
| AisAuthKVTLS = "AIS_AUTHN_KV_TLS" // optional TLS toggle for external KV service |
There was a problem hiding this comment.
I don't think we should support env var overrides for any of these except maybe password. We can expect the config contains everything we need to configure DB access.
| } | ||
| KVServiceConf struct { | ||
| Addr string `json:"addr,omitempty"` | ||
| Password string `json:"password,omitempty"` //nolint:gosec // configuration field, not a hardcoded credential |
There was a problem hiding this comment.
For password, I think we should not need to put it in config. If we do, we need to use cmn.Censored and use json:"-" similar to ServerConf.psecret`.
But I would question if there's any value in having it if we do not persist it. I would expect for real deployments we'll use k8s env from secret or vault injection.
| if c.Service.DBIndex < 0 { | ||
| return fmt.Errorf("invalid auth.db.service.db_index=%d (must be >= 0)", c.Service.DBIndex) | ||
| } | ||
| if c.DBType == "Redis" && c.Service.Addr == "" { |
There was a problem hiding this comment.
Can this be a typed enum like BuntDB in kvdb.go?
| for _, path := range batch { | ||
| val, err := r.client.Get(context.Background(), path).Result() | ||
| if err != nil { | ||
| if err == redis.Nil { |
| Addr: conf.Addr, | ||
| Password: conf.Password, | ||
| DB: conf.DBIndex, | ||
| } |
There was a problem hiding this comment.
I believe for the cancel context created below to work we need ContextTimeoutEnabled in the driver options here.
| opts.TLSConfig = &tls.Config{MinVersion: tls.VersionTLS12} | ||
| } | ||
| client := redis.NewClient(opts) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
There was a problem hiding this comment.
I think this timeout would be a valid addition to KVServiceConf
| } | ||
| client := redis.NewClient(opts) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
I think all calls below that are using context.Background should instead do a similar wrap, e.g.:
func (r *redisDriver) SetString(collection, key, data string) (int, error) {
ctx, cancel := r.ctx()
defer cancel()
if err := r.client.Set(ctx, kvdb.MakePath(collection, key), data, 0).Err(); err != nil {
return http.StatusInternalServerError, err
}
return http.StatusOK, nil
}and use a common ctx() func to define a context.WithTimeout(context.Background(), r.timeout)
For multi-step operations like GetAll, then re-use the same context across all calls.
|
Updated the PR to address the Redis KV review feedback:
Validation run locally:
|
2 participants
Summary
auth.db.serviceconfig plusAIS_AUTHN_KV_*environment overrides for external AuthN KV services.auth.db.type = "Redis"; local BuntDB remains the default backend.kvdb.MakePath.miniredisfor set/get/delete/list/get-all/delete-collection behavior.Closes #273.
Testing
gofmt -w api/env/authn.go api/authn/config.go api/authn/config_internal_test.go cmd/authn/config/cmgr.go cmd/authn/config/cmgr_test.go cmd/authn/kvdb/kvdb.go cmd/authn/kvdb/redis.go cmd/authn/kvdb/redis_internal_test.go cmn/kvdb/api.go cmn/kvdb/bunt.gogo test ./api/authn ./cmd/authn/config ./cmd/authn/kvdb ./cmd/authn/signing ./cmn/kvdbgo test ./cmd/authngit diff --check