From 9ca2c016bea1c0ba642846a30e4fdac33ad2a390 Mon Sep 17 00:00:00 2001 From: Roman Perekhod <2403905@gmail.com> Date: Wed, 18 Mar 2026 10:58:53 +0100 Subject: [PATCH] Revert "fix(proxy): [OCISDEV-460] deduplicate CreateHome calls in middleware" --- .../unreleased/fix-proxy-createhome-dedup.md | 11 -- services/proxy/pkg/command/server.go | 1 - services/proxy/pkg/config/config.go | 1 - services/proxy/pkg/middleware/create_home.go | 107 +++++------------- services/proxy/pkg/middleware/options.go | 9 -- 5 files changed, 30 insertions(+), 99 deletions(-) delete mode 100644 changelog/unreleased/fix-proxy-createhome-dedup.md diff --git a/changelog/unreleased/fix-proxy-createhome-dedup.md b/changelog/unreleased/fix-proxy-createhome-dedup.md deleted file mode 100644 index 5c539e83a59..00000000000 --- a/changelog/unreleased/fix-proxy-createhome-dedup.md +++ /dev/null @@ -1,11 +0,0 @@ -Bugfix: Deduplicate CreateHome calls in proxy middleware - -The CreateHome proxy middleware previously fired a CreateHome gRPC -request on every authenticated HTTP request with no deduplication. -On first login, the browser sends many parallel requests, each -triggering a redundant CreateHome call. This change uses singleflight -to collapse concurrent calls for the same user and caches successful -results in-process so subsequent requests skip the gRPC call entirely. - -https://github.com/owncloud/ocis/pull/12115 -https://github.com/owncloud/ocis/issues/12068 diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 7ce31a90ec9..c0a08116358 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -373,7 +373,6 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, middleware.Logger(logger), middleware.WithRevaGatewaySelector(gatewaySelector), middleware.RoleQuotas(cfg.RoleQuotas), - middleware.CreateHomeCacheDisabled(cfg.CreateHomeCacheDisabled), ), // trigger space assignment when a user logs in middleware.SpaceManager( diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index f826d9a4a89..9ce6faf3f1d 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -48,7 +48,6 @@ type Config struct { ClaimSpaceManagement ClaimSpaceManagement `yaml:"claim_space_management"` MultiFactorAuthentication MFAConfig `yaml:"mfa"` MultiInstance MultiInstanceConfig `yaml:"multi_instance"` - CreateHomeCacheDisabled bool `yaml:"create_home_cache_disabled" env:"PROXY_CREATEHOME_CACHE_DISABLED" desc:"Disable the process-lifetime cache that tracks which users already have a personal space. When enabled, the proxy skips redundant CreateHome gRPC calls for returning users. Set this to true as a fallback if the cache causes unexpected behavior." introductionVersion:"Daledda"` Context context.Context `json:"-" yaml:"-"` } diff --git a/services/proxy/pkg/middleware/create_home.go b/services/proxy/pkg/middleware/create_home.go index f859c065dff..a71f3825354 100644 --- a/services/proxy/pkg/middleware/create_home.go +++ b/services/proxy/pkg/middleware/create_home.go @@ -1,10 +1,8 @@ package middleware import ( - "context" "net/http" "strconv" - "sync" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -16,26 +14,20 @@ import ( "github.com/owncloud/reva/v2/pkg/rgrpc/status" "github.com/owncloud/reva/v2/pkg/rgrpc/todo/pool" "github.com/owncloud/reva/v2/pkg/utils" - "golang.org/x/sync/singleflight" "google.golang.org/grpc/metadata" ) -// CreateHome provides a middleware which sends a CreateHome request to the reva gateway. -// It deduplicates concurrent requests for the same user and caches successful results -// for the lifetime of the process. +// CreateHome provides a middleware which sends a CreateHome request to the reva gateway func CreateHome(optionSetters ...Option) func(next http.Handler) http.Handler { options := newOptions(optionSetters...) logger := options.Logger - cacheDisabled := options.CreateHomeCacheDisabled - return func(next http.Handler) http.Handler { return &createHome{ next: next, logger: logger, revaGatewaySelector: options.RevaGatewaySelector, roleQuotas: options.RoleQuotas, - cacheDisabled: cacheDisabled, } } } @@ -45,96 +37,57 @@ type createHome struct { logger log.Logger revaGatewaySelector pool.Selectable[gateway.GatewayAPIClient] roleQuotas map[string]uint64 - cacheDisabled bool - - knownHomes sync.Map // map[userID]struct{} — users whose home is confirmed - flight singleflight.Group // collapses concurrent CreateHome calls per user } -func (m *createHome) ServeHTTP(w http.ResponseWriter, req *http.Request) { +func (m createHome) ServeHTTP(w http.ResponseWriter, req *http.Request) { if !m.shouldServe(req) { m.next.ServeHTTP(w, req) return } - token := req.Header.Get("X-Access-Token") + token := req.Header.Get("x-access-token") // we need to pass the token to authenticate the CreateHome request. + //ctx := tokenpkg.ContextSetToken(r.Context(), token) ctx := metadata.AppendToOutgoingContext(req.Context(), revactx.TokenHeader, token) - u, ok := revactx.ContextGetUser(ctx) - if !ok { - m.next.ServeHTTP(w, req) - return - } - - userID := u.Id.OpaqueId - - // Fast path: home already known to exist for this user. - if m.homeKnown(userID) { - m.next.ServeHTTP(w, req) - return - } - createHomeReq := &provider.CreateHomeRequest{} - roleIDs, err := m.getUserRoles(u) - if err != nil { - m.logger.Error().Err(err).Str("userid", userID).Msg("failed to get roles for user") - errorcode.GeneralException.Render(w, req, http.StatusInternalServerError, "Unauthorized") - return - } - if limit, hasLimit := m.checkRoleQuotaLimit(roleIDs); hasLimit { - createHomeReq.Opaque = utils.AppendPlainToOpaque(nil, "quota", strconv.FormatUint(limit, 10)) - } - - // Deduplicate concurrent CreateHome calls for the same user. - _, err, _ = m.flight.Do(userID, func() (interface{}, error) { - return m.createHome(ctx, createHomeReq) - }) - - // Only cache on success — if CreateHome failed, retry on next request. - if err == nil && !m.cacheDisabled { - m.knownHomes.Store(userID, struct{}{}) - } - - m.next.ServeHTTP(w, req) -} - -func (m *createHome) homeKnown(userID string) bool { - if m.cacheDisabled { - return false + u, ok := revactx.ContextGetUser(ctx) + if ok { + roleIDs, err := m.getUserRoles(u) + if err != nil { + m.logger.Error().Err(err).Str("userid", u.Id.OpaqueId).Msg("failed to get roles for user") + errorcode.GeneralException.Render(w, req, http.StatusInternalServerError, "Unauthorized") + return + } + if limit, hasLimit := m.checkRoleQuotaLimit(roleIDs); hasLimit { + createHomeReq.Opaque = utils.AppendPlainToOpaque(nil, "quota", strconv.FormatUint(limit, 10)) + } } - _, ok := m.knownHomes.Load(userID) - return ok -} -func (m *createHome) createHome(ctx context.Context, req *provider.CreateHomeRequest) (interface{}, error) { client, err := m.revaGatewaySelector.Next() if err != nil { m.logger.Err(err).Msg("error selecting next gateway client") - return nil, err - } - - createHomeRes, err := client.CreateHome(ctx, req) - if err != nil { - m.logger.Err(err).Msg("error calling CreateHome") - return nil, err - } - - if createHomeRes.Status.Code != rpc.Code_CODE_OK && createHomeRes.Status.Code != rpc.Code_CODE_ALREADY_EXISTS { - err := status.NewErrorFromCode(createHomeRes.Status.Code, "gateway") - m.logger.Err(err).Msg("error when calling CreateHome") - return nil, err + } else { + createHomeRes, err := client.CreateHome(ctx, createHomeReq) + if err != nil { + m.logger.Err(err).Msg("error calling CreateHome") + } else if createHomeRes.Status.Code != rpc.Code_CODE_OK { + err := status.NewErrorFromCode(createHomeRes.Status.Code, "gateway") + if createHomeRes.Status.Code != rpc.Code_CODE_ALREADY_EXISTS { + m.logger.Err(err).Msg("error when calling Createhome") + } + } } - return true, nil + m.next.ServeHTTP(w, req) } -func (m *createHome) shouldServe(req *http.Request) bool { - return req.Header.Get("X-Access-Token") != "" +func (m createHome) shouldServe(req *http.Request) bool { + return req.Header.Get("x-access-token") != "" } -func (m *createHome) getUserRoles(user *userv1beta1.User) ([]string, error) { +func (m createHome) getUserRoles(user *userv1beta1.User) ([]string, error) { var roleIDs []string if err := utils.ReadJSONFromOpaque(user.Opaque, "roles", &roleIDs); err != nil { return nil, err @@ -152,7 +105,7 @@ func (m *createHome) getUserRoles(user *userv1beta1.User) ([]string, error) { return dedup, nil } -func (m *createHome) checkRoleQuotaLimit(roleIDs []string) (uint64, bool) { +func (m createHome) checkRoleQuotaLimit(roleIDs []string) (uint64, bool) { if len(roleIDs) == 0 { return 0, false } diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index bd030839459..503273a564e 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -77,8 +77,6 @@ type Options struct { // Service Accounts ServiceAccountID string ServiceAccountSecret string - // CreateHomeCacheDisabled disables the knownHomes cache in the CreateHome middleware - CreateHomeCacheDisabled bool // Multi-Instance Options MultiInstanceEnabled bool InstanceID string @@ -238,13 +236,6 @@ func AccessTokenVerifyMethod(method string) Option { } } -// CreateHomeCacheDisabled provides a function to disable the CreateHome cache. -func CreateHomeCacheDisabled(val bool) Option { - return func(o *Options) { - o.CreateHomeCacheDisabled = val - } -} - // RoleQuotas sets the role quota mapping setting func RoleQuotas(roleQuotas map[string]uint64) Option { return func(o *Options) {