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] Possible cluster Authorization Error during config reload #720

Merged
merged 1 commit into from Aug 16, 2018

Conversation

kozlovic
Copy link
Member

When changing something in the cluster, such as Timeout and doing
a config reload, the route could be closed with an Authorization Error report. Moreover, the route would not try to reconnect,
even if specified as an explicit route.

There were 2 issues:

  • When checking if a solicited route is still valid, we need to
    check the Routes' URL against the URL that we try to connect
    to but not compare the pointers, but either do a reflect
    deep equal, or compare their String representation (this is
    what I do in the PR).
  • We should check route authorization only if this is an accepted
    route, not an explicit one. The reason is that we a server
    explicitly connect to another server, it does not get the remote
    server's username and password. So the check would always fail.

Note: It is possible that a config reload even without any change
in the cluster triggers the code checking if routes are properly
authorized, and that happens if there is TLS specified. When
the reload code checks if config has changed, the TLSConfig
between the old and new seem to indicate a change, eventhough there
is apparently none. Another reload does not detect a change. I
suspect some internal state in TLSConfig that causes the
reflect.DeepEqual() to report a difference.

Note2: This commit also contains fixes to regex that staticcheck
would otherwise complain about (they did not have any special
character), and I have removed printing the usage on startup when
getting an error. The usage is still correctly printed if passing
a parameter that is unknown.

Resolves #719

Signed-off-by: Ivan Kozlovic ivan@synadia.com

When changing something in the cluster, such as Timeout and doing
a config reload, the route could be closed with an `Authorization
Error` report. Moreover, the route would not try to reconnect,
even if specified as an explicit route.

There were 2 issues:
- When checking if a solicited route is still valid, we need to
  check the Routes' URL against the URL that we try to connect
  to but not compare the pointers, but either do a reflect
  deep equal, or compare their String representation (this is
  what I do in the PR).
- We should check route authorization only if this is an accepted
  route, not an explicit one. The reason is that we a server
  explicitly connect to another server, it does not get the remote
  server's username and password. So the check would always fail.

Note: It is possible that a config reload even without any change
in the cluster triggers the code checking if routes are properly
authorized, and that happens if there is TLS specified. When
the reload code checks if config has changed, the TLSConfig
between the old and new seem to indicate a change, eventhough there
is apparently none. Another reload does not detect a change. I
suspect some internal state in TLSConfig that causes the
reflect.DeepEqual() to report a difference.

Note2: This commit also contains fixes to regex that staticcheck
would otherwise complain about (they did not have any special
character), and I have removed printing the usage on startup when
getting an error. The usage is still correctly printed if passing
a parameter that is unknown.

Resolves #719

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 92.306% when pulling d98d51c on fix_reload_cluster_auth into 6608e9a on master.

Copy link
Member

@derekcollison derekcollison 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 a2b5acf into master Aug 16, 2018
@derekcollison derekcollison deleted the fix_reload_cluster_auth branch August 16, 2018 05:45
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.

Possible cluster Authorization Error on config reload
3 participants