Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy accesstoken cache store #5829

Merged
merged 25 commits into from
Mar 22, 2023
Merged

Proxy accesstoken cache store #5829

merged 25 commits into from
Mar 22, 2023

Conversation

butonic
Copy link
Member

@butonic butonic commented Mar 14, 2023

This PR changes the cache implementations in ocis to the micro store interface.

  • it makes the in memory user info cache in the proxy as a micro store
  • it introduces the redis-sentinel type
  • it aligns all configurations for micro store
  • it introduces global OCIS_PERSISTENT_STORE_* options that are used for the userlog and eventhistory service
  • it corrects the usage of store database and table (which are used to build a prefix for in memory stores)

partly adresses #5781 by harmonizing the basic store options: type, adresses/nodes, database, table, size and ttl.

  • rename the userlog RecordExpiry option to ttl? before a release?

  • the graph service uses three in memory caches that still need to use a micro store, maybe in a subsequent PR?

  • make caches really optional. when setting OCIS_CACHE_STORE_TYPE=redis without having a local redis running the decomposedfs fails to create u user home becouse it cannot create a connection. We should log an error but continue, which is what I did in this PR for the proxy userinfo cache

  • The userinfo cache cannot directly be invalidated with the sid from a backchannel logout. For that a separate session id -> tokenhash cache should be used.

  • A 10 second userinfo cache TTL is probably too low. espesially when we have a backchannel logout and we actually use the expiration claim of access of tokens to set a TTL for the cache store.

@update-docs
Copy link

update-docs bot commented Mar 14, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic butonic force-pushed the proxy-accesstoken-cache-store branch 2 times, most recently from 0267690 to d339cd9 Compare March 14, 2023 20:34
@butonic butonic marked this pull request as ready for review March 14, 2023 20:52
@butonic butonic requested a review from kobergj March 14, 2023 21:17
@butonic butonic force-pushed the proxy-accesstoken-cache-store branch from ca9ac77 to 78e9892 Compare March 15, 2023 15:01
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Some questions

ocis-pkg/store/store.go Outdated Show resolved Hide resolved
ocis-pkg/store/options.go Outdated Show resolved Hide resolved
services/proxy/pkg/middleware/oidc_auth.go Outdated Show resolved Hide resolved
@butonic butonic force-pushed the proxy-accesstoken-cache-store branch from 1d5c00e to d532a64 Compare March 15, 2023 16:17
@butonic
Copy link
Member Author

butonic commented Mar 15, 2023

I need to dig into the failing e2e tests. At around 700s there seems to be a ~30sec timeout:

2023/03/15 16:01:22 http: TLS handshake error from 192.168.94.3:57696: remote error: tls: unknown certificate
2023/03/15 16:01:25 http: proxy error: context canceled
{"level":"error","service":"storage-users","pkg":"rgrpc","traceid":"00000000000000000000000000000000","error":"error: not found: space 945938d8-c8e0-41cd-8af8-cdf17d9bbd6f not found","status":{"code":6,"message":"not found when listing spaces","trace":"00000000000000000000000000000000"},"filters":[{"type":2,"Term":{"Id":{"opaque_id":"9816d9f2-c620-44a2-a5fb-135e0ca31465$945938d8-c8e0-41cd-8af8-cdf17d9bbd6f!945938d8-c8e0-41cd-8af8-cdf17d9bbd6f"}}},{"type":4,"Term":{"SpaceType":"+grant"}}],"time":"2023-03-15T16:01:25.231133587Z","message":"failed to list storage spaces"}
2023/03/15 16:01:25 http: TLS handshake error from 192.168.94.3:59386: remote error: tls: unknown certificate

🤔

@butonic butonic marked this pull request as draft March 16, 2023 12:49
@butonic
Copy link
Member Author

butonic commented Mar 16, 2023

faling tests unrelated to this PR: owncloud/web#8633 fixes the flaky test

@butonic butonic marked this pull request as ready for review March 16, 2023 14:37
@butonic butonic requested a review from kobergj March 16, 2023 14:37
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Still some questions

ocis-pkg/store/options.go Show resolved Hide resolved
services/graph/pkg/config/defaults/defaultconfig.go Outdated Show resolved Hide resolved
services/proxy/pkg/config/defaults/defaultconfig.go Outdated Show resolved Hide resolved
services/storage-users/pkg/config/config.go Outdated Show resolved Hide resolved
@butonic butonic force-pushed the proxy-accesstoken-cache-store branch 2 times, most recently from d09c1dd to 914464d Compare March 20, 2023 16:23
@butonic
Copy link
Member Author

butonic commented Mar 20, 2023

@kobergj @mmattel another naming mismatch I see in all the cache config is address vs adresses vs nodes.

Should I align that with the micro interface and use nodes everywhere? We can fallback to the old env var names, but in some places it is even the yaml tag that needs to change.

@butonic butonic requested a review from kobergj March 20, 2023 16:40
@butonic butonic force-pushed the proxy-accesstoken-cache-store branch from 1aa0753 to 1069ba0 Compare March 20, 2023 16:47
@mmattel
Copy link
Contributor

mmattel commented Mar 20, 2023

Should I align that with the micro interface and use nodes everywhere

This is just my personal opinion and though this creates possible a bit more work for you, I fully agree to harmonize stuff now and get rid of legacies.

@butonic
Copy link
Member Author

butonic commented Mar 20, 2023

urgh reva is importing ocis-pkg/store/etcd ... 😞

@mmattel
Copy link
Contributor

mmattel commented Mar 21, 2023

As written in issue: #5781, there are also some services who offer redis authentication. This either needs harmonisation over all services or to be removed.

@butonic butonic force-pushed the proxy-accesstoken-cache-store branch from bff4906 to ca1db03 Compare March 21, 2023 11:30
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

LGTM but e2e tests are red

@mmattel
Copy link
Contributor

mmattel commented Mar 22, 2023

Following topics:

  • There is no comment or change on redis user/pwd envvar, example:
    FRONTEND_OCS_RESOURCE_INFO_CACHE_REDIS_PASSWORD
    = one service uses this, others not...
  • Not part of this PR, but post merging we need:
    • changes in readme's and admin docs - I will take care to spare your time
  • We do not use deprecation - is this intended?
    Yes, some envvars are quite new and there it is not an issue, but some of them exist since 2.0.0 and there it could be an issue. Deprecation is easy to implement, see the link.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
butonic and others added 9 commits March 22, 2023 11:10
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Co-authored-by: kobergj <jkoberg@owncloud.com>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic force-pushed the proxy-accesstoken-cache-store branch from 2d2beaa to fb48abd Compare March 22, 2023 10:11
@mmattel
Copy link
Contributor

mmattel commented Mar 22, 2023

FIXME redis plugin does not support redis cluster or ring

Q: this is docs relevant or?
I could add this to the common text descriptions in readme/admin docs

@ScharfViktor
Copy link
Contributor

to unlock your PR I added waiting 50 millisecond. It's not good I know but main problem is that tests are very fast. It leads to aborting requests sometimes. We as QA team try to define each place where we add (wait for button appears, wait for response)

@sonarcloud
Copy link

sonarcloud bot commented Mar 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

7.1% 7.1% Coverage
3.4% 3.4% Duplication

@ScharfViktor
Copy link
Contributor

e2e is green. no failed/flaky test. difference 2m 34 sec. I think we can afford it.

image

image

@butonic butonic merged commit 6bec87f into master Mar 22, 2023
@delete-merged-branch delete-merged-branch bot deleted the proxy-accesstoken-cache-store branch March 22, 2023 14:22
ownclouders pushed a commit that referenced this pull request Mar 22, 2023
* refactor middleware options

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* use ocmemstore micro store implementaiton for token cache

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* refactor ocis store options, support redis sentinel

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* align cache configuration

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* database and tabe are used to build prefixes for inmemory stores

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* add global persistent store options to userlog config

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* log cache errors but continue

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* drup unnecessary type conversion

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* Better description for the default userinfo ttl

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* use global cache options for even more caches

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* don't log userinfo cache misses

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* default to stock memory store

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* use correct mem store typo string

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* split cache options, doc cleanup

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* mint and write userinfo to cache async

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* use hashed token as key

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* go mod tidy

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* update docs

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* update cache store naming

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* bring back depreceted ocis-pkg/store package for backwards compatability

Signed-off-by: Jörn Fri
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
@ScharfViktor ScharfViktor mentioned this pull request May 4, 2023
86 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants