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

eureka-1134: fix for StringIndexOutOfBoundsException #1135

Merged
merged 3 commits into from
Dec 18, 2018
Merged

eureka-1134: fix for StringIndexOutOfBoundsException #1135

merged 3 commits into from
Dec 18, 2018

Conversation

JosephWitthuhnTR
Copy link
Contributor

It looks like a -1 was used when it should be comparing against 3. Based on line 353, beginIndex will always be larger than -1, but it will be 3 if there is no "ec2-" in the URL.

This is for issue #1134 which I reported earlier this morning.

This can happen when "ec2-" isn't in the hostname.
@JosephWitthuhnTR JosephWitthuhnTR changed the title fix for StringIndexOutOfBoundsException eureka-1134: fix for StringIndexOutOfBoundsException Oct 15, 2018
@qiangdavidliu
Copy link
Contributor

@jawitthuhn thanks for the PR, this does look like a bug. Given what the logic is trying express, it would make this code block much easier to read if we apply the below changes (and also add a test case):

for (String cname : ec2Urls) {
            int beginIndex = cname.indexOf("ec2-");
            if (-1 < beginIndex) {  // contains "ec2-"
                int endIndex = cname.indexOf(regionPhrase + ".compute");
                String eipStr = cname.substring(beginIndex + 4, endIndex);
                String eip = eipStr.replaceAll("\\-", ".");
                returnedUrls.add(eip);
            } else {  // do nothing
              // Handle case where there are no cnames containing "ec2-"
              // Reasons include:
              //  Systems without public addresses - purely attached to corp lan via AWS Direct Connect             
              //  Use of EC2 network adapters that are attached to an instance after startup
            }
}

Would you mind updating the PR? Thanks!

@JosephWitthuhnTR
Copy link
Contributor Author

@qiangdavidliu - I believe I've made the requested changes. Could you check?

@elandau elandau merged commit aaa00c1 into Netflix:master Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants