Skip to content

[STORM-1642] Catch Exception when deserialization failed for 1.x branch. - #2076

Closed
liurenjie1024 wants to merge 1 commit into
apache:1.x-branchfrom
MediaV:STORM-1642
Closed

[STORM-1642] Catch Exception when deserialization failed for 1.x branch.#2076
liurenjie1024 wants to merge 1 commit into
apache:1.x-branchfrom
MediaV:STORM-1642

Conversation

@liurenjie1024

@liurenjie1024liurenjie1024 commented Apr 17, 2017

Copy link
Copy Markdown
Contributor

A new version for 1.x branch, the origin discussion can be found here

@liurenjie1024

liurenjie1024 commented Apr 17, 2017

Copy link
Copy Markdown
ContributorAuthor

@knusbaum Can you help to merge this?

revans2
revans2 suggested changes Apr 17, 2017
View reviewed changes

@revans2revans2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In addition to my other comments we also need a pull request against master. We cannot put in a fix that only goes into an older release line unless it is not an issue on the newer one.

Comment threadstorm-core/src/jvm/org/apache/storm/serialization/KryoTupleDeserializer.java
return new TupleImpl(_context, values, taskId, streamName, id);
} catch(IOException e) {
throw new RuntimeException(e);
LOG.error("Failed to deserialize tuple.", e);

@revans2revans2Apr 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Storm is designed to be a fail fast system, this fix appears to be doing the opposite of that. If we cannot deserialize a tuple we need to make it very very clear to the user that something bad is happening. Sadly in my experience even crashing the worker is not always enough, but in most cases it is. Can we do a System.exit after logging the error instead of returning null?

@liurenjie1024liurenjie1024Apr 18, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In our case this happens because storm's internal message exchange protocol is vulnerable to foreign attack. If deserialization fails, you should warn the user and go on with processing.

@revans2revans2Apr 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then we need to secure the connection against attach and not mask real errors.

@govind-menon

govind-menon commented May 1, 2017

Copy link
Copy Markdown
Contributor

@liurenjie1024 I have a pull-request #2095 that should handle this better. Specifically for your issue to protect against foreign attack, you can turn on SSL by following the instructions here - http://storm.apache.org/releases/0.10.0/SECURITY.html

@revans2

revans2 commented May 1, 2017

Copy link
Copy Markdown
Contributor

@govind-menon Actually I think it is SASL not SSL that you want.
if you just set

storm.messaging.netty.authentication=true

Either as the default for your cluster or even on a per topology bases (I think) each of the netty connections between workers will do a handshake and validate that each of them have the same secret, that is generated when the topology is submitted.

@govind-menon

govind-menon commented May 1, 2017

Copy link
Copy Markdown
Contributor

@revans2 Oops - thanks.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Derek Dagit
Closes Pull Requests unchanged in 2018
fcda81c
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Derek Dagit
Closes Pull Requests unchanged in 2018
fdb95d0
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
@d2rd2r mentioned this pull request Oct 16, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@revans2revans2revans2 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@liurenjie1024@govind-menon@revans2