feat(source-mssql): Add Microsoft Entra ID Service Principal Authentication Support (AI-Triage PR)#74906
Conversation
…cation support Add ActiveDirectoryServicePrincipal as a non-breaking authentication option for the MSSQL source connector, leveraging the JDBC driver's native support. Changes: - Add AuthenticationSpecification sealed interface with SqlPassword and ActiveDirectoryServicePrincipal variants - Add optional 'authentication' block to connector spec (oneOf selector) - Update MsSqlServerSourceConfigurationFactory to translate auth spec into JDBC connection properties - Keep top-level username/password fields for full backward compatibility - Add unit tests for backward compat, SQL password auth, and service principal auth No new dependencies needed - azure-identity:1.15.3 and mssql-jdbc:12.10.1 are already in build.gradle. Co-Authored-By: kevin.gavino <kevin.gavino@airbyte.io>
Co-Authored-By: kevin.gavino <kevin.gavino@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
|
… SpotBugs Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
|
↪️ Triggering Reason: Draft PR with CI fully green (99/99 tests pass). Feature adds Microsoft Entra ID Service Principal authentication support. |
|
Fix Validation EvidenceOutcome: Feature Proven Successfully Evidence SummaryTested backward compatibility of the new Entra ID Service Principal authentication feature via regression tests. Pre-release version Next Steps
Connector & PR DetailsConnector: Evidence PlanProving Criteria
Disproving Criteria
Cases Attempted
Pre-flight Checks
Detailed Evidence LogTimeline
Regression Test Details
Note: Connection IDs and detailed logs are recorded in the linked private issue. |
|
|
↪️ Triggering Reason: Prove-fix completed successfully — regression tests passed (99/99 unit tests, all CI green). Pre-release |
Reviewing PR for connector safety and quality.
|
|
Starting AI PR review for this PR. Will evaluate all gates and post a full report shortly. Session: https://app.devin.ai/sessions/6a675fc001744dcd9221ec2937cd31a4 |
AI PR Review ReportReview Action: NO ACTION (NOT ELIGIBLE)
📋 PR Details & EligibilityConnector & PR InfoConnector(s): Auto-Approve EligibilityEligible: No Review Action DetailsNO ACTION (NOT ELIGIBLE) — All enforced gates pass but the PR is not eligible for auto-approval because it contains functional code changes (new authentication method implementation). Two Anti-Pattern gates (Backwards Compatibility, Behavioral Changes) flagged as UNKNOWN — these require human sign-off. No PR review submitted.
🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Backwards Compatibility — UNKNOWN (Human Sign-Off Required)The following changes to the connector spec require human review:
Behavioral Changes — UNKNOWN (Human Sign-Off Required)
📚 Evidence ConsultedEvidence
❓ How to RespondProviding Context or JustificationYou can add explanations that the bot will see on the next review: Option 1: PR Description (recommended) ## AI PR Review Justification
### {Gate Name}
[Your explanation here]Option 2: PR Comment After adding your response, re-run Note: The Backwards Compatibility and Behavioral Changes gates require human sign-off. A justification provides context but these gates still need a human reviewer to approve. The version bump ( |
What
Adds Microsoft Entra ID (Azure Active Directory) Service Principal authentication as an alternative to SQL Password authentication for the source-mssql connector.
Resolves https://github.com/airbytehq/oncall/issues/11673:
Related community requests: airbytehq/airbyte#9557 (22+ upvotes), airbytehq/airbyte#20866, airbytehq/airbyte#61547.
No new dependencies are required —
mssql-jdbc:12.10.1andazure-identity:1.15.3are already inbuild.gradle.How
AuthenticationSpecificationsealed interface with two implementations:SqlPasswordAuthentication— existing username/password flowActiveDirectoryServicePrincipalAuthentication— new Entra ID service principal flow (tenant ID, client ID, client secret)MicronautPropertiesFriendlyAuthenticationSpecificationclass for Micronaut property binding (following the existing pattern used byEncryptionSpecificationandIncrementalConfigurationSpecification).username/passwordfields nullable for backward compatibility — existing configs without anauthenticationblock continue to work unchanged.authenticationblock is present: if yes, dispatches on auth method; if no, falls back to top-level credentials.getAuthenticationValue()uses a three-step fallback: explicitauthenticationJson→ top-levelusername/password→ Micronaut properties. The middle step ensures backward-compatible configs serialize correctly when Jackson invokes the@JsonGetter.Review guide
MsSqlServerSourceConfigurationSpecification.kt— Most important. Review the sealed interface hierarchy, the@JsonTypeInfo/@JsonSubTypesannotations, and the nullableusername/passwordchange. Verify the three-step fallback ingetAuthenticationValue()handles all deserialization scenarios safely.MsSqlServerSourceConfiguration.kt— Review the auth dispatch logic in the factory. Check that JDBC properties (authentication=ActiveDirectoryServicePrincipal,tenantId) are correct per the mssql-jdbc docs.MsSqlServerSourceConfigurationSpecificationTest.kt— Three new test cases: backward compat, SQL password auth block, and service principal auth block.MicronautPropertiesFriendlyAuthenticationSpecification.asAuthenticationSpecification()uses?.letto conditionally setlateinitfields — if bothauthenticationJsonand top-levelusernameare null, this path is reached and thelateinitfields onSqlPasswordAuthenticationmay remain uninitialized. Confirm this is only reachable in degenerate (invalid) configs."order":4is used on both the existingusernamefield and the newauthenticationgetter — confirm this doesn't cause UI ordering conflicts in the connector spec.password ?: ""fallback ingetAuthenticationValue(): if top-levelusernameis set butpasswordis null, password defaults to empty string. This is a backward-compat safeguard but may mask misconfiguration.pojo.username/pojo.passworddirectly (now nullable) that could NPE?User Impact
Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/4ed79c5b200a4336af875c6be1f5f987