Add SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES feature configuration #1931
Conversation
There was a problem hiding this comment.
Thanks, @poojanilangekar !
Changes LGTM with one minor comment.
| @@ -112,6 +112,7 @@ polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false | |||
| polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE"] | |||
| # polaris.features."ENABLE_CATALOG_FEDERATION"=true | |||
| polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] | |||
| polaris.features."SUPPORTED_FEDERATION_AUTHENTICATION_TYPES"=["OAUTH", "BEARER"] | |||
There was a problem hiding this comment.
why re-add the same value as the code-level default here?
There was a problem hiding this comment.
I actually followed the SUPPORTED_CATALOG_CONNECTION_TYPES here. Nonetheless, I've removed it.
There was a problem hiding this comment.
In turn, I followed SUPPORTED_CATALOG_STORAGE_TYPES for SUPPORTED_CATALOG_CONNECTION_TYPES :)
I think the idea is to be uber-explicit about having a "safe" default everywhere by excluding the unsafe options. However for SUPPORTED_FEDERATION_AUTHENTICATION_TYPES there is currently no unsafe option. There may be in the future though (NONE?) so keeping it wouldn't be the worst thing in light of that.
There was a problem hiding this comment.
Makes sense, I've added it back in.
service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java
Show resolved
Hide resolved
|
Maybe |
|
@eric-maynard I think the name I think the same goes for If you are opposed to adding |
| .getConfigurationStore() | ||
| .getConfiguration( | ||
| callContext.getRealmContext(), | ||
| FeatureConfiguration.SUPPORTED_FEDERATION_AUTHENTICATION_TYPES); |
There was a problem hiding this comment.
Do we need to handle uppercasing for this, similar to supportedConnectionTypes above? It might be redundant since those enums are uppercase anyway, so it's optional and upto you to make the change :p
Oh, that's a great idea. If you are amenable, let's do |
poojanilangekar
changed the title
| public static final FeatureConfiguration<List<String>> | ||
| SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES = | ||
| PolarisConfiguration.<List<String>>builder() | ||
| .key("SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES") |
There was a problem hiding this comment.
The change of config key is technically a breaking change. I'm personally ok with this, but it has sem. ver. implications per out current policy: https://polaris.apache.org/in-dev/unreleased/evolution/
Ideally we should support the old name for a while, but I do not think we have a mechanism for that ATM.
That said, since this PR is mostly about adding SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES, it might be best to do the renaming in a separate PR (plus add backward compatibility).
@eric-maynard WDYT?
There was a problem hiding this comment.
Done, I've removed it. I'll send out another PR and we can discuss options there.
There was a problem hiding this comment.
I'm okay with it too, but agreed that playing it safe is fine for now. We can always add support for multiple keys and then change this
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| entityManagerFactory = Mockito.mock(RealmEntityManagerFactory.class); |
There was a problem hiding this comment.
Do we need @Mock annotations above since we mock explicitly here?
There was a problem hiding this comment.
Good catch, I have removed it.
poojanilangekar
dismissed stale reviews from dimas-b and eric-maynard
via
3db6ed8
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Based on the discussion in #1925, this PR adds a
SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPESfeature configuration. For now, we only support OAUTH and BEARER. Eventually, we can enable SIGV4 and NONE.Modified the validation code in the
CreateCatalogservice to verify that connection and authentication types are supported. Added a unit test for the same.