Skip to content

Unify AllowedAddressConverter configuration code#10175

Open
chrsmith wants to merge 1 commit intomainfrom
chrsmith/unify-address-config
Open

Unify AllowedAddressConverter configuration code#10175
chrsmith wants to merge 1 commit intomainfrom
chrsmith/unify-address-config

Conversation

@chrsmith
Copy link
Copy Markdown
Contributor

@chrsmith chrsmith commented May 4, 2026

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:

  • Added a new chasm.callback.allowedAddresses config setting
  • Added the AddressMatchRules, AddressMatchRule types
  • Added the allowedAddressConverter for reading dynamic config

That 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:

  • Duplication of maintenance for the two pieces of code, e.g. tweaking the AddressMatchRules in two places in Hook up CHASM Scheduler to Frontend handler #8694.
  • Despite having two config keys, only the HSM one is actually honored. The CHASM-version is never referenced, leading to confusion if you ever try to use it.
  • The HSM address rules are converted into the CHASM-defined AddressMatchRules in service/frontend/fx.go. And so the CHASM Validator.go uses 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.

  1. Delete one of the AddressMatchRules implementation. Since components/callbacks already had a dependency on chasm/lib/callback, the components/callbacks version was deleted. (Assuming that the two versions were functionally identical.)
  2. Combine HSM and CHASM configuration settings at runtime, so either can be set and honored. (I assume this will allow for an easier transition from one config setting to another, when the HSM code is able to be removed.)
  3. To (arguably) keep things tidier, I moved the code into config_address_match[_test].go.
  4. Rename the chasm.callback.allowedAddresses configuration key to just callback.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
# Compare the config code. 3x differences:
# - Config key name
# - Exporting the `Allow` and `Validate` methods.
export HSM_CONFIG="components/callbacks/config.go"
export CHASM_CONFIG="chasm/lib/callback/config.go"
diff <(sed -n '55,167p' $HSM_CONFIG) <(sed -n '61,173p' $CHASM_CONFIG)
2c2
<       "component.callbacks.allowedAddresses",
---
>       "chasm.callback.allowedAddresses",
17c17
< func (a AddressMatchRules) validate(rawURL string) error {
---
> func (a AddressMatchRules) Validate(rawURL string) error {
33c33
<               allow, err := rule.allow(u)
---
>               allow, err := rule.Allow(u)
54c54
< func (a AddressMatchRule) allow(u *url.URL) (bool, error) {
---
> func (a AddressMatchRule) Allow(u *url.URL) (bool, error) {

---

# Just the package name change, and calling the exported
# version of `.Validate`.
export HSM_CONFIG_TEST="components/callbacks/config_test.go"
export CHASM_CONFIG_TEST="chasm/lib/callback/config_test.go"
diff -f $HSM_CONFIG_TEST -f $CHASM_CONFIG_TEST
c1
package callback
.
c327
                        tt.validateErr(t, rules.Validate(tt.args.rawURL))
.

Why?

  • Removes confusion. If you set either of the AllowedAddresses configuration values, things work as expected.
  • Having one canonical implementation of the logic for address checking makes things easier to reason about.

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

Potential risks

I'm honestly not sure. I would love your thoughts on things I may have missed.

@chrsmith chrsmith force-pushed the chrsmith/unify-address-config branch from 975d1ae to c95de7e Compare May 4, 2026 23:26
@chrsmith chrsmith marked this pull request as ready for review May 5, 2026 17:28
@chrsmith chrsmith requested review from a team as code owners May 5, 2026 17:28
@chrsmith chrsmith force-pushed the chrsmith/unify-address-config branch from c95de7e to a86c3b1 Compare May 5, 2026 17:28
}
}

var AllowedAddresses = dynamicconfig.NewNamespaceTypedSettingWithConverter(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changes in this file:

  • Move AllowedAddresses up, to reside with the other config options.
  • Move the AddressMatchRules and other implementation logic into the new config_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 {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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{
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@stephanos stephanos self-requested a review May 5, 2026 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant