Fail Stream Replicator on startup if source or target isn't reachable #183

TiganeteaRobert · 2022-08-04T12:36:56Z

Added connection checks to:

PubSub source (also added check for existing subscription in project)
PubSub target (also added check for existing topic in project)
Eventhub target
HTTP target (added HEAD request on start-up to check for a connection to the host)

Tested the rest of the sources and targets and they are checked on start-up.

config/config.go

…closes #182)

colmsnowplow · 2022-08-11T13:48:27Z

There are tests commented out for most of the sources and targets, which were written as a means of documenting the requirement.

Even where we don't have any code changes, we should uncomment those tests to ensure that for every source and target behaves as we expect.

If the pre-existing tests themselves require changes that's fine - but let's document why. Ofc it's always fine to add more tests too.

TiganeteaRobert · 2022-08-12T09:07:36Z

@colmsnowplow I have went through all sources and targets and uncommented and fixed those tests.

TiganeteaRobert · 2022-08-12T09:09:04Z

pkg/target/eventhub_test.go

@@ -395,19 +395,3 @@ func TestNewEventHubTarget_ConnVarsErr(t *testing.T) {
 	assert.EqualError(err, `Error initialising EventHub client: could not reach Event Hub: dial tcp: lookup test.servicebus.windows.net: no such host`)
 	assert.Nil(tgt)
 }
-


This test was removed as the same behaviour is tested in the unit test above it.

It doesn't look like the same behaviour to me...

There are two ways to authenticate against EH - connection string or key value. In the scenario where no valid combination of the required env vars for one of the two methods is present, the client doesn't provide a good debugging experience - so we manually check this in the code. The existing unit tests are for that check.

If the correct env vars are provided, however, we can still fail to connect to the client - it might be unavailable, the credentials provided might be incorrect, etc. So another test should be needed to check this.

I’m having a hard time understanding the issue here, it seems to me that the test that I deleted was not even correct after all. It sets env vars for EVENTHUB_KEY_NAME and EVENTHUB_KEY_VALUE and expects a No valid combination of authentication Env vars found error, which would not be the case. That error would appear if no env vars have been set, either for the connection string or the eventhub key. This possibility is tested above in TestNewEventHubTarget_CredentialsNotFound.

I might be missing something here so please if you could provide a bit more context on this, or even book a quick chat about it it would be really helpful.

Taking a second look, you're right, the different possible failures are indeed accounted for. I think I just looked in the wrong place and confused myself - these tests are indeed fine.

colmsnowplow · 2022-08-19T13:21:38Z

v1 has been merged, develop branch and this branch will both need to be rebased

colmsnowplow · 2022-08-19T13:28:01Z

pkg/source/pubsub/pubsub_source_test.go

@@ -77,7 +76,6 @@ func TestNewPubSubSource_Failure(t *testing.T) {
 	assert.Nil(pubsubSource)
 	// This should return an error when we can't connect, rather than proceeding to the Write() function before we hit a problem.


nit - would be nice to check for the error message as you have done for other sources

colmsnowplow · 2022-08-19T13:30:31Z

pkg/source/sqs/sqs_source_test.go

@@ -61,10 +61,12 @@ func TestNewSQSSourceWithInterfaces_Failure(t *testing.T) {

 	source, err := newSQSSourceWithInterfaces(client, "00000000000", 10, testutil.AWSLocalstackRegion, "nonexistent-queue")

-	assert.Nil(source)
-	assert.NotNil(err)
+	assert.NotNil(source)


What's the rationale for doing this?

As newSqsSourceWithInterfaces just returns a new sqsSource regardless of the parameters it gets and never errors, the assertion we had there was wrong.

Right but the issue this PR is for is to establish behaviour where functions like this one fail when we can't connect. The test is meant to illustrate that desired behaviour - so the test should stay as it was and the behaviour of the function should change in this PR

colmsnowplow · 2022-08-19T13:31:07Z

pkg/source/sqs/sqs_source_test.go

+	assert.Nil(err)
+
+	err = source.Read(nil)
+	assert.True(strings.HasPrefix(err.Error(), `Failed to get SQS queue URL`))


If err is nil this will panic - see the rest of the tests in the project on (on master at least) for pattern that avoids this

colmsnowplow · 2022-08-19T13:43:46Z

pkg/target/http_test.go

 		assert.Equal("Hello Server!!", string(result))
 	}

 	assert.Equal(int64(10), ackOps)
 }

-func TestHttpWrite_Failure(t *testing.T) {


This test was intended for a separate case to connection error - providing a bad path was just an easy way to achieve it.

It's to test the behaviour of stream replicator when requests fail.

Rather than removing a test for this case, we should amend the test to fail some other way.

For example, a malformed request body would achieve it. Or, we could add a case to the http server to respond to a given path with a success for the HEAD request, but a failure for all other requests.

Great idea with the path, I've added an exception in the http client to look for the /error path and return a 500 error if the request does not have the HEAD method. This allows us to test that the HEAD connection test can be successful but other requests can fail and test the behaviour.

colmsnowplow · 2022-08-19T13:45:10Z

pkg/target/pubsub_test.go

-	assert.NotNil(err)
+	pubsubTarget, err := newPubSubTarget(`project-test`, `test-topic-wrong`)
+
+	assert.EqualError(err, `Connection to PubSub failed, topic does not exist`)


Damn, wish I knew about EqualError before I spent ages wrapping stuff across the project in if statements 🤦

TiganeteaRobert force-pushed the startup-fail branch 2 times, most recently from cc541a4 to e5e7c65 Compare August 4, 2022 13:17

TiganeteaRobert commented Aug 4, 2022

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

Fail Stream Replicator on startup if source or target isn't reachable (…

adb452b

…closes #182)

TiganeteaRobert force-pushed the startup-fail branch from e5e7c65 to adb452b Compare August 4, 2022 13:31

TiganeteaRobert requested review from colmsnowplow and adatzer and removed request for colmsnowplow August 4, 2022 13:31

TiganeteaRobert marked this pull request as ready for review August 4, 2022 13:31

colmsnowplow changed the base branch from release/1.0.0 to develop August 11, 2022 12:29

added tests that were commented

5e9cf7c

TiganeteaRobert commented Aug 12, 2022

View reviewed changes

fixed unit test

dc9ee38

colmsnowplow reviewed Aug 19, 2022

View reviewed changes

TiganeteaRobert mentioned this pull request Aug 22, 2022

Fail Stream Replicator on startup if source or target isn't reachable #205

Draft

TiganeteaRobert closed this Aug 22, 2022

colmsnowplow deleted the startup-fail branch August 23, 2022 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail Stream Replicator on startup if source or target isn't reachable #183

Fail Stream Replicator on startup if source or target isn't reachable #183

TiganeteaRobert commented Aug 4, 2022 •

edited

Loading

colmsnowplow commented Aug 11, 2022

TiganeteaRobert commented Aug 12, 2022

TiganeteaRobert Aug 12, 2022

colmsnowplow Aug 19, 2022

TiganeteaRobert Aug 22, 2022 •

edited

Loading

colmsnowplow Aug 22, 2022

colmsnowplow commented Aug 19, 2022 •

edited

Loading

colmsnowplow Aug 19, 2022

TiganeteaRobert Aug 22, 2022

colmsnowplow Aug 19, 2022

TiganeteaRobert Aug 22, 2022

colmsnowplow Aug 22, 2022

colmsnowplow Aug 19, 2022

TiganeteaRobert Aug 22, 2022

colmsnowplow Aug 19, 2022

TiganeteaRobert Aug 22, 2022

colmsnowplow Aug 19, 2022

		@@ -77,7 +76,6 @@ func TestNewPubSubSource_Failure(t *testing.T) {
		assert.Nil(pubsubSource)
		// This should return an error when we can't connect, rather than proceeding to the Write() function before we hit a problem.

Fail Stream Replicator on startup if source or target isn't reachable #183

Fail Stream Replicator on startup if source or target isn't reachable #183

Conversation

TiganeteaRobert commented Aug 4, 2022 • edited Loading

colmsnowplow commented Aug 11, 2022

TiganeteaRobert commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TiganeteaRobert Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colmsnowplow commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TiganeteaRobert commented Aug 4, 2022 •

edited

Loading

TiganeteaRobert Aug 22, 2022 •

edited

Loading

colmsnowplow commented Aug 19, 2022 •

edited

Loading