-
Notifications
You must be signed in to change notification settings - Fork 761
Activity bounded context: /api/latest/fleet/activities (1 of 2)
#38115
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
base: main
Are you sure you want to change the base?
Conversation
- Extract common HTTP error types to server/platform/http package to enable decoupling bounded contexts from the fleet package - Make CommonEndpointer fully generic ([H any]) so handler function types can be defined in their respective packages - Replace deprecated AuthFunc/FleetService fields with pre-built AuthMiddleware for cleaner dependency injection - Remove fleet import from endpoint_utils package (now only imports contexts/*, platform/http, and middleware/*)
…-endpoint_utils # Conflicts: # server/service/endpoint_utils.go
…into victor/37244-refactor-common_mysql # Conflicts: # server/datastore/mysql/common_mysql/common.go # server/datastore/mysql/mysql.go # server/service/middleware/endpoint_utils/endpoint_utils.go
…-common_mysql # Conflicts: # server/contexts/ctxerr/ctxerr.go # server/contexts/ctxerr/ctxerr_otel_test.go # server/contexts/ctxerr/ctxerr_test.go # server/contexts/ctxerr/metadata.go # server/contexts/ctxerr/statistics.go # server/contexts/host/host.go # server/contexts/license/license.go # server/contexts/logging/logging.go # server/contexts/viewer/viewer.go # server/mdm/android/service/endpoint_utils.go # server/platform/http/errors.go # server/service/appconfig.go # server/service/endpoint_middleware.go # server/service/endpoint_utils.go # server/service/middleware/auth/http_auth.go # server/service/middleware/endpoint_utils/endpoint_utils.go # server/service/middleware/endpoint_utils/transport_error.go
iansltx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete!
Asking a bunch of questions here since this is supposed to set new patterns (and there's just a significant volume of code here to look at). The questions are generally detail-level rather than 30k feet; will defer to tech leads on that kind of feedback, though in some cases where functionality landed in files seemed a bit odd to me.
| {"by username prefix", "john", nil, 1}, | ||
| {"by email prefix", "jane@", nil, 1}, | ||
| {"no match", "nomatch", nil, 0}, | ||
| {"via matching user IDs", "nomatch", []uint{johnUserID}, 1}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be really helpful to write down, e.g. in activities.go, why we're doing union rather than intersection here. IIRC it's because we want users searchable by both current and past name/email but maybe @MagnusHJensen has a more firm answer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is to enable search by both the user's current name/email AND their past name/email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds right. Useful to throw a code comment in to indicate as much since normally one would expect filters to be intersection rather than union, so on initial glance this would look like a bug when it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry didn't see this notification.
I can't seem to find the mentioned UNION code, but I do recall it's there, can you point it out?
We wanted to search for the user details in the activity (which could be old and outdated) as well as match on users new data, in the user table itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use an explicit UNION in the query, but the implementation ends up behaving that way.
Glad we're confirmed on why this behavior is here. Definitely should comment that "why" for the next person taking a look at this though.
| if len(opts) > 0 { | ||
| opts[0].FeatureRoutes = append(opts[0].FeatureRoutes, android_service.GetRoutes(svc, opts[0].androidModule)) | ||
| } | ||
|
|
||
| // Add activity routes if DBConns is provided | ||
| if len(opts) > 0 && opts[0].DBConns != nil { | ||
| legacyAuthorizer, err := authz.NewAuthorizer() | ||
| require.NoError(t, err) | ||
| activityAuthorizer := authz.NewAuthorizerAdapter(legacyAuthorizer) | ||
| activityUserProvider := activityacl.NewFleetServiceAdapter(svc) | ||
| _, activityRoutesFn := activity_bootstrap.New( | ||
| opts[0].DBConns, | ||
| activityAuthorizer, | ||
| activityUserProvider, | ||
| logger, | ||
| ) | ||
| activityAuthMiddleware := func(next endpoint.Endpoint) endpoint.Endpoint { | ||
| return auth.AuthenticatedUser(svc, next) | ||
| } | ||
| opts[0].FeatureRoutes = append(opts[0].FeatureRoutes, activityRoutesFn(activityAuthMiddleware)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two different ways of adding routes here. While we don't necessarily need to clean this up in this PR, we should settle on an approach here so that there's a standard pattern for bounded contexts, allowing us to know what a well-formed N+1th vertical slice bounded context looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By two different ways, do you mean here and in serve.go?
It would be nice to have a shared method between production and tests. Added a TODO in #38234
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More talking about how we have FeatureRoutes above this that does a similar thing in a very different way for Android.
…tivities-1 # Conflicts: # server/service/integration_mdm_test.go
|
Had a few questions here but we're getting a lot closer here and I'm betting the remaining questions can get answered without code changes (once tests pass). |
cdcme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No other questions beyond what's already been discussed here.
Related issue: Resolves #37806
This PR creates an activity bounded context and moves the following HTTP endpoint (including the full vertical slice) there:
/api/latest/fleet/activitiesNONE of the other activity functionality is moved! This is an incremental approach starting with just 1 API/service endpoint.
A significant part of this PR is tests. This feature is now receiving significantly more unit/integration test coverage than before.
Also, this PR does not remove the
ListActivitiesdatastore method in the legacy code. That will be done in the follow up PR (part 2 of 2).This refactoring effort also uncovered an activity/user authorization issue: https://fleetdm.slack.com/archives/C02A8BRABB5/p1768582236611479
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.