[feat](authentication)Support AuthenticationIntegration DDL#60902
[feat](authentication)Support AuthenticationIntegration DDL#60902CalvinKirs wants to merge 2 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 28949 ms |
TPC-DS: Total hot run time: 184583 ms |
FE UT Coverage ReportIncrement line coverage |
| (WITH ROLLUP (rollupNames=identifierList)?)? #createTableLike | ||
| | CREATE ROLE (IF NOT EXISTS)? name=identifierOrText (COMMENT STRING_LITERAL)? #createRole | ||
| | CREATE AUTHENTICATION INTEGRATION integrationName=identifier | ||
| WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? #createAuthenticationIntegration |
There was a problem hiding this comment.
| WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? #createAuthenticationIntegration | |
| PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? #createAuthenticationIntegration |
| (WITH ROLLUP (rollupNames=identifierList)?)? #createTableLike | ||
| | CREATE ROLE (IF NOT EXISTS)? name=identifierOrText (COMMENT STRING_LITERAL)? #createRole | ||
| | CREATE AUTHENTICATION INTEGRATION integrationName=identifier | ||
| WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? #createAuthenticationIntegration |
There was a problem hiding this comment.
And resuse propertyClause
| LIKE existedTable=multipartIdentifier | ||
| (WITH ROLLUP (rollupNames=identifierList)?)? #createTableLike | ||
| | CREATE ROLE (IF NOT EXISTS)? name=identifierOrText (COMMENT STRING_LITERAL)? #createRole | ||
| | CREATE AUTHENTICATION INTEGRATION integrationName=identifier |
| private String name; | ||
| @SerializedName(value = "type") | ||
| private String type; | ||
| @SerializedName(value = "properties") |
There was a problem hiding this comment.
use short name, such as "p"
| properties=propertyClause? #alterComputeGroup | ||
| | ALTER CATALOG name=identifier SET PROPERTIES | ||
| LEFT_PAREN propertyItemList RIGHT_PAREN #alterCatalogProperties | ||
| | ALTER AUTHENTICATION INTEGRATION integrationName=identifier |
There was a problem hiding this comment.
How to delete an existing property?
| "globalVariable", "cluster", "broker", "resources", "exportJob", "backupHandler", | ||
| "paloAuth", "transactionState", "colocateTableIndex", "routineLoadJobs", "loadJobV2", "smallFiles", | ||
| "plugins", "deleteHandler", "sqlBlockRule", "policy", "globalFunction", "workloadGroups", | ||
| "plugins", "deleteHandler", "sqlBlockRule", "authenticationIntegrations", "policy", |
There was a problem hiding this comment.
Why put it in the middle of modules?
New module should be added to the end
| * Drop log for AUTHENTICATION INTEGRATION. | ||
| */ | ||
| public class DropAuthenticationIntegrationOperationLog implements Writable { | ||
| @SerializedName(value = "integrationName") |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary for PR #60902
This PR adds DDL support for CREATE / ALTER / DROP AUTHENTICATION INTEGRATION in Doris FE. The overall implementation is well-structured: grammar rules, command classes, manager, persistence (edit log + image), and tests are all present. Below are the issues found:
Issues Found
1. [Medium] Missing IF NOT EXISTS support for CREATE
The CREATE AUTHENTICATION INTEGRATION grammar rule does not support IF NOT EXISTS, which is inconsistent with nearly every other CREATE DDL command in Doris (CREATE CATALOG, CREATE ROLE, CREATE WORKLOAD GROUP, etc.). The DROP counterpart correctly supports IF EXISTS. This will make idempotent deployment scripts more difficult to write.
2. [Low-Medium] accept() uses generic visitor.visitCommand() instead of dedicated visitor methods
All three new command classes (CreateAuthenticationIntegrationCommand, AlterAuthenticationIntegrationCommand, DropAuthenticationIntegrationCommand) use the generic visitor.visitCommand(this, context) in their accept() methods. The established pattern in the codebase (e.g., CreateCatalogCommand, DropCatalogCommand, AlterCatalogPropertiesCommand) is to register a dedicated visit method in CommandVisitor.java and dispatch to it from accept(). This prevents visitor implementations from intercepting these specific command types.
3. [Low] No SHOW AUTHENTICATION INTEGRATION command
There is no SHOW command to list existing authentication integrations. While this may be intentional for the first iteration, it limits observability of the feature.
Checkpoints
- Correctness: The core logic (metadata management, edit log, image persistence, privilege checks) is correct. Locking patterns follow existing conventions. Gson deserialization of the Mgr class is safe because the default no-arg constructor is used.
- Backward compatibility: New operation type codes (493-495) don't conflict with existing ones. New meta module "authenticationIntegrations" is added in the correct position in
PersistMetaModules. - Tests: Unit tests cover Meta, Mgr, parser, and command classes well. Regression test covers the happy path and negative cases. Good coverage overall.
- Security: ADMIN privilege is correctly checked in all three commands.
NeedAuditEncryptionis implemented on Create and Alter commands (which handle sensitive properties like passwords). Encryption support is added inLogicalPlanBuilderForEncryption.
| (WITH ROLLUP (rollupNames=identifierList)?)? #createTableLike | ||
| | CREATE ROLE (IF NOT EXISTS)? name=identifierOrText (COMMENT STRING_LITERAL)? #createRole | ||
| | CREATE AUTHENTICATION INTEGRATION integrationName=identifier | ||
| WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec? #createAuthenticationIntegration |
There was a problem hiding this comment.
[Medium] Missing IF NOT EXISTS support: This CREATE AUTHENTICATION INTEGRATION rule does not support IF NOT EXISTS, unlike almost all other CREATE DDL commands in Doris (CREATE CATALOG (IF NOT EXISTS)?, CREATE ROLE (IF NOT EXISTS)?, CREATE WORKLOAD GROUP (IF NOT EXISTS)?). Consider adding it for consistency and to support idempotent DDL scripts:
| CREATE AUTHENTICATION INTEGRATION (IF NOT EXISTS)? integrationName=identifier
WITH PROPERTIES LEFT_PAREN propertyItemList RIGHT_PAREN commentSpec?
| @Override | ||
| public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { | ||
| return visitor.visitCommand(this, context); | ||
| } |
There was a problem hiding this comment.
[Low-Medium] Pattern deviation: This uses the generic visitor.visitCommand(this, context). The established codebase pattern is to add a dedicated visitCreateAuthenticationIntegrationCommand method to CommandVisitor.java (defaulting to visitCommand) and dispatch to it here. See CreateCatalogCommand.accept() for reference. Same applies to Alter and Drop commands.
| @Override | ||
| public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { | ||
| return visitor.visitCommand(this, context); | ||
| } |
There was a problem hiding this comment.
[Low-Medium] Same as above - should use a dedicated visitAlterAuthenticationIntegrationCommand method instead of generic visitor.visitCommand.
| @Override | ||
| public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { | ||
| return visitor.visitCommand(this, context); | ||
| } |
There was a problem hiding this comment.
[Low-Medium] Same as above - should use a dedicated visitDropAuthenticationIntegrationCommand method instead of generic visitor.visitCommand.
5 participants
This PR adds DDL support for managing AUTHENTICATION INTEGRATION objects in Doris FE.
#60361
It introduces: