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

MYST-324 Return "Service Unavailable" when ipify is unavailable #166

Merged
merged 3 commits into from Feb 21, 2018

Conversation

Projects
None yet
4 participants
@donce
Copy link
Contributor

commented Feb 19, 2018

No description provided.

@donce donce changed the title MYST-324 Return Service Unavailable" MYST-324 Return "Service Unavailable" when ipify is unavailable Feb 19, 2018

@shroomist shroomist self-requested a review Feb 19, 2018

@zolia

zolia approved these changes Feb 20, 2018

@@ -61,6 +64,7 @@ func (ce *connectionEndpoint) Create(resp http.ResponseWriter, req *http.Request
case connection.ErrAlreadyExists:
utils.SendError(resp, err, http.StatusConflict)
default:
log.Error(connectionLogPrefix, err)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 20, 2018

Member

Should not logs be handled in connection manager, and it it could have more detailed logs on intermediate steps

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 20, 2018

Member

At least, we applied this approach in other places

This comment has been minimized.

Copy link
@donce

donce Feb 21, 2018

Author Contributor

There is no convenient way to log all connect errors in connection manager - take a look at it's implementation:

func (manager *connectionManager) Connect(consumerID, providerID identity.Identity) error {
	if manager.status.State != NotConnected {
		return ErrAlreadyExists
	}

	proposal, err := manager.findProposalByProviderID(providerID)
	if err != nil {
		return err
	}

	dialogEstablisher := manager.newDialogEstablisher(consumerID)
	manager.dialog, err = dialogEstablisher.EstablishDialog(providerID, proposal.ProviderContacts[0])
	if err != nil {
		return err
	}

	vpnSession, err := session.RequestSessionCreate(manager.dialog, proposal.ID)
	if err != nil {
		return err
	}
	manager.sessionID = vpnSession.ID

	manager.openvpnClient, err = manager.newVpnClient(*vpnSession, consumerID, providerID, manager.onVpnStatusUpdate)
	if err != nil {
		manager.dialog.Close()
		return err
	}

	if err := manager.openvpnClient.Start(); err != nil {
		manager.dialog.Close()
		return err
	}
	return nil
}

There are 6 places where error can occur - I want to log all of them, but adding 6 log statements just because this class did not have any logs before doesn't make sense to me.

@Waldz

Waldz approved these changes Feb 21, 2018

@donce donce merged commit f0797bf into master Feb 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@donce donce deleted the feature/MYST-324-ip-detection-timeout branch Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.