[Feature] Improve Session Handling #1964
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR improves session handling and route management by introducing type safety for Envoy destinations and enhancing session lifecycle management. The changes focus on preventing route conflicts and optimizing session cache behavior.
- Introduced
EnvoyDestinationcustom type with reserved route checking functionality - Enhanced session management with cache TTL, refresh capabilities, and proper invalidation
- Added GRPC error handling utilities with protobuf message conversion support
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/grpc/errors.go | Adds GRPC error handling utilities with protobuf message conversion |
| pkg/util/constants/envoy.go | Introduces EnvoyDestination type with reserved route validation |
| pkg/deployment/resources/gateway/gateway_config_destination.go | Updates to use typed destinations with validation and append method |
| pkg/deployment/resources/gateway/gateway_config.go | Converts string-based destinations to typed EnvoyDestination |
| pkg/deployment/resources/config_map_gateway.go | Implements reserved route checking and typed destination handling |
| pkg/deployment/client/inventory.go | Updates to use typed destination constants |
| pkg/apis/shared/validate.go | Generalizes ValidateMap to support comparable key types |
| pkg/apis/shared/errors.go | Adds GRPC error conversion method to ResourceError |
| integrations/shared/v1/definition/errors.proto | Defines protobuf error message structures |
| integrations/shared/v1/definition/errors.pb.go | Generated protobuf Go code for error messages |
| integrations/envoy/auth/v3/impl/session/session.go | Adds cache TTL and refresh functionality to session manager |
| integrations/envoy/auth/v3/impl/auth_custom/openid/impl.go | Improves session invalidation and refresh logic |
| CHANGELOG.md | Documents the session and route handling improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func (p ResourceError) AsGRPCError() *pbSharedV1.PathError { | ||
| return &pbSharedV1.PathError{ | ||
| Path: p.Prefix, | ||
| Message: p.Err.Error(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The method returns a pointer to PathError but the interface in pkg/util/grpc/errors.go expects protoadapt.MessageV1. This type mismatch could cause runtime panics when the GRPC error conversion is used.
Copilot uses AI. Check for mistakes.
| session, ok, _, err := i.session.Refresh(ctx, cookie.Value) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| current.User = session.AsResponse() | ||
| if ok { | ||
| newSession, newCookie, err := i.handleOpenIDRefresh(ctx, cfg, ocfg, session) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The code calls i.session.Refresh() but ignores the returned duration (third parameter with _). This duration represents the time until session expiration and should be used to make informed decisions about whether refresh is needed or to set appropriate cache headers.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| var key = utilConstants.EnvoyDestination(target.Route.Path) | ||
| if key.IsReserved() { | ||
| log.Warn("ArangoRoute Route Path `%s` is reserved", key) |
There was a problem hiding this comment.
The log message format string expects a string parameter but key is of type EnvoyDestination. Use key.String() instead of key to properly format the log message.
| log.Warn("ArangoRoute Route Path `%s` is reserved", key) | |
| log.Warn("ArangoRoute Route Path `%s` is reserved", key.String()) |
Copilot uses AI. Check for mistakes.
| } | ||
| cfg.Destinations[target.Route.Path] = dest | ||
| if err := cfg.Destinations.Append(key, dest); err != nil { | ||
| log.Err(err).Warn("Unable to add destination `%s`", key) |
There was a problem hiding this comment.
Similar to the previous issue, the log message format expects a string but key is of type EnvoyDestination. Use key.String() for proper formatting.
| log.Err(err).Warn("Unable to add destination `%s`", key) | |
| log.Err(err).Warn("Unable to add destination `%s`", key.String()) |
Copilot uses AI. Check for mistakes.
No description provided.