[FIXED] Detect service import cycles. #1731
Conversation
Not sure if this should be detected as misconfiguration or if code need to be fixed to work properly. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
|
Ok I updated and do not try to detect configuration cycles but instead catch it at runtime. Please take another look. Thanks. |
|
Yes I am going back to detecting the cycle when you add a new import. |
|
ok pls take one more look. |
Signed-off-by: Derek Collison <derek@nats.io>
| // Check that what we are importing is not something we also export. | ||
| if a.serviceExportOverlaps(to) { | ||
| // So at this point if destination account is also importing from us, that forms a cycle. | ||
| if destination.serviceImportOverlaps(from) { |
There was a problem hiding this comment.
looks like this would trigger if: a importFormsCycle b
a serviceExportOverlaps with c
b serviceImportOverlaps with d
Seems the checks do not take accounts fully into account?
There was a problem hiding this comment.
It does take accounts into consideration since it looks at itself first during an import and sees if we have an overlapping export. AFAIK this is required to form any cycle. We then check the destination which is the destination account against the "from" subject to see if it overlaps in the destination account with its imports.
There was a problem hiding this comment.
Can a.serviceExportOverlaps(to) return true for a reason that has nothing to do with destination?
Similarly, can destination.serviceImportOverlaps(from) return true for a reason that has nothing to do with a?
On further reflection, I also wonder if we should prevent cycles irrespective of the subjects involved.
| _, err := a.addServiceImport(destination, from, to, imClaim) | ||
| return err | ||
| } | ||
|
|
||
| // Detects if we have a cycle. | ||
| func (a *Account) importFormsCycle(destination *Account, from, to string) bool { |
There was a problem hiding this comment.
Are we doing direct cycles here?
I would have expected a depth first search alternating between exports/imports facilitating a queue to avoid recursion.
There was a problem hiding this comment.
Yes we detect direct cycles here, hence the original test with just help and two accounts.
There was a problem hiding this comment.
I meant more like only direct cycles (involving 2 accounts) vs all cycles (involving more accounts)
| accounts { | ||
| A { | ||
| exports [ { service: * } ] | ||
| imports [ { service { subject: help, account: B } } ] | ||
| } | ||
| B { | ||
| exports [ { service: help } ] | ||
| imports [ { service { subject: help, account: C } } ] | ||
| } |
There was a problem hiding this comment.
I'd say the above check finds the loop here?
how about A export a, B import a to b and export b, C import b to c to d and export d
Then another test that rewrites subjects via mappings as well.
There was a problem hiding this comment.
I have a test with 3 accounts. I could add this one if you feel strongly about it.
There was a problem hiding this comment.
Well, if we only detect direct cycles, then a test with 3 accounts - no direct cycle - is not needed.
If we do want to detect cycles between more than two accounts I feel strongly about such a test.
My reading of the above code (importFormsCycle) is that you get an error because of these two accounts alone and so the test does not test what it's supposed to.
There was a problem hiding this comment.
I have tests with wildcards and import mappings as well IIRC. But happy to add another test if you feel something is missing.
|
Also
Why not A -> B and B to $SYS instead and avoid having to support this? |
|
I figured out the daisy chaining and export/import for all of JS in another PR. |
|
I want to know if this bug will release a new version for 2.1.x |
Simple configs that represent a service export/import cycle between accounts could crash the server.
Signed-off-by: Derek Collison derek@nats.io
/cc @nats-io/core