- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
Add ReusePort config to confighttp #14058
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.
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
base: main
Are you sure you want to change the base?
Add ReusePort config to confighttp #14058
Conversation
636ae93    to
    344b948      
    Compare
  
    | Codecov Report❌ Patch coverage is  
 ❌ Your patch check has failed because the patch coverage (71.42%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@            Coverage Diff             @@
##             main   #14058      +/-   ##
==========================================
- Coverage   92.27%   92.26%   -0.01%     
==========================================
  Files         656      657       +1     
  Lines       41050    41070      +20     
==========================================
+ Hits        37877    37892      +15     
- Misses       2172     2175       +3     
- Partials     1001     1003       +2     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
344b948    to
    bac556b      
    Compare
  
    | Contrib test failure fixed here: open-telemetry/opentelemetry-collector-contrib#43681 | 
Without a valid `runCtx`, anything that attempts to use the context segfaults. In open-telemetry/opentelemetry-collector#14058 I am using the context, so this test borks. This adds the runCtx to fix it. Signed-off-by: sinkingpoint <[email protected]>
Without a valid `runCtx`, anything that attempts to use the context segfaults. In open-telemetry/opentelemetry-collector#14058 I am using the context, so this test borks. This adds the runCtx to fix it. Signed-off-by: sinkingpoint <[email protected]>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Without a valid `runCtx`, anything that attempts to use the context segfaults. In open-telemetry/opentelemetry-collector#14058 I am using the context, so this test borks. This adds the runCtx to fix it. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue open-telemetry/opentelemetry-collector#14058 <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.--> Signed-off-by: sinkingpoint <[email protected]>
| Should we have this for configgrpc too? And non-HTTP/gRPC? I'm wondering if we should make a breaking change to confighttp.ServerConfig to use confignet.AddrConfig, similar to configgrpc: 
 Then we could add ReusePort to confignet.AddrConfig and have it in one place. (Not necessarily all in this PR, just thinking out loud about how to generalise this change.) | 
| 
 I was going to do those in a separate PR, but rely on "A little copying is better than a little dependency" and just copy the same config changes across. Breaking changes scare me | 
| @sinkingpoint 👍 that seems fine, we can always make a breaking change independently if desired. | 
bac556b    to
    8f6079d      
    Compare
  
    | Actually, looking at it a bit more: configgrpc provides  | 
        
          
                config/confighttp/server_test.go
              
                Outdated
          
        
      |  | ||
| func TestServerReusePort(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("skipping test: SO_REUSEPORT is not supported on windows") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test checking that we do get an error when trying to use SoReuse on windows?
3d1484b    to
    e063074      
    Compare
  
    | Could you avoid force-pushing? That breaks the GitHub review interface. | 
| Apologies 😬 I think I've fixed it up now anyway | 
| 
 Where did you see that being done? I found the stef receiver is not using confighttp, for example: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/e3f1205a034ff05c1c31e1e1b84a8bf735563d91/receiver/stefreceiver/stef.go#L57-L77 | 
| Oh hrm.. I checked several, like https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/skywalkingreceiver/skywalking_receiver.go#L144 , but you're right - missed the ones that use confignet. Want me to update that here? Or put in a seperate PR? | 
| Ah I see. That receiver probably should be updated, but I digress A separate PR sounds good to me, maybe change the description so the issue doesn't get closed when this PR is merged | 
| Done. Is there anything blocking this PR then? I see the CodeCov, but those lines are covered, just in the windows tests (which tracks the error flow) | 
| @sinkingpoint nope, looks good to go. I've marked it ready to merge, just needs a look from a maintainer now. | 
| func (sc *ServerConfig) getListenConfig() (net.ListenConfig, error) { | ||
| if sc.ReusePort { | ||
| return net.ListenConfig{}, errors.New("ReusePort is not supported on this platform") | ||
| } | ||
|  | ||
| return net.ListenConfig{}, nil | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to have this in Validate and fail during dry-run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogdandrutu The ServerConfig doesn't have a Validate yet - are you proposing I add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, introducing Validate should be another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, introducing Validate should be another PR.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ServerConfig doesn't have a Validate yet - are you proposing I add one?
Yes please, every time we add fields that need to be validated we should validate them and if that needs a Validate func we add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added @bogdandrutu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
Because that's a separate change that would need its own changelog entry, and having atomic PRs facilitates reviews.
This adds in a new field, `ReusePort` that, if set, sets the SO_REUSEPORT socket option on the listener port. If we're on non unix, this errors out instead as AFAIK SO_REUSEPORT isn't available. Cursory testing says that SO_REUSEADDR _might_ work, but I don't have a windows machine to test on. Signed-off-by: sinkingpoint <[email protected]>
Signed-off-by: sinkingpoint <[email protected]>
This adds a new Validate method that checks the getListenConfig call so that the new SO_REUSEPORT stuff fails in dry run on Windows Signed-off-by: sinkingpoint <[email protected]>
1073d97    to
    6a5a94c      
    Compare
  
    
Description
This adds in a new field,
ReusePortthat, if set, sets the SO_REUSEPORT socket option on the listener port. If we're on non unix, this errors out instead as AFAIK SO_REUSEPORT isn't available. Cursory googling says that SO_REUSEADDR might work, but I don't have a windows machine to test on.Link to tracking issue
#14046
Testing
We add a new test,
TestServerReusePortwhich checks the behaviour of both setting and unsetting the ReusePort config with the expectation of an error if it isn't set and it not erroring when binding twice to the same port