e2e test for elasticsearch token propagation #1705

rubenvp8510 · 2019-08-01T14:21:09Z

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

Which problem is this PR solving?

e2e tests for token propagation to ES

Short description of the changes

This is an integration test that make sure that the token reached ES storage.

codecov · 2019-08-01T14:37:32Z

Codecov Report

Merging #1705 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1705      +/-   ##
==========================================
- Coverage   98.36%   98.32%   -0.05%     
==========================================
  Files         193      193              
  Lines        9358     9358              
==========================================
- Hits         9205     9201       -4     
- Misses        119      122       +3     
- Partials       34       35       +1

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	`83.33% <0%> (-3.51%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5f9953...628d228. Read the comment docs.

objectiser

LGTM - just some minor

objectiser · 2019-08-02T11:23:59Z

.travis.yml

@@ -75,6 +78,7 @@ script:
  - if [ "$CASSANDRA_INTEGRATION_TEST" == true ]; then bash ./scripts/travis/cassandra-integration-test.sh ; else echo 'skipping cassandra integration test'; fi
  - if [ "$HOTROD" == true ]; then bash ./scripts/travis/hotrod-integration-test.sh ; else echo 'skipping hotrod example'; fi
  - if [ "$PROTO_GEN_TEST" == true ]; then make proto PROTOC=$HOME/protoc/bin/protoc && git diff --name-status --exit-code ; else echo 'skipping protoc validation'; fi
+  - if [ "TOKEN_INTEGRATION_TEST" == true ]; then bash ./scripts/travis/token-integration-test.sh ;  else echo 'skipping protoc validation'; fi


Message should be "skipping token propagation test".

objectiser · 2019-08-02T11:24:40Z

.travis.yml

@@ -36,6 +36,9 @@ matrix:
  - go: "1.12.1"
    env:
    - HOTROD=true
+  - go: "1.12.1"
+    env:
+    - TOKEN_INTEGRATION_TEST=true


Would prefer it to be called TOKEN_PROPAGATION_TEST

I'll integrate into ES tests as @pavolloffay suggested.

objectiser · 2019-08-02T11:25:10Z

cmd/query/integration/token_propagation_test.go

+const bearerToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI"
+const bearerHeader = "Bearer " + bearerToken
+
+


nit: Remove two lines.

objectiser · 2019-08-02T11:39:26Z

cmd/query/integration/token_propagation_test.go

+	}
+	stop := &sync.WaitGroup{}
+
+	go MockingESEToken(t, stop)


Does this need to be a goroutine?

Yes, I need to have this running in background mocking the ES, if I don't do this in a goroutine the function will run indefinitely listen on 9200 port, and the next section of the test won't be executed.

pavolloffay · 2019-08-02T12:34:31Z

To keep the number of jobs low, could we add this test to ES_INTEGRATION_TEST test. There are already other tests related to ES.

pavolloffay · 2019-08-02T12:40:54Z

cmd/query/integration/token_propagation_test.go

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+// +build token_propagation


nit: add a space before so it's better visible

yurishkuro · 2019-08-03T12:41:42Z

.travis.yml

@@ -63,6 +63,9 @@ install:
        curl -L https://github.com/google/protobuf/releases/download/v3.6.1/protoc-3.6.1-linux-x86_64.zip -o /tmp/protoc.zip
        unzip /tmp/protoc.zip -d "$HOME"/protoc
    fi
+  - rm -rf ~/.nvm && git clone https://github.com/creationix/nvm.git ~/.nvm && (cd ~/.nvm && git checkout `git describe --abbrev=0 --tags`) && source ~/.nvm/nvm.sh && nvm install 8.16.0
+  - npm install
+  - curl -o- -L https://yarnpkg.com/install.sh | bash


why do we need this unconditionally in all matrix runs? It's only needed for steps where the UI is built, and I think it's already being installed there

yurishkuro · 2019-08-03T12:42:40Z

Makefile

@@ -103,6 +103,11 @@ index-cleaner-integration-test: docker-images-elastic
 	go clean -testcache
 	bash -c "set -e; set -o pipefail; $(GOTEST) -tags index_cleaner $(STORAGE_PKGS) | $(COLORIZE)"

+.PHONY: token-propagation-integration-test
+query-integration-test:


target names don't match

yurishkuro · 2019-08-03T12:43:46Z

scripts/travis/es-integration-test.sh

@@ -9,3 +9,14 @@ STORAGE=elasticsearch make storage-integration-test
 make index-cleaner-integration-test

 docker kill $CID
+
+make build-ui


why does this need the UI? It should only need query service API

I received errors when I tried to compile the query service alone,.

we build query service for crossdock using a mock UI, check that

I'll check it, Thanks!

yurishkuro · 2019-08-03T12:45:35Z

plugin/storage/integration/token_propagation_test.go

+		req.Header.Add(testCase.headerName, testCase.headerValue)
+		assert.NoError(t, err)
+		client := &http.Client{}
+		_, err = client.Do(req)


I don't think this is a valid test, the query service can simply ignore the tokens, the test is not validating anything.

What do you mean? If query service ignore the token, the handler in the elasticsearch mocker won't received it and the test will fail.

but the ESMock doesn't return any sensible results, the query service will fail anyway

Ha sure, I'm not validating if the query service returns results or not, I only validate that the token is being propagated to the ES storage. I of course can implement a better mock of ES if you think we need to validate the query reply

Due to the nature of the e2e tests I decided to follow your recommendation, now I returned an empty result if with an http 200 if the token was received with mocked ES, and 403 (forbidden http) otherwise.

So the test validates that the query service propagated the token correctly.

Is the test better now?

yurishkuro · 2019-08-10T16:07:53Z

plugin/storage/integration/token_propagation_test.go

+	http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
+}
+
+func CreateElasticSearchMock(srv *http.Server, t *testing.T) {


private? should not be capitalized

yurishkuro · 2019-08-10T16:08:08Z

plugin/storage/integration/token_propagation_test.go

+	}
+}
+
+func Test_bearTokenPropagation(t *testing.T) {


TestBearerTokenPropagation

yurishkuro · 2019-08-10T16:09:43Z

plugin/storage/integration/token_propagation_test.go

+		// Ask for services query, this should return 200
+		req, err := http.NewRequest("GET", queryUrl, nil)
+		req.Header.Add(testCase.headerName, testCase.headerValue)
+		assert.NoError(t, err)


move one line above. Should use require instead of assert since you can't continue the test on error

yurishkuro · 2019-08-10T16:11:25Z

plugin/storage/integration/token_propagation_test.go

+		assert.NoError(t, err)
+		client := &http.Client{}
+		resp, err := client.Do(req)
+		// Should not have errors.


redundant comment

yurishkuro · 2019-08-10T16:11:33Z

plugin/storage/integration/token_propagation_test.go

+		client := &http.Client{}
+		resp, err := client.Do(req)
+		// Should not have errors.
+		assert.Nil(t, err)


assert.NoError

yurishkuro · 2019-08-10T16:12:56Z

plugin/storage/integration/token_propagation_test.go

+		}
+	}
+
+	srv.Shutdown(context.Background())


move to next line after creating a server and invoke via defer, to keep housekeeping logic together

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 · 2019-08-10T16:58:53Z

Ok, I addressed all comments, @yurishkuro thanks for the review!

yurishkuro

Thanks!

rubenvp8510 requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners August 1, 2019 14:21

objectiser reviewed Aug 2, 2019

View reviewed changes

pavolloffay reviewed Aug 2, 2019

View reviewed changes

rubenvp8510 force-pushed the token-propagation-e2e-simple branch 10 times, most recently from 9c86e36 to 5ee958c Compare August 3, 2019 02:39

yurishkuro requested changes Aug 3, 2019

View reviewed changes

rubenvp8510 force-pushed the token-propagation-e2e-simple branch 8 times, most recently from b744067 to aab79fe Compare August 4, 2019 02:26

rubenvp8510 closed this Aug 5, 2019

rubenvp8510 reopened this Aug 5, 2019

rubenvp8510 force-pushed the token-propagation-e2e-simple branch 4 times, most recently from 0265690 to 8511d74 Compare August 5, 2019 16:14

rubenvp8510 closed this Aug 5, 2019

rubenvp8510 reopened this Aug 5, 2019

rubenvp8510 force-pushed the token-propagation-e2e-simple branch 7 times, most recently from 3ebf761 to a39956c Compare August 8, 2019 11:33

yurishkuro reviewed Aug 10, 2019

View reviewed changes

rubenvp8510 force-pushed the token-propagation-e2e-simple branch 3 times, most recently from de4e4fc to ef3302c Compare August 10, 2019 16:18

e2e test for elasticsearch token propagation

628d228

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the token-propagation-e2e-simple branch from ef3302c to 628d228 Compare August 10, 2019 16:42

yurishkuro approved these changes Aug 10, 2019

View reviewed changes

yurishkuro merged commit 0341a42 into jaegertracing:master Aug 10, 2019

rubenvp8510 mentioned this pull request Nov 8, 2019

Token propagation E2E tests. jaegertracing/jaeger-operator#581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e test for elasticsearch token propagation #1705

e2e test for elasticsearch token propagation #1705

rubenvp8510 commented Aug 1, 2019 •

edited

Loading

codecov bot commented Aug 1, 2019 •

edited

Loading

objectiser left a comment

objectiser Aug 2, 2019

objectiser Aug 2, 2019

rubenvp8510 Aug 2, 2019

objectiser Aug 2, 2019

objectiser Aug 2, 2019

rubenvp8510 Aug 2, 2019

pavolloffay commented Aug 2, 2019

pavolloffay Aug 2, 2019

yurishkuro Aug 3, 2019

yurishkuro Aug 3, 2019

yurishkuro Aug 3, 2019

rubenvp8510 Aug 3, 2019

yurishkuro Aug 3, 2019

rubenvp8510 Aug 3, 2019

yurishkuro Aug 3, 2019

rubenvp8510 Aug 3, 2019

yurishkuro Aug 3, 2019

rubenvp8510 Aug 3, 2019

rubenvp8510 Aug 5, 2019

rubenvp8510 Aug 8, 2019

yurishkuro Aug 10, 2019

yurishkuro Aug 10, 2019

yurishkuro Aug 10, 2019

yurishkuro Aug 10, 2019

yurishkuro Aug 10, 2019

yurishkuro Aug 10, 2019

rubenvp8510 commented Aug 10, 2019

yurishkuro left a comment

		const bearerToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI"
		const bearerHeader = "Bearer " + bearerToken

e2e test for elasticsearch token propagation #1705

e2e test for elasticsearch token propagation #1705

Conversation

rubenvp8510 commented Aug 1, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Aug 1, 2019 • edited Loading

Codecov Report

objectiser left a 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

pavolloffay commented Aug 2, 2019

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

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

rubenvp8510 commented Aug 10, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

rubenvp8510 commented Aug 1, 2019 •

edited

Loading

codecov bot commented Aug 1, 2019 •

edited

Loading