-
Notifications
You must be signed in to change notification settings - Fork 31
Add param that disables Director's hosting of federation metadata #2887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@williamnswanson This new param also feels relevant to you, since we'll probably want to come up with a plan for turning it on in the OSDF (hopefully without exposing too many bugs at once...). |
|
If our goal is to find bugs related to Director-based federation discovery (or components/clients that are still using the director for discovery), could we use a less destructive method to find out what is currently using it, like logging access? That way we can reach out out-of-band about a transition plan instead of shutting it off and breaking their current workflows out of nowhere? |
turetske
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor issue regarding a println that was left in.
This was a subtle bug I uncovered while trying to write a unit test that distinguished between the fed metadata hosted by the discovery server and the test's Director. It turned out that the duplicate metadata hosted by the test Director wasn't matching the discovery server's because it wasn't enough to set the Discovery URL as a param for fed tests because by that point, global fed discovery had already run by the Director (who thought it was the fed root when it started -- a bootstrapping issue). To achieve the desired behavior, I also needed to overwrite the global fed info to reflect what the Director would have discovered had it used this as its source of truth. I don't think this is actually a bug in the code, only in the awkward approach we have to setting up federations for tests.
There is an entire class of bugs that revolves around having the Director host a copy of federation metadata when it isn't also the root discovery URL for the federation. Adding this knob doesn't _solve_ those bugs, but it should help us detect them more easily by allowing us to turn of Director metadata when we don't want people using the Director for that purpose.
2599c49 to
ceb4651
Compare
|
After an in-person discussion with @williamnswanson, we decided it'd also be nice to add some monitoring in the Director to detect when servers reach out to it for federation metadata. I plan to add that in this PR. |
This was requested by the Ops team to smooth the transition toward turning off metadata hosting at the Director in OSDF. This commit adds a metric we can use to watch who is still hitting the Director for fed metadata. I tried to give the metric enough contextual info to determine "who" is asking for the data by recording a masked address and a service type (bootstrapped from the request's user agent). Importantly, this changed my original feature design slightly -- rather than completely turning off the metadata hosting endpoint by de-registering it with gin, this keeps it registered so we can collect metrics. If the metadata hosting is disabled, we return a `410 Gone` as soon as we record who tried to access the info.
|
Added @patrickbrophy as a second reviewer because of the new Prometheus metric this adds. |
There is an entire class of bugs that revolves around having the
Director host a copy of federation metadata when it isn't also the
root discovery URL for the federation. We keep finding these bugs
in a trickle, but we haven't been able to squash them all because
of how entangled these concepts are.
Adding this knob doesn't solve those bugs, but it should help us
detect them more easily by allowing us to turn of Director metadata
when we don't want people using the Director for that purpose.