Skip to content

Commit

Permalink
Fix owner selection logic in table create (#525)
Browse files Browse the repository at this point in the history
  • Loading branch information
@swaranga-netflix
swaranga-netflix authored Jan 25, 2023
1 parent 516dd56 commit 0ef3420
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 63 deletions.
10 changes: 10 additions & 0 deletions metacat-common/src/main/java/com/netflix/metacat/common/dto/TableDto.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.netflix.metacat.common.QualifiedName;
import io.swagger.annotations.ApiModel;
Expand All @@ -37,6 +38,7 @@
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Optional;

/**
* Table DTO.
Expand Down Expand Up @@ -89,6 +91,14 @@ public QualifiedName getDefinitionName() {
return name;
}

@JsonIgnore
public Optional<String> getTableOwner() {
return Optional.ofNullable(definitionMetadata)
.map(definitionMetadataJson -> definitionMetadataJson.get("owner"))
.map(ownerJson -> ownerJson.get("userId"))
.map(JsonNode::textValue);
}

/**
* Returns the list of partition keys.
* @return list of partition keys
Expand Down
4 changes: 4 additions & 0 deletions metacat-main/src/main/java/com/netflix/metacat/main/configs/ServicesConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/
package com.netflix.metacat.main.configs;

import com.netflix.metacat.common.json.MetacatJson;
import com.netflix.metacat.common.server.api.ratelimiter.DefaultRateLimiter;
import com.netflix.metacat.common.server.api.ratelimiter.RateLimiter;
import com.netflix.metacat.common.server.converter.ConverterUtil;
Expand Down Expand Up @@ -191,6 +192,7 @@ public DatabaseService databaseService(
* @param databaseService database service
* @param tagService tag service
* @param userMetadataService user metadata service
* @param metacatJson metacat json utility
* @param eventBus Internal event bus
* @param registry registry handle
* @param config configurations
Expand All @@ -205,6 +207,7 @@ public TableService tableService(
final DatabaseService databaseService,
final TagService tagService,
final UserMetadataService userMetadataService,
final MetacatJson metacatJson,
final MetacatEventBus eventBus,
final Registry registry,
final Config config,
Expand All @@ -217,6 +220,7 @@ public TableService tableService(
databaseService,
tagService,
userMetadataService,
metacatJson,
eventBus,
registry,
config,
Expand Down
134 changes: 75 additions & 59 deletions metacat-main/src/main/java/com/netflix/metacat/main/services/impl/TableServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

package com.netflix.metacat.main.services.impl;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
Expand All @@ -27,6 +26,7 @@
import com.netflix.metacat.common.dto.TableDto;
import com.netflix.metacat.common.exception.MetacatBadRequestException;
import com.netflix.metacat.common.exception.MetacatNotSupportedException;
import com.netflix.metacat.common.json.MetacatJson;
import com.netflix.metacat.common.server.connectors.exception.NotFoundException;
import com.netflix.metacat.common.server.connectors.exception.TableMigrationInProgressException;
import com.netflix.metacat.common.server.connectors.exception.TableNotFoundException;
Expand Down Expand Up @@ -59,6 +59,7 @@
import com.netflix.metacat.main.services.TableService;
import com.netflix.spectator.api.Registry;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang.StringUtils;
import org.springframework.web.context.request.RequestAttributes;
Expand All @@ -80,56 +81,20 @@
* Table service implementation.
*/
@Slf4j
@RequiredArgsConstructor
public class TableServiceImpl implements TableService {
private final ConnectorManager connectorManager;
private final ConnectorTableServiceProxy connectorTableServiceProxy;
private final DatabaseService databaseService;
private final TagService tagService;
private final UserMetadataService userMetadataService;
private final MetacatJson metacatJson;
private final MetacatEventBus eventBus;
private final Registry registry;
private final Config config;
private final ConverterUtil converterUtil;
private final ConnectorTableServiceProxy connectorTableServiceProxy;
private final AuthorizationService authorizationService;

/**
* Constructor.
*
* @param connectorManager Connector manager to use
* @param connectorTableServiceProxy connector table service proxy
* @param databaseService database service
* @param tagService tag service
* @param userMetadataService user metadata service
* @param eventBus Internal event bus
* @param registry registry handle
* @param config configurations
* @param converterUtil utility to convert to/from Dto to connector resources
* @param authorizationService authorization service
*/
public TableServiceImpl(
final ConnectorManager connectorManager,
final ConnectorTableServiceProxy connectorTableServiceProxy,
final DatabaseService databaseService,
final TagService tagService,
final UserMetadataService userMetadataService,
final MetacatEventBus eventBus,
final Registry registry,
final Config config,
final ConverterUtil converterUtil,
final AuthorizationService authorizationService
) {
this.connectorManager = connectorManager;
this.connectorTableServiceProxy = connectorTableServiceProxy;
this.databaseService = databaseService;
this.tagService = tagService;
this.userMetadataService = userMetadataService;
this.eventBus = eventBus;
this.registry = registry;
this.config = config;
this.authorizationService = authorizationService;
this.converterUtil = converterUtil;
}

/**
* {@inheritDoc}
*/
Expand All @@ -139,10 +104,8 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) {
validate(name);
this.authorizationService.checkPermission(metacatRequestContext.getUserName(),
tableDto.getName(), MetacatOperation.CREATE);
//
// Set the owner,if null, with the session user name.
//
setOwnerIfNull(tableDto, metacatRequestContext.getUserName());

setDefaultAttributes(tableDto);
logOwnershipDiagnosticDetails(name, tableDto);

log.info("Creating table {}", name);
Expand Down Expand Up @@ -181,6 +144,12 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) {
return dto;
}

private void setDefaultAttributes(final TableDto tableDto) {
setDefaultSerdeIfNull(tableDto);
setDefaultDefinitionMetadataIfNull(tableDto);
setOwnerIfNull(tableDto);
}

/**
* Logs diagnostic data for debugging invalid owners.
*
Expand All @@ -189,13 +158,9 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) {
*/
private void logOwnershipDiagnosticDetails(final QualifiedName name, final TableDto tableDto) {
try {
final String tableOwner = Optional.ofNullable(tableDto.getDefinitionMetadata())
.map(node -> node.get("owner"))
.map(owner -> owner.get("userId"))
.map(JsonNode::textValue)
.orElse(null);
final String tableOwner = tableDto.getTableOwner().orElse(null);

if (StringUtils.isBlank(tableOwner) || "metacat".equals(tableOwner) || "root".equals(tableOwner)) {
if (!isOwnerValid(tableOwner)) {
registry.counter(
"unauth.user.create.table",
"catalog", name.getCatalogName(),
Expand Down Expand Up @@ -241,24 +206,75 @@ private Map<String, String> getHttpHeaders() {
return requestHeaders;
}

private void setOwnerIfNull(final TableDto tableDto, final String user) {
String owner = user;
private void setDefaultDefinitionMetadataIfNull(final TableDto tableDto) {
ObjectNode definitionMetadata = tableDto.getDefinitionMetadata();
if (definitionMetadata == null) {
definitionMetadata = metacatJson.emptyObjectNode();
tableDto.setDefinitionMetadata(definitionMetadata);
}
}

private void setDefaultSerdeIfNull(final TableDto tableDto) {
StorageDto serde = tableDto.getSerde();
if (serde == null) {
serde = new StorageDto();
tableDto.setSerde(serde);
}
final String serdeOwner = serde.getOwner();
if (Strings.isNullOrEmpty(serdeOwner)) {
serde.setOwner(user);
} else {
owner = serdeOwner;
}

/**
* Sets the owner of the table. The order of priority of selecting the owner is:
* <pre>
* 1. Explicitly set in the table dto
* 2. Username from the request headers
* 3. Owner set in the serde
* </pre>
*
* @param tableDto the table DTO
*/
private void setOwnerIfNull(final TableDto tableDto) {
final String ownerInDto = tableDto.getTableOwner().orElse(null);
if (isOwnerValid(ownerInDto)) {
return;
}

final String userInContext = MetacatContextManager.getContext().getUserName();
if (isOwnerValid(userInContext)) {
updateTableOwner(tableDto, userInContext);
return;
}

final String serdeOwner = tableDto.getSerde().getOwner();
if (isOwnerValid(serdeOwner)) {
updateTableOwner(tableDto, serdeOwner);
}

// At this point, if we still have not found a valid user,
// cycle through the list again and use the first non-null value

if (ownerInDto != null) {
return;
}
if (!Strings.isNullOrEmpty(owner)) {
userMetadataService.populateOwnerIfMissing(tableDto, owner);

if (userInContext != null) {
updateTableOwner(tableDto, userInContext);
return;
}

if (serdeOwner != null) {
updateTableOwner(tableDto, serdeOwner);
}
}

void updateTableOwner(final TableDto tableDto, final String userId) {
final ObjectNode ownerNode = tableDto.getDefinitionMetadata().with("owner");
ownerNode.put("userId", userId);
}

private boolean isOwnerValid(@Nullable final String userId) {
return StringUtils.isNotBlank(userId) && !"metacat".equals(userId) && !"root".equals(userId);
}

@SuppressFBWarnings
private void tag(final QualifiedName name, final ObjectNode definitionMetadata) {
final Set<String> tags = MetacatUtils.getTableTags(definitionMetadata);
Expand Down
49 changes: 45 additions & 4 deletions ...t-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import com.netflix.metacat.common.QualifiedName
import com.netflix.metacat.common.dto.AuditDto
import com.netflix.metacat.common.dto.StorageDto
import com.netflix.metacat.common.dto.TableDto
import com.netflix.metacat.common.json.MetacatJsonLocator
import com.netflix.metacat.common.server.connectors.ConnectorRequestContext
import com.netflix.metacat.common.server.connectors.ConnectorTableService
import com.netflix.metacat.common.server.connectors.exception.InvalidMetadataException
Expand All @@ -39,6 +40,7 @@ import com.netflix.metacat.common.server.spi.MetacatCatalogConfig
import com.netflix.metacat.common.server.usermetadata.DefaultAuthorizationService
import com.netflix.metacat.common.server.usermetadata.TagService
import com.netflix.metacat.common.server.usermetadata.UserMetadataService
import com.netflix.metacat.common.server.util.MetacatContextManager
import com.netflix.metacat.main.manager.ConnectorManager
import com.netflix.metacat.main.services.DatabaseService
import com.netflix.metacat.main.services.GetTableServiceParameters
Expand Down Expand Up @@ -90,7 +92,8 @@ class TableServiceImplSpec extends Specification {
connectorTableServiceProxy = new ConnectorTableServiceProxy(connectorManager, converterUtil)
authorizationService = new DefaultAuthorizationService(config)
service = new TableServiceImpl(connectorManager, connectorTableServiceProxy, databaseService, tagService,
usermetadataService, eventBus, registry, config, converterUtil, authorizationService)
usermetadataService, new MetacatJsonLocator(),
eventBus, registry, config, converterUtil, authorizationService)
}

def testTableGet() {
Expand Down Expand Up @@ -283,16 +286,15 @@ class TableServiceImplSpec extends Specification {
@Unroll
def "test ownership diagnostic logging"() {
given:
def definitionMetadataJson = definitionMetadata == null
? null : (objectMapper.readTree(definitionMetadata as String) as ObjectNode)
def definitionMetadataJson = toObjectNode(definitionMetadata)
tableDto = new TableDto(
name: name,
definitionMetadata: definitionMetadataJson
)
registry = new DefaultRegistry()
TableServiceImpl tableService = new TableServiceImpl(
connectorManager, connectorTableServiceProxy, databaseService, tagService,
usermetadataService, eventBus, registry, config, converterUtil, authorizationService)
usermetadataService, new MetacatJsonLocator(), eventBus, registry, config, converterUtil, authorizationService)

when:
tableService.logOwnershipDiagnosticDetails(name, tableDto)
Expand All @@ -316,4 +318,43 @@ class TableServiceImplSpec extends Specification {
"{\"owner\":{\"userId\":\"metacat\"}}" | "metacat"
"{\"owner\":{\"userId\":\"root\"}}" | "root"
}

@Unroll
def "test default attributes"() {
given:
def tableService = new TableServiceImpl(
connectorManager, connectorTableServiceProxy, databaseService, tagService,
usermetadataService, new MetacatJsonLocator(),
eventBus, new DefaultRegistry(), config, converterUtil, authorizationService)

def initialDefinitionMetadataJson = toObjectNode(initialDefinitionMetadata)
tableDto = new TableDto(
name: name,
definitionMetadata: initialDefinitionMetadataJson,
serde: initialSerde
)

MetacatContextManager.getContext().setUserName(sessionUser)

when:
tableService.setDefaultAttributes(tableDto)

then:
tableDto.getDefinitionMetadata() == toObjectNode(expectedDefMetadata)
tableDto.getSerde() == expectedSerde

where:
initialDefinitionMetadata | sessionUser | initialSerde || expectedDefMetadata | expectedSerde
null | null | null || "{}" | new StorageDto()
null | "ssarma" | null || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto()
"{\"owner\":{\"userId\":\"ssarma\"}}" | "asdf" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto(owner: "swaranga")
"{\"owner\":{\"userId\":\"metacat\"}}" | "ssarma" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto(owner: "swaranga")
"{\"owner\":{\"userId\":\"root\"}}" | "ssarma" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto(owner: "swaranga")
"{\"owner\":{\"userId\":\"root\"}}" | "metacat" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"swaranga\"}}" | new StorageDto(owner: "swaranga")
"{\"owner\":{\"userId\":\"root\"}}" | "metacat" | new StorageDto(owner: "metacat") || "{\"owner\":{\"userId\":\"root\"}}" | new StorageDto(owner: "metacat")
}

ObjectNode toObjectNode(jsonString) {
return jsonString == null ? null : (objectMapper.readTree(jsonString as String) as ObjectNode)
}
}

0 comments on commit 0ef3420

Please sign in to comment.