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

cmd/telemeter-server,pkg/cache: cache auth resps #273

Merged
merged 4 commits into from Dec 11, 2019
Merged

cmd/telemeter-server,pkg/cache: cache auth resps #273

merged 4 commits into from Dec 11, 2019

Conversation

squat
Copy link
Contributor

@squat squat commented Dec 10, 2019

This commit adds new functionality to Telemeter server to enable it to
cache the responses to authentication requests made while handling
remote-write requests. The only currently implemented backend is
Memcached, because it is so simple, but any shared k-v would serve our
purposes.

cc @bwplotka @aditya-konarde @brancz @kakkoyun

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 10, 2019
@squat
vendor: revendor
18dfe23 
Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
@squat
Copy link
Contributor Author

squat commented Dec 10, 2019

/retest

squat added a commit to squat/configuration that referenced this pull request Dec 10, 2019
@squat
environments: add basic memcached manifests
dd58910 
This commit is a follow up to
openshift/telemeter#273.

Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
@squat squat mentioned this pull request Dec 10, 2019
Closed
@lilic
Copy link
Contributor

lilic commented Dec 10, 2019

/retest

@brancz
Copy link
Contributor

brancz commented Dec 10, 2019

/lgtm

Impressed how fast you threw this out there. Nice!

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019
metalmatze
metalmatze reviewed Dec 10, 2019
View reviewed changes
Copy link
Contributor

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

We should add some end to end tests eventually

cmd/telemeter-server/main.go Show resolved Hide resolved
@squat
Copy link
Contributor Author

squat commented Dec 10, 2019

and totally agree about the e2e tests. Started getting into the weeds of it last night. We need:

  • obsv/up generating metrics
  • telemeter ingesting
  • telemeter auth as a stub authorizer
  • memcached for caching
  • thanos receive ingesting
  • thanos query for testing that metric made it through

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@squat
Copy link
Contributor Author

squat commented Dec 10, 2019

/test benchmark
/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019
metalmatze
metalmatze approved these changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019
bwplotka
bwplotka approved these changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Some suggestions otherwise LGTM (:

Tests would be nice, but let's have cached auth now straight away.

pkg/authorize/handler.go Show resolved Hide resolved
pkg/authorize/tollbooth/tollbooth.go Show resolved Hide resolved
pkg/cache/cache.go Outdated Show resolved Hide resolved
pkg/cache/cache.go Outdated Show resolved Hide resolved
pkg/cache/memcached/memcached.go Outdated Show resolved Hide resolved
@squat
cmd/telemeter-server,pkg/cache: cache auth resps
1cfc4ca 
This commit adds new functionality to Telemeter server to enable it to
cache the responses to authentication requests made while handling
remote-write requests. The only currently implemented backend is
Memcached, because it is so simple, but any shared k-v would serve our
purposes.

Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
@squat
jsonnet/telemeter: add Memcached manifests
479602b 
This commit adds Jsonnet code to generate Kubernetes manifests for the
Memcached instances used by Telemeter server for caching auth responses.
Generation of these manifests can be toggled off by setting the replica
count to `0`.

Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
@squat
test/integration: add v2 integration test
7d9ae00 
Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019
@squat
Copy link
Contributor Author

squat commented Dec 10, 2019

/retest

4 similar comments
@squat
Copy link
Contributor Author

squat commented Dec 10, 2019

/retest

@squat
Copy link
Contributor Author

squat commented Dec 10, 2019

/retest

@squat
Copy link
Contributor Author

squat commented Dec 11, 2019

/retest

@squat
Copy link
Contributor Author

squat commented Dec 11, 2019

/retest

@metalmatze
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, bwplotka, metalmatze, squat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brancz,metalmatze,squat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 0e8dbb2 into openshift:master Dec 11, 2019
@squat squat deleted the remotewriteauthcache branch December 11, 2019 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

@metalmatze metalmatze metalmatze approved these changes

@lilic lilic Awaiting requested review from lilic

Assignees

@metalmatze metalmatze

@brancz brancz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@squat @lilic @brancz @openshift-bot @metalmatze @openshift-ci-robot @bwplotka @openshift-merge-robot