-
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 8 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,123 @@ | ||
// +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, _ := startServicesForMetrics(t, e) // TODO: create another function just for rules? | ||
jessicalins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
api, err := newObservatoriumAPIService( | ||
e, | ||
withMetricsEndpoints("", "", "http://"+rulesEndpoint), | ||
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 think we might need to provide some write/read endpoint, otherwise API will complain that neither metrics nor logs endpoints are provided? 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 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 |
||
withRateLimiter(rateLimiterAddr), | ||
) | ||
testutil.Ok(t, err) | ||
testutil.Ok(t, e2e.StartAndWaitReady(api)) | ||
|
||
t.Run("rules", func(t *testing.T) { | ||
rulesEndpointURL := "https://" + api.Endpoint("https") + "/api/metrics/v1/test-oidc/api/v1/rules/raw" | ||
tr := &http.Transport{ | ||
TLSClientConfig: getTLSClientConfig(t, e), | ||
} | ||
|
||
client := &http.Client{ | ||
Transport: &tokenRoundTripper{rt: tr, token: token}, | ||
} | ||
|
||
// 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, res.StatusCode, http.StatusNotFound) | ||
|
||
// Set 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, res.StatusCode, http.StatusOK) | ||
|
||
// 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, res.StatusCode, http.StatusOK) | ||
|
||
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 |
||
|
||
// Set 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, res.StatusCode, http.StatusOK) | ||
|
||
// Check if recording rule and alerting rule 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, res.StatusCode, http.StatusOK) | ||
|
||
body, err = ioutil.ReadAll(res.Body) | ||
bodyStr = string(body) | ||
//TODO: check why this fails - shouldn't it join recording+alerting rules? | ||
//assertResponse(t, bodyStr, "sum by (job) (http_inprogress_requests)") | ||
assertResponse(t, bodyStr, "alert: HighRequestLatency") | ||
|
||
// TODO: add another test case for thanos-ruler-syncer flow | ||
}) | ||
} | ||
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) :(