You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Akanksha-kedia (via GitHub)" <gi...@apache.org> on 2026/01/27 14:11:17 UTC

[PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Akanksha-kedia opened a new pull request, #17584:
URL: https://github.com/apache/pinot/pull/17584

   
   
   This commit establishes consistent BasicAuth token configuration naming across all Apache Pinot components:
   
   - Controller: pinot.controller.segment.fetcher.auth.token (was controller.segment.fetcher.auth.token)
   - Server: pinot.server.segment.fetcher.auth.token (already correct)
   - Minion: pinot.minion.segment.fetcher.auth.token & pinot.minion.task.auth.token (were segment.fetcher.auth.token & task.auth.token)
   
   Changes Made:
   - Fixed BasicAuthTestUtils.java - corrected controller and minion token configurations
   - Fixed ControllerTest.java - corrected obfuscation test configuration
   
   Fixes #17506
   
   Instructions:
   1. The PR has to be tagged with at least one of the following labels (*):
      1. `feature`
      2. `bugfix`
      3. `performance`
      4. `ui`
      5. `backward-incompat`
      6. `release-notes` (**)
   2. Remove these instructions before publishing the PR.
    
   (*) Other labels to consider:
   - `testing`
   - `dependencies`
   - `docker`
   - `kubernetes`
   - `observability`
   - `security`
   - `code-style`
   - `extension-point`
   - `refactor`
   - `cleanup`
   
   (**) Use `release-notes` label for scenarios like:
   - New configuration options
   - Deprecation of configurations
   - Signature changes to public methods/interfaces
   - New plugins added or old plugins removed
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-5208447461

   Could you please rebase to the current master?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-5223952879

   Sure
   
   On Fri, 7 Aug 2026 at 12:39 AM, Xiaotian (Jackie) Jiang <
   ***@***.***> wrote:
   
   > *Jackie-Jiang* left a comment (apache/pinot#17584)
   > <https://github.com/apache/pinot/pull/17584#issuecomment-5208447461>
   >
   > Could you please rebase to the current master?
   >
   > —
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/pinot/pull/17584?email_source=notifications&email_token=AVL2AZVLDO4QPRDZ3MTNZ7T5ITJXBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRQHA2DINZUGYY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5208447461>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/AVL2AZVW2QJ2RUOGSCELRZT5ITJXBAVCNFSNUABEKJSXA33TNF2G64TZHMYTSOJWGEYDQNJ3JFZXG5LFHMZTQNRQG44TIMBYGKQXMAQ>
   > .
   > You are receiving this because you authored the thread.Message ID:
   > ***@***.***>
   >
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r2979997653


##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java:
##########
@@ -253,6 +253,22 @@ protected void overrideMinionConf(PinotConfiguration minionConf) {
     minionConf.setProperty("pinot.minion.tls.truststore.password", "changeit");
     minionConf.setProperty("pinot.minion.tls.truststore.type", "PKCS12");
     minionConf.setProperty("pinot.minion.tls.client.auth.enabled", "true");
+
+//    // Add segment fetcher and uploader configurations for TLS communication
+//    minionConf.setProperty("pinot.minion.segment.fetcher.protocols", "https");
+//    minionConf.setProperty("pinot.minion.segment.uploader.protocols", "https");
+//
+//    // Add segment fetcher auth provider for BasicAuth over TLS
+//    minionConf.setProperty("pinot.minion.segment.fetcher.auth.provider.class",
+//        "org.apache.pinot.common.auth.StaticTokenAuthProvider");
+//    // Explicitly set the auth token for segment fetcher
+//    minionConf.setProperty("pinot.minion.segment.fetcher.auth.token", AUTH_TOKEN);
+//
+//    // Add task auth provider for BasicAuth over TLS
+//    minionConf.setProperty("pinot.minion.task.auth.provider.class",
+//        "org.apache.pinot.common.auth.StaticTokenAuthProvider");
+//    // Explicitly set the auth token for task auth
+//    minionConf.setProperty("pinot.minion.task.auth.token", AUTH_TOKEN);

Review Comment:
   This adds a large block of commented-out configuration code in the test. It makes the test harder to maintain and can confuse future readers about what is actually being exercised. Please remove the commented block, or turn it into active test setup with assertions if it is required for the TLS+BasicAuth scenario.
   ```suggestion
   
   ```



##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1618,7 +1618,7 @@ public static class Minion {
      * Service token for accessing protected controller APIs.
      * E.g. null (auth disabled), "Basic abcdef..." (basic auth), "Bearer 123def..." (oauth2)
      */
-    public static final String CONFIG_TASK_AUTH_NAMESPACE = "task.auth";
+    public static final String CONFIG_TASK_AUTH_NAMESPACE = "pinot.minion.task.auth";

Review Comment:
   Changing Minion.CONFIG_TASK_AUTH_NAMESPACE from "task.auth" to "pinot.minion.task.auth" is a breaking configuration change: existing deployments (and docs/examples) using "task.auth.*" will silently stop applying task auth settings. Please keep backward compatibility by supporting the legacy namespace as a deprecated alias (e.g., read from the new namespace if present, otherwise fall back to the old one and log a deprecation warning).
   ```suggestion
       public static final String CONFIG_TASK_AUTH_NAMESPACE = "pinot.minion.task.auth";
   
       /**
        * Legacy config namespace for task auth settings.
        * Deprecated in favor of {@link #CONFIG_TASK_AUTH_NAMESPACE}.
        */
       @Deprecated
       public static final String DEPRECATED_CONFIG_TASK_AUTH_NAMESPACE = "task.auth";
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3847750742

   @Jackie-Jiang 2 failurs seems to be   [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.52 s -- in org.apache.pinot.server.realtime.ControllerLeaderLocatorIntegrationTest
     [INFO] org.apache.pinot.server.realtime.ControllerLeaderLocatorIntegrationTest.testControllerLeaderLocator -- Time elapsed: 2.732 s
     [INFO] 
     [INFO] Results:
     [INFO] 
     Error:  Failures: 
     Error:    TlsIntegrationTest.testRealtimeSegmentUploadDownload:485 » IllegalState Failed to return result in 30000ms
     [INFO] 
     Error:  Tests run: 456, Failures: 1, Errors: 0, Skipped: 1
     [INFO] 
     [INFO] ------------------------------------------------------------------------
     [INFO] BUILD FAILURE
     [INFO] ------------------------------------------------------------------------
     [INFO] Total time:  53:59 min
     [INFO] Finished at: 2026-02-03T06:32:48Z
     [INFO] ------------------------------------------------------------------------
     [INFO] 13 goals, 13 executed
     Error:  Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.4:test (default-test) on project pinot-integration-tests: There are test failures.
     Error:  
     Error:  See /home/runner/work/pinot/pinot/pinot-integration-tests/target/surefire-reports for the individual test results.
     Error:  See dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
     Error:  -> [Help 1]
     Error:  
     Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
     Error:  Re-run Maven using the -X switch to enable full debug logging.
     Error:  
     Error:  For more information about the errors and possible solutions, please read the following articles:
     Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
     
     
     flaky @xiangfu0 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3873598461

   Please take a look at these failed tests:
   ```
   2026-02-09T09:49:35.2268006Z [ERROR] Failures: 
   2026-02-09T09:49:35.2269244Z [ERROR]   TlsIntegrationTest.testRealtimeSegmentUploadDownload:496 » IllegalState Failed to return result in 30000ms
   2026-02-09T09:49:35.2270847Z [ERROR]   UrlAuthRealtimeIntegrationTest.testSegmentUploadDownload:168 » IllegalState Failed to return result in 30000ms
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3805446156

   @Jackie-Jiang 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3808101316

   Can you take a look at the test failure (`TlsIntegrationTest.testRealtimeSegmentUploadDownload:485 » IllegalState Failed to return result in 30000ms`)? Some key might not be handled correctly


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r2986244857


##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BasicAuthTestUtils.java:
##########
@@ -27,14 +27,14 @@ public final class BasicAuthTestUtils {
   private BasicAuthTestUtils() {
   }
 
-  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA=====";
+  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA==";
   public static final String AUTH_TOKEN_USER = "Basic dXNlcjpzZWNyZXQ==";
   public static final Map<String, String> AUTH_HEADER = Map.of("Authorization", AUTH_TOKEN);
   public static final BasicHeader AUTH_HEADER_BASIC = new BasicHeader("Authorization", AUTH_TOKEN);
   public static final Map<String, String> AUTH_HEADER_USER = Map.of("Authorization", AUTH_TOKEN_USER);
 
   public static void addControllerConfiguration(Map<String, Object> properties) {
-    properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+    properties.put("pinot.controller.segment.fetcher.auth.token", AUTH_TOKEN);

Review Comment:
   __Yes, it was a bug fix__ — `controller.segment.fetcher.auth.token` (without `pinot.` prefix) was never being picked up because `PinotIngestionRestletResource` calls `extractAuthProvider(_controllerConf, "pinot.controller.segment.fetcher.auth")` which does a subset lookup, so only keys under `pinot.controller.segment.fetcher.auth.*` are read. The old key was silently ignored, meaning the tests were not actually exercising authenticated segment fetching on the controller side.
   
   The current state of the code is already correct — `BasicAuthTestUtils.addControllerConfiguration()` only sets `pinot.controller.segment.fetcher.auth.token`. No code change is needed here; this is just a PR review response.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3811724726

   https://github.com/pinot-contrib/pinot-docs/pull/465 this also i have raised @jiang


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3811724896

   https://github.com/pinot-contrib/pinot-docs/pull/465 this also i have raised @jiang


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3805723326

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17584?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 55.48%. Comparing base ([`78613ef`](https://app.codecov.io/gh/apache/pinot/commit/78613ef31504bc5c9c6ffc8831fc29f8598b0af6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2a4f97b`](https://app.codecov.io/gh/apache/pinot/commit/2a4f97b830b29651f9e8c15f32032c87b9e69d2a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   > :exclamation:  There is a different number of reports uploaded between BASE (78613ef) and HEAD (2a4f97b). Click for more details.
   > 
   > <details><summary>HEAD has 32 uploads less than BASE</summary>
   >
   >| Flag | BASE (78613ef) | HEAD (2a4f97b) |
   >|------|------|------|
   >|java-21|5|1|
   >|unittests1|2|1|
   >|unittests|4|1|
   >|temurin|9|1|
   >|java-11|4|0|
   >|unittests2|2|0|
   >|integration|5|0|
   >|integration2|1|0|
   >|integration1|2|0|
   >|custom-integration1|2|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #17584      +/-   ##
   ============================================
   - Coverage     63.19%   55.48%   -7.72%     
   + Complexity     1476      702     -774     
   ============================================
     Files          3172     2473     -699     
     Lines        189806   140004   -49802     
     Branches      29046    22318    -6728     
   ============================================
   - Hits         119945    77675   -42270     
   + Misses        60548    55758    -4790     
   + Partials       9313     6571    -2742     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-11](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-21](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.48% <ø> (-7.67%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.48% <ø> (-7.72%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.48% <ø> (-7.71%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.48% <ø> (-0.07%)` | :arrow_down: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17584/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17584?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4815881953

   @xiangfu0 Fixed two test failures from CI:
   
   1. **`AuthProviderUtilsTest.testCorrectKeyProducesAuthHeader`** (Unit Test Set 1): `StaticTokenAuthProvider` prefixes the token with `"Basic "`, so `headers.containsValue("my-secret-token")` was always false. Fixed the assertion to check the `Authorization` header value contains the raw token as a substring.
   
   2. **`BasicAuthBatchIntegrationTest.testControllerSegmentFetcherUsesAuthToken`** (Integration Test Set 1): `ClassicRequestBuilder.addParameter()` on POST adds parameters to the form body, not the URL query string. The controller's `FineGrainedAuthUtils` reads `uriInfo.getQueryParameters()` (URL only), so `tableName` was not found. Fixed by embedding `?tableName=baseballStats` directly in the upload URI.
   
   Both fixes pushed — CI should pass now.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r2986246679


##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java:
##########
@@ -253,6 +253,22 @@ protected void overrideMinionConf(PinotConfiguration minionConf) {
     minionConf.setProperty("pinot.minion.tls.truststore.password", "changeit");
     minionConf.setProperty("pinot.minion.tls.truststore.type", "PKCS12");
     minionConf.setProperty("pinot.minion.tls.client.auth.enabled", "true");
+
+//    // Add segment fetcher and uploader configurations for TLS communication

Review Comment:
   we need to remove



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4332149457

   @xiangfu0 please review


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4661260121

   @Jackie-Jiang — thank you for the review. Regarding the `TlsIntegrationTest.testRealtimeSegmentUploadDownload` failure: I checked the test output and the failure appeared intermittent (timing issue in waiting 30000ms for result). Could you confirm whether this needs a specific key-handling change or whether re-running the test would be the right call? I want to make sure I address the right root cause before pushing a fix.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-5087036179

   cc @Jackie-Jiang @snleee @xiangfu0 — this has had a lot of discussion (33 comments) and I've addressed all the feedback. Would appreciate a final re-review to get this across the finish line\!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4741597890

   @xiangfu0 @Jackie-Jiang all CI checks pass, review comments addressed. Please review when you get a chance.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r3435445118


##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java:
##########
@@ -253,6 +253,22 @@ protected void overrideMinionConf(PinotConfiguration minionConf) {
     minionConf.setProperty("pinot.minion.tls.truststore.password", "changeit");
     minionConf.setProperty("pinot.minion.tls.truststore.type", "PKCS12");
     minionConf.setProperty("pinot.minion.tls.client.auth.enabled", "true");
+
+//    // Add segment fetcher and uploader configurations for TLS communication

Review Comment:
   The commented-out block was from an older iteration of this PR and has since been cleaned up. Additionally, the method override has been aligned with the upstream rename from `getControllerRequestClientHeaders` → `getAdminClientHeaders` (introduced in #18755) — latest commit updates this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4096699806

   @yashmayya 
   @Jackie-Jiang 
   @xiangfu0 please review


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4116096503

   @xiangfu0 please review and let me know wat i need to remove


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4066162213

   seems to flaky test and others are solved   [INFO] Results:
     [INFO] 
     Error:  Failures: 
     Error:    GroupByTrimmingTest.testMSQEGroupsTrimmedAtSegmentLevelWithOrderByOnAggregateIsNotSafe:227 expected [4] but found [3]
     [INFO] 
     Error:  Tests run: 376, Failures: 1, Errors: 0, Skipped:  
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-3811641222

   done
   @Jackie-Jiang 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4889809347

   Hi @Jackie-Jiang @xiangfu0 @yashmayya — rebased onto the latest master to resolve the Binary Compatibility Check failure (the failures were pre-existing in master, not caused by this PR's changes). All CI checks are now green. Kindly re-review\!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4066163582

   @xiangfu0 @Jackie-Jiang please review.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4116454401

   I think we need to handle the backward compatibility, existing configs should still be handled and work. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r2991751971


##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BasicAuthTestUtils.java:
##########
@@ -27,14 +27,14 @@ public final class BasicAuthTestUtils {
   private BasicAuthTestUtils() {
   }
 
-  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA=====";
+  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA==";
   public static final String AUTH_TOKEN_USER = "Basic dXNlcjpzZWNyZXQ==";
   public static final Map<String, String> AUTH_HEADER = Map.of("Authorization", AUTH_TOKEN);
   public static final BasicHeader AUTH_HEADER_BASIC = new BasicHeader("Authorization", AUTH_TOKEN);
   public static final Map<String, String> AUTH_HEADER_USER = Map.of("Authorization", AUTH_TOKEN_USER);
 
   public static void addControllerConfiguration(Map<String, Object> properties) {
-    properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+    properties.put("pinot.controller.segment.fetcher.auth.token", AUTH_TOKEN);

Review Comment:
   Thanks for the explanation.
   Do we have a test that can catch this bug? All tests pass with an invalid auth key does concern me..



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r2983391615


##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BasicAuthTestUtils.java:
##########
@@ -27,14 +27,14 @@ public final class BasicAuthTestUtils {
   private BasicAuthTestUtils() {
   }
 
-  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA=====";
+  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA==";
   public static final String AUTH_TOKEN_USER = "Basic dXNlcjpzZWNyZXQ==";
   public static final Map<String, String> AUTH_HEADER = Map.of("Authorization", AUTH_TOKEN);
   public static final BasicHeader AUTH_HEADER_BASIC = new BasicHeader("Authorization", AUTH_TOKEN);
   public static final Map<String, String> AUTH_HEADER_USER = Map.of("Authorization", AUTH_TOKEN_USER);
 
   public static void addControllerConfiguration(Map<String, Object> properties) {
-    properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+    properties.put("pinot.controller.segment.fetcher.auth.token", AUTH_TOKEN);

Review Comment:
   Is this a bug fix? Does `controller.segment.fetcher.auth.token` work or we were just not doing proper auth?



##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1618,7 +1618,7 @@ public static class Minion {
      * Service token for accessing protected controller APIs.
      * E.g. null (auth disabled), "Basic abcdef..." (basic auth), "Bearer 123def..." (oauth2)
      */
-    public static final String CONFIG_TASK_AUTH_NAMESPACE = "task.auth";
+    public static final String CONFIG_TASK_AUTH_NAMESPACE = "pinot.minion.task.auth";

Review Comment:
   +1. We need to handle backward compatibility for this change



##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1618,7 +1618,7 @@ public static class Minion {
      * Service token for accessing protected controller APIs.
      * E.g. null (auth disabled), "Basic abcdef..." (basic auth), "Bearer 123def..." (oauth2)
      */
-    public static final String CONFIG_TASK_AUTH_NAMESPACE = "task.auth";
+    public static final String CONFIG_TASK_AUTH_NAMESPACE = "pinot.minion.task.auth";

Review Comment:
   Take a look at how `segment.fetcher` is handled, you can handle it in a similar way.



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java:
##########
@@ -253,6 +253,22 @@ protected void overrideMinionConf(PinotConfiguration minionConf) {
     minionConf.setProperty("pinot.minion.tls.truststore.password", "changeit");
     minionConf.setProperty("pinot.minion.tls.truststore.type", "PKCS12");
     minionConf.setProperty("pinot.minion.tls.client.auth.enabled", "true");
+
+//    // Add segment fetcher and uploader configurations for TLS communication

Review Comment:
   Is this intentionally left, or should we remove it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r3420919584


##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BasicAuthTestUtils.java:
##########
@@ -27,14 +27,14 @@ public final class BasicAuthTestUtils {
   private BasicAuthTestUtils() {
   }
 
-  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA=====";
+  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA==";
   public static final String AUTH_TOKEN_USER = "Basic dXNlcjpzZWNyZXQ==";
   public static final Map<String, String> AUTH_HEADER = Map.of("Authorization", AUTH_TOKEN);
   public static final BasicHeader AUTH_HEADER_BASIC = new BasicHeader("Authorization", AUTH_TOKEN);
   public static final Map<String, String> AUTH_HEADER_USER = Map.of("Authorization", AUTH_TOKEN_USER);
 
   public static void addControllerConfiguration(Map<String, Object> properties) {
-    properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+    properties.put("pinot.controller.segment.fetcher.auth.token", AUTH_TOKEN);

Review Comment:
   Good point. The existing ControllerTest.java (line 1560) already sets pinot.controller.segment.fetcher.auth.token (correct key), so it passes. But there is no test that explicitly verifies the wrong key (controller.segment.fetcher.auth.token without the pinot. prefix) is rejected — the tests pass either way because auth simply is not applied when the key is wrong.
   
   I will add a unit test to PinotIngestionRestletResourceStatelessTest that:
   1. Configures the controller with the correct key pinot.controller.segment.fetcher.auth.token and asserts the AuthProvider produces the expected auth header.
   2. Configures it with the old wrong key controller.segment.fetcher.auth.token and asserts the AuthProvider returns no auth header.
   
   This makes the bug regression-testable. I will push the test in the next commit. @Jackie-Jiang



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on code in PR #17584:
URL: https://github.com/apache/pinot/pull/17584#discussion_r3420967205


##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BasicAuthTestUtils.java:
##########
@@ -27,14 +27,14 @@ public final class BasicAuthTestUtils {
   private BasicAuthTestUtils() {
   }
 
-  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA=====";
+  public static final String AUTH_TOKEN = "Basic YWRtaW46dmVyeXNlY3JldA==";
   public static final String AUTH_TOKEN_USER = "Basic dXNlcjpzZWNyZXQ==";
   public static final Map<String, String> AUTH_HEADER = Map.of("Authorization", AUTH_TOKEN);
   public static final BasicHeader AUTH_HEADER_BASIC = new BasicHeader("Authorization", AUTH_TOKEN);
   public static final Map<String, String> AUTH_HEADER_USER = Map.of("Authorization", AUTH_TOKEN_USER);
 
   public static void addControllerConfiguration(Map<String, Object> properties) {
-    properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+    properties.put("pinot.controller.segment.fetcher.auth.token", AUTH_TOKEN);

Review Comment:
   Added a dedicated unit test class AuthProviderUtilsTest in the latest commit with three cases: (1) correct key pinot.controller.segment.fetcher.auth.token → auth header present, (2) old wrong key controller.segment.fetcher.auth.token → no header produced, (3) null config → no header. This makes the regression directly testable without needing a full integration cluster. @Jackie-Jiang could you re-review?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org


Re: [PR] Fix BasicAuth token configuration naming consistency across Pinot components (#17506) [pinot]

Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #17584:
URL: https://github.com/apache/pinot/pull/17584#issuecomment-4378602978

   @xiangfu0 @Jackie-Jiang 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org