-
Notifications
You must be signed in to change notification settings - Fork 311
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
[#4993] feat(iceberg): integrate credential framework to iceberg REST server #5134
Changes from all commits
81f620e
a239f0c
63fc419
c872e43
3e35c8e
b72294a
6b37a88
92e2690
81b29e0
d142d02
e05e0e3
bbe673d
491e08d
38f9d6d
135b3cd
8ef9409
bc63bad
d4d4662
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* 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.gravitino.credential; | ||
|
||
public class CredentialConstants { | ||
public static final String CREDENTIAL_PROVIDER_TYPE = "credential-provider-type"; | ||
|
||
private CredentialConstants() {} | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* 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.gravitino.credential; | ||
|
||
import java.util.Map; | ||
|
||
/** | ||
* Helper class to generate specific credential properties for different table format and engine. | ||
*/ | ||
public class CredentialPropertyUtils { | ||
/** | ||
* Transforms a specific credential into a map of Iceberg properties. | ||
* | ||
* @param credential the credential to be transformed into Iceberg properties | ||
* @return a map of Iceberg properties derived from the credential | ||
*/ | ||
public static Map<String, String> toIcebergProperties(Credential credential) { | ||
// todo: transform specific credential to iceberg properties | ||
return credential.toProperties(); | ||
Comment on lines
+34
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This two lines code is not worthy to create a new class here, why can't you put this in iceberg package? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's like a placeholder function to support more credential properties transform logic, this function will be used in client side and Iceberg REST server side, so I place it in common module. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* | ||
* 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.gravitino.credential; | ||
|
||
import com.google.common.base.Preconditions; | ||
import java.io.IOException; | ||
import java.util.Map; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import javax.annotation.Nullable; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class CredentialProviderManager { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(CredentialProviderManager.class); | ||
private Map<String, CredentialProvider> credentialProviders; | ||
|
||
public CredentialProviderManager() { | ||
this.credentialProviders = new ConcurrentHashMap<>(); | ||
} | ||
|
||
public void registerCredentialProvider( | ||
String catalogName, CredentialProvider credentialProvider) { | ||
CredentialProvider current = credentialProviders.putIfAbsent(catalogName, credentialProvider); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we support one catalog with multiple credential vendors? If so, seems this map cannot support it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not in current PR, I prefer to support it later in integrate Credential vending to Gravitino server to have a better overall design. |
||
Preconditions.checkState( | ||
!credentialProvider.equals(current), | ||
String.format( | ||
"Should not register multiple times to CredentialProviderManager, catalog: %s, " | ||
+ "credential provider: %s", | ||
catalogName, credentialProvider.credentialType())); | ||
LOG.info( | ||
"Register catalog:%s credential provider:%s to CredentialProviderManager", | ||
catalogName, credentialProvider.credentialType()); | ||
} | ||
|
||
public void unregisterCredentialProvider(String catalogName) { | ||
CredentialProvider credentialProvider = credentialProviders.remove(catalogName); | ||
// Not all catalog has credential provider | ||
if (credentialProvider != null) { | ||
LOG.info( | ||
"Unregister catalog:{} credential provider:{} to CredentialProviderManager", | ||
catalogName, | ||
credentialProvider.credentialType()); | ||
try { | ||
credentialProvider.close(); | ||
} catch (IOException e) { | ||
LOG.warn("Close credential provider failed", e); | ||
} | ||
} | ||
} | ||
|
||
@Nullable | ||
public CredentialProvider getCredentialProvider(String catalogName) { | ||
return credentialProviders.get(catalogName); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
* 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.gravitino.credential; | ||
|
||
import com.google.common.collect.ImmutableSet; | ||
import org.apache.gravitino.utils.PrincipalUtils; | ||
|
||
public class CredentialUtils { | ||
public static Credential vendCredential(CredentialProvider credentialProvider, String path) { | ||
PathBasedCredentialContext pathBasedCredentialContext = | ||
new PathBasedCredentialContext( | ||
PrincipalUtils.getCurrentUserName(), ImmutableSet.of(path), ImmutableSet.of()); | ||
return credentialProvider.getCredential(pathBasedCredentialContext); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ | |
import com.google.common.collect.ImmutableSet; | ||
import java.sql.Driver; | ||
import java.sql.DriverManager; | ||
import java.util.Collections; | ||
import java.util.Locale; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
|
@@ -104,10 +103,6 @@ public IcebergCatalogWrapper(IcebergConfig icebergConfig) { | |
this.catalogPropertiesMap = icebergConfig.getIcebergCatalogProperties(); | ||
} | ||
|
||
public IcebergCatalogWrapper() { | ||
this(new IcebergConfig(Collections.emptyMap())); | ||
Comment on lines
-107
to
-108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can still maintain this default constructor like: public IcebergCatalogWrapper() {
this(xxxx, false)
} What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's used to test only, seems useless to maintain the default constructor |
||
} | ||
|
||
private void validateNamespace(Optional<Namespace> namespace) { | ||
namespace.ifPresent( | ||
n -> | ||
|
@@ -160,7 +155,7 @@ public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest | |
/** | ||
* Reload hadoop configuration, this is useful when the hadoop configuration UserGroupInformation | ||
* is shared by multiple threads. UserGroupInformation#authenticationMethod was first initialized | ||
* in KerberosClient, however, when switching to iceberg-rest thead, | ||
* in KerberosClient, however, when switching to iceberg-rest thread, | ||
* UserGroupInformation#authenticationMethod will be reset to the default value; we need to | ||
* reinitialize it again. | ||
*/ | ||
|
@@ -271,7 +266,7 @@ public void close() throws Exception { | |
private void closeMySQLCatalogResource() { | ||
try { | ||
// Close thread AbandonedConnectionCleanupThread if we are using `com.mysql.cj.jdbc.Driver`, | ||
// for driver `com.mysql.jdbc.Driver` (deprecated), the daemon thead maybe not this one. | ||
// for driver `com.mysql.jdbc.Driver` (deprecated), the daemon thread maybe not this one. | ||
Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread") | ||
.getMethod("uncheckedShutdown") | ||
.invoke(null); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered that we already defined this in
Credential
, we don't have to define this again here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the same, one is the credential type in
Credential
, this is the credential type in catalog properties. They are happen to the same name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you use a different name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use
credential-provider-type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is a catalog property, it is better to define in catalog-common, so other engines can also use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the property already defined in
catalog-common