-
Notifications
You must be signed in to change notification settings - Fork 62
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
Rules API: Add e2e test #198
Changes from 12 commits
e392176
23cf4d3
9b1403a
d536281
3907da5
9e4bc8d
9afa2be
58502d2
6a91a73
e0353b3
be5c99f
b51cc81
063a88e
7d50a67
0b60005
c27ab99
ccd9a59
ec05d23
79a0937
01485e6
7b543e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
// +build integration | ||
|
||
package e2e | ||
|
||
import ( | ||
"bytes" | ||
"io/ioutil" | ||
"net/http" | ||
"testing" | ||
|
||
"github.com/efficientgo/e2e" | ||
"github.com/efficientgo/tools/core/pkg/testutil" | ||
) | ||
|
||
func TestRulesAPI(t *testing.T) { | ||
t.Parallel() | ||
|
||
e, err := e2e.NewDockerEnvironment(envRulesAPIName) | ||
testutil.Ok(t, err) | ||
t.Cleanup(e.Close) | ||
|
||
prepareConfigsAndCerts(t, rules, e) | ||
_, token, rateLimiterAddr := startBaseServices(t, e, rules) | ||
rulesEndpoint := startServicesForRules(t, e) | ||
|
||
api, err := newObservatoriumAPIService( | ||
e, | ||
withRulesEndpoint("http://"+rulesEndpoint), | ||
withRateLimiter(rateLimiterAddr), | ||
) | ||
testutil.Ok(t, err) | ||
testutil.Ok(t, e2e.StartAndWaitReady(api)) | ||
|
||
rulesEndpointURL := "https://" + api.Endpoint("https") + "/api/metrics/v1/test-oidc/api/v1/rules/raw" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point..we define it manually everywhere in the tests: https://github.com/observatorium/api/search?q=test-oidc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added as a constant here: 0b60005 |
||
tr := &http.Transport{ | ||
TLSClientConfig: getTLSClientConfig(t, e), | ||
} | ||
|
||
client := &http.Client{ | ||
Transport: &tokenRoundTripper{rt: tr, token: token}, | ||
} | ||
|
||
t.Run("get-put-recording-rules", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I renamed to |
||
// Try to list rules | ||
r, err := http.NewRequest( | ||
http.MethodGet, | ||
rulesEndpointURL, | ||
nil, | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err := client.Do(r) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusNotFound, res.StatusCode) | ||
|
||
// Set a file containing a recording rule | ||
recordingRule := []byte(recordingRuleYamlTpl) | ||
r, err = http.NewRequest( | ||
http.MethodPut, | ||
rulesEndpointURL, | ||
bytes.NewReader(recordingRule), | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err = client.Do(r) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) | ||
|
||
// Check if recording rule is listed | ||
r, err = http.NewRequest( | ||
http.MethodGet, | ||
rulesEndpointURL, | ||
nil, | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err = client.Do(r) | ||
defer res.Body.Close() | ||
|
||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) | ||
|
||
body, err := ioutil.ReadAll(res.Body) | ||
bodyStr := string(body) | ||
|
||
assertResponse(t, bodyStr, "sum by (job) (http_inprogress_requests)") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be a good idea to check for tenant labels here? 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea! added here: 7d50a67 |
||
}) | ||
|
||
t.Run("get-put-alerting-rules", func(t *testing.T) { | ||
// Set a file containing an alerting rule | ||
alertingRule := []byte(alertingRuleYamlTpl) | ||
r, err := http.NewRequest( | ||
http.MethodPut, | ||
rulesEndpointURL, | ||
bytes.NewReader(alertingRule), | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err := client.Do(r) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) | ||
|
||
// Check if the alerting rule is listed | ||
r, err = http.NewRequest( | ||
http.MethodGet, | ||
rulesEndpointURL, | ||
nil, | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err = client.Do(r) | ||
defer res.Body.Close() | ||
|
||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) | ||
|
||
body, err := ioutil.ReadAll(res.Body) | ||
bodyStr := string(body) | ||
assertResponse(t, bodyStr, "alert: HighRequestLatency") | ||
}) | ||
|
||
t.Run("get-put-recording-alerting-rules", func(t *testing.T) { | ||
// Set a file containing both recording and alerting rules | ||
recordAndAlertingRules := []byte(recordAndAlertingRulesYamlTpl) | ||
r, err := http.NewRequest( | ||
http.MethodPut, | ||
rulesEndpointURL, | ||
bytes.NewReader(recordAndAlertingRules), | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err := client.Do(r) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) | ||
|
||
// Check if both recording and alerting rules are listed | ||
r, err = http.NewRequest( | ||
http.MethodGet, | ||
rulesEndpointURL, | ||
nil, | ||
) | ||
testutil.Ok(t, err) | ||
|
||
res, err = client.Do(r) | ||
defer res.Body.Close() | ||
|
||
testutil.Ok(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) | ||
|
||
body, err := ioutil.ReadAll(res.Body) | ||
bodyStr := string(body) | ||
assertResponse(t, bodyStr, "record: job:up:avg") | ||
assertResponse(t, bodyStr, "alert: ManyInstancesDown") | ||
}) | ||
|
||
t.Run("put-invalid-rules", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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 |
||
// Set an invalid rules file | ||
invalidRules := []byte(invalidRulesYamlTpl) | ||
r, err := http.NewRequest( | ||
http.MethodPut, | ||
rulesEndpointURL, | ||
bytes.NewReader(invalidRules), | ||
) | ||
testutil.Ok(t, err) | ||
res, err := client.Do(r) | ||
//TODO: an error/http status code is not being returned to the API | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good, I've removed the test case for now and will create a ticket by EOD :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a bug ticket here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should get fixed by #218. 🙂 |
||
//testutil.NotOk(t, err) | ||
testutil.Equals(t, http.StatusOK, res.StatusCode) // should this be http.StatusBadRequest instead? (from: https://github.com/observatorium/rules-objstore/blob/main/pkg/server/server.go#L80) | ||
}) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Missing newline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make format
should fix these rogue whitespaces 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weirdly if I run
make format
locally I get: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 thegolangci-lint
version (with the latest version I get also some warnings of linters that are deprecated and we use them in thenolinter
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 fixedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, shouldn't the lint step fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be fixed now 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean the whitespaces or
make format
?To fix the whitespaces I ran
gofmt
manually, but I thought would be fixed by runningmake format
instead (which does not seem to be the case) :(