Skip to content
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

[FIXED] Detect service import cycles. #1731

Merged
merged 2 commits into from
Nov 23, 2020
Merged

[FIXED] Detect service import cycles. #1731

merged 2 commits into from
Nov 23, 2020

Conversation

derekcollison
Copy link
Member

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

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>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

change LGTM.
But would this have happened with larger cycles as well?

server/accounts.go Outdated Show resolved Hide resolved
@derekcollison
Copy link
Member Author

Ok I updated and do not try to detect configuration cycles but instead catch it at runtime.

Please take another look. Thanks.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM. To be clear, we are not failing this as a misconfiguration now, just make sure we are not cycling, right?

test/service_latency_test.go Show resolved Hide resolved
test/service_latency_test.go Show resolved Hide resolved
server/client.go Outdated Show resolved Hide resolved
test/service_latency_test.go Show resolved Hide resolved
Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

For simple self imports, in jwt I intend to turn this into a validation error.

server/client.go Outdated Show resolved Hide resolved
@derekcollison
Copy link
Member Author

Yes I am going back to detecting the cycle when you add a new import.

@derekcollison
Copy link
Member Author

ok pls take one more look.

Signed-off-by: Derek Collison <derek@nats.io>
server/accounts.go Show resolved Hide resolved
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing direct cycles here?
I would have expected a depth first search alternating between exports/imports facilitating a queue to avoid recursion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we detect direct cycles here, hence the original test with just help and two accounts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant more like only direct cycles (involving 2 accounts) vs all cycles (involving more accounts)

Comment on lines +1344 to +1352
accounts {
A {
exports [ { service: * } ]
imports [ { service { subject: help, account: B } } ]
}
B {
exports [ { service: help } ]
imports [ { service { subject: help, account: C } } ]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a test with 3 accounts. I could add this one if you feel strongly about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tests with wildcards and import mappings as well IIRC. But happy to add another test if you feel something is missing.

@matthiashanel
Copy link
Contributor

Also

Daisy chain is export $JS.API.>. Will go from account A to account B to $SYS.

Why not A -> B and B to $SYS instead and avoid having to support this?
Also, would A to B and C to B and everything in B to $SYS work?

@derekcollison
Copy link
Member Author

I figured out the daisy chaining and export/import for all of JS in another PR.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 18108be into master Nov 23, 2020
@derekcollison derekcollison deleted the cycle branch November 23, 2020 17:43
@flymoondust
Copy link

flymoondust commented Mar 11, 2021

I want to know if this bug will release a new version for 2.1.x

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.

None yet

4 participants