Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

fix topic subscribing bug #426

Merged
merged 5 commits into from
Mar 7, 2017
Merged

Conversation

YujiOshima
Copy link
Contributor

fix #425
Signed-off-by: Yuji Oshima yuji.oshima0x3fd@gmail.com

Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
@codecov-io
Copy link

codecov-io commented Mar 7, 2017

Codecov Report

Merging #426 into master will decrease coverage by -0.09%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
- Coverage   62.75%   62.67%   -0.09%     
==========================================
  Files          72       72              
  Lines        4446     4455       +9     
==========================================
+ Hits         2790     2792       +2     
- Misses       1327     1331       +4     
- Partials      329      332       +3
Impacted Files Coverage Δ
pkg/broker/server/sse.go 72.58% <55.55%> (-1.34%)
pkg/broker/client/sse.go 61.62% <0%> (-3.49%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83ffc26...039bb27. Read the comment docs.

return false
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Clever to do the check here.

Would you mind put this code in a function? Something like if !checkPath(key, event.topic) { return false }?

Copy link
Contributor

Choose a reason for hiding this comment

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

s/notfyTopic/notifyTopic/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, moved to function!

@chungers
Copy link
Contributor

chungers commented Mar 7, 2017

@YujiOshima Thank you for catching and fixing this bug and adding tests.

Once you move the check to a function I will merge it in.

I still think a better way is to intercept the subscribe call and reject a topic that doesn't conform to the spec of the Event plugin. This way, we don't have to do this check at every outbound event publish. It's also better UX since the client knows it tried to subscribe to an incorrect topic.

One place this might work, would be to add checks as an interceptor to the ServerHTTP handler. This would be the place: https://github.com/docker/infrakit/blob/master/pkg/rpc/server/server.go#L132

Here we have references to all the event.Plugin implementations and we can wrap the broker.ServeHTTP handler with a handler that does proper topic validation before they are forwarded on to the real thing. This would also give meaningful HTTP error code on subscription, rather than events quietly dropped to the floor. Finally the pkg/broker doesn't have to have any knowledge of / convention for topics and can be used for something that uses a different topic naming convention (maybe .object1.property1).

I will file that as a TO-DO....

Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Copy link
Contributor

@chungers chungers left a comment

Choose a reason for hiding this comment

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

LGTM

@chungers chungers merged commit 2705efc into docker-archive:master Mar 7, 2017
chungers pushed a commit to chungers/infrakit that referenced this pull request Sep 30, 2017
Change subnet to workaround DNS server collision
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event subscribing bug
3 participants