Skip to content
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

Add manual implementation of APIv3 HTTP endpoints #5054

Merged
merged 16 commits into from
Dec 30, 2023
8 changes: 8 additions & 0 deletions .github/workflows/ci-unit-tests-go-tip.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,17 @@ jobs:
- name: Install Go Tip
uses: ./.github/actions/setup-go-tip

- name: Install test deps
# even though the same target runs from test-ci, running it separately makes for cleaner log in GH workflow
run: make install-test-tools

- name: Run unit tests
run: make test-ci

- name: Prepare unit tests report
if: always()
run: make test-report

- name: Publish Unit Test Summary 📑
uses: EnricoMi/publish-unit-test-result-action@v2
if: always()
Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/ci-unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,17 @@ jobs:
go-version: 1.21.x
cache-dependency-path: ./go.sum

- name: Install test deps
# even though the same target runs from test-ci, running it separately makes for cleaner log in GH workflow
run: make install-test-tools

- name: Run unit tests
run: make test-ci

- name: Prepare unit tests report
if: always()
run: make test-report

- name: Publish Unit Test Summary 📑
uses: EnricoMi/publish-unit-test-result-action@v2
if: always()
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ install-ci: install-test-tools install-build-tools

.PHONY: test-ci
test-ci: GOTEST := $(GOTEST_QUIET) -json
test-ci: install-test-tools build-examples cover test-report
test-ci: install-test-tools build-examples cover

.PHONY: test-report
test-report:
Expand Down
10 changes: 9 additions & 1 deletion cmd/query/app/apiv3/grpc_gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,15 @@ import (
)

// RegisterGRPCGateway registers api_v3 endpoints into provided mux.
func RegisterGRPCGateway(ctx context.Context, logger *zap.Logger, r *mux.Router, basePath string, grpcEndpoint string, grpcTLS *tlscfg.Options, tm *tenancy.Manager) error {
func RegisterGRPCGateway(
ctx context.Context,
logger *zap.Logger,
r *mux.Router,
basePath string,
grpcEndpoint string,
grpcTLS *tlscfg.Options,
tm *tenancy.Manager,
) error {
grpcEndpoint = netutils.FixLocalhost([]string{grpcEndpoint})[0]
jsonpb := &runtime.JSONPb{}

Expand Down
60 changes: 28 additions & 32 deletions cmd/query/app/apiv3/grpc_gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,16 @@ import (
"io"
"net"
"net/http"
"net/url"
"os"
"path"
"path/filepath"
"strings"
"testing"
"time"

gogojsonpb "github.com/gogo/protobuf/jsonpb"
gogoproto "github.com/gogo/protobuf/proto"
"github.com/gorilla/mux"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"google.golang.org/grpc"
Expand All @@ -60,9 +58,13 @@ const (
// REGENERATE_SNAPSHOTS=true go test -v ./cmd/query/app/apiv3/...
var regenerateSnapshots = os.Getenv("REGENERATE_SNAPSHOTS") == "true"

// The tests in http_gateway_test.go set this to true to use manual gateway implementation.
var useHTTPGateway = false
Copy link
Member Author

@yurishkuro yurishkuro Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the cleanest way, but hopefully we'll just delete grpc-gateway based implementation soon and simplify/consolidate the tests


type testGateway struct {
reader *spanstoremocks.Reader
url string
router *mux.Router
}

type gatewayRequest struct {
Expand All @@ -76,6 +78,9 @@ func setupGRPCGateway(
serverTLS, clientTLS *tlscfg.Options,
tenancyOptions tenancy.Options,
) *testGateway {
if useHTTPGateway {
return setupHTTPGateway(t, basePath, serverTLS, clientTLS, tenancyOptions)
}
gw := &testGateway{
reader: &spanstoremocks.Reader{},
}
Expand Down Expand Up @@ -159,11 +164,12 @@ func (gw *testGateway) execRequest(t *testing.T, gwReq *gatewayRequest) ([]byte,
func verifySnapshot(t *testing.T, body []byte) []byte {
// reformat JSON body with indentation, to make diffing easier
var data interface{}
require.NoError(t, json.Unmarshal(body, &data))
require.NoError(t, json.Unmarshal(body, &data), "response: %s", string(body))
body, err := json.MarshalIndent(data, "", " ")
require.NoError(t, err)

snapshotFile := filepath.Join(snapshotLocation, strings.ReplaceAll(t.Name(), "/", "_")+".json")
testName := path.Base(t.Name())
snapshotFile := filepath.Join(snapshotLocation, testName+".json")
if regenerateSnapshots {
os.WriteFile(snapshotFile, body, 0o644)
}
Expand All @@ -177,17 +183,6 @@ func parseResponse(t *testing.T, body []byte, obj gogoproto.Message) {
require.NoError(t, gogojsonpb.Unmarshal(bytes.NewBuffer(body), obj))
}

func parseChunkResponse(t *testing.T, body []byte, obj gogoproto.Message) {
// Unwrap the 'result' container generated by the gateway.
// See https://github.com/grpc-ecosystem/grpc-gateway/issues/2189
type resultWrapper struct {
Result json.RawMessage `json:"result"`
Copy link
Member Author

@yurishkuro yurishkuro Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid this ugliness use the new helper types from IDL and marshal in one go.

}
var result resultWrapper
require.NoError(t, json.Unmarshal(body, &result))
parseResponse(t, result.Result, obj)
}

func makeTestTrace() (*model.Trace, model.TraceID) {
traceID := model.NewTraceID(150, 160)
return &model.Trace{
Expand Down Expand Up @@ -282,36 +277,35 @@ func runGatewayGetTrace(t *testing.T, gw *testGateway, setupRequest func(*http.R
require.Equal(t, http.StatusOK, statusCode, "response=%s", string(body))
body = verifySnapshot(t, body)

var spansResponse api_v3.SpansResponseChunk
parseChunkResponse(t, body, &spansResponse)
var response api_v3.GRPCGatewayWrapper
parseResponse(t, body, &response)

assert.Len(t, spansResponse.GetResourceSpans(), 1)
assert.Equal(t, bytesOfTraceID(t, traceID.High, traceID.Low), spansResponse.GetResourceSpans()[0].GetScopeSpans()[0].GetSpans()[0].GetTraceId())
assert.Len(t, response.Result.ResourceSpans, 1)
assert.Equal(t,
bytesOfTraceID(t, traceID.High, traceID.Low),
response.Result.ResourceSpans[0].ScopeSpans[0].Spans[0].TraceId)
}

func runGatewayFindTraces(t *testing.T, gw *testGateway, setupRequest func(*http.Request)) {
trace, traceID := makeTestTrace()
q, qp := mockFindQueries()
gw.reader.
On("FindTraces", matchContext, mock.AnythingOfType("*spanstore.TraceQueryParameters")).
On("FindTraces", matchContext, qp).
Return([]*model.Trace{trace}, nil).Once()

q := url.Values{}
q.Set("query.service_name", "foobar")
q.Set("query.start_time_min", time.Now().Format(time.RFC3339))
q.Set("query.start_time_max", time.Now().Format(time.RFC3339))

body, statusCode := gw.execRequest(t, &gatewayRequest{
url: "/api/v3/traces?" + q.Encode(),
setupRequest: setupRequest,
})
require.Equal(t, http.StatusOK, statusCode, "response=%s", string(body))
body = verifySnapshot(t, body)

var spansResponse api_v3.SpansResponseChunk
parseChunkResponse(t, body, &spansResponse)
var response api_v3.GRPCGatewayWrapper
parseResponse(t, body, &response)

assert.Len(t, spansResponse.GetResourceSpans(), 1)
assert.Equal(t, bytesOfTraceID(t, traceID.High, traceID.Low), spansResponse.GetResourceSpans()[0].GetScopeSpans()[0].GetSpans()[0].GetTraceId())
assert.Len(t, response.Result.ResourceSpans, 1)
assert.Equal(t,
bytesOfTraceID(t, traceID.High, traceID.Low),
response.Result.ResourceSpans[0].ScopeSpans[0].Spans[0].TraceId)
}

func bytesOfTraceID(t *testing.T, high, low uint64) []byte {
Expand Down Expand Up @@ -382,8 +376,10 @@ func TestGRPCGatewayTenancyRejection(t *testing.T) {
// We don't set tenant header
response, err := http.DefaultClient.Do(req)
require.NoError(t, err)
body, err := io.ReadAll(response.Body)
require.NoError(t, err)
require.NoError(t, response.Body.Close())
require.Equal(t, http.StatusForbidden, response.StatusCode)
require.Equal(t, http.StatusUnauthorized, response.StatusCode, "response=%s", string(body))

// Try again with tenant header set
tm := tenancy.NewManager(&tenancyOptions)
Expand Down
Loading
Loading