Skip to content

Conversation

@ntrevino-virtru
Copy link
Contributor

Viper only loads environment values that have been identified (via BindEnv or loaded config file). Instead of using Viper we manually inspect the env and load the keys we care about.

Proposed Changes

  • Manually inspect the env and load the keys we care about

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

Viper only loads environment values that have been identified (via BindEnv or loaded config file).
Instead of using Viper we manually inspect the env and load the keys we care about.
@ntrevino-virtru ntrevino-virtru requested a review from a team as a code owner October 21, 2025 16:28
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @ntrevino-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical configuration loading issue by overhauling how environment variables are processed. It moves away from Viper's limited environment variable handling to a direct, manual inspection of system environment variables. This change ensures that all relevant environment variables, filtered by a prefix and an optional allow list, are correctly identified and loaded into the application's configuration. The refactoring improves the reliability and predictability of environment-based configuration, backed by new, thorough precedence tests.

Highlights

  • Environment Variable Loading Fix: The pull request resolves an issue where environment variables were not being loaded correctly by replacing the Viper-based environment variable loading with a manual inspection of system environment variables.
  • Viper Dependency Removal: The EnvironmentValueLoader has been refactored to remove its direct dependency on the Viper library for environment variable processing, opting for a custom implementation that directly reads from os.Environ().
  • Enhanced Configuration Precedence Testing: A comprehensive new test suite (TestLoad_Precedence) has been added to config_test.go to rigorously verify the loading order and precedence of various configuration sources, including defaults, file-based settings, and environment variables, ensuring robust configuration behavior.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Env vars flow free,No more Viper's binding chains,Config finds its way.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request addresses an issue where Viper was not correctly loading environment variables. The solution involves manually inspecting the environment and loading the necessary keys. The changes include modifications to the config_test.go and environment_value_loader.go files. The tests have been expanded to include precedence tests.

Copy link
Contributor

@jakedoublev jakedoublev left a comment

Choose a reason for hiding this comment

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

Some of these gemini comments look insightful

This is a defined key with snake-casing.
strantalis
strantalis previously approved these changes Oct 21, 2025
jakedoublev
jakedoublev previously approved these changes Oct 22, 2025
@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 161.661837ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 102.857187ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 378.967439ms
Throughput 263.87 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 40.100011059s
Average Latency 399.601945ms
Throughput 124.69 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 28.01022089s
Average Latency 279.107893ms
Throughput 178.51 requests/second

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 170.186235ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 96.305324ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 357.283027ms
Throughput 279.89 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.822460491s
Average Latency 386.084892ms
Throughput 128.79 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.270275024s
Average Latency 271.921549ms
Throughput 183.35 requests/second

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 169.923629ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 99.154579ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 352.404934ms
Throughput 283.76 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.558857688s
Average Latency 383.111336ms
Throughput 129.67 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.148899124s
Average Latency 270.728728ms
Throughput 184.17 requests/second

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 186.330077ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 102.674114ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 357.27973ms
Throughput 279.89 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 40.034012389s
Average Latency 398.606227ms
Throughput 124.89 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.964572021s
Average Latency 278.64763ms
Throughput 178.80 requests/second

@github-actions
Copy link
Contributor

@dmihalcik-virtru dmihalcik-virtru self-requested a review October 22, 2025 18:41
@ntrevino-virtru ntrevino-virtru added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit 9af3184 Oct 22, 2025
39 checks passed
@ntrevino-virtru ntrevino-virtru deleted the bug/env-loader-without-viper branch October 22, 2025 20:16
github-merge-queue bot pushed a commit that referenced this pull request Oct 22, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.11.0](service/v0.10.0...service/v0.11.0)
(2025-10-22)


### Features

* **authz:** add obligation fulfillment logic to obligation PDP
([#2740](#2740))
([2f8d30d](2f8d30d))
* **authz:** audit logs should properly handle obligations
([#2824](#2824))
([874ec7b](874ec7b))
* **authz:** defer to request auth as decision/entitlements entity
([#2789](#2789))
([feb34d8](feb34d8))
* **authz:** obligations protos within auth service
([#2745](#2745))
([41ee5a8](41ee5a8))
* **authz:** protovalidate tests for new authz obligations fields
([#2747](#2747))
([73e6319](73e6319))
* **authz:** service logic to use request auth as entity identifier in
PDP decisions/entitlements
([#2790](#2790))
([6784e88](6784e88))
* **authz:** wire up obligations enforcement in auth service
([#2756](#2756))
([11b3ea9](11b3ea9))
* **core:** propagate token clientID on configured claim via interceptor
into shared context metadata
([#2760](#2760))
([0f77246](0f77246))
* **kas:** Add required obligations to kao metadata.:
([#2806](#2806))
([16fb26c](16fb26c))
* **policy:** add FQNs to obligation defs + vals
([#2749](#2749))
([fa2585c](fa2585c))
* **policy:** Add obligation support to KAS
([#2786](#2786))
([bb1bca0](bb1bca0))
* **policy:** List obligation triggers rpc
([#2823](#2823))
([206abe3](206abe3))
* **policy:** namespace root certificates
([#2771](#2771))
([beaff21](beaff21))
* **policy:** Proto - root certificates by namespace
([#2800](#2800))
([0edb359](0edb359))
* **policy:** Protos List obligation triggers
([#2803](#2803))
([b32df81](b32df81))
* **policy:** Return built obligations fqns with triggers.
([#2830](#2830))
([e843018](e843018))
* **policy:** Return obligations from GetAttributeValue calls
([#2742](#2742))
([aa9b393](aa9b393))


### Bug Fixes

* **core:** CORS
([#2787](#2787))
([a030ac6](a030ac6))
* **core:** deprecate policy WithValue selector not utilized by RPC
([#2794](#2794))
([c573595](c573595))
* **core:** deprecated stale protos and add better upgrade comments
([#2793](#2793))
([f2678cc](f2678cc))
* **core:** Don't require known manager names
([#2792](#2792))
([8a56a96](8a56a96))
* **core:** Fix mode negation and core mode
([#2779](#2779))
([de9807d](de9807d))
* **core:** resolve environment loading issues
([#2827](#2827))
([9af3184](9af3184))
* **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.6.0 to
0.7.0 in /service
([#2812](#2812))
([a6d180d](a6d180d))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.12.0 to
0.13.0 in /service
([#2814](#2814))
([5e9c695](5e9c695))
* **deps:** bump github.com/opentdf/platform/sdk from 0.7.0 to 0.9.0 in
/service ([#2798](#2798))
([d6bc9a8](d6bc9a8))
* **deps:** bump github.com/opentdf/platform/sdk from 0.9.0 to 0.10.0 in
/service ([#2831](#2831))
([412dfd1](412dfd1))
* ECC key loading (deprecated)
([#2757](#2757))
([49990eb](49990eb))
* **policy:** Change to nil
([#2746](#2746))
([a449434](a449434))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants