xiangfu0 commented on code in PR #15226: URL: https://github.com/apache/pinot/pull/15226#discussion_r1996153933
########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { + + /** + * Stores a secret in the secret management system. + * + * @param secretName A unique identifier for the secret, typically following a hierarchical + * naming pattern (e.g., "pinot/tables/myTable/credentials") + * @param secretValue The actual secret value to be securely stored + * @param metadata Additional metadata to associate with the secret (e.g., tableName, type) + * @return A reference key that can be used later to retrieve the secret + * @throws SecretStoreException If the secret cannot be stored due to connectivity issues, + * permission problems, or other errors + */ + String storeSecret(String secretName, String secretValue, Map<String, String> metadata); Review Comment: is `secretName` same as `secretKey`? Let's keep the convention same. ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { + + /** + * Stores a secret in the secret management system. + * + * @param secretName A unique identifier for the secret, typically following a hierarchical + * naming pattern (e.g., "pinot/tables/myTable/credentials") + * @param secretValue The actual secret value to be securely stored + * @param metadata Additional metadata to associate with the secret (e.g., tableName, type) + * @return A reference key that can be used later to retrieve the secret + * @throws SecretStoreException If the secret cannot be stored due to connectivity issues, + * permission problems, or other errors + */ + String storeSecret(String secretName, String secretValue, Map<String, String> metadata); Review Comment: Missing `throw SecretStoreException` statement here. ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { Review Comment: I feel we also want to add `listSecrets()` or `exists(secretKey)` Methods ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStoreException.java: ########## @@ -0,0 +1,55 @@ +/** + * 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.pinot.spi.secretstore; + +/** + * Exception thrown when operations on the {@link SecretStore} fail. + * This exception encapsulates errors that may occur during secret storage, + * retrieval, updating, or deletion operations. + */ +public class SecretStoreException extends RuntimeException { Review Comment: Instead of catching all errors under SecretStoreException, you might want to add errorCode and a few exceptions to extend: • SecretNotFoundException • SecretPermissionException • SecretStoreConnectionException ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { + + /** + * Stores a secret in the secret management system. + * + * @param secretName A unique identifier for the secret, typically following a hierarchical + * naming pattern (e.g., "pinot/tables/myTable/credentials") + * @param secretValue The actual secret value to be securely stored + * @param metadata Additional metadata to associate with the secret (e.g., tableName, type) + * @return A reference key that can be used later to retrieve the secret + * @throws SecretStoreException If the secret cannot be stored due to connectivity issues, + * permission problems, or other errors + */ + String storeSecret(String secretName, String secretValue, Map<String, String> metadata); + + /** + * Retrieves a secret from the secret management system. + * + * @param secretKey The reference key obtained when the secret was stored + * @return The actual secret value + * @throws SecretStoreException If the secret cannot be retrieved or doesn't exist + */ + String getSecret(String secretKey); Review Comment: Missing `throw SecretStoreException` statement here. ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { + + /** + * Stores a secret in the secret management system. + * + * @param secretName A unique identifier for the secret, typically following a hierarchical + * naming pattern (e.g., "pinot/tables/myTable/credentials") + * @param secretValue The actual secret value to be securely stored + * @param metadata Additional metadata to associate with the secret (e.g., tableName, type) + * @return A reference key that can be used later to retrieve the secret + * @throws SecretStoreException If the secret cannot be stored due to connectivity issues, + * permission problems, or other errors + */ + String storeSecret(String secretName, String secretValue, Map<String, String> metadata); + + /** + * Retrieves a secret from the secret management system. + * + * @param secretKey The reference key obtained when the secret was stored + * @return The actual secret value + * @throws SecretStoreException If the secret cannot be retrieved or doesn't exist + */ + String getSecret(String secretKey); + + /** + * Updates an existing secret with a new value. + * + * @param secretKey The reference key for the secret to be updated + * @param newSecretValue The new value to store + * @param metadata Updated metadata to associate with the secret + * @throws SecretStoreException If the secret cannot be updated or doesn't exist + */ + void updateSecret(String secretKey, String newSecretValue, Map<String, String> metadata); Review Comment: Is there a way to handle concurrent update? Feeling we might need to define a version with the secret. So the implementation could be get the current version then use version to update the underly secret store. ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { + + /** + * Stores a secret in the secret management system. + * + * @param secretName A unique identifier for the secret, typically following a hierarchical + * naming pattern (e.g., "pinot/tables/myTable/credentials") + * @param secretValue The actual secret value to be securely stored + * @param metadata Additional metadata to associate with the secret (e.g., tableName, type) + * @return A reference key that can be used later to retrieve the secret + * @throws SecretStoreException If the secret cannot be stored due to connectivity issues, + * permission problems, or other errors + */ + String storeSecret(String secretName, String secretValue, Map<String, String> metadata); + + /** + * Retrieves a secret from the secret management system. + * + * @param secretKey The reference key obtained when the secret was stored + * @return The actual secret value + * @throws SecretStoreException If the secret cannot be retrieved or doesn't exist + */ + String getSecret(String secretKey); + + /** + * Updates an existing secret with a new value. + * + * @param secretKey The reference key for the secret to be updated + * @param newSecretValue The new value to store + * @param metadata Updated metadata to associate with the secret + * @throws SecretStoreException If the secret cannot be updated or doesn't exist + */ + void updateSecret(String secretKey, String newSecretValue, Map<String, String> metadata); + + /** + * Deletes a secret when it is no longer needed. + * + * This method should be called when the associated resource (e.g., a table or connection) + * is being deleted to ensure proper cleanup of sensitive information. + * + * @param secretKey The reference key for the secret to be deleted + * @throws SecretStoreException If the secret cannot be deleted or doesn't exist + */ + void deleteSecret(String secretKey); Review Comment: Missing `throw SecretStoreException` statement here. ########## pinot-spi/src/main/java/org/apache/pinot/spi/secretstore/SecretStore.java: ########## @@ -0,0 +1,77 @@ +/** + * 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.pinot.spi.secretstore; + +import java.util.Map; + +/** + * Interface for managing secrets in Apache Pinot. + * + * This interface abstracts away the details of the underlying secret storage mechanism, + * allowing Pinot to work with various secret management systems like AWS Secrets Manager, + * HashiCorp Vault, or other custom implementations. + * + * Implementations of this interface should handle all aspects of secret management including + * secure storage, retrieval, and cleanup of sensitive information such as connection credentials. + */ +public interface SecretStore { + + /** + * Stores a secret in the secret management system. + * + * @param secretName A unique identifier for the secret, typically following a hierarchical + * naming pattern (e.g., "pinot/tables/myTable/credentials") + * @param secretValue The actual secret value to be securely stored + * @param metadata Additional metadata to associate with the secret (e.g., tableName, type) + * @return A reference key that can be used later to retrieve the secret + * @throws SecretStoreException If the secret cannot be stored due to connectivity issues, + * permission problems, or other errors + */ + String storeSecret(String secretName, String secretValue, Map<String, String> metadata); + + /** + * Retrieves a secret from the secret management system. + * + * @param secretKey The reference key obtained when the secret was stored + * @return The actual secret value + * @throws SecretStoreException If the secret cannot be retrieved or doesn't exist + */ + String getSecret(String secretKey); + + /** + * Updates an existing secret with a new value. + * + * @param secretKey The reference key for the secret to be updated + * @param newSecretValue The new value to store + * @param metadata Updated metadata to associate with the secret + * @throws SecretStoreException If the secret cannot be updated or doesn't exist + */ + void updateSecret(String secretKey, String newSecretValue, Map<String, String> metadata); Review Comment: Missing `throw SecretStoreException` statement 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
