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.

Sign up for GitHub

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

Jump to bottom

Allow SPM to pass bearer token to Prometheus #4247

Closed
wants to merge 24 commits into from
Closed

Allow SPM to pass bearer token to Prometheus #4247

wants to merge 24 commits into from

Conversation

ChillOrb
Copy link
Contributor

@ChillOrb ChillOrb commented Feb 21, 2023 •
edited
Loading

@ChillOrb ChillOrb requested a review from a team as a code owner February 21, 2023 07:04
@ChillOrb ChillOrb changed the title Dev Allow SPM to pass bearer token to Prometheus #4239 Feb 21, 2023
@ChillOrb ChillOrb changed the title Allow SPM to pass bearer token to Prometheus #4239 Allow SPM to pass bearer token to Prometheus Feb 21, 2023
@ChillOrb
Copy link
Contributor Author

@albertteoh , please review when you have time. I had to fix my auto save settings which had shuffled some of the imports so had to recreate a new PR and could not reopen previous one .

albertteoh
albertteoh requested changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the feedback from the last PR, it looks much better! I've added some extra comments; mostly nits. I noticed the token propagation flag is a little unique, which I didn't realise until taking a closer look at the code.

Please also sign your commits.

plugin/metrics/prometheus/metricsstore/reader_test.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/metricsstore/reader_test.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/metricsstore/reader_test.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/options.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/options.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/options.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/options.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/factory_test.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/metricsstore/reader_test.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/metricsstore/reader_test.go Outdated Show resolved Hide resolved
dependabot bot and others added 7 commits February 21, 2023 22:08
@dependabot @yurishkuro @ChillOrb
Bump google.golang.org/grpc from 1.52.3 to 1.53.0 (jaegertracing#4223)
9cee6a6 
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from
1.52.3 to 1.53.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/grpc/grpc-go/releases">google.golang.org/grpc's
releases</a>.</em></p>
<blockquote>
<h2>Release 1.53.0</h2>
<h1>API Changes</h1>
<ul>
<li>balancer: support injection of per-call metadata from LB policies
(<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5853">#5853</a>)</li>
<li>resolver: remove deprecated field
<code>resolver.Target.Endpoint</code> and replace with
<code>resolver.Target.Endpoint()</code> (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5852">#5852</a>)
<ul>
<li>Special Thanks: <a
href="https://github.com/kylejb"><code>@​kylejb</code></a></li>
</ul>
</li>
</ul>
<h1>New Features</h1>
<ul>
<li>xds/ringhash: introduce <code>GRPC_RING_HASH_CAP</code> environment
variable to override the maximum ring size. (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5884">#5884</a>)</li>
<li>rls: propagate headers received in RLS response to backends (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5883">#5883</a>)</li>
</ul>
<h1>Bug Fixes</h1>
<ul>
<li>transport: drain client transport when streamID approaches
MaxStreamID (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5889">#5889</a>)</li>
<li>server: after GracefulStop, ensure connections are closed when final
RPC completes (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5968">#5968</a>)</li>
<li>server: fix a few issues where grpc server uses RST_STREAM for
non-HTTP/2 errors (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5893">#5893</a>)</li>
<li>xdsclient: fix race which can happen when multiple load reporting
calls are made at the same time. (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5927">#5927</a>)</li>
<li>rls: fix a data race involving the LRU cache (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5925">#5925</a>)</li>
<li>xds: fix panic involving double close of channel in xDS transport
(<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5959">#5959</a>)</li>
<li>gcp/observability: update method name validation (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5951">#5951</a>)</li>
</ul>
<h1>Documentation</h1>
<ul>
<li>credentials/oauth: mark <code>NewOauthAccess</code> as deprecated
(<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5882">#5882</a>)
<ul>
<li>Special Thanks: <a
href="https://github.com/buzzsurfr"><code>@​buzzsurfr</code></a></li>
</ul>
</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/grpc/grpc-go/commit/dba26e15a07f43875ccf806a2dd6cbcbc1c12eab"><code>dba26e1</code></a>
Change version to 1.53.0 (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5983">#5983</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/2a1e9348ff7b5d9f4b5039e84e6c9873b5b3e26e"><code>2a1e934</code></a>
server: after GracefulStop, ensure connections are closed when final RPC
comp...</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/e2d69aa076dd070e3668784c4dc8bcf7131b3f67"><code>e2d69aa</code></a>
tests: fix spelling of variable (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5966">#5966</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/a6376c9893f56fc3819bee9ef5d71f55cc2d38dd"><code>a6376c9</code></a>
xds/resolver: cleanup tests to use real xDS client 3/n (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5953">#5953</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/bf8fc46fa6eb913e4ed0f6dee6c6a7b75e85fbf0"><code>bf8fc46</code></a>
xds/resolver: cleanup tests to use real xDS client 5/n (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5955">#5955</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/3930549b38c0fc4cd94a95efccf7cef5f90515fd"><code>3930549</code></a>
resolver: replace resolver.Target.Endpoint field with Endpoint() method
(<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5852">#5852</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/894816c487f8dd48fc971c45a7c5baa4b86ef7de"><code>894816c</code></a>
grpclb: rename <code>grpclbstate</code> package back to
<code>state</code> (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5962">#5962</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/e5a0237a46a5f95fa571624929be10c7afebb180"><code>e5a0237</code></a>
encoding: fix duplicate compressor names (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5958">#5958</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/4adb2a7a00d8b62df5ea34d520fe3ca13bffd31a"><code>4adb2a7</code></a>
xds/resolver: cleanup tests to use real xDS client 2/n (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5952">#5952</a>)</li>
<li><a
href="https://github.com/grpc/grpc-go/commit/52a8392f374b8cd60e176b67925a7f8c1605d014"><code>52a8392</code></a>
gcp/observability: update method name validation (<a
href="https://github-redirect.dependabot.com/grpc/grpc-go/issues/5951">#5951</a>)</li>
<li>Additional commits viewable in <a
href="https://github.com/grpc/grpc-go/compare/v1.52.3...v1.53.0">compare
view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=google.golang.org/grpc&package-manager=go_modules&previous-version=1.52.3&new-version=1.53.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)

</details>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
revert to previous state
2e54cc6 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
refactoring as per requested changes
577bfb3 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
refactoring as per requested changes
80bd8e5 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
refactoring as per requested changes
2c11989 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
refactoring as per requested changes
7413144 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
refactoring as per requested changes
7088586 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb ChillOrb closed this Feb 22, 2023
ChillOrb and others added 8 commits February 21, 2023 22:22
@ChillOrb
replacing asser with require
0ffdde3 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
replacing asser with require
49bc635 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
setting allowfrom ctx false
92d44e4 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
Merge branch 'jaegertracing:main' into dev
b71b689
@ChillOrb
removing allow token from context
d2db27b 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
adding space test case in file path
73baec3 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
test case for read tls config
c2d0758 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
separating roundtrupper test cases
a04f09a 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb ChillOrb reopened this Mar 8, 2023
@ChillOrb
removing bad comments
7b1be1a 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
ChillOrb and others added 3 commits March 8, 2023 23:09
@ChillOrb
removing sllow token from context for SPM toekn reading from file
a17ea5b 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
Merge branch 'main' into dev
b41d3a0
@ChillOrb @albertteoh
Update plugin/metrics/prometheus/metricsstore/reader_test.go
70f137e 
Co-authored-by: Albert <26584478+albertteoh@users.noreply.github.com>
Signed-off-by: Rakshit Parashar <34675136+ChillOrb@users.noreply.github.com>
@ChillOrb ChillOrb requested review from albertteoh and removed request for joe-elliott March 9, 2023 07:14
albertteoh
albertteoh reviewed Mar 9, 2023
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

It's looking good, just some minor issues.

I also noticed a change in the jaeger-ui submodule. You'll probably want to revert this with:

git submodule deinit -f .
git submodule update --init

plugin/metrics/prometheus/factory_test.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/factory_test.go Outdated
})
require.NoError(t, err)
f.InitFromViper(v, zap.NewNop())
assert.Equal(t, f.options.Primary.TokenFilePath, "test/test_file.txt")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay, but what about the case where the user inputs "test/test file.txt"? I think the f.options.Primary.TokenFilePath should still stay as "test/test file.txt", because it's perfectly valid (but not advised) to have spaces in a filename (you can try it out in TestLoadToken).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh.. I am sorry i misunderstood your suggestion. I am modifying this test case to correctly depict required scenario.

Copy link
Contributor Author

@ChillOrb ChillOrb Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is okay, but what about the case where the user inputs "test/test file.txt"? I think the f.options.Primary.TokenFilePath should still stay as "test/test file.txt", because it's perfectly valid (but not advised) to have spaces in a filename (you can try it out in TestLoadToken).

I removed stripWhiteSpace() from cfg.TokenFilePath = v.GetString(cfg.namespace + suffixTokenFilePath) which fixed this edge case

plugin/metrics/prometheus/metricsstore/reader.go Outdated Show resolved Hide resolved
plugin/metrics/prometheus/metricsstore/reader.go Show resolved Hide resolved
plugin/metrics/prometheus/metricsstore/reader_test.go Outdated Show resolved Hide resolved
ChillOrb and others added 3 commits March 9, 2023 19:57
@ChillOrb @albertteoh
Update plugin/metrics/prometheus/metricsstore/reader.go
06fdb07 
Co-authored-by: Albert <26584478+albertteoh@users.noreply.github.com>
Signed-off-by: Rakshit Parashar <34675136+ChillOrb@users.noreply.github.com>
@ChillOrb
fixing seggestions for PR 4247
c13dc64 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb
Merge branch 'main' into dev
ba9ef18
@albertteoh
Copy link
Contributor

Please:

@ChillOrb ChillOrb closed this Mar 12, 2023
@ChillOrb ChillOrb reopened this Mar 12, 2023
@ChillOrb ChillOrb closed this Mar 12, 2023
@ChillOrb
reverting jaugar ui submodule changes
3fce1f0 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
@ChillOrb ChillOrb reopened this Mar 12, 2023
@ChillOrb ChillOrb closed this Mar 12, 2023
@ChillOrb ChillOrb mentioned this pull request Mar 12, 2023
Merged
ChillOrb added a commit to ChillOrb/jaeger that referenced this pull request Mar 12, 2023
@ChillOrb
adding changes from jaegertracing#4247 as new branch
7f60639 
Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
yurishkuro added a commit that referenced this pull request Mar 19, 2023
@ChillOrb @yurishkuro
Allow SPM to pass bearer token to Prometheus #4247 (#4295)
adfb97c 
## Which problem is this PR solving?
- Resolves #4219  

## Short description of the changes
- Follow up to #4247

---------

Signed-off-by: ChillOrb <rakshitparashar1@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertteoh albertteoh albertteoh requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: spm bearer token required for prometheus
2 participants
@ChillOrb @albertteoh