server: fix fips_mode stat#16140
Conversation
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
|
@raakella Are you able to iterate locally to get basic tests passing with |
|
/wait |
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
|
@jmarantz Locally it has been working. Seems like something is broken when these test are run in CI. Seems like https://github.com/envoyproxy/envoy/blob/main/source/common/version/version.cc#L41 is evaluating to true from tests but from the actual code, it is evaluating to false. |
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
test/common/common/BUILD
Outdated
| srcs = ["version_test.cc"], | ||
| copts = envoy_select_boringssl( | ||
| ["-DENVOY_SSL_FIPS"], | ||
| ), |
There was a problem hiding this comment.
Nit: This could have been written as copts = envoy_select_boringssl(["-DENVOY_SSL_FIPS"]),
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Looks fine generally with one nit
source/common/version/version.cc
Outdated
| #ifdef BORINGSSL_FIPS | ||
| fipsCompliant = true; | ||
| #ifdef ENVOY_SSL_FIPS | ||
| static bool fipsCompliant = true; |
There was a problem hiding this comment.
Why do we need the static? Why not just
#ifdef ENVOY_SSL_FIPS
return true;
#else
return false;
#endif
There was a problem hiding this comment.
@jmarantz Initially I thought this initialization must happen during compile time as "BORINGSSL_FIPS" is being pass during compilation. But it seems like it is not required. Should I change that?
There was a problem hiding this comment.
Yes, you don't need a static at all for this use-case. Just return the value based on the ifdef.
There was a problem hiding this comment.
Thanks, I will change it.
|
/wait |
Signed-off-by: Ravindra Akella <rakella@salesforce.com>
|
windows failure was a flake in pulling build image. /retest |
|
Retrying Azure Pipelines: |
|
@ggreenway @lizan Please merge this. |
Commit Message: Fix fips_mode stat by using a static variable to check if the ssl version is fips compliant or not. Additional Description: Originally added as part of envoyproxy#14719 Risk Level: Low Testing: Updated unit tests Docs Changes: None. Already documented Release Notes: Platform Specific Features: Signed-off-by: Ravindra Akella <rakella@salesforce.com> Signed-off-by: Gokul Nair <gnair@twitter.com>
5 participants
Signed-off-by: Ravindra Akella rakella@salesforce.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message: Fix fips_mode stat by using a static variable to check if the ssl version is fips compliant or not.
Additional Description: Originally added as part of #14719
Risk Level: Low
Testing: Updated unit tests
Docs Changes: None. Already documented
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]