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

update 3rd party libraries, part2 #3707

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

georgeliao
Copy link
Contributor

@georgeliao georgeliao commented Sep 30, 2024

WIP
close #3568

Original repo
1. vcpkg, updated to 2024.09.30.

vcpkg:
1. grpc, 1.52.1-> 1.60.0, ongoing.

There are a lot hassles in grpc update:, so it is worth to talk about it. grpc has two layers of patches in the multipass use case, the first layer is the commits on the top of the standard grpc release.

Layer 1:
Orignally, there are 5 commits on the top of 1.52.1 release tag, that is the branch. They are around two things.

  1. The first 3 commits are about adding the grpc_ssl_server_certificate_request_type options to the client side and disable pem_root_certs check along the way, see this section in the spec for details.
  2. The 4th and 5th commits are simply just using getsockname to get local server address instead of getpeername to get the remote address.
  3. Besides the orginal 5 commits, the 6th commit added and it is about removing newly added config_.pem_root_certs == nullptr checks based on the workflow first 3 commits setup, meaning that config_.pem_root_certs is supposed to be nullptr because we do not want to verify the server certificate. Because of that, the SslCredentialsOptions::pem_root_certs field which is the trust store of server certificates is set nulltptr when we create a channel on the client side.
  4. At last, the 7th commit is added because grpc' change in this duration broke the backward compatibility. The particular commit is this one. There is also another github issue which has relevant discussions about this. This change affects grpc version 1.57.0 and onwards. The fix in the 7th commit is just simply reverting the change on the unix domain socket name solver GetDefaultAuthority function. It is a quick fix and it is intrusive as well. The alternative of this is mentioned in the github issue, where they call grpc.insecure_channel(srv_endpoint, options=(('grpc.default_authority', 'localhost'),)) on python client to overwrite the default authority from the client code, the C++ equivalent to this would be changing grpc::CreateChannel calls to grpc::CreateCustomChannel calls with channel arguments, see below code snippet for details
    grpc::ChannelArguments channel_args;
    channel_args.SetString(GRPC_ARG_DEFAULT_AUTHORITY, "localhost");

    // Use CreateCustomChannel to apply ChannelArguments
    auto channel = grpc::CreateCustomChannel(
        server_address, grpc::InsecureChannelCredentials(), channel_args);

Be aware that this change affects all types of address resolvers as opposed to just unix domain socket address resolver. Besides, this approach is also a workaround based on the discussions in the thread. So I personally prefer the current fix and wait for the update of grpc.

Layer 2:
The second layer patches is the new patches applied and the changes made (the diff between the vcpkg tempate file and multipass cutom file) in the vcpkg grpc portfile.cmake file. The goal of the custom portfile.cmake is to cut off the unneeded library build and cut down the library size, which will reduce the build time of grpc, multipass and the binary size of them.

snap:

1. sshfs, will be done it in a different PR. 

@georgeliao georgeliao marked this pull request as draft September 30, 2024 16:23
@georgeliao georgeliao force-pushed the update_3rd_lib_part2 branch 2 times, most recently from a45865d to 5bb9771 Compare October 3, 2024 09:50
Base automatically changed from update_3rd_lib_version to main October 3, 2024 13:50
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.85%. Comparing base (183583d) to head (4d4ad86).
Report is 277 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3707      +/-   ##
==========================================
- Coverage   88.92%   88.85%   -0.07%     
==========================================
  Files         254      256       +2     
  Lines       14294    14560     +266     
==========================================
+ Hits        12711    12938     +227     
- Misses       1583     1622      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ricab ricab added this to the 1.15.0 milestone Oct 7, 2024
@@ -1,5 +1,5 @@
{
"builtin-baseline": "ca7b1b15f548c25c766360593a2c732d56ed0133",
"builtin-baseline": "c82f74667287d3dc386bce81e44964370c91a289",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

builtin-baseline field defines the versions of the dependencies of our vcpkg packages. In this particular case, grpc needs this update to get his dependency versions right because of his own update. The value of builtin-baseline field is simply the commit hash of the vcpkg repo.

@georgeliao
Copy link
Contributor Author

georgeliao commented Nov 6, 2024

Hi @Saviq @townsend2010

Sorry to bother you guys. Recently, multipass team has been dealing with rebasing the patches (code commits on the top of the release tag) of grpc after updating grpc to newer version. It has been a painful process due to the size of the patches and conflicts with the grpc refactors and changes. This really got us thinking what is the exact reason to keep these patches. Why multipass has a unique use case from the regular ones?

By looking at commit messages and the code, my superficial understanding is that. Multipass wants to support skipping verifying server certificate. Because of that, the grpc_ssl_server_certificate_request_type enum is added and all the corresponding changes are made. But why exactly do we need that is still not clear to me, meaning why does multipass uniquely need this skip the server certificate verification option.

Any information would be appreciated in case you guys know about it, thanks a lot.

@Saviq
Copy link
Collaborator

Saviq commented Nov 6, 2024

why does multipass uniquely need this skip the server certificate verification option

Basically to support SSH-style authorization. Both sides generate their encryption keys in isolation, without a CA between them. All communication is encrypted, and the key fingerprints are what determines whether they want to talk to one other on a higher level, not whether the certificates are valid or not.

@georgeliao
Copy link
Contributor Author

georgeliao commented Nov 12, 2024

Basically to support SSH-style authorization. Both sides generate their encryption keys in isolation, without a CA between them. All communication is encrypted, and the key fingerprints are what determines whether they want to talk to one other on a higher level, not whether the certificates are valid or not.

Thanks for the reply. But why exactly does multipass have to use the SSH-style authorization? The standard TLS/mTLS always requires the server certificate and verifies it. As a result, grpc api only provides grpc_ssl_client_certificate_request_type but not grpc_ssl_server_certificate_request_type and it seems to make sense. The orthodox way based on the documentation of doing TLS/mTLS grpc is to fill in the pem_root_certs field of grpc::SslCredentialsOptions object on the client side, and later it will be used to verify the server cert and key pair internally.

My wondering here is whether this orthodox way has some inadequacies we are not aware of or it somehow does not fit in the multipass use case. Any info will be appreciated, thanks.

…, which is non-intrusiveserver address resolver from intrusive change to
@@ -0,0 +1,2 @@
list(REMOVE_AT ARGS 0)
_find_package(gRPC ${ARGS}) # Shouldn't this be fixed downstream instead of using a Wrapper?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_find_package(gRPC ${ARGS}) # Shouldn't this be fixed downstream instead of using a Wrapper?
_find_package(gRPC ${ARGS}) # Shouldn't this be fixed downstream instead of using a Wrapper?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update all our dependencies
3 participants