Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Fix a couple TR NullPointerExceptions when no caches are available#4765

Merged
mattjackson220 merged 1 commit intoapache:masterfrom
rawlinp:change-ip-addr-check-master
Jun 9, 2020
Merged

Fix a couple TR NullPointerExceptions when no caches are available#4765
mattjackson220 merged 1 commit intoapache:masterfrom
rawlinp:change-ip-addr-check-master

Conversation

@rawlinp
Copy link
Copy Markdown
Contributor

@rawlinp rawlinp commented Jun 8, 2020

What does this PR (Pull Request) do?

Fixes a Null Pointer Exception (where there are no available caches for a requested DS) and converts some == usage to .equals() (since I believe we want to compare the actual IP addresses and not the pointer references). Also, change the method for determining whether a given IP address is IPv4 or IPv6 to match what is used elsewhere in similar cases (since getByName can have the side-effect of doing DNS lookups).

  • This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

  • Traffic Router

What is the best way to verify this PR?

Run the TR unit tests, verify they still pass:

mvn clean test -Djava.library.path=/usr/local/opt/tomcat-native/lib

Make a request for a DS that doesn't have any available caches, make sure that editCacheListForIpVersion does not throw a NullPointerException.

If this is a bug fix, what versions of Traffic Control are affected?

  • master
  • 4.1.0-RC1

The following criteria are ALL met by this PR

  • Fairly minor fixes, performed manual tests to validate since there aren't pre-existing unit tests in this specific area (as far as I can tell)
  • Bugfix, docs unnecessary
  • Unreleased bug, changelog entry unnecessary
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

@rawlinp rawlinp added bug something isn't working as intended Traffic Router related to Traffic Router 4.1.x-backport-candidate labels Jun 8, 2020
@rawlinp rawlinp force-pushed the change-ip-addr-check-master branch from a450de7 to 93e0a73 Compare June 8, 2020 18:06
@rawlinp rawlinp requested a review from mattjackson220 June 8, 2020 19:04
@rawlinp rawlinp changed the title Fix a couple TR NullPointerExceptions Fix a couple TR NullPointerExceptions when no caches are available Jun 8, 2020
@rawlinp
Remove InetAddress.getByName usage since it makes DNS requests
d9cb518 
Remove some getAddress toString usage since it does dns lookups

Check if caches is null

Use .equals instead of ==, fix another NPE

Undo ZoneManager NPE fix (SERVFAIL probably better than REFUSED)
@rawlinp rawlinp force-pushed the change-ip-addr-check-master branch from 93e0a73 to d9cb518 Compare June 8, 2020 23:53
@rawlinp
Copy link
Copy Markdown
Contributor Author

rawlinp commented Jun 8, 2020

I rebased to resolve merge conflicts, but I didn't actually have to change any of the code. Cache.java was basically just renamed to Node.java without having to change anything else.

mattjackson220
mattjackson220 approved these changes Jun 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mattjackson220 mattjackson220 left a comment

Choose a reason for hiding this comment

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

Code looks good, TR tests pass, NPE no longer shows up. Nice work!

@mattjackson220 mattjackson220 merged commit c8db585 into apache:master Jun 9, 2020
@rawlinp rawlinp deleted the change-ip-addr-check-master branch June 9, 2020 21:15
rawlinp added a commit to rawlinp/trafficcontrol that referenced this pull request Jun 10, 2020
@rawlinp
Remove InetAddress.getByName usage since it makes DNS requests (apach…
9e1b734 
…e#4765)

Remove some getAddress toString usage since it does dns lookups

Check if caches is null

Use .equals instead of ==, fix another NPE

Undo ZoneManager NPE fix (SERVFAIL probably better than REFUSED)

(cherry picked from commit c8db585)
@rawlinp rawlinp mentioned this pull request Jun 10, 2020
Merged
rawlinp added a commit that referenced this pull request Jun 10, 2020
@rawlinp
Remove InetAddress.getByName usage since it makes DNS requests (#4765) (
4408a9c 
#4780)

Remove some getAddress toString usage since it does dns lookups

Check if caches is null

Use .equals instead of ==, fix another NPE

Undo ZoneManager NPE fix (SERVFAIL probably better than REFUSED)

(cherry picked from commit c8db585)
@zrhoffman
Copy link
Copy Markdown
Member

zrhoffman commented Jun 15, 2020

I rebased to resolve merge conflicts, but I didn't actually have to change any of the code. Cache.java was basically just renamed to Node.java without having to change anything else.

I think the intent is to still use Cache because

  • Although Cache was renamed to Node, a new class also named Cache was added in the Edge Traffic Routing PR, and it extends Node.
  • The usage of Cache doesn't change in ConfigHandler after the Edge Traffic Routing PR was merged
  • Node looks like it is used for Edge Traffic Routing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mattjackson220 mattjackson220 mattjackson220 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug something isn't working as intended Traffic Router related to Traffic Router

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rawlinp @zrhoffman @mattjackson220