chore(graph): disable HTTP or eventhandlers by configuration - #3293
chore(graph): disable HTTP or eventhandlers by configuration#3293pbleser-oc wants to merge 2 commits into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 4 high |
| Security | 2 critical |
🟢 Metrics 494 complexity · 324 duplication
Metric Results Complexity 494 Duplication 324
🟢 Coverage 19.17% diff coverage · -0.25% coverage variation
Metric Results Coverage variation ✅ -0.25% coverage variation (-1.00%) Diff coverage ✅ 19.17% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (cc8a161) 84494 19647 23.25% Head commit (e279617) 86215 (+1721) 19828 (+181) 23.00% (-0.25%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3293) 2123 407 19.17% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
d892695 to
ec49989
Compare
89661f4 to
1db7cb2
Compare
6fbfdf1 to
ef27ec5
Compare
In the scope of the broader issue opencloud-eu#1312, this PR deals with performing those changes for the `graph` service, namely to add the ability to disable the HTTP API or to disable the events API handler by configuration. It also adds metrics for the events processing, and tests for the events processing. The previous implementation was combining the HTTP server service and the events consumption, which is why this PR refactors the composition of those services: * the event consumption has been moved into its own service * the identity.Backend is created beforehand, and then injected as a collaborator in both the HTTP service as well as the event consumer service It also adds metrics, mainly for the event processing. To encourage re-use in latter implementations and changes, it also introduces two top-level package changes: * internal/eventstest/events_test_helpers: contains a TestBus implementation to unit-test event consumers without NATS * internal/metricstest/metrics_test_helpers: contains assertion functions to test Prometheus metrics
ef27ec5 to
c196096
Compare
c196096 to
6aee3e2
Compare
Introducing gowrap as a build-time tool to generate interface delegate structs from templates: * added as a 'make go-generate' target in services/graph, * added as a build-time dependency in .bingo/ Introduce an LDAP client abstraction interface to be able to wrap the go-ldap client API with metrics transparently (and possibly hooks and such in the future), in order to use delegation patterns to measure the time LDAP (client) operations take to finish, as well as to track their results (success, failure, not-found). Has two implementations that are generated using gowrap: * a go-ldap adapter implementation that directly delegates to a go-ldap connection * a time measuring and metrics collecting implementation that delegates to another LdapClient The metrics collecting one is disabled by default, can be enabled with GRAPH_LDAP_METRICS_DISABLE=false It collects durations of outbound LDAP client operations into a histogram, as well as the number of concurrent outbound LDAP operations in a gauge (via an atomic int and a gauge function, as that performs best). Add an HTTP middleware that measures how long Graph HTTP API requests take, storing taken time into a histogram along with labels for * method, * path pattern (from the chi routes), * Graph API version prefix, * Graph API resource name, * and the resulting status code. It also tracks the number of concurrent inbound Graph API HTTP requests using a gauge (also using an atomic int and a gauge function). Disabled by default, can be enabled with GRAPH_HTTP_METRICS_DISABLE=false Add Backend and EducationBackend delegate implementations that measure execution time on the level of the higher API call operations there (CreateUser, DeleteUser, ..., CreateSchool, ...), generated using gowrap. Disabled by default, can be enabled with GRAPH_IDENTITY_BACKEND_METRICS_DISABLE=false Also added a small k6 script to produce some read-only load on the Graph API, for a casual test of the metrics, as well as k6 in mise.toml. Make an internal changes to how singular LDAP entry searches work in the LDAP identity backends: * check whether searches for a singular entry returns more than one result, in which case a new error TooManyResults is returned, instead of leaving that undetected, blindly taking the first result, and potentially risking data inconsistencies Improve the loggers in identity backends by adding attributes for their request targets (Reva gateway address or LDAP URI, respectively). Also add a "backend" attribute for all Graph API logs (set to "ldap" or "cs3"), to help debug potential issues, and remove them from all the logger debug calls at the beginning of each LDAP-related function as those should really be part of the logger and set beforehand. The LDAP identity backend logger also has two new attributes to help debugging with logs: * write (bool): whether write operations are enabled * refint (bool): whether refint is enabled or not Also adds a dedicated counter metric for user password change operations. Minor campfire improvements: * add a constructor func for the CS3 backend * add a constructor func for the LDAP backend * in the LDAP identity backend, in searchLDAPEntryByFilter (used by all search/get public functions), errors that occur when performing LDAP SEARCH operations were blindly mapped to a ItemNotFound error, instead of being analyzed as it could be caused by a technical error * in the requireadmin middleware, add debug logging to explain why a request is denied * when an LDAP password change fails because the user entry was not found in LDAP, we now have a log message that tracks that
6aee3e2 to
e279617
Compare
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
I've focussed the review of the LDAP related changes. Main issue is that we need to map the ldap.LDAPResultSizeLimitExceeded error instead of checking the number of returned entries, since we already set a sizelimit of 1 for all request that expect just a single result entry.
Other than that looks nice. I like the logging improvements, and improved error handling.
I must admin that am not really a fan of generating code from templates, but I also see the benefit here. I am already wondering if we could "re-implement" the LDAP reconnect wrapper (https://github.com/opencloud-eu/reva/blob/main/pkg/utils/ldap/reconnect.go) in a similar way :).
| @@ -1,5 +1,8 @@ | |||
| package identity | |||
|
|
|||
| //go:generate gowrap gen -g -i Backend -t ./backend_prometheus.tmpl -o backend_prometheus.go | |||
There was a problem hiding this comment.
This does not work with the bingo installed version-pinned gowrap as it needs gowrap got be installed in $PATH.
To make it work with the bingo installed gowrap I think you need to replace the go generate ./... in the Makefile with GOWRAP=$(GOWRAP) go generate ./... and use
//go:generate $GOWRAP gen ....
<rant> IMO at some point we should really get away from bingo and switch to using go tool for managing our external go based dependencies. (reva does that already) </rant>
| @@ -0,0 +1,26 @@ | |||
| package identity | |||
|
|
|||
| //go:generate gowrap gen -g -i LdapClient -t ./ldap_client_prometheus.tmpl -o ldap_client_prometheus.go | |||
There was a problem hiding this comment.
Same here about the bingo installed gowrap
| case 1: | ||
| return res.Entries[0], nil | ||
| default: | ||
| return nil, ErrTooManyResults |
There was a problem hiding this comment.
we're already searching with a sizelimit of 1, here (see the NewSearchRequest call above), so this line will never be reached.
If a search would have returned more that 1 result, Search will return ldap.LDAPResultSizeLimitExceeded in err.
| case 1: | ||
| return res.Entries[0], nil | ||
| default: | ||
| return nil, ErrTooManyResults |
There was a problem hiding this comment.
Same here. We need to map the LDAPResultSizeLimitExceeded error instead.
| errmsg := "too many results searching for school" | ||
| i.logger.Warn().Err(lerr). | ||
| Str("schoolfilter", filter).Msg("too many results searching for school") | ||
| return nil, errorcode.New(errorcode.TooManyResults, errmsg) |
There was a problem hiding this comment.
Can't we include this into the ldapResultToErrMap below?
| case 1: | ||
| return e[0], nil | ||
| default: | ||
| return nil, ErrTooManyResults |
There was a problem hiding this comment.
One more about the LDAPResultSizeLimitExceeded error. getLDAPGroupsByFilter sets a sizelimit for us and returns errcode.ItemNotFound when it is hit. I guess the error handling in getLDAPGroupsByFilter needs to be adjusted.
3 participants
Description
In the scope of the broader issue #1312, this PR deals with performing those changes for the
graphservice, namely to add the ability to disable the HTTP API or to disable the events API handler by configuration.It also adds numerous metrics, and tests for the events processing.
Making Event Processing and HTTP Optional (#1312)
The previous implementation was combining the HTTP server service and the events consumption, which is why this PR refactors the composition of those services:
To encourage re-use in latter implementations and changes, it also introduces two top-level package changes:
internal/eventstest/events_test_helpers: contains aTestBusimplementation to unit-test event consumers without the need to engage NATSinternal/metricstest/metrics_test_helpers: contains assertion functions to test Prometheus metricsRun-down of changes for this aspect:
GRAPH_HTTP_DISABLED)GRAPH_EVENTS_DISABLE_CONSUMER)internal/eventstestandinternal/metricstestwith utilities for testing event consumers and Prometheus metrics, respectivelyIncludes metrics for whether the HTTP API handler is enabled, and another for whether the Event consumer is enabled, to allow counting the number of instances that do either, and potentially define alerts when one of those reaches 0 (or another low threshold).
Adding Metrics
Introducing gowrap as a build-time tool to generate interface delegate structs from templates:
make generatetarget inservices/graph,.bingo/LDAP Client Metrics
Introduces LDAP client abstraction interface to be able to wrap the
go-ldapclient API with metrics transparently (and possibly hooks and such in the future), in order to use delegation patterns to measure the time LDAP (client) operations take to finish, as well as to track their results (success, failure, not-found).Has two implementations that are generated using
gowrap:go-ldapadapter implementation that directly delegatesLdapClientThe metrics collecting one is disabled by default, can be enabled with
GRAPH_LDAP_METRICS_DISABLE=falseIt collects the duration of outbound LDAP client operations into a histogram, as well as the number of concurrent outbound LDAP operations in a gauge (via an atomic int and a gauge func for best performance).
HTTP Requests Metrics
Add an HTTP middleware that measures how long Graph HTTP API requests take, storing taken time into a histogram along with labels for
It also tracks the number of concurrent inbound Graph API HTTP requests using a gauge (also via an atomic int and a gauge func for best performance).
Disabled by default, can be enabled with
GRAPH_HTTP_METRICS_DISABLE=falseLDAP Identity Backend Metrics
Add
BackendandEducationBackenddelegate implementations that measure execution time on the level of the higher API call operations there (CreateUser,DeleteUser, ...,CreateSchool, ...), generated usinggowrap.Disabled by default, can be enabled with
GRAPH_IDENTITY_BACKEND_METRICS_DISABLE=falseLDAP Client API Change
Made an internal change to how the singular entry search operations work in the LDAP identity backend:
TooManyResultsis returned, instead of leaving that undetected, blindly taking the first result, and potentially risking data inconsistenciesLogging Improvements
Improve the loggers in identity backends by adding attributes for their request targets (Reva gateway address or LDAP URI, respectively).
Also add a "backend" attribute for all Graph API logs (set to
"ldap"or"cs3"), to help debug potential issues.The LDAP identity backend logger also has two new attributes to help debugging with logs:
write(bool): whether write operations are enabledrefint(bool): whether refint is enabled or notAlso adds a dedicated counter for user password change operations.
Additional boy-scouting
searchLDAPEntryByFilter(used by all search/get public functions), errors that occur when performing LDAPSEARCHoperations were blindly mapped to aItemNotFounderror, instead of being analyzed as it could be caused by a technical errorrequireadminmiddleware, add debug logging to explain why a request is deniedRelated Issue
Motivation and Context
Details can be found in #1312
How Has This Been Tested?
HTTP API
Tested the HTTP API (when enabled) using:
and
and
and
Events API
Tested the event API (when enabled) using:
Tested combinations of enabling/disabling the events and HTTP APIs.
When both are disabled, the service refuses to start and exits early with an error message.
Metrics
Metrics were tested with a small
k6script included in this PR (namelyservices/graph/load_test.js) that performs various read-only operations with parallel VUs.Types of changes
Checklist:
services/graph/README.md)