Allow SPM to pass bearer token to Prometheus #4239
Conversation
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
| @@ -16,8 +16,10 @@ | |||
|
|
|||
| package mocks | |||
|
|
|||
| import mock "github.com/stretchr/testify/mock" | |||
| import processor "github.com/jaegertracing/jaeger/cmd/ingester/app/processor" | |||
| import ( | |||
There was a problem hiding this comment.
Please revert any changes to mocks; they're generated code, so if we regenerate them again, it will revert these changes.
| @@ -33,7 +33,6 @@ var frontendCmd = &cobra.Command{ | |||
| Short: "Starts Frontend service", | |||
| Long: `Starts Frontend service.`, | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
|
|
|||
There was a problem hiding this comment.
I suggest reverting any changes not related to the problem being solved by this PR. So for example, I don't think any changes should be made in:
examples/pkg/cassandrapkg/clientcfgpkg/es
| "strings" | ||
| "time" | ||
| "unicode" | ||
|
|
||
| "github.com/opentracing/opentracing-go" | ||
|
|
There was a problem hiding this comment.
nit: the import grouping convention used in jaeger is:
- stdlib
- 3rd party
- local
This applies to other files in this PR as well.
| TokenFilePath string | ||
| AllowTokenFromContext bool |
There was a problem hiding this comment.
These fields don't need to be exported.
Also, as suggested below, I propose to have a wantBearer field.
| TokenFilePath string | |
| AllowTokenFromContext bool | |
| tokenFilePath string | |
| allowTokenFromContext bool | |
| wantBearer string |
| if tc.TokenFilePath != "" { | ||
| dir, file := filepath.Split(tc.TokenFilePath) | ||
| err := os.MkdirAll(dir, 0o750) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
It's better to use require here because executing anything else below wouldn't be valid if there is an error.
| assert.NoError(t, err) | |
| require.NoError(t, err) |
| TLS: tlscfg.Options{ | ||
| Enabled: tc.tlsEnabled, | ||
| }, | ||
| }, logger) | ||
| require.NoError(t, err) | ||
|
|
||
| server := httptest.NewServer( | ||
| server1 := httptest.NewServer( |
There was a problem hiding this comment.
We can simplify the rest of this test body:
- there shouldn't be a need for two http servers, just one will do, which uses a new
wantBearertest case attribute. - we just need to create a new context if
tc.AllowTokenFromContext. Note that before, the context was added when bothtc.AllowTokenFromContext = trueandtc.AllowTokenFromContext = falsewhich meant the token loaded from file was not tested! 😄
e.g.
server := httptest.NewServer(
http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "Bearer "+tc.wantBearer, r.Header.Get("Authorization"))
},
),
)
defer server.Close()
ctx := context.Background()
if tc.AllowTokenFromContext {
ctx = bearertoken.ContextWithBearerToken(ctx, "token from context")
}
req, err := http.NewRequestWithContext(
ctx,
http.MethodGet,
server.URL,
nil,
)
require.NoError(t, err)
resp, err := rt.RoundTrip(req)
require.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
| }{ | ||
| {"tls tlsEnabled", true}, | ||
| {"tls disabled", false}, | ||
| {"tls enabled with token from file", true, "testdir/test_file.txt", false}, |
There was a problem hiding this comment.
Given there are more fields per test, it's getting hard to read. Can each field be placed on a separate line along with the field key e.g.:
{
name: "tls enabled with token from file",
tlsEnabled: true,
tokenFilePath: "testdir/test_file.txt",
allowTokenFromContext: false,
wantBearer: "token from context",
},
| {"tls disabled", false}, | ||
| {"tls enabled with token from file", true, "testdir/test_file.txt", false}, | ||
| {"tls enabled with token from context", true, "", true}, | ||
| {"tls enabled with token from file", true, "testdir/test_file.txt", true}, |
There was a problem hiding this comment.
The descriptions are duplicated, can they be more descriptive and differentiated? For example, for this test case:
tls enabled with token from file with allowTokenFromContext should prefer using token from context
| if token != "" || c.AllowTokenFromContext { | ||
| transport = bearertoken.RoundTripper{ | ||
| Transport: httpTransport, | ||
| OverrideFromCtx: c.AllowTokenFromContext, | ||
| StaticToken: token, | ||
| } | ||
| } | ||
|
|
||
| return transport, nil |
There was a problem hiding this comment.
I think we can simplify this to:
return bearertoken.RoundTripper{
Transport: httpTransport,
OverrideFromCtx: c.AllowTokenFromContext,
StaticToken: token,
}, nil
Also, the above suggestion should fix a bug where it's possible for transport to not be set if token == "", which means we lose our httpTransport configuration, including TLS configuration (it would be nice if there's a way to test TLS enablement).
|
Closing PR , since more work is required before we can resolve the issue as discussed here |
Resolves #4219
Short description of the changes