Skip to content
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

[Windows] Fixes UDS integration & caches socket domain #13123

Merged
merged 14 commits into from
Sep 22, 2020

Conversation

davinci26
Copy link
Member

Commit Message:

The PR aims to fix UDS integration tests on Windows. To that end the following changes were made:

  1. Fix domain function usage on Windows by caching the domain from the socket creation and passing down to the io_handler.
  2. Adds the correct length for UDS on Windows.
  3. Enables the test.

Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A

Sotiris Nanopoulos added 2 commits September 15, 2020 17:39
@davinci26
Copy link
Member Author

cc: @envoyproxy/windows-dev

Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@wrowe
Copy link
Contributor

wrowe commented Sep 16, 2020

/wait

For defaults resolution

@davinci26
Copy link
Member Author

@wrowe marking it as a draft to query some full builds in the CI to figure out which consumers are setting the default value for domain and see why this is the case

@davinci26 davinci26 changed the title [Windows] Fixes UDS integration & caches socket domain Draft: [Windows] Fixes UDS integration & caches socket domain Sep 16, 2020
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26 davinci26 marked this pull request as draft September 16, 2020 19:39
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26 davinci26 changed the title Draft: [Windows] Fixes UDS integration & caches socket domain [Windows] Fixes UDS integration & caches socket domain Sep 17, 2020
@davinci26 davinci26 marked this pull request as ready for review September 17, 2020 15:52
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

@wrowe i expect the ci to go green and it should be ready for review

Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

clang-tidy fails due to #13162 that is coming from upstream

Sotiris Nanopoulos added 2 commits September 17, 2020 17:01
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
wrowe
wrowe previously approved these changes Sep 18, 2020
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@davinci26
Copy link
Member Author

@envoyproxy/senior-maintainers can I also get a review from you.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM with small comments, thanks.

/wait

source/common/network/io_socket_handle_impl.cc Outdated Show resolved Hide resolved
source/common/network/io_socket_handle_impl.h Outdated Show resolved Hide resolved
Signed-off-by: Sotiris Nanopoulos <[email protected]>
mattklein123
mattklein123 previously approved these changes Sep 21, 2020
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@davinci26
Copy link
Member Author

Looks like clang-format is not happy with the new changes. My working hypothesis is that it has some trouble with the following functions

absl::optional<uint32_t> maybeGetPacketsDroppedFromHeader(
#ifdef SO_RXQ_OVFL
    const cmsghdr& cmsg) {
  if (cmsg.cmsg_type == SO_RXQ_OVFL) {
    return *reinterpret_cast<const uint32_t*>(CMSG_DATA(&cmsg));
  }
#else
    const cmsghdr&) {
#endif
  return absl::nullopt;
}

and Address::InstanceConstSharedPtr maybeGetDstAddressFromHeader(const cmsghdr& cmsg, uint32_t self_port, os_fd_t fd) {

working in simplifying the #ifeds

@mattklein123
Copy link
Member

If it forces it and is a bug in the formatter don't worry about it.

@davinci26
Copy link
Member Author

Actually it is easy to fix if you simplify the functions above so it might be an overall win. I just transformed the #ifdefs into some constexpr functions.

Verifying my changes and pushing another commit

@davinci26
Copy link
Member Author

WDYT of the changes in 509d1c3? Locally it fixes my clang-format.

@davinci26
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (macOS), please wait.

🐱

Caused by: a #13123 (comment) was created by @davinci26.

see: more, trace.

@davinci26
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13123 (comment) was created by @davinci26.

see: more, trace.

Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

davinci26 commented Sep 21, 2020

@mattklein123 the CI is all green again. feel free to take a look at the last 2 commits when you get some time.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, nice cleanup!

@mattklein123 mattklein123 merged commit 9a3c970 into envoyproxy:master Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants