Add support for discovering peering pods #582

Merged
prologic merged 41 commits from pod_peers into master 2 months ago
Owner
There is no content yet.
Poster
Owner
There is no content yet.
prologic force-pushed pod_peers from ba43d69dc8 to b15dd962dc 2 months ago
prologic force-pushed pod_peers from b15dd962dc to a2ca67e8c7 2 months ago
prologic added 1 commit 2 months ago
20458652bb
Update endpoint used in tools/check-pod-versions.sh
prologic added 1 commit 2 months ago
prologic added 1 commit 2 months ago
lyse reviewed 2 months ago
SoftwareVersion string `json:"software_version"`
// Maybe we store future data about other peer pods in the future?
// Right now the above is basically what is exposed now as the Pod's name, Descirption and what version of yarnd is running.
lyse commented 2 months ago
Poster
Collaborator

Typo in "desc_ri_ption".

Typo in "desc_ri_ption".
prologic marked this conversation as resolved
// Maybe we store future data about other peer pods in the future?
// Right now the above is basically what is exposed now as the Pod's name, Descirption and what version of yarnd is running.
// This information will likely be used for Pod Owner/Operators to manage Image Domain whitelisting between pods and internal
// automated operations like Pod gossiping of Twts for things like Missing Root Twts for converstaion views, etc.
lyse commented 2 months ago
Poster
Collaborator

Typo in "convers_at_ion".

Typo in "convers_at_ion".
prologic marked this conversation as resolved
if !isPod {
log.Debugf("%s is not a pod!", xPoweredBy)
return nil
lyse commented 2 months ago
Poster
Collaborator

Don't really get the pattern when something is considered an error (returning err) or not (returning nil) in this function.

Don't really get the pattern when something is considered an error (returning `err`) or not (returning `nil`) in this function.
Poster
Owner

This is not considered an error in this case.

This is not considered an error in this case.
prologic marked this conversation as resolved
return nil
}
baseURL := req.Header.Get("X-yarnd-callback")
lyse commented 2 months ago
Poster
Collaborator

RFC 6648 deprecates the use of the "X-" prefix, just name the header without that prefix. Maybe the same above, but X-Powered-By could be seen as a de-facto standard, PHP comes to mind.

[RFC 6648](https://datatracker.ietf.org/doc/html/rfc6648) deprecates the use of the "X-" prefix, just name the header without that prefix. Maybe the same above, but `X-Powered-By` could be seen as a de-facto standard, PHP comes to mind.
prologic marked this conversation as resolved
data, err := io.ReadAll(res.Body)
if err != nil {
log.WithError(err).Errorf("error reading request body for /info of pod running at %s", baseURL)
lyse commented 2 months ago
Poster
Collaborator

s/request/response/

s/request/response/
prologic marked this conversation as resolved
var podInfo PodInfo
if err := json.Unmarshal(data, &podInfo); err != nil {
log.WithError(err).Errorf("error decoding request body for /info of pod running at %s", baseURL)
lyse commented 2 months ago
Poster
Collaborator

s/request/response/

s/request/response/
prologic marked this conversation as resolved
// Always set an X-Powered-By header (used to detect peering pods)
// Set a X-yarnd-callback so pods know what Base URL to callback on!
headers.Set("X-Powered-By", fmt.Sprintf("running yarnd %s (a Yarn.social pod)", yarn.FullVersion()))
headers.Set("X-yarnd-callback", conf.BaseURL)
lyse commented 2 months ago
Poster
Collaborator

I'm wondering whether the base URL could be defered by the User-Agent header. Maybe I'm missing something, but we need to consider three different User-Agent formats (two of which are defined in our lovely Multi-User User-Agent Extension Specification):

  1. Single-User User-Agent such as twtxt/1.2.3 (+https://example.com/twtxt.txt; @somebody).
  2. Multi-User User-Agent such as twtxt/0.1.0@abcdefg (~https://example.com/whoFollows?token=randomtoken123; contact=https://example.com/support).
  3. Some other User-Agent not adhering to our specification.

Now the last case is trivial, we're done.

But in case we receive either Single- or Multi-User User-Agents, we check whether the client name matches yarnd. If not, we're out.

Then depending on the exact User-Agent format, we should be able to extract the base URL from any of the included URLs. Either strip the /user/[a-z0-9_-]+/twtxt.txt from the end of the URL or – probably easiest – strip the /support form the end of the contact URL.

The only drawback I can see of at the moment is, that this would not work with renamed yarnd forks or other clients, that implement your API. Although, unlikely, that this happens any time soon in my opinion.

I'm wondering whether the base URL could be defered by the `User-Agent` header. Maybe I'm missing something, but we need to consider three different `User-Agent` formats (two of which are defined in [our lovely Multi-User User-Agent Extension Specification](https://dev.twtxt.net/doc/useragentextension.html)): 1. Single-User `User-Agent` such as `twtxt/1.2.3 (+https://example.com/twtxt.txt; @somebody)`. 2. Multi-User `User-Agent` such as `twtxt/0.1.0@abcdefg (~https://example.com/whoFollows?token=randomtoken123; contact=https://example.com/support)`. 3. Some other `User-Agent` not adhering to our specification. Now the last case is trivial, we're done. But in case we receive either Single- or Multi-User `User-Agent`s, we check whether the client name matches `yarnd`. If not, we're out. Then depending on the exact `User-Agent` format, we should be able to extract the base URL from any of the included URLs. Either strip the `/user/[a-z0-9_-]+/twtxt.txt` from the end of the URL or – probably easiest – strip the `/support` form the end of the contact URL. The only drawback I can see of at the moment is, that this would not work with renamed yarnd forks or other clients, that implement your API. Although, unlikely, that this happens any time soon in my opinion.
prologic marked this conversation as resolved
if [ $? -ne 0 ]; then
printf "???"
else
printf "%s" "$version"
lyse commented 2 months ago
Poster
Collaborator

Could be simplified to echo -n "$version". Or the -n to omit the newline at the end could even be omitted.

Could be simplified to `echo -n "$version"`. Or the `-n` to omit the newline at the end could even be omitted.
prologic marked this conversation as resolved
jq -er '.FullVersion'; then
printf "???\n"
fi
printf "%s %s\n" "$pod" "$(get_pod_version "$pod")"
lyse commented 2 months ago
Poster
Collaborator

And here just echo "$pod $(get_pod_version "$pod")".

And here just `echo "$pod $(get_pod_version "$pod")"`.
prologic marked this conversation as resolved
lyse reviewed 2 months ago
<tr>
<th>Name</th>
<th>Description</th>
<th>SoftwareVersion</th>
lyse commented 2 months ago
Poster
Collaborator

Although we're probably all techies, I believe we should use proper English spelling for everything displayed to the user and thus not use CamelCase. Thus, put a space between Software and Version.

Although we're probably all techies, I believe we should use proper English spelling for everything displayed to the user and thus not use CamelCase. Thus, put a space between `Software` and `Version`.
Poster
Owner

Ahh yes! Of course agreed 😂

Ahh yes! Of course agreed 😂
prologic marked this conversation as resolved
prologic added 1 commit 2 months ago
316b4ce5c3
Skip callback if we've seen this pod before
prologic added 1 commit 2 months ago
2f95edec23
Avoid multiple callbacks for Multi-User pods
prologic added 1 commit 2 months ago
6ede750b7f
Delete partial/empty PodInfo on errors
prologic added 1 commit 2 months ago
af498eb088
Recheck peering pods every hour
prologic added 1 commit 2 months ago
a0ee8446dd
s/ls/lastSeen
prologic added 1 commit 2 months ago
75782063f8
Set lastSeen (d'oh)
prologic added 1 commit 2 months ago
a641e86382
Fix typo
prologic added 1 commit 2 months ago
3b15a255a9
Fix typo
prologic added 1 commit 2 months ago
lyse reviewed 2 months ago
// Set an empty &PodInfo{} to avoid multiple concurrent calls from making
// multiple callbacks to peering pods unncessarilly for Multi-User pods.
cache.mu.Lock()
lyse commented 2 months ago
Poster
Collaborator

We should guard against a possible race from another goroutine doing the same thing:

// guard against race from other goroutine doing the same thing
cache.mu.Lock()
oldPodInfo, hasSeen = cache.Pods[baseUrl]
if hasSeen && !oldPodInfo.ShouldRefresh() {
	cache.mu.Unlock()
	log.Debugf("already seen pod %s", baseURL)
	return nil
}

cache.Pods[baseURL] = &PodInfo{}
cache.mu.Unlock()
We should guard against a possible race from another goroutine doing the same thing: ```go // guard against race from other goroutine doing the same thing cache.mu.Lock() oldPodInfo, hasSeen = cache.Pods[baseUrl] if hasSeen && !oldPodInfo.ShouldRefresh() { cache.mu.Unlock() log.Debugf("already seen pod %s", baseURL) return nil } cache.Pods[baseURL] = &PodInfo{} cache.mu.Unlock() ```
Poster
Owner

Ahh nice! Applied!

Ahh nice! Applied!
prologic marked this conversation as resolved
prologic added 1 commit 2 months ago
0b4084e3bc
Simplified tools/check-pod-versions.sh script
prologic added 1 commit 2 months ago
90ce66f6c9
Fix table header for displaying peers
prologic added 1 commit 2 months ago
6dcbbf3bd4
Apply @lyse's double-guard suggestion
prologic added 1 commit 2 months ago
prologic requested review from ullarah 2 months ago
prologic requested review from movq 2 months ago
prologic requested review from lyse 2 months ago
prologic self-assigned this 2 months ago
prologic added 2 commits 2 months ago
prologic added 3 commits 2 months ago
prologic added 1 commit 2 months ago
8c9c71fa01
Fix bad log.Error() call
prologic added 1 commit 2 months ago
57e069d02c
Add display of LastSeen to Manage Peers view
prologic added 1 commit 2 months ago
d36e67d859
Fix bug in PodInfo().IsZero()
prologic added 1 commit 2 months ago
8fd9bda144
Skip dummpy or zero valur PodInfo in GetPeers()
prologic added 1 commit 2 months ago
lyse reviewed 2 months ago
if hasSeen && !oldPodInfo.ShouldRefresh() {
log.Debugf("already seen pod %s", podURI)
oldPodInfo.lastSeen = time.Now()
lyse commented 2 months ago
Poster
Collaborator

If we update lastSeen every time, then we cannot use it to decide whether to callback again or not. Chances are pretty high, that we never attempt to update the pod info again (unless it experienced a longer outage).

If we update `lastSeen` every time, then we cannot use it to decide whether to callback again or not. Chances are pretty high, that we never attempt to update the pod info again (unless it experienced a longer outage).
Poster
Owner
  • Add lastSeen timestamp
  • Re-fetch PodInfo once per day
- Add `lastSeen` timestamp - Re-fetch PodInfo once per day
Poster
Owner

Done

Done
prologic marked this conversation as resolved
cache.mu.Unlock()
}
log.WithError(err).Errorf("error decoding request body for /info of pod running at %s", podURI)
lyse commented 2 months ago
Poster
Collaborator

s/request/response/

s/request/response/
prologic marked this conversation as resolved
// IsPod returns true if the Twtxt client's User-Agent appears to be a Yarn.social pod (single or multi-user)
IsPod() bool
// PodURI returns the Base URL of the client's User-Agent iif it appears to be a Yarn.social pod (single or multi-user)
lyse commented 2 months ago
Poster
Collaborator

Typo: _if or if_f_

Typo: \_if or if_f_
lyse commented 2 months ago
Poster
Collaborator

Typo: _if or if_f_

Typo: \_if or if_f_
prologic marked this conversation as resolved
// PodURI returns the Base URL of the client's User-Agent iif it appears to be a Yarn.social pod (single or multi-user)
PodURI() string
// IsPublicURL returns true if the Twttx client's User-Agent is from what appears to be the public internet
lyse commented 2 months ago
Poster
Collaborator

Typo: Twt_xt_

Typo: Twt_xt_
prologic marked this conversation as resolved
log.WithError(err).Warnf("error parsing SingleUserAgent URL: %s", ua.URL)
return ""
}
u.Path = ""
lyse commented 2 months ago
Poster
Collaborator

That breaks if somebody decided to serve from a subdirectory and run their yarnd with -u https://example.com/subdir. Same down below. I reckon we need to explicitly cut off the "suffix" from the end.

That breaks if somebody decided to serve from a subdirectory and run their `yarnd` with `-u https://example.com/subdir`. Same down below. I reckon we need to explicitly cut off the "suffix" from the end.
Poster
Owner

Okay 👌

Okay 👌
prologic marked this conversation as resolved
prologic added 1 commit 2 months ago
122868914e
Fix some typos
prologic added 1 commit 2 months ago
4505d20761
Fix PodURI()
prologic added 1 commit 2 months ago
50735b2c7e
Fix periodic refresh bug
prologic added 1 commit 2 months ago
884273ed3d
Fix more stuff
prologic added 1 commit 2 months ago
ef60bc983a
Dedupe some code
prologic added 1 commit 2 months ago
d4e265ec19
Cleanup
lyse added 1 commit 2 months ago
22b6e002d0 Refactor code, add tests and fix revealed bug
lyse added 1 commit 2 months ago
72579a24b4 Add tests for DetectPodFromRequest
lyse added 1 commit 2 months ago
2e31b28dd5 Include last update of peering pod's info
lyse added 1 commit 2 months ago
lyse added 1 commit 2 months ago
c40eeb8665 Unify test data for refactoring
lyse force-pushed pod_peers from 3213bd87fd to d82fb31c61 2 months ago
lyse added 1 commit 2 months ago
6837616a24 Reuse cache's own config
lyse added 1 commit 2 months ago
0c981a223a Refactor unit tests II
prologic merged commit fabe632491 into master 2 months ago

Reviewers

ullarah was requested for review 2 months ago
movq was requested for review 2 months ago
lyse was requested for review 2 months ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as fabe632491.
Sign in to join this conversation.
Loading…
There is no content yet.