-
Notifications
You must be signed in to change notification settings - Fork 22
Add environment variable configuration loading on library initialization #190
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
- Add support for reading configuration from environment variables on library load - Environment variable format: SCITOKEN_CONFIG_<KEY> where <KEY> maps to config keys - Support case-insensitive environment variable names - Automatically detect and parse int vs string configuration values - Fix static initialization order issue using construct-on-first-use idiom - Add comprehensive tests for environment variable configuration Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
- Use std::array instead of plain C array for better type safety - Replace catch-all exception handler with specific std::invalid_argument and std::out_of_range - Use std::stoi with exception handling instead of std::atoi in tests - Fix include path in test to use public header instead of relative path Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
lint
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 16 in 0f45e25
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 18 in 0f45e25
| const char *env_update = std::getenv("SCITOKEN_CONFIG_KEYCACHE_UPDATE_INTERVAL_S"); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 22 in 0f45e25
| int actual = scitoken_config_get_int("keycache.update_interval_s", &err_msg); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Lines 24 to 25 in 0f45e25
| std::cerr << "FAIL: keycache.update_interval_s expected " << expected | |
| << " but got " << actual << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 33 in 0f45e25
| std::cout << "PASS: keycache.update_interval_s = " << actual << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 36 in 0f45e25
| std::cerr << "FAIL: Could not parse env var value: " << e.what() << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Lines 40 to 42 in 0f45e25
| // Test 2: Check if SCITOKEN_CONFIG_KEYCACHE_EXPIRATION_INTERVAL_S was loaded | |
| const char *env_expiry = std::getenv("SCITOKEN_CONFIG_KEYCACHE_EXPIRATION_INTERVAL_S"); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 46 in 0f45e25
| int actual = scitoken_config_get_int("keycache.expiration_interval_s", &err_msg); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Lines 48 to 49 in 0f45e25
| std::cerr << "FAIL: keycache.expiration_interval_s expected " << expected | |
| << " but got " << actual << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 57 in 0f45e25
| std::cout << "PASS: keycache.expiration_interval_s = " << actual << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 60 in 0f45e25
| std::cerr << "FAIL: Could not parse env var value: " << e.what() << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 64 in 0f45e25
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 69 in 0f45e25
| int rv = scitoken_config_get_str("keycache.cache_home", &actual, &err_msg); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 71 in 0f45e25
| std::cerr << "FAIL: Could not retrieve keycache.cache_home" << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 79 in 0f45e25
| std::cerr << "FAIL: keycache.cache_home expected '" << env_cache |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 85 in 0f45e25
| if (actual) free(actual); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 87 in 0f45e25
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 102 in 0f45e25
| std::cerr << "FAIL: tls.ca_file expected '" << env_ca |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 108 in 0f45e25
| if (actual) free(actual); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 110 in 0f45e25
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 112 in 0f45e25
| const char *env_lower = std::getenv("scitoken_config_keycache_update_interval_s"); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 116 in 0f45e25
| int actual = scitoken_config_get_int("keycache.update_interval_s", &err_msg); |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 118 in 0f45e25
| std::cerr << "FAIL: lowercase env var - keycache.update_interval_s expected " |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 127 in 0f45e25
| std::cout << "PASS: lowercase env var - keycache.update_interval_s = " << actual << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 130 in 0f45e25
| std::cerr << "FAIL: Could not parse env var value: " << e.what() << std::endl; |
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 134 in 0f45e25
[lint] reported by reviewdog 🐶
scitokens-cpp/test/test_env_config.cpp
Line 136 in 0f45e25
| std::cout << "\nAll environment variable configuration tests passed!" << std::endl; |
bbockelm
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.
|
@copilot - please address linter issues. |
- Run clang-format on all modified files to fix formatting - No functional changes, only whitespace and formatting adjustments Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
Addressed in commit 1ead755. All files have been formatted with clang-format according to the project's LLVM-based style configuration. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot - please address linter issues. |
- Format src/verify.cpp to fix line length violation - Split long fprintf line across multiple lines per LLVM style Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
Fixed in commit dcdb1be. The linter was reporting a formatting issue in src/verify.cpp where a long fprintf line exceeded the line length limit. Split the line across multiple lines per LLVM style guide. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
bbockelm
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.
| // See scitokens_internal.h for the new implementation | ||
| std::atomic_int configurer::Configuration::m_next_update_delta{0}; | ||
| std::atomic_int configurer::Configuration::m_expiry_delta{0}; | ||
| std::shared_ptr<std::string> configurer::Configuration::m_cache_home; |
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.
std::shared_ptr is not thread-safe; there may be multiple writers to these strings.
Instead, replace with a mutex around a std::string.
To keep a "fast path", add an atomic flag that indicates the variable may have been set. Updates should be atomic but we can use relaxed semantics as to when they become available. If the flag is set, then the mutex should be taken and the variables set. If the flag is not set, then assume the variables are unset.
@copilot - please make this small adjustment. Run clang-format once done.
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.
Fixed in commit 87b727b. Replaced std::shared_ptr<std::string> with mutex-protected std::string and added atomic flags for fast-path checking with relaxed memory ordering. The get functions now check the atomic flag first (fast path) and only acquire the mutex if the value has been set (slow path).
- Replace std::shared_ptr<std::string> with mutex-protected std::string - Add atomic flags for fast-path checking to avoid lock contention - Use relaxed memory ordering for atomic flags as suggested - Ensures thread-safe access to cache_home and tls_ca_file configuration - All formatting done with clang-format per project standards Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot - merge and resolve conflicts. |
Merged in commits 9b07612 and 7692efb. Resolved conflicts in src/scitokens.cpp and test/CMakeLists.txt. Added support for the new monitoring configuration keys (SCITOKEN_CONFIG_MONITORING_FILE and SCITOKEN_CONFIG_MONITORING_FILE_INTERVAL_S) to the environment variable loader. All tests pass successfully. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…config-inheritance
7692efb to
942c5ef
Compare
Implementation Plan for Environment-based Global Config
SCITOKEN_CONFIG_<key>environment variableskeycache.update_interval_s→SCITOKEN_CONFIG_KEYCACHE_UPDATE_INTERVAL_S)__attribute__((constructor))to run on library loadSecurity Summary
No security vulnerabilities were detected by CodeQL analysis. The implementation:
Summary
This PR successfully implements environment-based global configuration for the scitokens-cpp library. Configuration keys can now be inherited from environment variables on library load using the format
SCITOKEN_CONFIG_<KEY>. The implementation:SCITOKEN_CONFIG_prefix with underscores replacing dots in configuration keysSupported environment variables:
SCITOKEN_CONFIG_KEYCACHE_UPDATE_INTERVAL_S(int)SCITOKEN_CONFIG_KEYCACHE_EXPIRATION_INTERVAL_S(int)SCITOKEN_CONFIG_KEYCACHE_CACHE_HOME(string)SCITOKEN_CONFIG_TLS_CA_FILE(string)SCITOKEN_CONFIG_MONITORING_FILE(string)SCITOKEN_CONFIG_MONITORING_FILE_INTERVAL_S(int)All existing unit tests pass (29/31, with 2 pre-existing network-related failures). The new environment configuration test and monitoring test pass successfully.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.