Rules API: Add e2e test #198

jessicalins · 2021-12-17T19:55:48Z

This PR ~~starts to~~ adds an e2e test to the Rules API. ~~Already opening as a draft to get some feedback about the overall structure.~~

~~What is currently missing:~~
~~- Test thanos-ruler-syncer flow~~ - after talking with @onprem we thought not adding a test here since this is already covered in the other repos (rules-objstore / thanos-rule-syncer)
~~- Test 'unhappy' cases~~

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

matej-g

Looks good so far 👍 Will wait on the remaining parts of the PR.

test/e2e/rules_test.go

matej-g · 2021-12-20T15:24:42Z

test/e2e/rules_test.go

+
+	api, err := newObservatoriumAPIService(
+		e,
+		withMetricsEndpoints("", "", "http://"+rulesEndpoint),


I think we might need to provide some write/read endpoint, otherwise API will complain that neither metrics nor logs endpoints are provided?

I think for the test environment, since is using https://github.com/observatorium/api/blob/main/test/e2e/services.go#L224 this does not apply? It seems to append in case they are not empty: https://github.com/observatorium/api/blob/main/test/e2e/services.go#L250-L253 but no validation in case they are empty.

I also searched in the test output for this error message "Neither logging nor metrics endpoints are enabled. Specifying at least a logging or a metrics endpoint is mandatory" from https://github.com/observatorium/api/blob/main/main.go#L199-L202 but I haven't found any matches

test/e2e/services.go

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

jessicalins · 2022-01-19T21:19:18Z

test/e2e/rules_test.go

+		assertResponse(t, bodyStr, "alert: ManyInstancesDown")
+	})
+
+	t.Run("put-invalid-rules", func(t *testing.T) {


@onprem for the case that invalid rules are submitted, I keep receiving no error/200 HTTP response status code, instead of https://github.com/observatorium/rules-objstore/blob/main/pkg/server/server.go#L80
I got this in the test output:

17:10:54 rules_objstore: level=debug name=rules-objstore ts=2022-01-19T20:10:54.115959093Z caller=server.go:81 component=server handler=setrules tenant=test-oidc msg="request body failed rule group validation" errs="unsupported value type" 17:10:54 observatorium_api: level=debug name=observatorium ts=2022-01-19T20:10:54.116175567Z caller=instrumentation.go:36 request=observatorium_api/NKsl1S6qrw-000009 proto=HTTP/1.1 method=PUT status=200 content= path=/api/metrics/v1/test-oidc/api/v1/rules/raw duration=1.492672ms bytes=42

I'll try to take a better look on this tomorrow, but I think either obs API is not fetching err/status code or rules-objstore is not returning/propagating them properly

bill3tt · 2022-01-20T12:01:54Z

test/e2e/configs.go

@@ -18,6 +18,7 @@ type testType string

 const (
 	metrics     testType = "metrics"
+	rules 		testType = "rules"


make format should fix these rogue whitespaces 👍

weirdly if I run make format locally I get:

jessica@cupsofwonder:~/workspace/api|rules-add-e2e ⇒ make format /home/jessica/go/bin/golangci-lint-v1.21.0 run --fix --enable-all -c .golangci.yml WARN [runner/nolint] Found unknown linters in //nolint directives: cyclop, exhaustivestruct, goerr113, gomnd, intefacer, paralleltest, testpackage

so somehow it did not give any more output than that and also did not change any of the files.

I then tried to run with my local golangci-lint (with the latest version - 1.43.0) and it found some issues in the code, so I think we may tackle this separately? I wonder if would be good to also update the golangci-lint version (with the latest version I get also some warnings of linters that are deprecated and we use them in the nolinter directive (e.g. WARN [runner/nolint] Found unknown linters in //nolint directives: intefacer)

Should I create a separate issue for that? Maybe makes sense to also link to this issue @matej-g already opened: #212
For now I run manually gofmt to this config file, so the whitespaces should be fixed

Also, shouldn't the lint step fail?

Seems to be fixed now 🤷

you mean the whitespaces or make format?
To fix the whitespaces I ran gofmt manually, but I thought would be fixed by running make format instead (which does not seem to be the case) :(

bill3tt · 2022-01-20T12:04:04Z

test/e2e/rules_test.go

+		Transport: &tokenRoundTripper{rt: tr, token: token},
+	}
+
+	t.Run("get-put-recording-rules", func(t *testing.T) {


get-put-recording-rules this is kind of confusing to me as to what the test is aiming to do?

I renamed to read-write-recording-rules (also to the further ones) - hopefully now is clearer?
It basically tests the reading (GET) / writing (PUT) of rules via the rules API defined in observatorium/api going through rules-objectstore https://github.com/observatorium/api/pull/198/files#diff-368252fd7020798243fc9b3793b03207f23440bf25424123e38b77eabf42c3d3R28

bill3tt · 2022-01-20T12:05:21Z

test/e2e/rules_test.go

+	testutil.Ok(t, err)
+	testutil.Ok(t, e2e.StartAndWaitReady(api))
+
+	rulesEndpointURL := "https://" + api.Endpoint("https") + "/api/metrics/v1/test-oidc/api/v1/rules/raw"


Is the test-oidc tenant name defined anywhere else? I just imagine if we change that we'll unexpectedly break this test

good point..we define it manually everywhere in the tests: https://github.com/observatorium/api/search?q=test-oidc
maybe makes sense to define it as a constant in configs.go and use its value? I could open another pr for that or just already change it here (?)

added as a constant here: 0b60005

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

saswatamcode · 2022-01-21T08:40:05Z

test/e2e/rules_test.go

+		body, err := ioutil.ReadAll(res.Body)
+		bodyStr := string(body)
+
+		assertResponse(t, bodyStr, "sum by (job) (http_inprogress_requests)")


It might be a good idea to check for tenant labels here? 🙂
With something like: assertResponse(t, bodyStr, "tenant_id: 1610b0c3-c509-4592-a256-a1871353dbfa") (id taken from here).

good idea! added here: 7d50a67

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

bill3tt

Couple of non-blocking naming nits & ticket to be created.

LGTM 🎉

bill3tt · 2022-01-24T17:35:42Z

test/e2e/rules_test.go

+		Transport: &tokenRoundTripper{rt: tr, token: token},
+	}
+
+	t.Run("read-write-recording-rules", func(t *testing.T) {


Sorry to be a pedant with this - but the actual flow is more write-then-read-recording-rules in the test happy path which would be the most accurate description. Not a blocker.

actually for this test case is more like read-then-write-then-read (because we read first, even though we have no rules submitted yet) but I'd prefer your suggestion too :)

bill3tt · 2022-01-24T17:37:00Z

test/e2e/rules_test.go

+		assertResponse(t, bodyStr, "tenant_id: "+defaultTenantID)
+	})
+
+	t.Run("read-write-alerting-rules", func(t *testing.T) {


Ditto - write-then-read-alerting-rules would be my first choice but not a blocker.

bill3tt · 2022-01-24T17:38:22Z

test/e2e/rules_test.go

+		)
+		testutil.Ok(t, err)
+		res, err := client.Do(r)
+		//TODO: an error/http status code is not being returned to the API


If we're not fixing this issue in this PR then please let's create a ticket and reference it so we don't loose sight of this problem :)

TODO (MON-XXXX) - an error/http...

sounds good, I've removed the test case for now and will create a ticket by EOD :)

Added a bug ticket here

This should get fixed by #218. 🙂

bill3tt · 2022-01-24T17:40:59Z

Also - we can see that your tests are passing in CI here!

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

matej-g

I think this is looking great, nice job @jessicalins 🎉

matej-g · 2022-01-25T09:27:53Z

test/e2e/configs.go


 	defaultTenantID = "1610b0c3-c509-4592-a256-a1871353dbfa"
 	mtlsTenantID    = "845cdfd9-f936-443c-979c-2ee7dc91f646"
+
+	defaultTenantName = "test-oidc"


👍 👍 👍

matej-g · 2022-01-25T09:30:01Z

test/e2e/rules_test.go

+		assertResponse(t, bodyStr, "alert: ManyInstancesDown")
+		assertResponse(t, bodyStr, "tenant_id: "+defaultTenantID)
+	})
+}


Nit: Missing newline

This reverts commit ccd9a59.

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

matej-g

I think we addressed everything, thanks again @jessicalins 🎉

jessicalins added 8 commits December 6, 2021 11:22

Start e2e test

e392176

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Fix flag, command

23cf4d3

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Fix s3 file

9b1403a

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Dynamically create yaml file

d536281

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Start to add test

3907da5

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Fix endpoint path

9e4bc8d

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Create rules_test, leaner yaml

9afa2be

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Finish first test cases

58502d2

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

matej-g reviewed Dec 20, 2021

View reviewed changes

jessicalins added 4 commits January 18, 2022 17:59

Merge branch 'main' into rules-add-e2e

6a91a73

Update test image, add more test cases

e0353b3

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Code review suggestions, add startServicesForRules function

be5c99f

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Update flag syntax

b51cc81

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

jessicalins marked this pull request as ready for review January 19, 2022 21:10

jessicalins commented Jan 19, 2022

View reviewed changes

bill3tt reviewed Jan 20, 2022

View reviewed changes

Format, rewrite test names

063a88e

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

saswatamcode reviewed Jan 21, 2022

View reviewed changes

jessicalins added 2 commits January 24, 2022 14:05

Check for tenant labels

7d50a67

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

Create defaultTenantName const

0b60005

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

jessicalins changed the title ~~Rules API: Start e2e test~~ Rules API: Add e2e test Jan 24, 2022

bill3tt previously approved these changes Jan 24, 2022

View reviewed changes

Rename test cases, remove write invalid rules case for now

c27ab99

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

jessicalins dismissed bill3tt’s stale review via c27ab99 January 24, 2022 17:50

Remove unused invalidRulesYamlTpl

ccd9a59

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

saswatamcode mentioned this pull request Jan 25, 2022

Propagate rules client status code #218

Merged

matej-g previously approved these changes Jan 25, 2022

View reviewed changes

jessicalins added 2 commits January 25, 2022 12:30

Revert "Remove unused invalidRulesYamlTpl"

ec05d23

This reverts commit ccd9a59.

Readd invalid rules test

79a0937

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

jessicalins dismissed matej-g’s stale review via 79a0937 January 25, 2022 15:46

jessicalins added 2 commits January 25, 2022 12:47

Merge branch 'main' into rules-add-e2e

01485e6

Add missing new line

7b543e0

Signed-off-by: Jéssica Lins <jessicaalins@gmail.com>

matej-g approved these changes Jan 26, 2022

View reviewed changes

matej-g merged commit e486b89 into observatorium:main Jan 26, 2022

jessicalins deleted the rules-add-e2e branch January 26, 2022 14:25

saswatamcode mentioned this pull request Jan 27, 2022

Enforce tenant labels on rule storage PUT request #219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rules API: Add e2e test #198

Rules API: Add e2e test #198

jessicalins commented Dec 17, 2021 •

edited

Loading

matej-g left a comment

matej-g Dec 20, 2021

jessicalins Jan 19, 2022

jessicalins Jan 19, 2022

bill3tt Jan 20, 2022

jessicalins Jan 20, 2022

jessicalins Jan 20, 2022

bill3tt Jan 24, 2022

jessicalins Jan 24, 2022

bill3tt Jan 20, 2022

jessicalins Jan 20, 2022

bill3tt Jan 20, 2022

jessicalins Jan 20, 2022

jessicalins Jan 24, 2022

saswatamcode Jan 21, 2022

jessicalins Jan 24, 2022

bill3tt left a comment

bill3tt Jan 24, 2022

jessicalins Jan 24, 2022

bill3tt Jan 24, 2022

bill3tt Jan 24, 2022

jessicalins Jan 24, 2022

jessicalins Jan 24, 2022

saswatamcode Jan 25, 2022

bill3tt commented Jan 24, 2022

matej-g left a comment

matej-g Jan 25, 2022

matej-g Jan 25, 2022

matej-g left a comment

Rules API: Add e2e test #198

Rules API: Add e2e test #198

Conversation

jessicalins commented Dec 17, 2021 • edited Loading

matej-g 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

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

bill3tt 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

Choose a reason for hiding this comment

bill3tt commented Jan 24, 2022

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

jessicalins commented Dec 17, 2021 •

edited

Loading