feat(GPX-698): Refactor and cleanup #71
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added file with configuration for different routes
tenant field will be used to apply the X-Scope-OrgID in query frontend mode
Test cases in `main_test.go` were updated to better reflect the expected error messages when incorrect or missing authorization headers are found. The expected body of the response is now more precise, covering a variety of error scenarios like missing headers, invalid or malformed headers, and errors parsing Keycloak tokens. This brings more accuracy to the test cases. In addition, redundant authorization functions `TestHasAuthorizationHeader` and `TestGetBearerToken` were removed as they were not properly encapsulating the logic of the main app. The `TestIsAdminSkip` function was also refactored for better naming, as it is checking if a user is an admin, not if an admin should be skipped.
This refactor changes how `request.go` handles requests, authentication, and logging. Instead of individually parsing authorization headers for each route, we've added authentication middleware that parses the bearer token, verifies if it is valid, and adds the parsed Keycloak token to the request context for easy access in the route handlers. The refactor also simplifies route definitions by replacing the dedicated enforcer function and upstream URL fields in the Route struct with a generic datasource field. We determine the proper function and upstream URL based on the provided datasource at the beginning of the `handleRoute` function. Finally, a logging middleware was introduced before the route handlers to log the incoming HTTP request, its header, method, URL, and body. The logging output is redacted when a configuration flag is set. These changes will make the codebase easier to maintain and enhance debugging by improving logging.
Modified the initialization code to improve logger and configuration error-handling, providing more informative panic logs. Enhanced flexibility by enabling the choice of tenant providers - either "configmap" or "mysql". The logging system now responds to configuration changes by updating the logging level dynamically for better log control.
Adjusted 'GetLabelsCM' and 'GetLabelsDB' functions in labeler.go to take value of type KeycloakToken instead of distinct username and email parameters.
The request.go file was deleted as part of a code restructuring effort to separate concerns. All route handlers were moved to a separate file for better code readability and simplify future development.
Removed unused package imports and handlers including promhttp handler, pprof Index handler, and HealthCheckHandler because they were not needed anymore. Also re-arranged the http server initialization process.
Refactor the `handleRoute` function in the handler.go file to remove the `thanosUrl` and `lokiUrl` parameters and to retrieve the datasource and Keycloak token from the request context instead.
This commit modifies the 'routes.go' file to substantially improve how routes are assigned and configured by creating separate handlers for Loki and Thanos API endpoints. Alongside this, the Datasource struct has been introduced to encapsulate all necessary information per data source allowing us to handle route via EnforeceFunc. The changes also consist of the implementation of 'UseMutualTLS' in the struct to increase security measures when communicating with upstream servers.
…arious files Updated comment blocks and function signatures across several files to improve clarity and readability. This includes more detailed descriptions for each function, giving a clearer explanation of their purpose and behavior. Changes were made in the following files: - enforcer_promql.go - routes.go - init.go - handler.go - main.go - enforcer_logql.go - log.go - labeler.go - authorization.go - enforce.go
…ibility This patch updates the proxy routing and authorization logic to clarify its purpose and make it more extensible. The "KeycloakToken" has been renamed to "OAuthToken" to make its use for general OAuth strategy clear. URL parsing error handling has been moved nearer to its invocation site for better error clarity and has been standardized across functions. Unnecessarily re-parsed URLs for Loki and Thanos have been replaced by earlier parsed URL, simplifying the logic and eliminating redundancy. Also, the imported useless library "golang.org/x/exp/maps" has been removed to clean up the dependencies. In authorization logic, more specific error messages are now returned for better diagnostics during debugging. Additionally, header assignment has been modified to depend on the request method, making it more dynamic.
Code refactoring was carried out to improve clarity and good practices. KeycloakToken was renamed to OAuthToken making it generic. Unnecessary URL parsing was reduced by reusing earlier parsed URL, which simplifies the logic and reduces redundancy. Error messages were made more specific for better debug information. Additional changes include: removing an unused library for cleaner dependencies, moving error handling to the site of the error triggering for clearer code flow, and updating header assignment to be dynamic based on the request method.
Refactored the multena-proxy code to improve its readability and maintainability. A large chunk of App and Config structs, previously part of types.go and main.go, were moved into a new file named config.go. The aim of the refactoring is to have a cleaner separation of the configuration handling from the main business logic. This further allows easier debuggability and can enhance the productivity of the developers, by providing a clear understanding and better navigation of the codebase.
Refactoring of the application included elaborating comment blocks for better understanding the functionality of methods and types, rearranging import statements according to best practices, and division of code in files based on functionality. This change makes the code more readable, maintainable and debuggable while ensuring better code standards. Further, unnecessary / redundant code pieces were removed from a few test files.
The key name 'preferredUsername' in tokenMap has been renamed to 'username' for consistent naming throughout the codebase. In addition to this, 'email' has been set up as the 'token_key' in the MySQL query string, enabling more targeted user searches.
…nd architecture diagrams Made several modifications to Multena Proxy's README.md, "config.yaml" and "labels.yaml" files to improve the clarity of existing instructions and provide more detailed explanation of the system. Changes include adjustments to the architecture diagram in the 'How does it work?' section, modifications to the 'request processing flow', and updates to the example 'labels.yaml' and 'config.yaml' files. These changes were necessary to improve the readability of the documentation, and keep them in line with the latest version of the software after recent updates to its functionality.
…nstructions Updated the installation and configuration instructions in README.MD of Multena Proxy to provide more detailed and organized steps for the user. This revision includes new sections on how to install and upgrade Multena using Helm, how to choose a Labelstore provider between ConfigMap and MySQL, and explains each section in the `config.yaml` file. This is to help users understand and navigate through the proxy setup process more efficiently.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fhochleitner
approved these changes
Oct 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
zaplogging withzerolog..github/workflows/release.ymlPublish new releasetoCIgolangcitolint,scan-codetoscan,build-and-testsplit intotestandbuild1.20to1.21actions/checkoutandactions/setup-goupgradedautotagtoanothrNick/github-tag-actionauth.goKeycloakTokentoOAuthTokengetTokenandtrimBearerTokenfor token parsingparseJwtTokento handle JWT tokensvalidateLabelsvalidates user labelsisAdminchecks if user is an adminbuild/Containerfileubi9/ubi-minimaltoscratch.alpine:latestfor CA certificates.config.goconfigs/config.yamlDEBUG->1insecure_skip_verify->tls_verify_skiplabel_store_kind,service_account_token,oauth_group_namejwks_cert_urltoken_keyHeaderHeaderconfigs/labels.yamlgroupsusersgroup1:#cluster-wideflaguser1:hogaramanamespaceuser3:grafana,opernshift-logging,opernshift-monitoringnamespacesenforce.goEnforcefor query enforcementenforcer_logql.goenforcer_promql.goPromQLEnforcerfor enforcing PromQL queries via EnforceQL interface.promqlEnforcer->PromQLEnforcer.EnforceenforceLabels,checkLabelsandcreateEnforcertake an additionallabelMatchparameter.go.modgithub.com/gorilla/mux v1.8.0github.com/rs/zerolog v1.30.0github.com/mattn/go-colorable v0.1.13github.com/mattn/go-isatty v0.0.19github.com/go-openapi/*packagesgithub.com/prometheus/alertmanagertov0.26.0go.mongodb.org/mongo-drivertov1.12.0go.uber.org/zap v1.24.0github.com/prometheus/prometheustov0.46.0init.golabeler.golabelstore.goLabelstore: DefinesConnect,GetLabelsWithLabelStore(): Connects LabelStore to AppConfigMapHandler.Connect(): Reads labels from YAMLConfigMapHandler.GetLabels(): Merges labelsMySQLHandler.Connect(): Connects to MySQLMySQLHandler.Close(): Closes DB connectionMySQLHandler.GetLabels(): Queries DB for labelslabelstore_test.go(Renamed from labeler_test.go)ConfigMapHandler.GetLabelssetupTestLabeler()andteardown()ConfigMapHandlerinstancelog.gorequestDatafor HTTP request details.loggingMiddlewarefor HTTP request logging.readBody: Reads HTTP request body.logRequestData: Logs HTTP request details.cleanSensitiveHeaders: Removes sensitive headers.logAndWriteError: Logs and writes errors.github.com/rs/zerolog/log.main.goAppstruct, removed globalLoggerzaptozerologAppmethods (WithConfig,WithSAT, etc.)main(),healthz,reverseProxy. IntroducedStartServerinAppstruct.slok/go-http-metricsmain_test.gosetupTestMain()returnsAppand tokensAppandConfigMapHandlerintroduceddeferroutes.gox-pluging-idrouting.Routestruct withUrlandMatchWord./healthz,/metrics,/debug/pprof/).zerologfor logging activities and errors.a.Cfg.structs.goutil.goContainsIgnoreCase,MapKeysToArray,teardownREADME.md