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

Fixed OPA baseUri path #144

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Fixed OPA baseUri path #144

merged 1 commit into from
Aug 12, 2024

Conversation

pranavr12
Copy link
Contributor

No description provided.

@@ -61,7 +62,7 @@ public static class Filter
public TestingTrinoAwsProxyServer.Builder filter(TestingTrinoAwsProxyServer.Builder builder)
{
return builder.withProperty("s3-security.type", OPA_S3_SECURITY_IDENTIFIER)
.withProperty("opa-s3-security.server-base-uri", "http://localhost")
.withProperty("opa-s3-security.server-base-uri", "http://localhost:%s/v1/data".formatted(OPA_CONTAINER_PORT))
Copy link
Member

Choose a reason for hiding this comment

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

This is not guaranteed to be 8181, 8181 is the port in the OPA container.

I think we need to keep the port resolution in the way it was implemented before

@vagaerg
Copy link
Member

vagaerg commented Aug 12, 2024

For context, the motivation is to let more of the settings be purely derived from config files and to make the configs more explicit.

Appending /v1/data is something that the end users can do by setting this on the URI. Making the URI be read from config and not get modified further lets us potentially introduce a base OpaS3SecurityMapper implementation in the future that sends a standard-format (which we'd need to decide upon) OPA request to whatever URI the user configured (similar to what the Trino OPA plugin does)

@vagaerg vagaerg merged commit b0d1a0d into trinodb:main Aug 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants