-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[core] Support auth in REST Catalog #4648
Conversation
import java.time.Duration; | ||
|
||
/** Options for REST Catalog Auth. */ | ||
public class AuthOptions { |
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 is better to move all options to RESTCatalogOptions
.
.noDefaultValue() | ||
.withDescription("REST Catalog auth token."); | ||
public static final ConfigOption<Duration> TOKEN_EXPIRES_IN = | ||
ConfigOptions.key("token-expires-in") |
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.
token.expiration-time
.durationType() | ||
.defaultValue(Duration.ofHours(1)) | ||
.withDescription("REST Catalog auth token expires in."); | ||
public static final ConfigOption<Boolean> TOKEN_REFRESH_ENABLED = |
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.
Do we really need this option? Maybe just remove this.
.defaultValue(false) | ||
.withDescription("REST Catalog auth token refresh enable."); | ||
public static final ConfigOption<String> TOKEN_FILE_PATH = | ||
ConfigOptions.key("token-file-path") |
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.
token.provider.path
?
.noDefaultValue() | ||
.withDescription("REST Catalog auth token file path."); | ||
public static final ConfigOption<String> CREDENTIALS_PROVIDER = | ||
ConfigOptions.key("credentials_provider") |
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 seems we don't need to provide this now, we can infer the provider from token.provider.path
option.
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.
Move to RESTCatalogInternalOptions, if the user defines this, will use it. If not we'll use the option to get provider.
private String getTokenFromFile() { | ||
try { | ||
// todo: handle exception | ||
return new String(Files.readAllBytes(Paths.get(tokenFilePath)), StandardCharsets.UTF_8); |
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.
FileIOUtils.readFileUtf8
|
||
private String getTokenFromFile() { | ||
try { | ||
// todo: handle exception |
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.
remove this todo.
// todo: handle exception | ||
return new String(Files.readAllBytes(Paths.get(tokenFilePath)), StandardCharsets.UTF_8); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); |
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.
throw new UncheckIOException
@Override | ||
public boolean refresh() { | ||
long start = System.currentTimeMillis(); | ||
this.token = getTokenFromFile(); |
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.
Do not update this field if refresh fails?
68c69dd
to
858f13b
Compare
…ROVIDER to RESTCatalogInternalOptions
…vider
bddc7ea
to
314f965
Compare
…class
…ialsProvider
…CatalogOptions
int threadNum, String namePrefix) { | ||
return new ScheduledThreadPoolExecutor( | ||
threadNum, | ||
new ThreadFactoryBuilder().setDaemon(true).setNameFormat(namePrefix).build()); |
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.
newDaemonThreadFactory?
private ResourcePaths resourcePaths; | ||
private Map<String, String> options; | ||
private Map<String, String> baseHeader; | ||
// a lazy thread pool for token refresh | ||
private final AuthSession catalogAuth; | ||
private volatile ScheduledExecutorService refreshExecutor = 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.
close this in catalog.close?
import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.databind.ObjectMapper; | ||
|
||
import java.time.Duration; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.concurrent.ScheduledExecutorService; | ||
|
||
import static org.apache.paimon.utils.ThreadPoolUtils.createScheduledThreadPool; | ||
|
||
/** A catalog implementation for REST. */ | ||
public class RESTCatalog implements Catalog { | ||
private RESTClient client; |
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.
close this in catalog.close?
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.
+1
Purpose
Support auth in REST Catalog
Linked issue: #4540
Tests
API and Format
No
Documentation
No