Skip to content

feat: add AAD auth to azure search writer - #2285

Closed
mhamilton723 wants to merge 1 commit into
microsoft:masterfrom
mhamilton723:fix-azure-search
Closed

feat: add AAD auth to azure search writer#2285
mhamilton723 wants to merge 1 commit into
microsoft:masterfrom
mhamilton723:fix-azure-search

Conversation

@mhamilton723

Copy link
Copy Markdown
Contributor

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Briefly describe the changes included in this Pull Request.

How is this patch tested?

  • I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.

Does this PR change any dependencies?

  • No. You can skip this section.
  • Yes. Make sure the dependencies are resolved correctly, and list changes here.

Does this PR add a new feature? If so, have you added samples on website?

  • No. You can skip this section.
  • Yes. Make sure you have added samples following below steps.
  1. Find the corresponding markdown file for your new feature in website/docs/documentation folder.
    Make sure you choose the correct class estimators/transformers and namespace.
  2. Follow the pattern in markdown file and add another section for your new API, including pyspark, scala (and .NET potentially) samples.
  3. Make sure the DocTable points to correct API link.
  4. Navigate to website folder, and run yarn run start to make sure the website renders correctly.
  5. Don't forget to add <!--pytest-codeblocks:cont--> before each python code blocks to enable auto-tests for python samples.
  6. Make sure the WebsiteSamplesTests job pass in the pipeline.

@mhamilton723

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@codecov-commenter

codecov-commenter commented Sep 12, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 2.85714% with 34 lines in your changes missing coverage. Please review.

Project coverage is 83.09%. Comparing base (6854b5f) to head (4061715).

Files with missing lines Patch % Lines
...re/synapse/ml/services/search/AzureSearchAPI.scala 4.16% 23 Missing ⚠️
...azure/synapse/ml/services/search/AzureSearch.scala 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2285      +/-   ##
==========================================
- Coverage   83.70%   83.09%   -0.61%     
==========================================
  Files         327      327              
  Lines       16788    16802      +14     
  Branches     1500     1499       -1     
==========================================
- Hits        14052    13962      -90     
- Misses       2736     2840     +104     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhamilton723

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mhamilton723

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

ranadeepsingh added a commit to ranadeepsingh/SynapseML that referenced this pull request Aug 1, 2026
## Summary
Add shared Azure Search authentication configuration for subscription keys, AAD tokens, custom authorization, and custom headers across writer preparation and index list/get/create/statistics APIs. Preserve legacy key-based signatures and add secret-free unit coverage.

## Prompting Intent
Recreate the intent of the stale Azure Search AAD PR on current master without breaking callers. Use existing service authentication behavior, retain subscription-key support, reject missing credentials, avoid live-secret tests, and validate security and API compatibility.

## Linked Sources
- Original Azure Search AAD PR: microsoft#2285
- AAD cognitive-services foundation: microsoft#1778

## Rationale
Centralize header precedence in the existing cognitive-service path and delegate Azure Search requests to it rather than duplicating authentication logic. Use additive AzureSearchAuth overloads so legacy JVM and Scala signatures remain intact, while request-builder unit tests cover every management API without contacting Azure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ranadeepsingh

Copy link
Copy Markdown
Collaborator

Superseded by #2591, which merged to master in 04897ba — closing this out.

Thanks @mhamilton723 — this PR identified the gap and sketched the approach. Rather than modify this branch (which had drifted from master; SearchWriterSuite.scala has since been split into split1/split2), #2591 reimplements the feature on top of the HasAADToken foundation from #1778 and extends it with:

  • a single AzureSearchAuth model covering subscription key, AAD token, custom Authorization value, and custom headers;
  • one case-insensitive credential-precedence chain shared with the other cognitive services, with the automatic Fabric fallback ranked lowest and evaluated lazily, so a token fetch never runs (or fails) when an explicit credential is present;
  • credential validation before any network call, and redaction of auth values from diagnostic output;
  • every existing key-based public method preserved, with auth-aware overloads added, so existing user code is unaffected;
  • a secret-free AzureSearchAuthSuite plus generic param/header persistence suites.

The user-facing outcome you were after here is in master and ships in the first release after v1.1.3. Please reopen if this branch covered a case that #2591 missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants