jbonofre commented on code in PR #1532: URL: https://github.com/apache/polaris/pull/1532#discussion_r2086123649
########## quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReadinessConfiguration.java: ########## @@ -0,0 +1,36 @@ +/* + * 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.polaris.service.quarkus.config; + +import io.quarkus.runtime.annotations.StaticInitSafe; +import io.smallrye.config.ConfigMapping; +import io.smallrye.config.WithDefault; + +@StaticInitSafe +@ConfigMapping(prefix = "polaris.readiness") +public interface QuarkusReadinessConfiguration { + + /** + * Setting this to {@code true} means that Polaris will start up even if severe security risks + * have been detected, accepting the risk of denial-of-service, data-loss, corruption and other + * risks. + */ + @WithDefault("false") + boolean ignoreSevereIssues(); Review Comment: If I generally agree with the last comments, specifically in this PR, I consider the changes "related": several/rendering messages is related to clearly inform the users about allowing insecure file types. So, it's fair to "group" related changes in one PR in this case. Another option would have been to just focus on "warning messages" (not rendered) when allowing insecure file types. As I'm sure that 90% of the users won't use insecure file types (and use the default configuration), a warning (not seen 90% of the time) would be sufficient. ########## quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java: ########## @@ -55,26 +62,53 @@ public class ProductionReadinessChecks { */ private static final String WARNING_SIGN_UTF_8 = "\u0000\u26A0\uFE0F"; + private static final String SEVERE_SIGN_UTF_8 = "\u0000\uD83D\uDED1"; Review Comment: As I think the changes are "related", I think it's fair to do the changes in one PR (even if it's just "display/rendering" messages). ########## polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java: ########## @@ -234,4 +233,36 @@ public static void enforceFeatureEnabledOrThrow( .description("If true, the policy-store endpoints are enabled") .defaultValue(true) .buildFeatureConfiguration(); + + public static final FeatureConfiguration<Boolean> ALLOW_SPECIFYING_FILE_IO_IMPL = + PolarisConfiguration.<Boolean>builder() + .key("ALLOW_SPECIFYING_FILE_IO_IMPL") + .description( + "Config key for whether to allow setting the FILE_IO_IMPL using catalog properties. " + + "Must only be enabled in dev/test environments, should not be in production systems.") + .defaultValue(false) + .buildFeatureConfiguration(); + + public static final FeatureConfiguration<Boolean> ALLOW_INSECURE_STORAGE_TYPES = Review Comment: As this is a user facing property, we need to be consistent (hard to change later). I think this property is good enough as it's generic to any storage type (not only `FILE`). ########## quarkus/defaults/src/main/resources/application.properties: ########## @@ -109,7 +109,7 @@ polaris.realm-context.header-name=Polaris-Realm polaris.realm-context.require-header=false polaris.features.defaults."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false -polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE","FILE"] +polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE"] Review Comment: Nit: For clarify, I wonder if we should not add `polaris.features.defaults."ALLOW_INSECURE_STORAGE_TYPES"=false` here. -- 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]
