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

Add kube-rbac-proxy for alerts/rules in front of Thanos querier #736

Merged
merged 4 commits into from Jul 4, 2020
Merged

Add kube-rbac-proxy for alerts/rules in front of Thanos querier #736

merged 4 commits into from Jul 4, 2020

Conversation

simonpasquier
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2020
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from a4ac20f to d7b8427 Compare April 3, 2020 09:28
@simonpasquier
Copy link
Contributor Author

/test generate

@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from d7b8427 to 1fec143 Compare April 3, 2020 13:42
@simonpasquier simonpasquier changed the title [WIP] Add kube-rbac-proxy for alerts/rules in front of Thanos querier Add kube-rbac-proxy for alerts/rules in front of Thanos querier Apr 6, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch 2 times, most recently from feef6d9 to 3f8d65f Compare April 7, 2020 15:33
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from 3f8d65f to 1f6852d Compare April 7, 2020 15:41
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@simonpasquier
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2020
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch 2 times, most recently from 518f604 to 4ef8d36 Compare April 9, 2020 08:47
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from 4ef8d36 to e047fcb Compare April 15, 2020 08:33
@simonpasquier
Copy link
Contributor Author

/test e2e-aws-operator

@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from e047fcb to d0ab68a Compare April 16, 2020 14:25
@simonpasquier
Copy link
Contributor Author

/retest e2e-aws-operator

@openshift-ci-robot
Copy link
Contributor

@simonpasquier: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

  • /test e2e-aws
  • /test e2e-aws-operator
  • /test e2e-aws-upgrade
  • /test generate
  • /test images
  • /test shellcheck
  • /test unit
  • /test vendor

Use /test all to run all jobs.

In response to this:

/retest e2e-aws-operator

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch 2 times, most recently from d48b744 to 60d8a13 Compare April 24, 2020 07:23
@simonpasquier simonpasquier changed the base branch from master to master-4.6 May 11, 2020 12:18
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from 60d8a13 to 9a1802f Compare May 11, 2020 12:18
@paulfantom
Copy link
Contributor

It needs a rebase, but lgtm now.

@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from 9a1802f to 1fd9cef Compare June 5, 2020 07:34
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2020
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from 5ec419d to 7ebb5e4 Compare June 29, 2020 12:50
@simonpasquier
test/e2e: add test for rules tenancy
1144e13 
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier
Update CHANGELOG.md
ac6eb3a
@simonpasquier simonpasquier force-pushed the add-rbac-and-label-proxy-to-querier branch from 7ebb5e4 to ac6eb3a Compare June 29, 2020 15:16
@paulfantom
Copy link
Contributor

Tests seemed flaky
/retest

@simonpasquier
Copy link
Contributor Author

/test vendor

@simonpasquier
Copy link
Contributor Author

/retest

@simonpasquier
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2020
@simonpasquier
Copy link
Contributor Author

@openshift/openshift-team-monitoring this is ready for review at least.

@brancz
Copy link
Contributor

brancz commented Jul 1, 2020

/lgtm

but giving others a chance to review as well as this is a rather large PR

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2020
s-urbaniak
s-urbaniak reviewed Jul 3, 2020
View reviewed changes
pkg/manifests/manifests.go
THANOS_QUERIER_CONTAINER_KUBE_RBAC_PROXY = 2
THANOS_QUERIER_CONTAINER_PROM_LABEL_PROXY = 3
)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

s-urbaniak
s-urbaniak reviewed Jul 3, 2020
View reviewed changes
test/e2e/user_workload_monitoring_test.go
@@ -61,7 +63,8 @@ func TestUserWorkloadMonitoring(t *testing.T) {
{"create prometheus and alertmanager in user namespace", createPrometheusAlertmanagerInUserNamespace},
{"assert user workload metrics", assertUserWorkloadMetrics},
{"assert user workload rules", assertUserWorkloadRules},
{"assert tenancy model is enforced", assertTenancyForMetrics},
{"assert tenancy model is enforced for metrics", assertTenancyForMetrics},
{"assert tenancy model is enforced for rules", assertTenancyForRules},
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to add another bullet point in https://issues.redhat.com/browse/MON-1170 to separate out the e2e tenancy test for user workload monitoring into its own e2e scenario?
(not an issue we have to address in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since tenancy tests require metrics and alerts in the first place, I would assume that they can be included in the user workload metric tests and alerts (respectively).

s-urbaniak
s-urbaniak reviewed Jul 3, 2020
View reviewed changes
test/e2e/user_workload_monitoring_test.go
},
)

t.Logf("Retrieving rules")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: test logs will not be emitted at the time of invocation, but after the test, hence imho this could be removed, but can be done as a follow-up too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most probably a left-over piece. I'll remove it.

@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, s-urbaniak, simonpasquier

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,s-urbaniak,simonpasquier]

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

@simonpasquier
Copy link
Contributor Author

/unhold

I'll address the remaining concerns in a follow-up PR

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2020
@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.

@openshift-merge-robot openshift-merge-robot merged commit 57d3e97 into openshift:master Jul 4, 2020
@simonpasquier simonpasquier deleted the add-rbac-and-label-proxy-to-querier branch December 7, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@paulfantom paulfantom Awaiting requested review from paulfantom

@squat squat Awaiting requested review from squat

Assignees

@s-urbaniak s-urbaniak

@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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@simonpasquier @openshift-ci-robot @paulfantom @lilic @brancz @s-urbaniak @openshift-bot @openshift-merge-robot