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

Fix #1335: add general-purpose Feature flag system #1390

Merged
merged 20 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
975bdc0
Fix #1355: add general-purpose Feature flag system
tatu-at-datastax Sep 4, 2024
15d73d5
Remove Tables-enabled check from NamespaceCache, cannot override on p…
tatu-at-datastax Sep 4, 2024
2674491
Fix unit test wrt now passing access to table schema info
tatu-at-datastax Sep 4, 2024
ba97987
Re-wire enable-tables, temporarily
tatu-at-datastax Sep 4, 2024
4cd7526
Add sysprop for ITs to enable Tables feature
tatu-at-datastax Sep 4, 2024
68a3cfb
Incremental progress...
tatu-at-datastax Sep 4, 2024
a4b10e1
Wire DataApiFeatures in CommandContext for better access
tatu-at-datastax Sep 4, 2024
7a5278c
Complete initial implementation: things now seem to work as designed
tatu-at-datastax Sep 5, 2024
594ebb4
Comment fixes
tatu-at-datastax Sep 5, 2024
4e0fad7
Change API Feature flags to use lower-case names in all config (but U…
tatu-at-datastax Sep 5, 2024
f7920f3
Renaming from code review
tatu-at-datastax Sep 5, 2024
b68176f
More changes from code review
tatu-at-datastax Sep 5, 2024
c3b6032
Merge branch 'main' into tatu/1335-feature-flags
tatu-at-datastax Sep 6, 2024
26ceefa
...
tatu-at-datastax Sep 6, 2024
fa09a86
Fix CreateTableIntegrationTest
tatu-at-datastax Sep 6, 2024
f378b9a
Add an IT to verify blocking of CreateTable if feature flag not enabled
tatu-at-datastax Sep 6, 2024
bb355c7
Complete IT to verify `ApiFeature.TABLES` functioning, header override
tatu-at-datastax Sep 7, 2024
e318d5a
Final touches wrt naming
tatu-at-datastax Sep 7, 2024
be9acd7
Further improve docs (comment)
tatu-at-datastax Sep 7, 2024
7ca1f3e
Last changes from code review
tatu-at-datastax Sep 9, 2024
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
9 changes: 8 additions & 1 deletion CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,17 @@ Other Quarkus properties that are specifically relevant for the service:


## Command level logging configuration
*Configuration for command level logging, defined by [CommandLoggingConfig.java](src/main/java/io/stargate/sgv2/jsonapi/config/CommandLoggingConfig.java).*
*Configuration for command level logging, defined by [CommandLevelLoggingConfig.java](src/main/java/io/stargate/sgv2/jsonapi/config/CommandLoggingConfig.java).*
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved

| Property | Type | Default | Description |
|-----------------------------------------------------|-----------|---------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `stargate.jsonapi.logging.enabled` | `boolean` | `false` | Setting it to `true` enables command level logging. |
| `stargate.jsonapi.logging.only-results-with-errors` | `boolean` | `true` | Setting it to `true` prints the command level info only for the commands where the command result has errors. |
| `stargate.jsonapi.logging.enabled-tenants` | `string` | `ALL` | Comma separated list of tenants for which command level logging should be enabled. Default is a special keyword called `ALL` which prints this log for all tenants |

## API Feature enabling configuration
*Configuration for enabling Features, defined by [FeaturesConfig.java](src/main/java/io/stargate/sgv2/jsonapi/config/CommandLoggingConfig.java).*

| Property | Type | Default | Description |
|---------------------------------|-----------|---------|---------------------------------------------------------------------------------------------------------------------|
| `stargate.feature.flags.tables` | `boolean` | `null` | Setting it to `true` enables Tables functionality; `false` disables; leaving as `null` allows per-request override.|
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@
<dependency>
<groupId>org.junit-pioneer</groupId>
<artifactId>junit-pioneer</artifactId>
<version>2.0.0</version>
<version>2.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.google.common.base.Preconditions;
import io.stargate.sgv2.jsonapi.api.v1.metrics.JsonProcessingMetricsReporter;
import io.stargate.sgv2.jsonapi.config.feature.ApiFeatures;
import io.stargate.sgv2.jsonapi.exception.ErrorCodeV1;
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.*;
import io.stargate.sgv2.jsonapi.service.embedding.operation.EmbeddingProvider;
Expand All @@ -16,7 +17,8 @@ public record CommandContext<T extends SchemaObject>(
T schemaObject,
EmbeddingProvider embeddingProvider,
String commandName,
JsonProcessingMetricsReporter jsonProcessingMetricsReporter) {
JsonProcessingMetricsReporter jsonProcessingMetricsReporter,
ApiFeatures apiFeatures) {

// TODO: this is what the original EMPTY had, no idea why the name of the command is missing
// this is used by the GeneralResource
Expand All @@ -40,26 +42,31 @@ public static <T extends SchemaObject> CommandContext<T> forSchemaObject(
T schemaObject,
EmbeddingProvider embeddingProvider,
String commandName,
JsonProcessingMetricsReporter jsonProcessingMetricsReporter) {
JsonProcessingMetricsReporter jsonProcessingMetricsReporter,
ApiFeatures apiFeatures) {

// TODO: upgrade to use the modern switch statements
// TODO: how to remove the unchecked cast ? Had to use unchecked cast to get back to the
// CommandContext<T>
if (schemaObject instanceof CollectionSchemaObject cso) {
return (CommandContext<T>)
forSchemaObject(cso, embeddingProvider, commandName, jsonProcessingMetricsReporter);
forSchemaObject(
cso, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}
if (schemaObject instanceof TableSchemaObject tso) {
return (CommandContext<T>)
forSchemaObject(tso, embeddingProvider, commandName, jsonProcessingMetricsReporter);
forSchemaObject(
tso, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}
if (schemaObject instanceof KeyspaceSchemaObject kso) {
return (CommandContext<T>)
forSchemaObject(kso, embeddingProvider, commandName, jsonProcessingMetricsReporter);
forSchemaObject(
kso, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}
if (schemaObject instanceof DatabaseSchemaObject dso) {
return (CommandContext<T>)
forSchemaObject(dso, embeddingProvider, commandName, jsonProcessingMetricsReporter);
forSchemaObject(
dso, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}
throw ErrorCodeV1.SERVER_INTERNAL_ERROR.toApiException(
"Unknown schema object type: %s", schemaObject.getClass().getName());
Expand All @@ -79,9 +86,10 @@ public static CommandContext<CollectionSchemaObject> forSchemaObject(
CollectionSchemaObject schemaObject,
EmbeddingProvider embeddingProvider,
String commandName,
JsonProcessingMetricsReporter jsonProcessingMetricsReporter) {
JsonProcessingMetricsReporter jsonProcessingMetricsReporter,
ApiFeatures apiFeatures) {
return new CommandContext<>(
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter);
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}

/**
Expand All @@ -98,9 +106,10 @@ public static CommandContext<TableSchemaObject> forSchemaObject(
TableSchemaObject schemaObject,
EmbeddingProvider embeddingProvider,
String commandName,
JsonProcessingMetricsReporter jsonProcessingMetricsReporter) {
JsonProcessingMetricsReporter jsonProcessingMetricsReporter,
ApiFeatures apiFeatures) {
return new CommandContext<>(
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter);
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}

/**
Expand All @@ -117,9 +126,10 @@ public static CommandContext<KeyspaceSchemaObject> forSchemaObject(
KeyspaceSchemaObject schemaObject,
EmbeddingProvider embeddingProvider,
String commandName,
JsonProcessingMetricsReporter jsonProcessingMetricsReporter) {
JsonProcessingMetricsReporter jsonProcessingMetricsReporter,
ApiFeatures apiFeatures) {
return new CommandContext<>(
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter);
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}

/**
Expand All @@ -136,9 +146,10 @@ public static CommandContext<DatabaseSchemaObject> forSchemaObject(
DatabaseSchemaObject schemaObject,
EmbeddingProvider embeddingProvider,
String commandName,
JsonProcessingMetricsReporter jsonProcessingMetricsReporter) {
JsonProcessingMetricsReporter jsonProcessingMetricsReporter,
ApiFeatures apiFeatures) {
return new CommandContext<>(
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter);
schemaObject, embeddingProvider, commandName, jsonProcessingMetricsReporter, apiFeatures);
}

@SuppressWarnings("unchecked")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public class DataApiRequestInfo {
private final Optional<String> tenantId;
private final Optional<String> cassandraToken;
private final EmbeddingCredentials embeddingCredentials;
private final HttpHeaderAccess httpHeaders;
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved

/**
* Constructor that will be useful in the offline library mode, where only the tenant will be set
Expand All @@ -29,6 +30,7 @@ public DataApiRequestInfo(Optional<String> tenantId) {
this.tenantId = tenantId;
this.cassandraToken = Optional.empty();
this.embeddingCredentials = null;
httpHeaders = null;
}

@Inject
Expand All @@ -41,6 +43,7 @@ public DataApiRequestInfo(
this.embeddingCredentials = apiKeysResolver.get().resolveEmbeddingCredentials(routingContext);
this.tenantId = (tenantResolver.get()).resolve(routingContext, securityContext);
this.cassandraToken = (tokenResolver.get()).resolve(routingContext, securityContext);
httpHeaders = new HttpHeaderAccess(routingContext.request().headers());
}

public Optional<String> getTenantId() {
Expand All @@ -54,4 +57,40 @@ public Optional<String> getCassandraToken() {
public EmbeddingCredentials getEmbeddingCredentials() {
return this.embeddingCredentials;
}

public HttpHeaderAccess getHttpHeaders() {
return this.httpHeaders;
}

/**
* Simple wrapper around internal HTTP header container, providing safe(r) access to typed header
* values. Minimal API, currently mainly used for feature flags.
*/
public static class HttpHeaderAccess {
private final io.vertx.core.MultiMap headers;

public HttpHeaderAccess(io.vertx.core.MultiMap headers) {
this.headers = headers;
}

/**
* Accessor for getting value of given header, as {@code Boolean} if (and only if!) value is one
* of "true" or "false". Access by name is (and has to be) case-insensitive as per HTTP
* standard.
*
* @param headerName Name of header to check
* @return Boolean.TRUE if header value is "true", Boolean.FALSE if "false", or null if not
*/
public Boolean getHeaderAsBoolean(String headerName) {
String str = headers.get(headerName);
// Only consider strict "true" and "false"; ignore other values
if ("true".equals(str)) {
return Boolean.TRUE;
}
if ("false".equals(str)) {
return Boolean.FALSE;
}
return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,14 @@
import io.stargate.sgv2.jsonapi.api.request.DataApiRequestInfo;
import io.stargate.sgv2.jsonapi.api.v1.metrics.JsonProcessingMetricsReporter;
import io.stargate.sgv2.jsonapi.config.constants.OpenApiConstants;
import io.stargate.sgv2.jsonapi.config.feature.ApiFeature;
import io.stargate.sgv2.jsonapi.config.feature.ApiFeatures;
import io.stargate.sgv2.jsonapi.config.feature.FeaturesConfig;
import io.stargate.sgv2.jsonapi.exception.ErrorCodeV1;
import io.stargate.sgv2.jsonapi.exception.JsonApiException;
import io.stargate.sgv2.jsonapi.exception.mappers.ThrowableCommandResultSupplier;
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.SchemaCache;
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.SchemaObject;
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.VectorConfig;
import io.stargate.sgv2.jsonapi.service.embedding.operation.EmbeddingProvider;
import io.stargate.sgv2.jsonapi.service.embedding.operation.EmbeddingProviderFactory;
Expand Down Expand Up @@ -68,6 +73,8 @@ public class CollectionResource {

@Inject private DataApiRequestInfo dataApiRequestInfo;

@Inject FeaturesConfig apiFeatureConfig;

@Inject private JsonProcessingMetricsReporter jsonProcessingMetricsReporter;

@Inject
Expand Down Expand Up @@ -181,6 +188,14 @@ public Uni<RestResponse<CommandResult>> postCommand(
return Uni.createFrom().item(new ThrowableCommandResultSupplier(error));
} else {
// TODO No need for the else clause here, simplify
final ApiFeatures apiFeatures =
ApiFeatures.fromConfigAndRequest(
apiFeatureConfig, dataApiRequestInfo.getHttpHeaders());
if ((schemaObject.type == SchemaObject.SchemaObjectType.TABLE)
&& !apiFeatures.isFeatureEnabled(ApiFeature.TABLES)) {
return Uni.createFrom()
.failure(ErrorCodeV1.TABLE_FEATURE_NOT_ENABLED.toApiException());
}
// TODO: refactor this code to be cleaner so it assigns on one line
EmbeddingProvider embeddingProvider = null;
final VectorConfig.VectorizeConfig vectorizeConfig =
Expand All @@ -203,7 +218,8 @@ public Uni<RestResponse<CommandResult>> postCommand(
schemaObject,
embeddingProvider,
command.getClass().getSimpleName(),
jsonProcessingMetricsReporter);
jsonProcessingMetricsReporter,
apiFeatures);

return meteredCommandProcessor.processCommand(
dataApiRequestInfo, commandContext, command);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import io.stargate.sgv2.jsonapi.api.model.command.impl.CreateNamespaceCommand;
import io.stargate.sgv2.jsonapi.api.request.DataApiRequestInfo;
import io.stargate.sgv2.jsonapi.config.constants.OpenApiConstants;
import io.stargate.sgv2.jsonapi.config.feature.ApiFeatures;
import io.stargate.sgv2.jsonapi.config.feature.FeaturesConfig;
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.DatabaseSchemaObject;
import io.stargate.sgv2.jsonapi.service.processor.MeteredCommandProcessor;
import jakarta.inject.Inject;
Expand Down Expand Up @@ -37,6 +39,8 @@ public class GeneralResource {

@Inject private DataApiRequestInfo dataApiRequestInfo;

@Inject FeaturesConfig apiFeatureConfig;

public static final String BASE_PATH = "/v1";

private final MeteredCommandProcessor meteredCommandProcessor;
Expand Down Expand Up @@ -75,10 +79,16 @@ public GeneralResource(MeteredCommandProcessor meteredCommandProcessor) {
})))
@POST
public Uni<RestResponse<CommandResult>> postCommand(@NotNull @Valid GeneralCommand command) {
final ApiFeatures apiFeatures =
ApiFeatures.fromConfigAndRequest(apiFeatureConfig, dataApiRequestInfo.getHttpHeaders());

var commandContext =
CommandContext.forSchemaObject(
new DatabaseSchemaObject(), null, command.getClass().getSimpleName(), null);
new DatabaseSchemaObject(),
null,
command.getClass().getSimpleName(),
null,
apiFeatures);

return meteredCommandProcessor
.processCommand(dataApiRequestInfo, commandContext, command)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
import io.stargate.sgv2.jsonapi.api.model.command.impl.DeleteCollectionCommand;
import io.stargate.sgv2.jsonapi.api.model.command.impl.FindCollectionsCommand;
import io.stargate.sgv2.jsonapi.api.request.DataApiRequestInfo;
import io.stargate.sgv2.jsonapi.config.ApiTablesConfig;
import io.stargate.sgv2.jsonapi.config.constants.OpenApiConstants;
import io.stargate.sgv2.jsonapi.config.feature.ApiFeature;
import io.stargate.sgv2.jsonapi.config.feature.ApiFeatures;
import io.stargate.sgv2.jsonapi.config.feature.FeaturesConfig;
import io.stargate.sgv2.jsonapi.exception.ErrorCodeV1;
import io.stargate.sgv2.jsonapi.exception.mappers.ThrowableCommandResultSupplier;
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.KeyspaceSchemaObject;
Expand Down Expand Up @@ -51,7 +53,7 @@ public class NamespaceResource {

@Inject private DataApiRequestInfo dataApiRequestInfo;

@Inject ApiTablesConfig apiTablesConfig;
@Inject FeaturesConfig apiFeatureConfig;

@Inject
public NamespaceResource(MeteredCommandProcessor meteredCommandProcessor) {
Expand Down Expand Up @@ -106,22 +108,26 @@ public Uni<RestResponse<CommandResult>> postCommand(
@Size(min = 1, max = 48)
String namespace) {

if (command instanceof TableOnlyCommand && !apiTablesConfig.enabled()) {
return Uni.createFrom()
.item(
new ThrowableCommandResultSupplier(
ErrorCodeV1.TABLE_FEATURE_NOT_ENABLED.toApiException()))
.map(commandResult -> commandResult.toRestResponse());
}
final ApiFeatures apiFeatures =
ApiFeatures.fromConfigAndRequest(apiFeatureConfig, dataApiRequestInfo.getHttpHeaders());

// create context
// TODO: Aaron , left here to see what CTOR was used, there was a lot of different ones.
// CommandContext commandContext = new CommandContext(namespace, null);
// HACK TODO: The above did not set a command name on the command context, how did that work ?
CommandContext<KeyspaceSchemaObject> commandContext =
new CommandContext<>(new KeyspaceSchemaObject(namespace), null, "", null);
new CommandContext<>(new KeyspaceSchemaObject(namespace), null, "", null, apiFeatures);

// Need context first to check if feature is enabled
if (command instanceof TableOnlyCommand && !apiFeatures.isFeatureEnabled(ApiFeature.TABLES)) {
return Uni.createFrom()
.item(
new ThrowableCommandResultSupplier(
ErrorCodeV1.TABLE_FEATURE_NOT_ENABLED.toApiException()))
.map(commandResult -> commandResult.toRestResponse());
}

// call processor
// call processor
return meteredCommandProcessor
.processCommand(dataApiRequestInfo, commandContext, command)
// map to 2xx unless overridden by error
Expand Down

This file was deleted.

Loading