-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Build Envoy with aws_lc on Power (ppc64le) #38403
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
Conversation
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
|
This seems OK to me but will defer to @ggreenway |
|
needs |
ggreenway
left a comment
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 think we need to document somewhere that any builds using aws-lc or on PPC are not covered by the envoy security policy, and that PPC is currently best-effort and not maintained by the Envoy maintainers.
Both of those caveats are necessary because we don't have any CI coverage for this build variant. If that changes at some point in the future that can be re-evaluated.
|
|
||
| // Check for duplicates. | ||
| #ifdef OPENSSL_IS_AWSLC | ||
| if (sk_X509_NAME_find_awslc(list.get(), nullptr, name)) { |
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.
Can you explain why this, and only this, function needs a _awslc suffix? Is there a defined policy in aws-lc for when a different function name is used?
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.
Thinking through this a little more, I think it would be easier to read and reason about if you made a header for api compatibility that you include in needed files, and the entire content is inside #ifdef OPENSSL_IS_AWSLC, and you either #define sk_X509_NAME_find sk_X509_NAME_find_awslc, or define a function named sk_X509_NAME_find in the Envoy namespace so that it's a closer match than the global namespace version, which does the needed API translation. I'd prefer that to sprinkling preprocessor directives around the code.
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.
Can you explain why this, and only this, function needs a
_awslcsuffix? Is there a defined policy in aws-lc for when a different function name is used?
Yes, the function sk_X509_NAME_find_awslc is consistent with the boringssl implementation while the sk_X509_NAME_find function in aws-lc has a different number of parameters in order to be openssl compatible.
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.
Thinking through this a little more, I think it would be easier to read and reason about if you made a header for api compatibility that you include in needed files, and the entire content is inside
#ifdef OPENSSL_IS_AWSLC, and you either#define sk_X509_NAME_find sk_X509_NAME_find_awslc, or define a function namedsk_X509_NAME_findin theEnvoynamespace so that it's a closer match than the global namespace version, which does the needed API translation. I'd prefer that to sprinkling preprocessor directives around the code.
I'll make that fix.
|
@ggreenway I've made the change to move the preprocessor conditions to a header file. Let me know if there is a better location for the header file or a better place to include it in the build process. |
ggreenway
left a comment
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.
This looks fine from the point-of-view of the TLS code; this is pretty unobtrusive.
I need someone else to review the bazel changes. Either @phlax or @keith probably.
Is it intended that use of aws-lc will always be FIPS compliant?
Please document somewhere that any builds using aws-lc or on PPC are not covered by the envoy security policy, and that PPC is currently best-effort and not maintained by the Envoy maintainers.
If in the future the boringssl and aws-lc APIs diverge, Envoy will follow boringssl, even if it breaks aws-lc.
keith
left a comment
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.
on the bazel side i think this looks good! but i would love it if there were links to upstream patches for all the new patches here so we can work to eliminate them. they all seem super reasonable so hopefully they can land upstream
| config_setting( | ||
| name = "platform_cpu_ppc64le", | ||
| - constraint_values = ["@platforms//cpu:ppc"], | ||
| + constraint_values = ["@platforms//cpu:ppc64le"], |
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.
is there an upstream patch for this? seems like a good change
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.
There is no upstream patch for this change yet.
| "aarch64-pc-windows-msvc": "rust_windows_aarch64", | ||
| "aarch64-unknown-linux-gnu": "rust_linux_aarch64", | ||
| "s390x-unknown-linux-gnu": "rust_linux_s390x", | ||
| + "powerpc64le-unknown-linux-gnu": "rust_linux_powerpc64le", |
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.
is there an upstream patch for all this?
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 have submitted a PR for this one: bazelbuild/rules_rust#3259
| struct( | ||
| name = "remotejdk21_linux_ppc64le", | ||
| - target_compatible_with = ["@platforms//os:linux", "@platforms//cpu:ppc"], | ||
| + target_compatible_with = ["@platforms//os:linux", "@platforms//cpu:ppc64le"], |
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.
upstream patch?
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.
There is no upstream patch for this change yet.
I have submitted a PR for this change: bazelbuild/rules_java#274
| + actual = select({ | ||
| + ":linux-aarch_64": "@com_google_protobuf_protoc_linux_aarch_64//:protoc", | ||
| + ":linux-x86_64": "@com_google_protobuf_protoc_linux_x86_64//:protoc", | ||
| + ":linux-ppcle_64": "@com_google_protobuf_protoc_linux_ppcle_64//:protoc", |
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.
upstream patch?
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.
This is one patch file that existed before my changes (I just added the linux-ppcle_64 case). I have not created an upstream patch for this one either.
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.
this is our patch to try and prevent building of protoc
|
needs main merge and DCO fix @keith will land when you are happy /wait |
Build Envoy with aws_lc instead of boringssl when building for the ppc64le architecture. Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
Remove conditional compilation gaurds for aws-lc, adding the new header file to do the proper translation. Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
5bace09 to
b224c51
Compare
Rebased changes on the main branch and signed all commits. |
|
@Jenkins-J the format and deps fails are real |
Thank you, I will start fixing those now. |
|
/retest flakey tsan/quic |
bd7025b to
0ecbfb9
Compare
|
@phlax There remains a "spelling" error in the aws_lc_compat.h file because the check identifies "ppc64le" as a spelling error. Is the preferred way to resolve this by adding "ppc64le" to the dictionary file or should I just remove the term from the comment? |
|
Put backticks around it and the word won't be spell checked. Or possibly double backticks. |
Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
0ecbfb9 to
25d2f46
Compare
|
Please do not force-push/rebase. It makes PRs difficult to review. Line 136 in 6317db1
|
|
@ggreenway I'm so sorry, I did not see that before. |
|
I have made all of the formatting and dependency corrections. There is one failing check but it appears to be unrelated to the changes in this PR. Do those checks need to be run again? |
|
yep - looks unrelated - you can /retest ... yourself if you need to (but please check it looks unrelated first) |
|
argh - didnt work /retest |
phlax
left a comment
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.
lgtm, thanks @Jenkins-J
Signed-off-by: James Jenkins <James.Jenkins@ibm.com>
Fix FIPS builds on ARM, broken by #38403 Signed-off-by: Kuat Yessenov <kuat@google.com>
Fix FIPS builds on ARM, broken by envoyproxy#38403 Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Ryan Northey <ryan@synca.io>
Fix FIPS builds on ARM, broken by #38403 Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Ryan Northey <ryan@synca.io>
Fix FIPS builds on ARM, broken by envoyproxy#38403 Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Kyle O'Connor <oconnorkyle@google.com>
Fix FIPS builds on ARM, broken by envoyproxy#38403 Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Kyle O'Connor <oconnorkyle@google.com>
Fix FIPS builds on ARM, broken by envoyproxy#38403 Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Jacob Lisi <jtlisi@google.com>
|
@Jenkins-J Can you tell me the exact bazel command I can use to build envoy on ppc64le? Thanks. |
Commit Message: Build Envoy with aws_lc instead of boringssl when building for the ppc64le architecture.
Additional Description: Envoy uses boringssl as the default ssl implementation. As boringssl does not support the ppc64le architecture, a different ssl implementation is necessary to build Envoy for ppc64le.
Risk Level: High
Testing:
Docs Changes:
Release Notes:
Platform Specific Features: Boringssl no longer supports the ppc64le architecture. Here, aws_lc is used as a "drop-in" replacement for boringssl when building Envoy for ppc64le. Aws_lc is conditionally built only when Envoy is built for ppc64le with FIPS support.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]