Unify AllowedAddressConverter configuration code#10175
Open
Conversation
975d1ae to
c95de7e
Compare
c95de7e to
a86c3b1
Compare
chrsmith
commented
May 5, 2026
| } | ||
| } | ||
|
|
||
| var AllowedAddresses = dynamicconfig.NewNamespaceTypedSettingWithConverter( |
Contributor
Author
There was a problem hiding this comment.
Changes in this file:
- Move
AllowedAddressesup, to reside with the other config options. - Move the
AddressMatchRulesand other implementation logic into the newconfig_address_match.go.
| Wildcards, '*', are supported and can match any number of characters (e.g. '*' matches everything, 'prefix.*.domain' matches 'prefix.a.domain' as well as 'prefix.a.b.domain'). | ||
| - "AllowInsecure":bool (optional, default=false) indicates whether https is required`) | ||
|
|
||
| type AddressMatchRules struct { |
Contributor
Author
There was a problem hiding this comment.
Deleted this code and associated tests, since it was a nearly identical copy of the version in chasm/lib/callback.
| config.MaxLinksPerRequest = dc.GetIntPropertyFnFilteredByNamespace(10) | ||
| config.CallbackEndpointConfigs = dc.GetTypedPropertyFnFilteredByNamespace(callbacks.AddressMatchRules{ | ||
| Rules: []callbacks.AddressMatchRule{ | ||
| config.CallbackEndpointConfigs = dc.GetTypedPropertyFnFilteredByNamespace(callback.AddressMatchRules{ |
Contributor
Author
There was a problem hiding this comment.
NOTE: There are plenty of tests that reference and update the components/callbacks.AddressMatchRules field directly. Those will continue to work as-is. (And moving forward, tests can also set the chasm/lib/callback.AddressMatchRules config value too, and achieve the same thing. Since the frontend/fx.go is using both sets of values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Despite the large diff, the result is just simplifying some quirks introduced when we added the CHASM-implementation of callbacks.
Background
When the CHASM callbacks code was added in #8473, it forked the configuration setting from HSM. Specifically:
chasm.callback.allowedAddressesconfig settingAddressMatchRules,AddressMatchRuletypesallowedAddressConverterfor reading dynamic configThat seemed like a reasonable approach. But unfortunately it's left the code in a kind of rickety state.
Problem
Because of this duplication there's a number of quirks:
AddressMatchRulesin two places in Hook up CHASM Scheduler to Frontend handler #8694.AddressMatchRulesin service/frontend/fx.go. And so the CHASMValidator.gouses the "newer copy" of the address matching code. Despite it looking like the "older code" is being used.Changes
This PR applies a few key changes to make this situation a bit easier to navigate.
AddressMatchRulesimplementation. Sincecomponents/callbacksalready had a dependency onchasm/lib/callback, thecomponents/callbacksversion was deleted. (Assuming that the two versions were functionally identical.)config_address_match[_test].go.chasm.callback.allowedAddressesconfiguration key to justcallback.allowedAddresses. Since the value was never read, I'm assuming this won't break in any non-pathological instances.Confirmation the duplicated code is still (mostly) identical
Why?
AllowedAddressesconfiguration values, things work as expected.How did you test it?
Potential risks
I'm honestly not sure. I would love your thoughts on things I may have missed.