Skip to content

Fixing TSHttpTxnServerAddrSet #10189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
bneradt merged 1 commit into from
Aug 17, 2023
Merged

Fixing TSHttpTxnServerAddrSet #10189

bneradt merged 1 commit into from
Aug 17, 2023

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented Aug 16, 2023

PR #9054 broke hostdb behavior for TSHttpTxnServerAddrSet(). This fixes the behavior and adds a regression test for the API.

@bneradt bneradt added this to the 10.0.0 milestone Aug 16, 2023
@bneradt bneradt self-assigned this Aug 16, 2023
SolidWallOfCode
SolidWallOfCode reviewed Aug 16, 2023
View reviewed changes
ywkaras
ywkaras reviewed Aug 16, 2023
View reviewed changes
proxy/http/HttpSM.cc
break;
} else if (t_state.parent_result.result == PARENT_UNDEFINED && t_state.dns_info.resolve_immediate()) {
} else if (t_state.dns_info.resolved_p) {
SMDebug("dns", "Skipping DNS lookup because the address is already set.");
Copy link
Contributor

@ywkaras ywkaras Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, TSHttpTxnServerAddrSet() is called in the CACHE_LOOKUP_COMPLETE hook in the new Au test, which presumably comes after DNS lookup?

Copy link
Contributor Author

@bneradt bneradt Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Yahoo's internal plugin relies upon redirecting after CACHE_LOOKUP_COMPLETE in order to work. Fortunately it seems to work. :)

Copy link
Contributor Author

@bneradt bneradt Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like CACHE_LOOKUP comes before DNS lookup. Here's the relevant debug logs from the autest that exercises this (note that SM_ACTION_CACHE_LOOKUP comes before SM_ACTION_DNS_LOOKUP):

  147 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpTransact.cc:2106 (DecideCacheLookup)> (http_seq) [0] Will do cache lookup                                  
  148 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpTransact.cc:2149 (DecideCacheLookup)> (http_trans) Next action SM_ACTION_CACHE_LOOKUP; nullptr            
  149 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:7735 (call_transact_and_set_next_state)> (http) [0] State Transition: SM_ACTION_API_POST_REMAP -> SM_ACTION_CACHE_LOOKUP
  150 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:4922 (do_cache_lookup_and_read)> (http_seq) [0] Issuing cache lookup for URL http://non.existent.server.com:61008/pictures/flower.jpe
      g                                                                                                                                                                    
  151 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpCacheSM.cc:100 (state_cache_open_read)> (http_cache) [0] [&HttpCacheSM::state_cache_open_read, CACHE_EVENT_OPEN_READ_FAILED/TS_EVENT_CACHE_
      OPEN_READ_FAILED]                                                                                                                                          
  152 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:2644 (main_handler)> (http) [0] CACHE_EVENT_OPEN_READ_FAILED/TS_EVENT_CACHE_OPEN_READ_FAILED, 1103                         
  153 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:2572 (state_cache_open_read)> (http) [0] [&HttpSM::state_cache_open_read, CACHE_EVENT_OPEN_READ_FAILED/TS_EVENT_CACHE_OPEN_READ_FAILE
      D]                                                                                                                                                      
  154 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:2606 (state_cache_open_read)> (http) [0] cache_open_read - CACHE_EVENT_OPEN_READ_FAILED with ECACHE_NO_DOC (20400)
  155 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:2609 (state_cache_open_read)> (http) [0] open read failed.
  156 [Aug 16 15:52:47.829] [ET_NET 8] DEBUG: <HttpSM.cc:1409 (state_api_callout)> (http) [0] calling plugin on hook TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK at hook 0x3dcf6fe0
  157 [Aug 16 15:52:47.830] [ET_NET 8] DIAG: (test_plugin) Successfully set a transaction's origin to 127.0.0.1:61001                                                                      
  158 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpSM.cc:1298 (state_api_callback)> (http) [0] [&HttpSM::state_api_callback, HTTP_API_CONTINUE/TS_EVENT_HTTP_CONTINUE]
  159 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpSM.cc:1338 (state_api_callout)> (http) [0] [&HttpSM::state_api_callout, HTTP_API_CONTINUE/TS_EVENT_HTTP_CONTINUE]                   
  160 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:2365 (HandleCacheOpenRead)> (http_trans) [0] [HttpTransact::HandleCacheOpenRead]                                     
  161 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:2395 (HandleCacheOpenRead)> (http_trans) [0] CacheOpenRead -- miss             
  162 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:3258 (HandleCacheOpenReadMiss)> (http_trans) [0] --- MISS
  163 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:3259 (HandleCacheOpenReadMiss)> (http_seq) [0] Miss in cache                                                      
  164 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:5194 (get_ka_info_from_config)> (http_trans) [0] server_info->http_version 1.1, check_hostdb 0                          
  165 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:2622 (CallOSDNSLookup)> (http) [0] non.existent.server.com                                                     
  166 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:2636 (CallOSDNSLookup)> (http_trans) Next action SM_ACTION_DNS_LOOKUP; OSDNSLookup
  167 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpSM.cc:7735 (call_transact_and_set_next_state)> (http) [0] State Transition: SM_ACTION_CACHE_LOOKUP -> SM_ACTION_DNS_LOOKUP
  168 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpSM.cc:7836 (set_next_state)> (dns) [0] Skipping DNS lookup because the address is already set.                            
  169 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:1885 (OSDNSLookup)> (http_trans) [0] Entering HttpTransact::OSDNSLookup                                              
  170 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:1928 (OSDNSLookup)> (http_seq) [0] DNS Lookup successful                                                           
  171 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:1966 (OSDNSLookup)> (http_trans) [0] DNS lookup for O.S. successful IP: 127.0.0.1:61001
  172 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpTransact.cc:2045 (OSDNSLookup)> (http_trans) Next action SM_ACTION_API_OS_DNS; HandleCacheOpenReadMiss                          
  173 [Aug 16 15:52:47.830] [ET_NET 8] DEBUG: <HttpSM.cc:7735 (call_transact_and_set_next_state)> (http) [0] State Transition: SM_ACTION_DNS_LOOKUP -> SM_ACTION_API_OS_DNS

Copy link
Contributor

@ywkaras ywkaras Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this diagram isn't accurate then: https://docs.trafficserver.apache.org/developer-guide/plugins/hooks-and-transactions/index.en.html#http-transaction-state-diagram

@bneradt
Fixing TSHttpTxnServerAddrSet
36d1f84 
PR apache#9054 broke hostdb behavior for TSHttpTxnServerAddrSet(). This fixes
the behavior and adds a regression test for the API.
@bneradt bneradt force-pushed the fix_TSHttpTxnServerAddrSet branch from 7854239 to 36d1f84 Compare August 16, 2023 15:54
@bneradt
Copy link
Contributor Author

bneradt commented Aug 17, 2023

@fatih-acar : I'm ready to merge this, but I don't have access to machines that use parent selection. This shouldn't break parent selection, but I'd feel more comfortable merging this if you or someone with your organization can confirm that for me before I merge.

@fatih-acar
Copy link
Contributor

fatih-acar commented Aug 17, 2023

Hello @bneradt thanks for following up
You can move on with this. As you pointed out, it was not the right patch for our initial issue. I have created a new PR which should be the right one: #10234 (note: I'm not sure with the approach but I've described the actual issue in the commit message so feel free to propose another solution).

@bneradt
Copy link
Contributor Author

bneradt commented Aug 17, 2023

Hello @bneradt thanks for following up You can move on with this. As you pointed out, it was not the right patch for our initial issue. I have created a new PR which should be the right one: #10234 (note: I'm not sure with the approach but I've described the actual issue in the commit message so feel free to propose another solution).

Thank you @fatih-acar for the quick reply.

@bneradt bneradt merged commit b6a5371 into apache:master Aug 17, 2023
@bneradt bneradt deleted the fix_TSHttpTxnServerAddrSet branch August 17, 2023 19:12
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
0aec18a 
* asf/master:
  Upgrade yaml-cpp version to 0.8.0 (apache#10249)
  This dependency is not needed, cmake did it right (apache#10250)
  Revert "Make OSX and FreeBSD not required temporarily (apache#10237)" (apache#10248)
  Python 3.12: microserver.test.ext wrap_socket update (apache#10247)
  Coverity 1508984: Dereference null return value (apache#10245)
  fall back to configure file for older cmake versions (apache#10236)
  TLS early data: logging updates (apache#10115)
  Fixing TSHttpTxnServerAddrSet (apache#10189)
  Make OSX and FreeBSD not required temporarily (apache#10237)
  Fixes a problem which can decrement milestone metrics unintentionally (apache#10188)
  Fix editor config for makefiles. (apache#10190)
  Correctly handle encoding for cache hash generation (apache#10126)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ywkaras ywkaras ywkaras left review comments

+1 more reviewer

@SolidWallOfCode SolidWallOfCode SolidWallOfCode approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bneradt bneradt

Labels

HostDB TS API

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@bneradt @fatih-acar @SolidWallOfCode @ywkaras