Skip to content
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

Allow topaz to run as an edge authorizer #111

Merged
merged 24 commits into from
Aug 8, 2023
Merged

Allow topaz to run as an edge authorizer #111

merged 24 commits into from
Aug 8, 2023

Conversation

carabasdaniel
Copy link
Contributor

Closes: #110

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Pull Request Test Coverage Report for Build 5787683224

  • 203 of 743 (27.32%) changed or added relevant lines in 17 files are covered.
  • 31 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-3.6%) to 31.587%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/testing/engine.go 13 14 92.86%
pkg/cc/config/topaz_config.go 1 3 33.33%
pkg/app/topaz.go 29 33 87.88%
pkg/app/edgedir.go 45 51 88.24%
pkg/app/middlewares/tenant_middleware.go 16 22 72.73%
pkg/app/middlewares/middlewares.go 19 26 73.08%
pkg/app/topaz/wire_gen.go 11 19 57.89%
plugins/noop/plugin.go 0 10 0.0%
plugins/noop/factory.go 0 14 0.0%
cmd/topazd/topaz_run.go 0 15 0.0%
Files with Coverage Reduction New Missed Lines %
cmd/topazd/topaz_run.go 1 29.85%
pkg/app/authorizer.go 5 61.11%
decision_log/logger/file/logger.go 12 0%
decision_log/logger/file/config.go 13 0%
Totals Coverage Status
Change from base Build 5627278529: -3.6%
Covered Lines: 1085
Relevant Lines: 3435

💛 - Coveralls

@carabasdaniel carabasdaniel marked this pull request as ready for review August 2, 2023 14:58
@carabasdaniel
Copy link
Contributor Author

@ronenh Do you think we could maybe switch the middleware handling so it would be service type specific ? Would it make sense to add them to the constructor and expose them through the service type interface for the service host factory ?

pkg/app/controller/controller.go Outdated Show resolved Hide resolved
pkg/app/middlewares/instance_middleware.go Outdated Show resolved Hide resolved
plugins/dummy/factory.go Outdated Show resolved Hide resolved
@ronenh
Copy link
Contributor

ronenh commented Aug 3, 2023

@ronenh Do you think we could maybe switch the middleware handling so it would be service type specific ? Would it make sense to add them to the constructor and expose them through the service type interface for the service host factory ?

I think it depends on what we want to accomplish. On one hand, we want to enforce some uniformity in the middleware we use. It makes sense for the service host to be able to ensure that a well-known set of middleware is configured on all services. That could be part of the service host interface. Maybe it can be constructed with an optional list of grpc.ServerOptions.

The question then is whether we want to give individual services the ability to add their own middleware. But since users can choose to group services on ports in whatever way they choose, a service can never know who else would be affected by middleware that it adds. Unless we have a specific example of where that would be necessary, I'd opt for configuring middleware at the service host level.

pkg/cc/config/topaz_config.go Outdated Show resolved Hide resolved
gertd
gertd previously approved these changes Aug 7, 2023
Copy link
Member

@gertd gertd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some suggested changes to be considered

pkg/cli/cmd/templates.go Outdated Show resolved Hide resolved
pkg/cli/cmd/templates.go Outdated Show resolved Hide resolved
pkg/cli/cmd/templates.go Outdated Show resolved Hide resolved
@gertd gertd merged commit 05bfafb into main Aug 8, 2023
5 checks passed
@gertd gertd deleted the sidecar branch August 8, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow topaz to run as an Aserto edge authorizer
3 participants