frankgh commented on code in PR #264: URL: https://github.com/apache/cassandra-sidecar/pull/264#discussion_r2461891742
########## integration-tests/src/integrationTest/org/apache/cassandra/sidecar/routes/v2/V2NodeSettingsIntegrationTest.java: ########## @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.sidecar.routes.v2; + +import java.util.HashMap; +import java.util.Map; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import io.vertx.core.VertxException; +import io.vertx.core.buffer.Buffer; +import io.vertx.core.http.HttpResponseExpectation; +import io.vertx.ext.web.client.HttpResponse; +import org.apache.cassandra.sidecar.common.response.v2.V2NodeSettings; +import org.apache.cassandra.sidecar.testing.SharedClusterSidecarIntegrationTestBase; + +import static io.netty.handler.codec.http.HttpResponseStatus.SERVICE_UNAVAILABLE; +import static org.apache.cassandra.testing.utils.AssertionUtils.getBlocking; +import static org.apache.cassandra.testing.utils.AssertionUtils.loopAssert; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * V2NodeSettingsIntegrationTest is responsible for verifying the behavior of the /api/v2/cassandra/settings + * endpoint. This includes: + * - Node settings are returned when the node is healthy. + * - A specific error is returned when CQL is not healthy. + * - A separate error is returned when the node is down. + */ +public class V2NodeSettingsIntegrationTest extends SharedClusterSidecarIntegrationTestBase +{ + + @Test + public void testV2NodeSettings() + { + ensureSettingsAvailable(); + + // Disabling NTR should make CQL settings become unavailable + cluster.getFirstRunningInstance().nodetool("disablebinary"); + ensureSettingsBecomeUnavailable("CQL NodeSettings unavailable"); + + // Re-enable NTR, settings should become available again. + cluster.getFirstRunningInstance().nodetool("enablebinary"); + ensureSettingsAvailable(); + + // Changing a configuration should eventually reflect in settings API. + cluster.getFirstRunningInstance().nodetool("setconcurrency", "READ", "10"); + ensureSettingsAvailable(); Review Comment: I think during this verification we need to make sure that the value for concurrency is changed. The current validation is not really making sure this value is 10, so maybe we can add validation for that? ########## adapters/adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraAdapter.java: ########## @@ -92,6 +93,13 @@ public NodeSettings nodeSettings() throw new UnsupportedOperationException("Node settings are not provided by this adapter"); } + @Override + @NotNull + public Map<String, String> cqlNodeSettings() Review Comment: This is a smell, we are leaking implementation details to the interface. Maybe for the legacy node settings method we can rename it internally as `legacyNodeSettings` or `v1NodeSettings`? Alternatively, we can name this method `v2NodeSettings`? What do you think? ########## server/src/main/java/org/apache/cassandra/sidecar/CassandraSidecarDaemon.java: ########## @@ -73,12 +73,12 @@ static boolean close(Server app) app.close() .toCompletionStage() .toCompletableFuture() - .get(1, TimeUnit.MINUTES); + .get(2, TimeUnit.MINUTES); Review Comment: any reason we want to change this value? ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/BasicPermissions.java: ########## @@ -88,4 +88,8 @@ public class BasicPermissions // Lifecycle permissions public static final Permission READ_LIFECYCLE = new DomainAwarePermission("LIFECYCLE:READ", CLUSTER_SCOPE); public static final Permission MODIFY_LIFECYCLE = new DomainAwarePermission("LIFECYCLE:MODIFY", CLUSTER_SCOPE); + + // Cassandra Settings Permissions + // Caller must have been granted SELECT on `system_view.settings` to hit the /api/v2/cassandra/settings API. + public static final Permission READ_SETTINGS = new StandardPermission("SELECT", TABLE_SCOPE); Review Comment: I think we will not need this change ########## client-common/src/main/java/org/apache/cassandra/sidecar/common/response/v2/V2NodeSettings.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.sidecar.common.response.v2; + +import java.util.Map; +import java.util.Objects; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.cassandra.sidecar.common.DataObjectBuilder; +import org.apache.cassandra.sidecar.common.response.NodeSettings; + +/** + * V2NodeSettings stores settings information for Cassandra. + */ +public class V2NodeSettings +{ + // Contains Cassandra node settings from system_views.setting table. + @JsonProperty("nodeSettings") + private final Map<String, String> nodeSettings; + + /** + * Constructs a new {@link NodeSettings}. + */ + public V2NodeSettings() + { + this(builder()); + } + + public V2NodeSettings(V2NodeSettings.Builder builder) + { + this.nodeSettings = builder.nodeSettings; + + } + + @JsonProperty("nodeSettings") + public Map<String, String> nodeSettings() + { + return nodeSettings; + } + + public static Builder builder() + { + return new Builder(); + } + + @Override + public boolean equals(Object o) + { + if (o == null || getClass() != o.getClass()) return false; + V2NodeSettings that = (V2NodeSettings) o; + return Objects.equals(nodeSettings, that.nodeSettings); + } + + @Override + public int hashCode() + { + return Objects.hash(nodeSettings); + } + + /** + * A builder class to enable construction of V2NodeSettings objects. + */ + public static final class Builder implements DataObjectBuilder<Builder, V2NodeSettings> Review Comment: even though I love the builder pattern, it seems like for this use case it is not really necessary. I would just keep a constructor ``` @JsonCreator public V2NodeSettings(@JsonProperty("nodeSettings") Map<String, String> nodeSettings) { this.nodeSettings = nodeSettings; } ``` ########## server/src/main/java/org/apache/cassandra/sidecar/db/SystemViewsDatabaseAccessor.java: ########## @@ -94,12 +95,24 @@ public Long cdcTotalSpaceBytesSetting() throws SchemaUnavailableException public Map<String, String> getSettings(String... names) throws SchemaUnavailableException { BoundStatement statement = tableSchema.selectSettings().bind(Arrays.asList(names)); + return querySettings(statement); + } + + @NotNull + public Map<String, String> getSettings() throws SchemaUnavailableException Review Comment: NIT, maybe we can rename it to something more descriptive like `allSettings` ```suggestion public Map<String, String> allSettings() throws SchemaUnavailableException ``` ########## server/src/main/java/org/apache/cassandra/sidecar/handlers/v2/cassandra/V2NodeSettingsHandler.java: ########## @@ -0,0 +1,76 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.sidecar.handlers.v2.cassandra; + +import java.util.Map; +import java.util.Set; + +import com.google.inject.Inject; +import io.vertx.core.http.HttpServerRequest; +import io.vertx.core.net.SocketAddress; +import io.vertx.ext.auth.authorization.Authorization; +import io.vertx.ext.web.RoutingContext; +import org.apache.cassandra.sidecar.acl.authorization.BasicPermissions; +import org.apache.cassandra.sidecar.common.response.v2.V2NodeSettings; +import org.apache.cassandra.sidecar.concurrent.ExecutorPools; +import org.apache.cassandra.sidecar.handlers.AbstractHandler; +import org.apache.cassandra.sidecar.handlers.AccessProtected; +import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher; +import org.jetbrains.annotations.NotNull; + +/** + * V2NodeSettingsHandler is responsible for providing access to the configurations of the + * Cassandra instances managed by this sidecar. This includes settings accessible via CQL from the system_views.settings table. + */ +public class V2NodeSettingsHandler extends AbstractHandler<Void> implements AccessProtected +{ + + /** + * Constructs a handler with the provided {@code metadataFetcher} + * + * @param metadataFetcher the interface to retrieve instance metadata + */ + @Inject + V2NodeSettingsHandler(InstanceMetadataFetcher metadataFetcher, ExecutorPools executorPools) + { + super(metadataFetcher, executorPools, null); + } + + @Override + protected Void extractParamsOrThrow(RoutingContext context) + { + return null; + } + + @Override + protected void handleInternal(RoutingContext context, HttpServerRequest httpRequest, @NotNull String host, SocketAddress remoteAddress, Void request) + { + Map<String, String> cqlSettings = metadataFetcher.delegate(host).cqlNodeSettings(); + V2NodeSettings v2nodeSettings = V2NodeSettings.builder() + .nodeSettings(cqlSettings) + .build(); + context.json(v2nodeSettings); + } + + @Override + public Set<Authorization> requiredAuthorizations() + { + Authorization authorization = BasicPermissions.READ_SETTINGS.toAuthorization("data/system_views/settings"); + return Set.of(authorization); Review Comment: I think this is not correct, I checked with @sarankk and she suggested we do the following instead: ```suggestion Set<String> eligibleResources = Set.of(DATA_SCOPE.variableAwareResource(), // Keyspace access to system_views "data/system_views", // Access to all tables in keyspace system_views "data/system_views/*", // Access to the settings table in the system_views keyspace "data/system_views/settings"); return Set.of(CassandraPermissions.SELECT.toAuthorization(eligibleResources)); ``` ########## server/src/main/java/org/apache/cassandra/sidecar/modules/multibindings/VertxRouteMapKeys.java: ########## @@ -88,6 +89,11 @@ interface CassandraNodeSettingsRouteKey extends RouteClassKey HttpMethod HTTP_METHOD = HttpMethod.GET; String ROUTE_URI = ApiEndpointsV1.NODE_SETTINGS_ROUTE; } + interface V2CassandraNodeSettingsRouteKey extends RouteClassKey Review Comment: let's preserve the alphabetical order here as mentioned in https://github.com/apache/cassandra-sidecar/blob/trunk/server/src/main/java/org/apache/cassandra/sidecar/modules/multibindings/VertxRouteMapKeys.java#L35 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
