Skip to content

Pass OpenTelemetry to SystemAccessControl #18899

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.

Sign up for GitHub

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

Merged
findepi merged 1 commit into from
Sep 5, 2023
Merged

Pass OpenTelemetry to SystemAccessControl #18899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions core/trino-main/src/main/java/io/trino/security/AccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import com.google.inject.Inject;
import io.airlift.log.Logger;
import io.airlift.stats.CounterStat;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.Tracer;
import io.trino.connector.CatalogServiceProvider;
import io.trino.eventlistener.EventListenerManager;
import io.trino.metadata.QualifiedObjectName;
Expand All @@ -43,6 +45,7 @@
import io.trino.spi.security.Privilege;
import io.trino.spi.security.SystemAccessControl;
import io.trino.spi.security.SystemAccessControlFactory;
import io.trino.spi.security.SystemAccessControlFactory.SystemAccessControlContext;
import io.trino.spi.security.SystemSecurityContext;
import io.trino.spi.security.TrinoPrincipal;
import io.trino.spi.security.ViewExpression;
Expand Down Expand Up @@ -88,6 +91,7 @@ public class AccessControlManager
private final TransactionManager transactionManager;
private final EventListenerManager eventListenerManager;
private final List<File> configFiles;
private final OpenTelemetry openTelemetry;
private final String defaultAccessControlName;
private final Map<String, SystemAccessControlFactory> systemAccessControlFactories = new ConcurrentHashMap<>();
private final AtomicReference<CatalogServiceProvider<Optional<ConnectorAccessControl>>> connectorAccessControlProvider = new AtomicReference<>();
Expand All @@ -102,11 +106,13 @@ public AccessControlManager(
TransactionManager transactionManager,
EventListenerManager eventListenerManager,
AccessControlConfig config,
OpenTelemetry openTelemetry,
@DefaultSystemAccessControlName String defaultAccessControlName)
{
this.transactionManager = requireNonNull(transactionManager, "transactionManager is null");
this.eventListenerManager = requireNonNull(eventListenerManager, "eventListenerManager is null");
this.configFiles = ImmutableList.copyOf(config.getAccessControlFiles());
this.openTelemetry = requireNonNull(openTelemetry, "openTelemetry is null");
this.defaultAccessControlName = requireNonNull(defaultAccessControlName, "defaultAccessControl is null");
addSystemAccessControlFactory(new DefaultSystemAccessControl.Factory());
addSystemAccessControlFactory(new AllowAllSystemAccessControl.Factory());
Expand Down Expand Up @@ -178,7 +184,7 @@ private SystemAccessControl createSystemAccessControl(File configFile)

SystemAccessControl systemAccessControl;
try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(factory.getClass().getClassLoader())) {
systemAccessControl = factory.create(ImmutableMap.copyOf(properties));
systemAccessControl = factory.create(ImmutableMap.copyOf(properties), createContext(name));
}

log.info("-- Loaded system access control %s --", name);
Expand All @@ -196,7 +202,7 @@ public void loadSystemAccessControl(String name, Map<String, String> properties)

SystemAccessControl systemAccessControl;
try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(factory.getClass().getClassLoader())) {
systemAccessControl = factory.create(ImmutableMap.copyOf(properties));
systemAccessControl = factory.create(ImmutableMap.copyOf(properties), createContext(name));
}

systemAccessControl.getEventListeners()
Expand All @@ -205,6 +211,26 @@ public void loadSystemAccessControl(String name, Map<String, String> properties)
setSystemAccessControls(ImmutableList.of(systemAccessControl));
}

private SystemAccessControlContext createContext(String systemAccessControlName)
{
return new SystemAccessControlContext()
{
private final Tracer tracer = openTelemetry.getTracer("trino.system-access-control." + systemAccessControlName);
Copy link
Copy Markdown
Contributor

@wendigo wendigo Sep 5, 2023

Choose a reason for hiding this comment

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

nit: verify that systemAccessControlName is not empty or null

Copy link
Copy Markdown
Member Author

@findepi findepi Sep 5, 2023

Choose a reason for hiding this comment

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

this is verified by the caller within the class and the method is private
also, empty string is currently valid factory name :)


@Override
public OpenTelemetry getOpenTelemetry()
{
return openTelemetry;
}

@Override
public Tracer getTracer()
{
return tracer;
}
};
}

@VisibleForTesting
public void addSystemAccessControl(SystemAccessControl systemAccessControl)
{
Expand Down
11 changes: 8 additions & 3 deletions core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.inject.Inject;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.eventlistener.EventListenerManager;
import io.trino.metadata.QualifiedObjectName;
import io.trino.plugin.base.security.DefaultSystemAccessControl;
Expand Down Expand Up @@ -141,14 +142,18 @@ public class TestingAccessControlManager
private BiPredicate<Identity, String> denyIdentityTable = IDENTITY_TABLE_TRUE;

@Inject
public TestingAccessControlManager(TransactionManager transactionManager, EventListenerManager eventListenerManager, AccessControlConfig accessControlConfig)
public TestingAccessControlManager(
TransactionManager transactionManager,
EventListenerManager eventListenerManager,
AccessControlConfig accessControlConfig,
OpenTelemetry openTelemetry)
{
super(transactionManager, eventListenerManager, accessControlConfig, DefaultSystemAccessControl.NAME);
super(transactionManager, eventListenerManager, accessControlConfig, openTelemetry, DefaultSystemAccessControl.NAME);
}

public TestingAccessControlManager(TransactionManager transactionManager, EventListenerManager eventListenerManager)
{
this(transactionManager, eventListenerManager, new AccessControlConfig());
this(transactionManager, eventListenerManager, new AccessControlConfig(), OpenTelemetry.noop());
}

public static TestingPrivilege privilege(String entityName, TestingPrivilegeType type)
Expand Down
2 changes: 2 additions & 0 deletions core/trino-main/src/test/java/io/trino/dispatcher/TestLocalDispatchQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import io.airlift.json.JsonCodec;
import io.airlift.node.NodeInfo;
import io.airlift.units.Duration;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.client.NodeVersion;
import io.trino.connector.CatalogProperties;
Expand Down Expand Up @@ -98,6 +99,7 @@ public void testSubmittedForDispatchedQuery()
transactionManager,
emptyEventListenerManager(),
new AccessControlConfig(),
OpenTelemetry.noop(),
DefaultSystemAccessControl.NAME);
accessControl.setSystemAccessControls(List.of(AllowAllSystemAccessControl.INSTANCE));
QueryStateMachine queryStateMachine = QueryStateMachine.begin(
Expand Down
3 changes: 2 additions & 1 deletion core/trino-main/src/test/java/io/trino/execution/TestCommitTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package io.trino.execution;

import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.Session.SessionBuilder;
import io.trino.client.NodeVersion;
Expand Down Expand Up @@ -132,7 +133,7 @@ private QueryStateMachine createQueryStateMachine(String query, Session session,
new ResourceGroupId("test"),
true,
transactionManager,
new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME),
new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME),
executor,
metadata,
WarningCollector.NOOP,
Expand Down
3 changes: 2 additions & 1 deletion core/trino-main/src/test/java/io/trino/execution/TestDeallocateTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package io.trino.execution;

import com.google.common.collect.ImmutableSet;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.client.NodeVersion;
import io.trino.execution.warnings.WarningCollector;
Expand Down Expand Up @@ -84,7 +85,7 @@ public void testDeallocateNoSuchStatement()
private Set<String> executeDeallocate(String statementName, String sqlString, Session session)
{
TransactionManager transactionManager = createTestTransactionManager();
AccessControlManager accessControl = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
AccessControlManager accessControl = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);
accessControl.setSystemAccessControls(List.of(AllowAllSystemAccessControl.INSTANCE));
QueryStateMachine stateMachine = QueryStateMachine.begin(
Optional.empty(),
Expand Down
3 changes: 2 additions & 1 deletion core/trino-main/src/test/java/io/trino/execution/TestPrepareTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package io.trino.execution;

import com.google.common.collect.ImmutableMap;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.client.NodeVersion;
import io.trino.execution.warnings.WarningCollector;
Expand Down Expand Up @@ -104,7 +105,7 @@ public void testPrepareInvalidStatement()
private Map<String, String> executePrepare(String statementName, Statement statement, String sqlString, Session session)
{
TransactionManager transactionManager = createTestTransactionManager();
AccessControlManager accessControl = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
AccessControlManager accessControl = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);
accessControl.setSystemAccessControls(List.of(AllowAllSystemAccessControl.INSTANCE));
QueryStateMachine stateMachine = QueryStateMachine.begin(
Optional.empty(),
Expand Down
2 changes: 2 additions & 0 deletions core/trino-main/src/test/java/io/trino/execution/TestQueryStateMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableMap;
import io.airlift.testing.TestingTicker;
import io.airlift.units.Duration;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.client.FailureInfo;
import io.trino.client.NodeVersion;
Expand Down Expand Up @@ -522,6 +523,7 @@ private QueryStateMachine createQueryStateMachineWithTicker(Ticker ticker)
transactionManager,
emptyEventListenerManager(),
new AccessControlConfig(),
OpenTelemetry.noop(),
DefaultSystemAccessControl.NAME);
accessControl.setSystemAccessControls(List.of(AllowAllSystemAccessControl.INSTANCE));
QueryStateMachine stateMachine = QueryStateMachine.beginWithTicker(
Expand Down
3 changes: 2 additions & 1 deletion core/trino-main/src/test/java/io/trino/execution/TestStartTransactionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.google.common.collect.ImmutableList;
import io.airlift.units.Duration;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.Session;
import io.trino.Session.SessionBuilder;
import io.trino.client.NodeVersion;
Expand Down Expand Up @@ -253,7 +254,7 @@ private QueryStateMachine createQueryStateMachine(String query, Session session,
new ResourceGroupId("test"),
true,
transactionManager,
new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME),
new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME),
executor,
metadata,
WarningCollector.NOOP,
Expand Down
7 changes: 4 additions & 3 deletions core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.connector.CatalogServiceProvider;
import io.trino.connector.MockConnectorFactory;
import io.trino.eventlistener.EventListenerManager;
Expand Down Expand Up @@ -490,17 +491,17 @@ private AccessControlManager createAccessControlManager(TestingEventListenerMana

private AccessControlManager createAccessControlManager(TransactionManager testTransactionManager)
{
return new AccessControlManager(testTransactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
return new AccessControlManager(testTransactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);
}

private AccessControlManager createAccessControlManager(EventListenerManager eventListenerManager, AccessControlConfig config)
{
return new AccessControlManager(createTestTransactionManager(), eventListenerManager, config, DefaultSystemAccessControl.NAME);
return new AccessControlManager(createTestTransactionManager(), eventListenerManager, config, OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);
}

private AccessControlManager createAccessControlManager(EventListenerManager eventListenerManager, String defaultAccessControlName)
{
return new AccessControlManager(createTestTransactionManager(), eventListenerManager, new AccessControlConfig(), defaultAccessControlName);
return new AccessControlManager(createTestTransactionManager(), eventListenerManager, new AccessControlConfig(), OpenTelemetry.noop(), defaultAccessControlName);
}

private SystemAccessControlFactory eventListeningSystemAccessControlFactory(String name, EventListener... eventListeners)
Expand Down
7 changes: 4 additions & 3 deletions core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.inject.CreationException;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.metadata.QualifiedObjectName;
import io.trino.plugin.base.security.DefaultSystemAccessControl;
import io.trino.plugin.base.security.FileBasedSystemAccessControl;
Expand Down Expand Up @@ -133,7 +134,7 @@ public void testCanImpersonateUserOperations()
public void testDocsExample()
{
TransactionManager transactionManager = createTestTransactionManager();
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);
accessControlManager.loadSystemAccessControl(
FileBasedSystemAccessControl.NAME,
ImmutableMap.of("security.config-file", new File("../../docs/src/main/sphinx/security/user-impersonation.json").getAbsolutePath()));
Expand Down Expand Up @@ -775,7 +776,7 @@ public void testRefreshing()
throws Exception
{
TransactionManager transactionManager = createTestTransactionManager();
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);
File configFile = newTemporaryFile();
configFile.deleteOnExit();
copy(new File(getResourcePath("catalog.json")), configFile);
Expand Down Expand Up @@ -835,7 +836,7 @@ public void testAllowModeInvalidValue()

private AccessControlManager newAccessControlManager(TransactionManager transactionManager, String resourceName)
{
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), OpenTelemetry.noop(), DefaultSystemAccessControl.NAME);

accessControlManager.loadSystemAccessControl(FileBasedSystemAccessControl.NAME, ImmutableMap.of("security.config-file", getResourcePath(resourceName)));

Expand Down
2 changes: 2 additions & 0 deletions core/trino-main/src/test/java/io/trino/sql/analyzer/TestAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.io.Closer;
import io.opentelemetry.api.OpenTelemetry;
import io.trino.FeaturesConfig;
import io.trino.Session;
import io.trino.SystemSessionProperties;
Expand Down Expand Up @@ -6730,6 +6731,7 @@ public void setup()
transactionManager,
emptyEventListenerManager(),
new AccessControlConfig(),
OpenTelemetry.noop(),
DefaultSystemAccessControl.NAME);
accessControlManager.setSystemAccessControls(List.of(AllowAllSystemAccessControl.INSTANCE));
this.accessControl = accessControlManager;
Expand Down
16 changes: 16 additions & 0 deletions core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControlFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,27 @@
*/
package io.trino.spi.security;

import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.Tracer;

import java.util.Map;

public interface SystemAccessControlFactory
{
String getName();

@Deprecated
SystemAccessControl create(Map<String, String> config);

default SystemAccessControl create(Map<String, String> config, SystemAccessControlContext context)
{
return create(config);
}

interface SystemAccessControlContext
{
OpenTelemetry getOpenTelemetry();
Copy link
Copy Markdown
Member

@martint martint Sep 2, 2023

Choose a reason for hiding this comment

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

Why do you need an instance of OpenTelemetry? Generally, all you need is a properly configured Tracer.

Copy link
Copy Markdown
Member Author

@findepi findepi Sep 4, 2023

Choose a reason for hiding this comment

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

I didn't see how access control needs would be different than those of a connector. I followed ConnectorContext where @electrum added both

trino/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorContext.java

Lines 31 to 39 in bda3510

default OpenTelemetry getOpenTelemetry()
{
throw new UnsupportedOperationException();
}
default Tracer getTracer()
{
throw new UnsupportedOperationException();
}

Copy link
Copy Markdown
Member Author

@findepi findepi Sep 4, 2023

Choose a reason for hiding this comment

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

I couldn't find any authoritative docs or code comments, thus may be drawing wrong conclusions, but both are expected here
https://github.com/airlift/airlift/blob/c55517863fe246f8921b0a03034b0e2f0d7a31c9/http-client/src/main/java/io/airlift/http/client/HttpClientModule.java#L121-L131
and both are later used, if provided.


Tracer getTracer();
}
}