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

[MON-1695] expose /api/v1/labels end point for Thanos query. #1299

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

raptorsun
Copy link
Contributor

@raptorsun raptorsun commented Jul 25, 2021

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

Expose /api/v1/labels end point for Thanos query.
End point for /api/v1/label//values is added but not working yet, need to update kube-rbac-proxy to support at least trailing wildcard patterns.

This PR requires the 2 PRs below done before merge, and they are merged now:

  1. This PR is merged for allowing trailing wildcard in kube-rbac-proxy --allow-paths .
  2. This PR is merged for version bump to 0.11.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2021
Copy link
Contributor

@philipgough philipgough 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2021
@philipgough
Copy link
Contributor

/hold

do we want to wait for the kube-rbac-proxy change before testing/merging?

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2021
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2021
@raptorsun
Copy link
Contributor Author

/hold

do we want to wait for the kube-rbac-proxy change before testing/merging?

yes, let's hold it before modifying kube-rbac-proxy.

@raptorsun
Copy link
Contributor Author

This PR is created for allowing trailing wildcard in kube-rbac-proxy --allow-paths .

@raptorsun
Copy link
Contributor Author

/retest

@raptorsun
Copy link
Contributor Author

/retest

@raptorsun
Copy link
Contributor Author

now we are waiting for a new release from kube-rbac-proxy that includes This PR

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2021
@@ -493,6 +493,7 @@ function(params)
'--insecure-listen-address=127.0.0.1:9095',
'--upstream=http://127.0.0.1:9090',
'--label=namespace',
'-enable-label-apis',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'-enable-label-apis',
'--enable-label-apis',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thanks 👍

@raptorsun raptorsun force-pushed the feature/MON-1695 branch 2 times, most recently from 136aa84 to 84cd17b Compare August 4, 2021 16:04
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021
@paulfantom
Copy link
Contributor

jsonnet/thanos-querier.libsonnet was moved to https://github.com/openshift/cluster-monitoring-operator/blob/master/jsonnet/components/thanos-querier.libsonnet

@raptorsun
Copy link
Contributor Author

/retest-required

t.Fatal(err)
}

// Grant enough permissions to read labels. todo: check if need create new role
Copy link
Contributor

Choose a reason for hiding this comment

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

The labels endpoints are similar to the query endpoints: if you are authorized for /api/v1/query, you should also be for /api/v1/labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification. The test for labels will use the same role as that for query.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove the todo: ... part (or address it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup. already checked: no need for another account


t.Logf("Checking all labels")

// check /api/v1/labels end point
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// check /api/v1/labels end point
// check /api/v1/labels endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

still need fixing :)

}

// check /api/v1/label/<label>/value using some examples
for _, label := range []string{"endpoint", "event", "node"} {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would check that the namespace label returns only 1 item (e.g. userWorkloadTestNs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will modify the test accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test has been updated to include the namespace label.

@juzhao
Copy link
Contributor

juzhao commented Sep 16, 2021

can get /api/v1/labels and /api/v1/label/$({label_name}/values for user project, example

# curl -k -H "Authorization: Bearer $token" 'https://thanos-querier.openshift-monitoring.svc:9092/api/v1/labels?namespace=ns1' | jq
{
  "status": "success",
  "data": [
    "__name__",
    "alertname",
    "alertstate",
    "cluster_ip",
    "condition",
    "configmap",
    "container",
    "container_id",
    "cpu",
    "created_by_kind",
    "created_by_name",
    "deployment",
    "device",
    "endpoint",
    "host_ip",
    "host_network",
    "id",
...
}

# curl -k -H "Authorization: Bearer $token" 'https://thanos-querier.openshift-monitoring.svc:9092/api/v1/label/alertname/values?namespace=ns1'  | jq
{
  "status": "success",
  "data": [
    "TestAlert"
  ]
}

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 16, 2021
@raptorsun
Copy link
Contributor Author

/retest-required

@raptorsun
Copy link
Contributor Author

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Sep 24, 2021
t.Fatalf("failed to query labels from Thanos querier: %v", err)
}

// check /api/v1/label/<label>/value using some examples
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that checking /api/v1/label/namespace/values is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests for other labels have been removed.

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

/hold

still some comments to be addressed.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021
@raptorsun
Copy link
Contributor Author

/hold

still some comments to be addressed.

E2E test has been updated according to comments.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 24, 2021

@raptorsun: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/versions 14bf8bc link false /test versions
ci/prow/e2e-aws-single-node 14bf8bc link false /test e2e-aws-single-node

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@raptorsun
Copy link
Contributor Author

/retest-required

@simonpasquier
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, PhilipGough, raptorsun, 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 [PhilipGough,jan--f,raptorsun,simonpasquier]

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

@raptorsun
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021
@openshift-bot
Copy link
Contributor

/retest-required

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@raptorsun
Copy link
Contributor Author

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Sep 27, 2021
@openshift-bot
Copy link
Contributor

/retest-required

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants