Skip to content

feat: crawler config and a switch to turn it on or off - #97

Merged
manjudr merged 43 commits into
NetworkofNetworkfrom
feat/72-crawler-config-quick-start
Oct 28, 2026
Merged

manjudr merged 43 commits into
NetworkofNetworkfrom
feat/72-crawler-config-quick-start

Conversation

@ameersohel45

@ameersohel45 ameersohel45 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Turns the crawler on or off when you bring a network up, and splits make setup into one command per step.

What

Commands — bin/stack.sh, Makefile, bin/setup.py
make setup becomes four: keys, register-network, register-provider PROVIDER=, configure-adapter. make up now stops at the containers. configure-adapter asks whether to crawl and which mode, saves both to .env, renders the config, then starts the adapters. A write the registry refuses now stops the command instead of printing and carrying on.

Providers — config/adapters/providers/*.yaml.tmpl
One file per provider, holding its capability block. configure-adapter includes only the providers the registry holds.

Paths — the four *.tmpl, server_proxy.conf, npm-advanced/network.conf
Tiers move to /consumer/, /network/ and /provider/. Callers still use /discover and /select; the mapping between them is a file now, pasted onto the proxy host. The crawler hands catalogs to /publish, which signs them — /network/publish rejected every one.

Federation — bin/oanfed.py
PACKS=all takes everything the other network publishes. suspend switches a network off once nothing is agreed either way.

Keycloak — docker-compose.yml, realm-export.json, config/grafana/EXPLORE.md
Both go; the registry takes writes from inside the stack only.

README — 1,308 lines to 97
The appendices were deleted, not moved. What survives is: prerequisites, bringing it up, checking it works, talking to another network, and the command list. Everything else — the NPM walkthrough, TLS, the observability tour — is in git history only.

Why

The registry never overwrites a row that exists. One provider at a time means a bad .env value costs one provider, not the network.

/discover used to reach the consumer tier, which checks no signature and no agreement.

Testing

Two networks on one machine, admitting each other both ways, in both modes. Unsigned /network/discover → 401, /provider/select → 401. 39 oanfed tests.

A crawl indexing a catalog end to end is not yet verified.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

Self-review.

Bugs

The shipped defaults contradict each other. .env.example sets FEDERATION_PROJECTION_TTL=30 and FEDERATION_REFRESH_SECONDS=900. The TTL is in seconds — the adapter reads projectionTtl as an int and multiplies by time.Second. So a network that crawls this stack is told it may keep a copy for 30 seconds, and the crawl that renews it runs every 15 minutes. The catalogues are visible for half a minute out of every fifteen. The adapter logs a warning for exactly this combination, and our own default triggers it. The TTL should be comfortably longer than the refresh.

A refused registry write prints and exits 0. bin/setup.py post() returns the registry's error envelope for a rejected write, and ensure_participant, ensure_binding and ensure_capability_schema only print params.status. So make register-provider PROVIDER=agmarknet can print UNSUCCESSFUL, exit 0, and make configure-adapter then renders an adapter missing that capability. A write that was refused should stop the command.

Admit succeeds for a network the crawler will skip. projectionTtl is optional at every step: validate_descriptor accepts it absent, cmd_publish omits the flag when the variable is unset, peer_record copies it only when present, and cmd_admit never looks at it. The adapter then treats a missing or zero TTL as "no copy may be kept" and skips the network. So admit prints admitted ... covering N packs and the crawl never happens — the only sign is one error line in the adapter log. Admit should refuse a source direction with no TTL to agree about.

Suspending one direction can drop the other. cmd_suspend clears federationRole to {} and then writes back the directions to keep, because the registry merges objects on update. If the second write fails, the direction the user did not suspend is gone. It fails closed, but silently.

Comments

33 blocks run over two lines. The longest:

  • .env.example:148 — 8 lines on NETWORK_BASE_URL
  • config/reverse-proxy/npm-custom/server_proxy.conf:33 — 6 lines on what the file does not do
  • Makefile:52 — 5 lines above a one-line target
  • .env.example:4 — a 5-line header
  • bin/stack.sh:493 — 5 lines

Some restate the code. Makefile:52 says "renders from the registry's bindings, then starts the adapters" above configure-adapter: ; @$(STACK) configure-adapter. The target name already says it.

@manjudr manjudr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Self review missing. No comments from the author on 24 changed files, and this PR deletes 4,201 lines. Please self review first, particularly the README deletions.


1. The crawler publishes to a mount that rejects it

Problem

bin/setup.py:549 renders the push URL as:

("__DISCOVERY_PUSH_URL__", "http://localhost:9201/network/publish"),

The comment in config/adapters/single-adapter/network-adapter.yaml.tmpl:58-59 says "this deployment's own /publish mount", but /network/publish is not the /publish mount. It lands on the oanNetwork module, whose first step is validateSign (single-adapter/network-adapter.yaml.tmpl:194-197, same in multi-adapter:150-153).

The crawler's push sets one header:

req.Header.Set("Content-Type", "application/json")   // internal/sink/client.go:73

No Authorization header anywhere in sink.go or client.go. validateSign returns AUT_SIGNATURE_MISSING when that header is empty (core/module/handler/step.go:210-213). Every crawled catalogue is rejected with a 401.

In multi-adapter mode it is worse: the network-adapter template has only the /network/ mount. /publish is on the provider-adapter at port 9200, a different container, so localhost:9201 has no publish endpoint at all.

Recommendation

Point it at the real publish mount. Single-adapter: http://localhost:9201/publish. Multi-adapter: the provider adapter, http://provider-adapter:9200/publish. The two modes need different values, so derive it rather than hardcoding one string — and verify a crawl actually indexes a catalogue before merging.


2. The proxy mapping federation depends on is not in version control

Problem

config/reverse-proxy/npm-custom/server_proxy.conf:30-35 says the /discover to /network/ mapping is deliberately not in the file, and that an operator has to add it by hand in Nginx Proxy Manager's Advanced tab. The README no longer documents it — the 93 lines left in README.md have no proxy or TLS section. PR #84 removed the /discover append from discoverURL, so services.discovery now has to be the complete URL, which makes this mapping load-bearing. This PR's own testing note records /discover returning 404.

Recommendation

Put the mapping in version control. Either add a per-host config file that the setup script renders with the right upstream, or have setup.py create the proxy host through the NPM API with the advanced config included. If it genuinely has to be manual, it needs a documented step with the exact snippet, not a comment explaining its own absence.


3. A refused registry write is printed and ignored

Problem

bin/setup.py:386-394, ensure_participant:

result = post("Participant", payload)
status = result["params"]["status"]
print(f"  {participant_id}: {status} {result['params'].get('errmsg','')[:160]}")

No check on status, no non-zero exit. ensure_binding (:316-344) does the same. Sunbird RC never overwrites an existing row, so a re-run against a registry that already has the participant prints a failure and carries on. make setup reports success with the network half-registered, and the first symptom is a crawl that returns nothing.

Recommendation

Raise on a non-success status in both functions and let make setup fail. If a re-run is meant to be idempotent, detect the already-exists case explicitly and update the record instead of printing the error.


4. .env.example points at an appendix that no longer exists

Problem

.env.example:13 says "Appendix O walks it". The README was cut from roughly 1,308 lines to 93 in this PR and has no appendices at all.

Recommendation

Replace the reference with the actual steps, or with a link to wherever the long-form content moved. Grep the tree for other Appendix references while you are in there.


5. The second-network instructions leave out the one variable that must change

Problem

.env.example:153 sets NETWORK_BASE_URL=https://oan-dev.local. Bringing up a second network to test federation against means changing it, because it is the network's identity in the descriptor, and nothing in the quick-start text says so.

Recommendation

Add NETWORK_BASE_URL to the second-network section of the README with a note that it must differ from the first network's, since the descriptor and the participant id are both derived from it.


6. Catalogues expire thirty times faster than they are refreshed

Problem

.env.example:156 sets FEDERATION_PROJECTION_TTL=30 and :163 sets FEDERATION_REFRESH_SECONDS=900. A crawled catalogue is stamped with an end date 30 seconds out and is not re-fetched for 15 minutes. The defaults make a crawled catalogue invisible for 870 seconds out of every 900.

Recommendation

Set FEDERATION_PROJECTION_TTL to comfortably more than the refresh interval — 1800 against a 900 second refresh. The crawler already has warnIfRefreshOutpacesTtl (networkcrawl.go:227); the shipped defaults should not be the case it warns about.


7. The PR does two things

Problem

This change splits the setup make target into five, adds four federation targets, rewrites the proxy config, and deletes 4,201 lines of README. The "Quick Start section only" framing covers the first part; the README deletion is a separate decision that no reviewer can check inside this diff.

Recommendation

Split the README rewrite out. If the content moved somewhere, say where in the PR description; if it was dropped, that needs its own review.

@manjudr manjudr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

8. Keycloak is removed from the stack and one doc still names it

Problem

This PR deletes config/registry/imports/realm-export.json (2,321 lines) and every Keycloak reference in Makefile, README.md, bin/setup.py, bin/stack.sh and docker-compose.yml. The registry now runs with authentication_enabled=false (docker-compose.yml:245), which is a coherent decision and matches the loopback-only argument in the comment above it.

config/grafana/EXPLORE.md was not updated. It still lists sunbird-registry-keycloak as a live service at :55, :95 and :120, including a copy-pasteable query filtered on a container that no longer exists.

Recommendation

Update EXPLORE.md in this PR. Separately, dropping the identity provider is a much larger decision than "crawler config and a switch to turn it on or off" — put it in the PR description, or move it to its own PR, so it gets reviewed as the change it is.

@manjudr manjudr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A pass over naming, patterns and comments, line by line. The behaviour findings are in the earlier reviews.


9. suspend clears every direction before putting the kept ones back

Problem

bin/oanfed.py:749-751:

registry.update({**entry, "federationRole": {}})
if kept:
    registry.update({**registry.participant(args.network_id), "federationRole": kept})

Suspending one direction of a peer admitted in both drops both, then restores one. Anything that fails between the two writes — the registry restarting, a network blip, Ctrl-C — leaves the peer with no admission at all, and the operator sees a traceback rather than a state. cmd_admit at :716 does its own write in one call, so the two commands behave differently.

Recommendation

Do it in one write. If the registry really merges objects and cannot drop a key, send the direction as an explicit tombstone instead of clearing the parent. If the two-step is unavoidable, wrap it in try/finally that restores roles on any failure and prints what state the record is in.


10. Function docstrings carry the architecture

Problem

The docstrings in bin/setup.py are multi-paragraph design notes, not descriptions of what the function does:

  • :284-297 node — seven lines on what baseUrl means to a peer
  • :316-330 ensure_binding — four paragraphs on why actions is a list and why method is a parameter
  • :363-374 ensure_capability_schema — three paragraphs on registry behaviour, in capitals
  • :346-360 schema_url — four paragraphs, and the last one is a sentence fragment: "openagrinet.github.io, which catalogs carry in their @context; a crawl derived from a raw.githubusercontent URL matches nothing." There is no verb and no subject. An earlier sentence was edited away and nobody noticed, because nothing reads these.

That fragment is the evidence for the whole finding: prose this long inside a function rots without anyone seeing it.

Recommendation

Move the reasoning into ADRs — one for the registry's write-once behaviour and what that means for a wrong schemaUrl, one for the binding shape, one for baseUrl being the published address. Leave one line per function saying what it does. Fix or delete the fragment at :356-357 either way.


11. The shell script says outright that it is a design document

Problem

bin/stack.sh:10-11 — "The Makefile is the front door; this file is where the reasoning lives." Makefile:1-4 says the same from the other side. stack.sh then spends :13-23 and :27-41 arguing for the startup order and the profile list.

The reasoning is good and the ordering constraint at :15-21 is genuinely non-obvious. It is in the one file that nobody opens unless the stack is already broken.

Recommendation

Put the startup-order constraint and the profiles-on-teardown rule in an ADR. Leave stack.sh with the usage block and a one-line pointer to it.


12. The argparse namespace is passed into functions that are not about the CLI

Problem

oanfed.py:667 our_schema_packs(args) uses one field, args.out. oanfed.py:534 fetcher_from(args) reads four. cmd_admit threads args through everything below it. These functions are about federation, not about argument parsing, but none of them can be called or tested without building an argparse namespace first.

Recommendation

Pass the values: our_schema_packs(out_dir), Fetcher(scheme, ca_file, resolve, timeout). Keep args inside the cmd_* functions, which is where it belongs.


13. Names that hide what the value is

Problem

  • setup.py:416 — p = PROVIDERS[name], then p['prefix'] five lines later. One letter for the provider's whole configuration.
  • oanfed.py:623 — peer_record returns (kid, record); oanfed.py:631 binds it as kid, facts. Same value, two names, two lines apart.
  • oanfed.py:626 file_peer does three things: creates or updates the record, then re-reads it, then asserts the registry stored the key under the right osid. The name describes only the first.

Recommendation

p → provider. Pick record or facts and use it in both places. Split the verification in file_peer:653-663 into assert_peer_stored(registry, participant_id, kid, key) so the write and the check read as the two things they are.

@manjudr
manjudr added this pull request to stack #99 October 28, 2026 06:44
@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 1. The crawler publishes to a mount that rejects it —

Confirmed on every link and fixed in 5b169b4. publish_url(folder) renders http://localhost:9201/publish for single and http://provider-adapter:9200/publish for multi, rather than one hardcoded string that is wrong in both. Both templates claimed "this deployment's own /publish mount" while naming another, and now name the mount and say why it is not /network/publish.

Worth recording why it stayed quiet: the adapter warns when discoveryPushUrl does not end in /publish, and /network/publish does.

Not yet proven on a running stack — a crawl indexing a catalog end to end is still to do.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 2. The proxy mapping federation depends on is not in version control —

Fixed by following the pattern already here rather than adding one: config/reverse-proxy/npm-advanced/ holds paste-job snippets with consumer.conf and grafana.conf, each headed with where it goes and why it cannot auto-load. The mapping is now npm-advanced/network.conf, server_proxy.conf points at it instead of explaining its own absence, and the README's federation section says to paste it and what breaks without it.

The headers are set in the block rather than inherited: a location of ours does not pick up the ones NPM generates for the host's main location, and nothing in this repo references an includable proxy.conf to lean on.

Not automated through NPM's API. That needs the API token handled in setup.py and is worth its own change rather than riding this one.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 3. A refused registry write is printed and ignored —

Fixed in 49a38d3. All three writes — participant, binding and capability schema — go through created(), which exits naming the row and the registry's own errmsg, so make stops rather than reporting success with the registry half filled. Success prints one line instead of echoing a status word.

It follows oanfed.py's existing check rather than a second shape: params.status != "SUCCESSFUL" is a refusal.

The already-exists case was already handled a step earlier — each function searches first and returns "already present" without writing — so a refusal reaching created() is a real one.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 4. .env.example points at an appendix that no longer exists and 5. The second-network instructions leave out the one variable that must change —

Both in 5fa0031.

Grepping found four dangling references rather than one: .env.example:13 and :85, and docker-compose.yml:438 and :750. Each is gone — in two of them the reason was already written out after the pointer, so only the pointer went.

A fifth is in npm-advanced/grafana.conf. That file is not in this PR — it was reverted earlier as unrelated to crawler config — so it keeps its "Appendix H" for now.

NETWORK_BASE_URL is in both lists now, in .env.example and in the README's second-network paragraph, with why: it and BECKN_NETWORK_ID are the identity and the published address, so two stacks sharing either are one network as far as anyone else can tell.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 6. Catalogues expire thirty times faster than they are refreshed —

.env.example is the template for a local test stack, not a deployment's values, and both numbers are set per deployment.

Leaving them. The crawler logs warnIfRefreshOutpacesTtl naming the network and both values when the pairing is wrong, so a deployment that copies these unchanged is told.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 7. The PR does two things and 8. Keycloak is removed from the stack and one doc still names it —

EXPLORE.md is fixed in f910d30. Six references, not three: the table row, the service list and the query you found, plus three in prose using Keycloak as the example of a service with no traces and no parsed severity. None of them described anything this stack still runs.

The description now names what the diff does rather than the feature: the README went from 1,308 lines to 97 and the appendices were deleted, not moved — what survives is listed, and the rest is in git history. Keycloak and the paths move are named the same way.

Not splitting the branch. The README cut and the Keycloak removal were decided before this work and are what the quick-start is meant to look like; rewriting the history of a branch reviewed three times would cost more than it buys. The description is where that belongs, and it now says so.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 9. suspend clears every direction before putting the kept ones back —

Fixed as your fallback, in 81ff2cc, because one write is not available. The registry merges a nested object key by key, so sending federationRole: {source: …} leaves requester exactly where it was — omitting a key is not deleting it. A requester: null tombstone does not help either: the schema declares that key as an object with required fields, so a null is refused rather than stored.

So the two writes stay and the window now explains itself:

oan-b.local now has no admission at all: clearing it worked, putting back source did not.
  Re-admit it in the directions you meant to keep.
  registry PUT returned 503

A test fails the second write specifically — not the first — and asserts the message names the state rather than surfacing the PUT's own error. 40 tests.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 10. Function docstrings carry the architecture and 11. The shell script says outright that it is a design document —

Both in one change. Two ADRs rather than the four you listed, because the registry ones answer the same question:

  • docs/adr/0002-filling-the-registry.md — create-only rows, baseUrl as the address we publish, why actions is a list, mappings covering both directions, method being the provider's contract, and schemaUrl derived and permanent.
  • docs/adr/0003-bringing-the-stack-up.md — the adapters starting after their configs are rendered, and teardown naming every profile.

node, ensure_binding, ensure_capability_schema and schema_url are a line or two each now, 57 lines lighter. stack.sh keeps its usage block and points at 0003; "this file is where the reasoning lives" is gone, because it no longer is.

The fragment is rewritten rather than patched: the sentence was trying to say the host has to be the one catalogs carry in their @context, which is now in 0002 where it can be read.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

On 12. The argparse namespace is passed into functions that are not about the CLI and 13. Names that hide what the value is —

Both in 9fc6776.

our_schema_packs(out_dir) takes the directory now, and cmd_admit passes args.out.

fetcher_from(args) stays. It is the args-to-Fetcher adapter and says so in its name; Fetcher(scheme, ca_file, resolve, timeout) is already the value-taking constructor beside it, and the tests substitute at this seam.

For 13: p is provider throughout. peer_record returns a record and file_peer binds it as record, one name in both places. The read-back is stored_as_written(registry, participant_id, kid, key), so file_peer writes and that checks, which is the two things you named.

40 tests.

@ameersohel45

Copy link
Copy Markdown
Contributor Author

All thirteen are answered, each in its own comment above. Summary for a second pass:

Changed

  • 1 — crawled catalogs go to /publish, which signs them, derived per mode; /network/publish had rejected every one (5b169b4)
  • 2 — the /discover mapping is npm-advanced/network.conf, with a README pointer (0c31ac4)
  • 3 — a write the registry refuses stops the command, naming the row and the registry's own error (49a38d3)
  • 4, 5 — four dead appendix pointers gone, and NETWORK_BASE_URL named in both second-network lists (5fa0031)
  • 8 — six stale sunbird-registry-keycloak references out of EXPLORE.md, not the three you found (f910d30)
  • 9 — a failed restore during suspend says what state the record is in (81ff2cc)
  • 10, 11 — docs/adr/0002-filling-the-registry.md and 0003-bringing-the-stack-up.md; four docstrings and the script header cut to what the code does, 57 lines lighter (b8844ee)
  • 12, 13 — our_schema_packs(out_dir), p is provider, one name for the record, and the read-back is stored_as_written (9fc6776)

Plus suspend setting status: inactive once nothing is agreed either way (a532b6d), which came from #84's first finding and belongs here.

Answered without a change

  • 6 — .env.example is a local test template; the crawler logs warnIfRefreshOutpacesTtl naming both values when the pairing is wrong
  • 7 — not splitting a branch reviewed three times; the description now names the README cut and says the appendices were deleted rather than moved
  • 12, in part — fetcher_from(args) stays, as the args-to-Fetcher adapter its name describes

Still open

A crawl indexing a catalog end to end is not verified. Finding 1 means it cannot have worked before this branch, so it is the thing to run before merging.

Base automatically changed from feat/75-admit-peer-network to NetworkofNetwork October 28, 2026 12:13
@manjudr

manjudr commented Oct 28, 2026

Copy link
Copy Markdown
Member

@ameersohel45 #86, #82 and #90 are now squash-merged into NetworkofNetwork, so this branch needs a rebase before it can be reviewed or merged.

Problem

The branch carries 41 commits, and the bottom 13 are the #86/#82/#90 work, which is now in the base as three squash commits. Git cannot match them up, so the merge conflicts in ten files, including oanfed.py and test_oanfed.py as add/add.

More than a mechanical conflict: some of this branch's own commits patch code that no longer exists.

Resolving those by hand would mean guessing which of your commits are superseded, so it is yours to do rather than mine.

Recommendation

Rebase onto the merged base and drop the commits that are now in it:

git fetch origin
git rebase --onto origin/NetworkofNetwork b365120 feat/72-crawler-config-quick-start

b365120 is the last commit here that belongs to #90. That replays your own 29 commits onto the merged base. Drop 81ff2cc and 3aaf90c with git rebase --skip when they come up, and check the result against the cmd_suspend and peer_record that #90 merged. Then force-push with --force-with-lease.

Separately: this PR has no review on it yet and its description covers a lot of ground — commands, providers, paths, the crawler switch. Once it is rebased I will review it properly. Splitting the make setup change out from the crawler switch would make that easier, but I will take it either way.

Federation needs the registry to record peer networks: our own entry
builds the Network Descriptor (#74) and admitting a peer writes one (#75).

- role accepts a list as well as a single value, so one participant can
  serve several layers; a single string stays valid for existing rows
- federationRole (self/source/requester/both): our admission decision,
  never read from a peer's Descriptor; added to indexFields
- revision: the Descriptor revision accepted; only increases
- schemaPacks: the peer's declared schema pack URLs, https only
- projectionTtl: seconds we may retain a peer's data; 0 means live query

All fields are optional and no existing rule is tightened, so stored
records keep their osid and stay editable.
…ields [#71]

On a PUT, Sunbird RC merges osid and the os* audit fields into each
stored key before re-validating. PublicKey did not declare them, so
every update of a node failed, including the revision bump that
refreshes an admitted peer.

Declare those five fields and keep additionalProperties false, so typos
and unknown key fields are still refused. osOwner is left out: the
registry never sets it on a key.
#71]

A network recognises its own entry from its configuration, so a stored
self value is redundant. federationRole now only records our decision
about a peer: source, requester or both.
federationRole is now an object with optional source and requester
halves instead of a source/requester/both string; both halves present
replaces "both". Each half carries its own schemaPacks (moved from the
top level), descriptorRevision, admittedAt and updatedAt, since the
design doc stores admission independently for each direction.
federationRole drops out of indexFields, as it is no longer a scalar.
An agreement that names no packs covers nothing, yet the registry
accepted one, so the mistake only showed up later as refused calls.
Both directions now need a schemaPacks list with at least one entry.
make setup did everything in one pass: keys, five participants, four bindings
and every adapter config. A wrong value in .env was therefore a wrong row in a
registry that creates only, and the blast radius was the whole network.

It splits into make keys, make register-network, make register-provider
PROVIDER=, and make configure-adapter. make up now stops at the containers --
it writes nothing and starts no adapter, because an adapter config is a
bind-mounted file and Docker answers a missing one by creating a directory.
configure-adapter is what starts them, so a rendered config is never left
unpicked-up.

configure-adapter renders from the registry's own ProviderSchema rows rather
than from .env, so the binding key a provider step is configured with and the
one it is called with are the same string. A provider with no row contributes
no block. That took the four capability blocks out of the two adapter templates
and into config/adapters/providers/<name>.yaml.tmpl, one per provider, holding
the part the registry deliberately does not: how to authenticate to it.

It also asks for the deployment mode and whether to crawl other networks, and
writes both answers back to .env -- settings that were only ever an environment
variable prefixed to the command. Neither is asked when it was given as an
environment variable, or when there is no terminal.

Three template comments said signing was off, which stopped being true when
quick-start started signing. MODE= never worked despite the help text saying so.
configure-adapter had no wait for the registry, so a stack still starting gave
a traceback rather than the message that names the cause.
SchemaRegistry was the only entity whose root set additionalProperties false.
The registry merges its own osCreatedAt, osUpdatedAt, osCreatedBy and
osUpdatedBy into a record before validating it, so every PUT was rejected over
fields the registry itself had added -- a 500 no caller could avoid. Sending a
body with none of them in it failed the same way.

Measured against a live registry: Participant and ProviderSchema both update
cleanly and neither sets it. A capability's name, version and pack URL were
permanent for no stated reason.
admit filed another network's key under the kid its descriptor signs with, and
then refused its own write: "the registry did not store net-b.oan.local's key
as 1-a74e3ca6-...". Sunbird prepends "1-" to any osid supplied on create,
including a nested one, so passing the kid whole stored 1-1-a74e3ca6-... and no
signature naming the kid would have resolved to a key.

The entry's osid already had this strip; the key's did not, because the kid
moved from the entry to the key without it. Found on two networks admitting
each other, not in the tests -- the registry double kept a supplied nested osid
verbatim, which is not what the registry does. It now prepends, as measured.

Both directions now store a key osid equal to the other network's own kid.
The descriptor advertises NETWORK_BASE_URL + /discover. In single mode /discover
matched the `/` mount -- the consumer tier, whose steps are validateSchema,
addRoute, sign. No validateSign, no checkCrossNetworkRequester. The tier that
checks both sat at /_internal/network/, a path the descriptor never named and
the edge explicitly denied.

So another network's call arrived at the one door that checks nothing, while
the door that checks was sealed off. Measured on two networks admitting each
other: an unsigned POST to /discover was 500 with no authentication attempted,
the same payload to /_internal/network/discover was 401 AUT_SIGNATURE_MISSING.

The three tiers are now /consumer/, /network/ and /provider/ in BOTH deployment
modes, as networkofnetwork already had them, and NETWORK_BASE_URL ends in
/network either way. The edge denies /consumer and /crawl in place of
/_internal; /network and /provider reach the adapter normally and both check
the signature and the agreement.

The crawler's push target is now /network/publish, one address in both modes
rather than an action on / in multi and a mount in single.

Verified on a clean stack, single and multi: unsigned to /network/discover and
/provider/select is 401, /discover is 404, and all three containers start with
no restarts.
ADR-0002 records why the tiers moved to /consumer/, /network/ and /provider/,
and how the paths callers see stay what ADR-0001 promised: the edge rewrites
/discover onto /network/ and the transaction actions onto /provider/, so the
mount names are ours and nobody else carries them. NETWORK_BASE_URL goes back
to no path.

The chart banned snippet, rewrite and regex annotations outright, because one
could reach /_internal or /publish. That ban also forbids the mapping this
needs. It is replaced by a check on the rewrite TARGET, refusing only /publish
and /consumer -- the two mounts that take unsigned calls. Anything else is the
deployment's business, so a mount added later needs no chart release. No
ingress controller is named: Kong, ingress-nginx and ALB spell a rewrite
differently and the annotations come from values.

quick-start does the same job in nginx, but on the individual proxy host rather
than server_proxy.conf: that file is included in every server block and the
upstream differs per host -- in multi mode /select belongs to provider-adapter
and /discover to network-adapter, which no shared rule can express. The shared
file keeps what is true everywhere, denying every mount by name. Appendix B
carries the per-host rule.

Verified: the descriptor advertises /discover again, the adapter answers 401 on
/network/discover and /provider/select, 404 on /discover, and the chart accepts
a rewrite onto /network/ while refusing one onto /publish.
Nothing in the charts served .well-known, so a cluster could neither tell
another network who it was nor read anyone else's answer. Federation existed
only in quick-start.

A Job signs the descriptor, JWKS and schema packs from this network's registry
entry and writes them into a ConfigMap; a small server mounts that ConfigMap
and answers for exactly those three paths. Kong routes to it.

The Job mounts the SAME Secret the adapter reads. oanfed refuses to sign with a
key the registry does not publish for this network, so sharing it is what
guarantees the descriptor names the key that signs traffic -- a mismatch there
surfaces much later, as another network failing to verify anything we send. The
private key exists only inside the Job; what reaches the ConfigMap is public.

Each run publishes the next revision, read from what the ConfigMap already
holds. oanfed refuses a revision that does not increase on the one served, and
so does every network holding the old one, so the number cannot come from a
value in git that a re-run would repeat.

Admissions are values, one Job each, so who we agreed to talk to is reviewable
rather than a command somebody ran once. They need no key: admission reads the
other network's public documents and writes a row in our registry.

oanfed's open_registry imported quick-start's setup.py and read .env, which a
Job has neither of. It now takes REGISTRY_URL and BECKN_NETWORK_ID from the
environment when both are set and falls back to setup.py otherwise, so the same
file runs in both places.

The server holds nothing secret and serves nothing else: 404 for any other
path, and the Ingress template refuses one outside /.well-known/.
]

The Advanced-tab rule covered discover, select, init, confirm and status. The
Beckn v2 spec has nine request paths a network sends us -- track, cancel,
update, rate and support as well -- and publish, which goes to the network tier
like discover does.

The on_* callbacks are deliberately absent. A callback returns to whoever SENT
the request, so it belongs to the consumer tier, which takes unsigned calls and
is never public.

Only select answers today: ensure_binding writes one action per ProviderSchema
row and that action is select. The other eight are routed anyway, because the
path belongs to this network either way -- a 404 from the mount that would
serve it says "not bound yet", where no route at all says "wrong address".
make host-descriptor writes .well-known/ per deployment -- a descriptor, a
public key and a pack list that belong to one network, not to the repo.
1390 lines, of which 1008 were fifteen appendices. Nobody getting started reads
that, and the detail they held sits next to the thing it describes anyway:
POCRA's rate limit and the shape budget in .env.example, the startup order in
stack.sh, what the registry holds in setup.py.

93 lines now: what you need, the commands in order, how to check it worked,
talking to another network, and the five things that actually break. The old
text is in git history.

Three files pointed at appendix letters that no longer exist.
The 25 commits above were authored on a merge of this stack at older SHAs.
Replaying them onto its current tip resolved ten files differently; this is
those ten, taken from the branch that was tested.
The observability docs name every container whose logs land in ClickHouse.
Replacing Keycloak with the registry left five sentences starting lowercase.
Its Appendix H pointer is about telemetry, not about this work.
They name Keycloak as a log source. That it is gone is a telemetry change, not
this one.
Suspending the last direction left the participant active with its keys, so it passed signature validation and the admission step exactly as before. An inactive participant is refused before any federation rule is read.
The crawler pushed to /network/publish, whose first step is validateSign, carrying no Authorization header at all. Every crawled catalog was refused. /publish signs and forwards it on, and lives on the provider adapter in multi mode.
The rule that makes another network's /discover reach the adapter lived only in a textarea in NPM's database. It follows the npm-advanced paste-job pattern now, and the README says to paste it.
All three writes printed the status and carried on, so make reported success with the registry half filled and the first symptom was a crawl collecting nothing.
…d network's address [#72]

Four references survived the README cut and point at nothing. The second-network list also left out NETWORK_BASE_URL, which is the address the descriptor publishes.
EXPLORE.md listed sunbird-registry-keycloak as a live service, including a query filtered on it, after the registry stopped needing it.
A direction is dropped by clearing all of them and writing back what stays, because the registry merges. A failure in that window left the network with no admission and surfaced as the write's own error.
Four docstrings and the script header argued for decisions instead of saying what the code does. One had lost its subject and verb to an edit nobody noticed, which is the case for moving them.
our_schema_packs took a whole argparse namespace to read one field. The record peer_record returns had two names two lines apart, and file_peer's read-back is its own step.
… changed [#72]

The rebase pulled in non-conflicting hunks from upstream -- discoveryUrl on Participant, a mkdir in stack.sh, the registry chart. This branch's behaviour is the reviewed one, so its versions stand.
The record, the cases and ADR-0002 described a schema this branch does not carry. They say what the code does now, including that suspend writes a federationRole the schema refuses.
@ameersohel45
ameersohel45 force-pushed the feat/72-crawler-config-quick-start branch from 9fc6776 to b67b17e Compare October 28, 2026 12:53
@manjudr
manjudr merged commit 6602f52 into NetworkofNetwork Oct 28, 2026
2 checks passed
@manjudr
manjudr deleted the feat/72-crawler-config-quick-start branch October 28, 2026 13:17
@manjudr

manjudr commented Oct 28, 2026

Copy link
Copy Markdown
Member

Reviewed at b67b17e, against NetworkofNetwork with #86, #82 and #90 merged into it.

The branch is mergeable again and CI is green, but the way it got there undoes all three of those PRs. ec9f5e0 "keep this branch's own copies of the files NetworkofNetwork also changed" resolved every conflict in favour of this branch, which holds the versions from before four rounds of review. I have not reviewed this PR's own crawler and setup work yet — that comes after this is sorted out, because right now I cannot tell the two apart in the diff.


1. The merge resolution reverts every fix from #86, #82 and #90

Problem

ec9f5e0 took this branch's copy of oanfed.py, test_oanfed.py, Participant.json and docs/adr/0002-federation-admission-record.md. Those are the four files the merged PRs own. Each finding they fixed is back:

Was fixed in Now at
#82/1 revision kept on the registry entry, so a lost .well-known/ keeps counting oanfed.py:451 reads the file only
#82/2 JWKS publishes every active key oanfed.py:500 keyset = {"keys": [network_jwk]} — one key, so a rotation breaks peers
#82/3 --scheme only on verify oanfed.py:783 is in add_peer_access, applied to admit at :838 and refresh at :843
#90/1 suspend never writes federationRole: {} oanfed.py:757 writes exactly that
#90/2 suspend is one write oanfed.py:757 then :760 — two, with a window where nothing is admitted
#90/3 a peer files its own kid oanfed.py:611 RECORD_ID.fullmatch(kid) — the peer must run Sunbird RC
#90/4 peer's discovery URL goes in discoveryUrl oanfed.py:616 "baseUrl": descriptor["services"]["discovery"]
#90/5 no validFrom on a peer key oanfed.py:618 "validFrom": descriptor["issuedAt"]
#90/6 refresh compares descriptorRevision no comparison anywhere
#90/7 admit checks the operating profile no check anywhere

Participant.json goes with it: discoveryUrl and keyId are gone entirely, projectionTtl moves back to the top level, and role returns to oneOf: [string, array].

Recommendation

Redo the merge taking the base's copy of those four files, plus scripts/registry-schema-cases.json:

git fetch origin
git checkout feat/72-crawler-config-quick-start
git merge origin/NetworkofNetwork
git checkout origin/NetworkofNetwork -- \
  quick-start/bin/oanfed.py \
  quick-start/bin/test_oanfed.py \
  quick-start/config/registry/schemas/Participant.json \
  docs/adr/0002-federation-admission-record.md \
  scripts/registry-schema-cases.json

Then put back only the changes this PR genuinely needs on top of them, and drop e27596a "say what a half-finished suspend left behind" — it describes a two-write suspend that no longer exists.


2. The schema test cases were rewritten to assert the opposite, so nothing catches it

Problem

scripts/registry-schema-cases.json is the file that is supposed to fail when the schema changes meaning. b67b17e flipped it to match, so all 18 cases pass against the reverted schema. Reading the two side by side:

On NetworkofNetwork On this branch
a peer is reached at its discoveryUrl, with no baseUrl a peer is reached at its baseUrl
federationRole may be empty: suspend leaves the key behind federationRole may not be empty: a direction is removed, not emptied
projectionTtl belongs to source projectionTtl is the participant's
a suspended admission must say when it was suspended (gone) — replaced by "a status inside a direction is refused"
role as a bare string is refused: 0.4.1 rows need the migration role as a bare string is accepted: an older row still reads
an osid the registry did not write is refused when it is not a string the registry's own osid passes back whatever type it carries

The 40 unit tests pass for the same reason: test_oanfed.py came from this branch too, and OurRegistry validates each write against this branch's Participant.json.

So green CI here means the fixtures agree with the schema, not that the schema is the one that was agreed. scripts/check-registry-schema.py itself is untouched and working — it is the cases that moved.

Recommendation

Restore both files from the base, as in finding 1. If this PR needs a new case, add it rather than editing an existing one, so the change is visible as an addition in the diff.


3. ADR-0002 was rewritten back to the design its alternatives section rejects

Problem

docs/adr/0002-federation-admission-record.md on this branch is the pre-review version, 136 lines different from the merged one. Four of its decisions are things the merged ADR lists under Alternatives considered and explains why it did not take:

  • Decision 4, "a direction is withdrawn by removing it". The merged ADR rejects this because it loses admittedAt and descriptorRevision, which the attribution requirement needs, and because it needs federationRole: {} as an intermediate write.
  • Decision 5, projectionTtl at the top level. Rejected because it then exists on upstreams and requester-only peers, where it means nothing.
  • Decision 5, the discovery URL in baseUrl. Rejected because baseUrl means the base an action is appended to, so a caller appending /discover gets /discover/discover.
  • Decision 6, a peer's key named by its Sunbird osid. Rejected because it refuses any peer not running Sunbird RC. oanfed.py:611 enforces exactly that here.
  • Decision 7, role as string or array. Rejected because every reader then handles two shapes forever.

It also drops the Consequences section recording what the registry does and does not enforce on an update, which I checked against a live registry yesterday and corrected.

An ADR is the record of a decision. Changing one so it matches the branch in front of you reverses that, and the next person reads a settled question as open.

Recommendation

Restore 0002 from the base. 0005-filling-the-registry.md and 0006-bringing-the-stack-up.md are this PR's own and are fine where they are — keep them.


Still owed

This PR has had no first-pass review. Once the merge is redone I will go through the crawler switch, the split of make setup, the per-provider files and the path changes properly. Two things that will come up then, flagged now so they are not a surprise:

  • The diff deletes config/registry/imports/realm-export.json, 2321 lines, and most of quick-start/README.md. Neither is obviously part of "crawler config and a switch to turn it on or off". If dropping Keycloak is a separate decision, it wants its own PR or at least its own ADR.
  • The description covers commands, providers, paths and the crawler switch. Splitting the make setup change out from the crawler switch would make both reviewable.

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.

3 participants