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

Add a workaround to fix CAF OpenSSL options #2908

Merged
merged 1 commit into from Feb 2, 2023

Conversation

tobim
Copy link
Member

@tobim tobim commented Feb 2, 2023

In the actor_system_config, some settings are not like the others. While most modules read their options directly from the content object, the SSL configuration is stored in member variables of the main config class. The CAF command line and config file parsers handle this correctly, but settings from vast.yaml files or those that are passed in as environment variables are injected through a different code path.

This commit adds assignments to set the aforementioned class members explicitly.

I considered the alternative solution of translating non CAF-native option formats to native ones and let the parse function handle it, but that approach would be a large amount of work rather little benefit.

@tobim tobim added the bug Incorrect behavior label Feb 2, 2023
@tobim tobim requested a review from lava February 2, 2023 15:27
In the `actor_system_config`, some settings are not like the others.
While most modules read their options directly from the `content`
object, the SSL configuration is stored in member variables of the
main config class. The CAF command line and config file parsers
handle this correctly, but settings from `vast.yaml` files or those
that are passed in as environment variables are injected through
a different code path.

This commit adds assignments to set the aforementioned class members
explicitly.

I considered the alternative solution of translating non CAF-native
option formats to native ones and let the `parse` function handle
it, but that approach would be a large amount of work rather little
benefit.
@tobim tobim force-pushed the topic/fix-openssl-configuration branch from 6822ce1 to ab29dce Compare February 2, 2023 15:35
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

The fix makes sense to me, and I agree it's okay to fix this up afterwards instead of swapping the order. Should we ask upstream why the SSL options are treated this way rather than being read from the content settings object?

@tobim tobim enabled auto-merge February 2, 2023 16:39
@tobim
Copy link
Member Author

tobim commented Feb 2, 2023

Should we ask upstream why the SSL options are treated this way rather than being read from the content settings object?

I asked about it in the chat.

@tobim tobim merged commit 0b3b1a5 into master Feb 2, 2023
@tobim tobim deleted the topic/fix-openssl-configuration branch February 2, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
3 participants