Issue 4546: Refactor FD with a general I/O Handle#5128
Issue 4546: Refactor FD with a general I/O Handle#5128sbelair2 wants to merge 17 commits intoenvoyproxy:masterfrom sbelair2:master
Conversation
Signed-off-by: sbelair2 <sbelair@cisco.com>
Signed-off-by: sbelair2 <sbelair@cisco.com>
Signed-off-by: sbelair2 <sbelair@cisco.com>
Pull in commits that were pushed to evnoyfnew1/envoy but for which the PR failed (check_repositoriws failure)
|
@mattklein123 It looked like the circle:mac test passed. I couldn't view the cicrleci:ipv6 test, so Im not sure what the issue is there. Please review. |
This makes life easier when debugging SSL handshake issues. Part of #1319. Risk Level: Low Testing: ssl_integration_test with debug and !debug. Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
…5096) Also, some bonus cleanups to improve DRY and maintainability of SSL related test code encountered while doing this. Part of #1319. Risk Level: Low Testing: bazel test //test/integration/..., new tests for client cipher suites and ECDSA server certs added to ssl_integration_test. Signed-off-by: Harvey Tuch <htuch@google.com>
This PR starts to plumb multiple TLS certs from the proto level into the SSL context. We stop short of enabling multiple TLS certificates, but instead have sufficient mechanism and interface changes to propagate them to the SSL context. Future PRs will extend this with the SSL context implementation. Risk Level: Low Testing: bazel test //test/... Part of #1319. Signed-off-by: Harvey Tuch <htuch@google.com>
#4682) Add filter chain match for source_type. Possible options are ANY (default) LOCAL EXTERNAL This allows for explicitly specifying local connectivity detection, which is needed in specific use cases. Risk Level: Low Docs Changes: Inline proto comments Related to #4535. Signed-off-by: Nikolay Nikolaev <nnikolay@vmware.com>
|
@sbelair2 it seems there have been a lot of PRs opened/closed for this patch. It's kind of hard to reason across this chain; can you try and stick to one PR per change? If you need git assistance, happy to help out here or on Slack. |
|
@htuch Yes, I totally botched previous PR attempts. I then hit issues with check_repositories, which I did post on slack. As of this posting of this PR last night, there were no conflicts and the diffs presented were final. I really am not sure how you would prefer the next steps to be- I thought that the state of the PR was fine in that the diffs represent the final outcome of the PR. What would you suggest? |
|
@sbelair2 I think we are just trying to understand whether you have figured out your workflow issues before we start reviewing. This PR already needs a master merge. Can you do that to make sure that workflow works for you? Thanks. |
|
@mattklein123 I have basically figured out my workflow issues, thanks to Ed Warnicke's help (don't know his GitHub handle). I am working on the master merge as we speak. The several consecutive pushes to my fork were a result of my fetching upstream, merging upstream changes with my changes, and then pushing. Once pushed, I see that I was out of sync again, and again. Is the issue for the reviewers that there were several pushes to my fork? In which case, a squash is required? My code changes are indeed in a final state, but I appear to be chasing the parade of changes here. Feel free to advise- thank you. |
Signed-off-by: Daniel Hochman <danielhochman@users.noreply.github.com>
|
@sbelair2 sometimes merging master is going to be needed. It should be a pretty simple operation. Is there any chance that you have someone local to where you work that can help you a bit with git? |
Merge of stale #4682 caused this. Signed-off-by: Harvey Tuch <htuch@google.com>
|
@mattklein123 Understood. Closing this PR and coming back with another correct one. |
|
@sbelair2 you didn't need to close this PR ... Just merge master ... I would really commend syncing up with someone a little bit about git or maybe looking at a GitHub tutorial? |
Signed-off-by: sbelair2 <sbelair@cisco.com>
Signed-off-by: sbelair2 <sbelair@cisco.com>
|
@mattklein123 I did as you suggested: merged with master and eventually pushed. Hopefully the DCO will still be ok after the merge. |
Signed-off-by: sbelair2 <sbelair@cisco.com>
|
@mattklein123 As suspected, there was an issue with DCO after the merge. I tried "git commit --amend --signoff" after the merge completed, but it has no affect. Note that the DCO was correct previously. Please advise. |
Retry merge to see if DCO is fixed Signed-off-by: sbelair2 <sbelair@cisco.com>
|
@sbelair2 I don't think you actually did a merge commit. Can I kindly ask that you find someone to help you with git and GitHub? Unfortunately we don't have the resources to help with this. |
|
/sub |
|
@mattklein123 I do have someone I can ask, I'm not going it alone here. I did a "git merge upstream/master", which worked fine except for the signoff (It appears that the --signoff flag can now be included with merge). Is the only issue the missing DCO signoff, or are there other issues that you see? If other issues, I can close the PR and open a new one that fixes them, Thank you. |
|
I have a higher level question on the plan with this PR -- there have been a few other attempts to abstract network handles, and I think it's a very achievable, beneficial project. However there are a couple of complexities that at first glance this PR does not tackle:
It might be helpful to start with a design doc and iterate on that. |
|
Ah I see that you were one of the authors of the earlier PRs: #4579 -- sorry for the confusion. |
|
@sbelair2 I'm sorry I'm not sure where you went wrong with the DCO. You can fix by rebasing and force pushing. Please do not open new PRs. |
|
I don't think DCO is the only remaining issue. If you look at the files view, I see many changes which aren't yours, which indicates something else has gone wrong :-/ |
|
@alyssawilk Correct! I see 89 files. changed, whereas my commit involved 41 files. Can this PR be salvaged? |
|
Yep, if you git rebase correctly (normally strongly discouraged but no one has reviewed this so it should be fine) and force push per Matt's earlier advice you should be able to correct this. |
|
@alyssawilk I've been looking at the extra files in this PR, and those are all the files that have been pulled from upstream/master and are being pushed into my fork- it would be incorrect to exclude them. I do think that DCO is the only apparent issue. Do you agree? |
|
Nope, that's not quite it. For example take a look at |
|
@alyssawilk Got it- makes sense. Thanks! |
|
🙀 Error while processing event: |
Description:
Introduce an abstract I/O Handle and one derivative for sockets, the IoSocketHandle.
Preserve fd semantics in all socket-related system calls with ioHandle->fd(). In this
initial PR, all calls which take an fd retain their same parameters.
Risk Level:
Low
Testing:
Everything covered by bazel test //test...