Skip to content

Unify AllowedAddressConverter configuration code#10175

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

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.
  • 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.

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