feat: support secrets in scrape authorization #776

TheSpiritXIII · 2024-01-22T16:56:20Z

Fixes #450 and #241.

This PR adds the following types to PodMonitoring/ClusterPodMonitoring's ScrapeEndpoint:

scrapeEndpoint.tls.ca
scrapeEndpoint.tls.cert
scrapeEndpoint.tls.key
scrapeEndpoint.oauth2.clientSecret
scrapeEndpoint.auth.credentials
scrapeEndpoint.basicAuth.password

These are all the same object pointing to a Kubernetes Secret with these fields:

secret.name
secret.key
secret.namespace

bwplotka

Epic work!

LGTM, modulo small comments (some of them non-blocking when marked so). Thanks!

During my testing I noticed we could improve error handling for permissions on our Prom fork--added issue for that, but the consequences are not as bad as I thought. 💪🏽

bwplotka · 2024-03-27T08:06:19Z

e2e/authorization_test.go

+	t.Run("patch-example-app-args", testPatchExampleAppArgs(ctx, kubeClient, []string{
+		"--tls-create-self-signed=true",
+	}))
+	authorizationClusterPodMonitoringTest(ctx, t, kubeClient, opClient, "tls",


nit(non-blocking): There might be opportunity to make those table tests at some point: TestTLSPodMonitoring and TestTLSClusterPodMonitoring is only different by one thing - first is invoking authorizationClusterPodMonitoringTest second authorizationPodMonitoringTest

Similar to other tests for diff security types.

Another argument for table test is authorizationPodMonitoringTest (and cluster one) has a few arguments where you really have understand order and meaning of them. Table test would allow describing scenarios in explicit struct perhaps.

e2e/authorization_test.go

pkg/operator/apis/monitoring/v1/http_types.go

bwplotka · 2024-03-27T09:05:14Z

Proposed adding example #916

Some decision rationales: * Only ClusterSecretKeySelector for consistency and explicitness. We could rename it to SecretKeySelector I guess. * Credentials and others etc are optional for compatibility AND name is similar to e.g. secretKeyRef in https://pkg.go.dev/k8s.io/api/core/v1#EnvVarSource It feels cleaner this way and allows extensions if needed (e.g. credentials from some different secret provider). It could be *SecretKeyRef or *Ref or *SecretRef suffix but chosen something that reflects it's not a string, but also not too verbose. * Flattened the structure, make it a pointer, for simplicity. * Detached low level types from scrape in comments (it's generic HTTP) * One method for accessing things (and adding used secrets) for smaller boilerplate --------- Signed-off-by: bwplotka <bwplotka@google.com>

e2e/authorization_test.go

bwplotka

Thanks! 💪🏽 Let's go!

mpskovvang · 2024-04-30T20:20:26Z

Thrilled about the new feature! Could you possibly share when it's expected to be available on GKE? Much appreciated!

TheSpiritXIII · 2024-04-30T20:26:49Z

Thrilled about the new feature! Could you possibly share when it's expected to be available on GKE? Much appreciated!

The feature is already available on the GKE rapid release channel 1.29.3-gke.1113000 and above. We expect to port this change to the regular release channel sometime before the end of Q2.

For an example configuration, see: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/main/examples/instrumentation/go-synthetic/go-synthetic-basic-auth.yaml

For full usage, see: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/main/doc/api.md

Feel free to report any issues you see as a new bug on GitHub. Thanks for the interest!

mpskovvang · 2024-04-30T22:33:02Z

Feel free to report any issues you see as a new bug on GitHub. Thanks for the interest!

Thanks for the quick reply. I've successfully upgraded to 1.29.3-gke.1282000.

Unfortunately, I'm having an issue with namespaces:

invalid definition for endpoint with index 0: unable to parse or invalid Prometheus HTTP client config: must use namespace "ecommerce", got: "default"

If I omit the authorization the request succeeds.

apiVersion: monitoring.googleapis.com/v1
kind: PodMonitoring
metadata:
  name: meilisearch
  namespace: ecommerce
  labels:
    app.kubernetes.io/name: meilisearch
    app.kubernetes.io/instance: meilisearch
    app.kubernetes.io/version: "v1.7.0"
    app.kubernetes.io/component: search-engine
    app.kubernetes.io/part-of: meilisearch
spec:
  selector:
    matchLabels:
      app.kubernetes.io/name: meilisearch
      app.kubernetes.io/instance: meilisearch
  endpoints:
    - port: http
      path: /metrics
      interval: 1m
      timeout: 10s
      authorization:
        type: Bearer
        credentials:
          secret:
            name: meilisearch-master-key
            key: MEILI_MASTER_KEY

I've also attempted with ClusterPodMonitoring with secret.namespace: ecommerce, but faces another issue:

unable to read authorization credentials: secret ecommerce/meilisearch-master-key not found or forbidden

I guess this is related to #789

Fixes the issue shared through [1]. The current implementation only supports a PodMonitoring selecting a Secret from the `default` namespace. The existing E2E test cases were using a Secret deployed inside the `default` namespace therefore not covering this "edge" case [2]. Since the PodMonitoring CRD does not support selecting a Secret from another namespace [3], existing unit tests covering such scenario were removed. [1]: GoogleCloudPlatform#776 (comment) [2]: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/f21c2ba4c0a9c3108209c1e59ec75d52e388235e/e2e/authorization_test.go#L96-L177 [3]: GoogleCloudPlatform@99444af Signed-off-by: Florian Daguin <git@fdaguin.dev>

Fixes the issue shared through [1]. The current implementation only supports a PodMonitoring selecting a Secret from the `default` namespace. The existing E2E test cases were using a Secret deployed inside the `default` namespace therefore not covering this "edge" case [2]. Since the PodMonitoring CRD does not support selecting a Secret from another namespace [3], existing unit tests covering such scenario were removed. [1]: #776 (comment) [2]: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/f21c2ba4c0a9c3108209c1e59ec75d52e388235e/e2e/authorization_test.go#L96-L177 [3]: 99444af Signed-off-by: Florian Daguin <git@fdaguin.dev>

TheSpiritXIII · 2024-07-03T15:23:15Z

Unfortunately, I'm having an issue with namespaces

The fix is available in version:

GKE 1.30: 1.30.2-gke.1054000 or later.

And will be available in versions:

GKE 1.29: 1.29.6-gke.1150000 or later.
GKE 1.28: 1.28.11-gke.1107000 or later.

Please allow 2-3 weeks for these versions to get rolled out. Thanks!

cagataygurturk · 2024-07-28T20:18:17Z

Unfortunately, I'm having an issue with namespaces

The fix is available in version:

GKE 1.30: 1.30.2-gke.1054000 or later.

And will be available in versions:

GKE 1.29: 1.29.6-gke.1150000 or later. GKE 1.28: 1.28.11-gke.1107000 or later.

Please allow 2-3 weeks for these versions to get rolled out. Thanks!

Hi, on 1.29.6-gke.1254000 I am still experiencing the same issue. Can you confirm the release schedule?

TheSpiritXIII · 2024-07-29T13:17:50Z

Can you confirm the release schedule?

Hi, we ended up doing additional security patch releases. 0.12 as shown in the releases page is available on the following GKE minor versions:

1.28.11-gke.1176000
1.29.6-gke.1256000
1.30.2-gke.1394000

These are the corresponding available versions on the GKE rapid release channel:

1.28.11-gke.1260000
1.29.6-gke.1326000
1.30.2-gke.1394003

Please create a new issue if you're having still problems since messages here could easily get lost! Thanks!

github-actions bot assigned maxamins Jan 22, 2024

github-actions bot requested a review from maxamins January 22, 2024 16:56

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch from d64dac7 to cdbf808 Compare January 22, 2024 18:07

bwplotka requested review from bwplotka and removed request for maxamins January 24, 2024 17:43

bwplotka assigned bwplotka and unassigned maxamins Jan 24, 2024

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch from 930fba0 to 057922a Compare January 30, 2024 16:41

bwplotka mentioned this pull request Feb 1, 2024

Add secretless OAuth 2 support #651

Merged

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch 9 times, most recently from 7f3cf8b to 0120bb6 Compare February 6, 2024 13:11

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch 12 times, most recently from 51538a8 to 5afff72 Compare February 12, 2024 15:48

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch from cbd0401 to 45a47fd Compare March 25, 2024 16:50

bwplotka approved these changes Mar 27, 2024

View reviewed changes

bwplotka mentioned this pull request Mar 27, 2024

secrets: Improve debuggability & reliability of misconfigured *Monitoring CRs with secrets. #917

Open

TheSpiritXIII and others added 6 commits March 27, 2024 12:39

feat: update common library for secret management

2a2434f

feat: implement secret management for authorization

1246085

refactor: use field scopes for secret management namespace

c56bb87

refactor: reduce endpoint scrape config function args

fec2bbf

fix: default secret namespace for cluster-scoped resources

39ef068

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch from 45a47fd to 190c580 Compare March 27, 2024 19:11

TheSpiritXIII added 2 commits March 27, 2024 15:26

fix: rename kubernetesSecret to secret

aebe5d3

refactor(e2e): simplify authorization tests

7df84e1

TheSpiritXIII force-pushed the TheSpiritXIII/secret-data-model branch from 190c580 to 7df84e1 Compare March 27, 2024 19:26

bwplotka reviewed Mar 28, 2024

View reviewed changes

e2e/authorization_test.go Show resolved Hide resolved

bwplotka approved these changes Mar 28, 2024

View reviewed changes

TheSpiritXIII merged commit 3877b4a into main Mar 28, 2024
27 checks passed

TheSpiritXIII deleted the TheSpiritXIII/secret-data-model branch March 28, 2024 13:06

fdaguin mentioned this pull request Jun 3, 2024

fix(operator): support secrets outside default ns for scrape auth #995

Merged

bwplotka mentioned this pull request Aug 9, 2024

secret injection for PodMonitoring in non default namespace not working #1100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support secrets in scrape authorization #776

feat: support secrets in scrape authorization #776

TheSpiritXIII commented Jan 22, 2024 •

edited

Loading

bwplotka left a comment •

edited

Loading

bwplotka Mar 27, 2024

bwplotka Mar 27, 2024

bwplotka Mar 27, 2024

bwplotka commented Mar 27, 2024

bwplotka left a comment

mpskovvang commented Apr 30, 2024

TheSpiritXIII commented Apr 30, 2024

mpskovvang commented Apr 30, 2024 •

edited

Loading

TheSpiritXIII commented Jul 3, 2024

cagataygurturk commented Jul 28, 2024

TheSpiritXIII commented Jul 29, 2024

feat: support secrets in scrape authorization #776

feat: support secrets in scrape authorization #776

Conversation

TheSpiritXIII commented Jan 22, 2024 • edited Loading

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

bwplotka Mar 27, 2024

Choose a reason for hiding this comment

bwplotka Mar 27, 2024

Choose a reason for hiding this comment

bwplotka Mar 27, 2024

Choose a reason for hiding this comment

bwplotka commented Mar 27, 2024

bwplotka left a comment

Choose a reason for hiding this comment

mpskovvang commented Apr 30, 2024

TheSpiritXIII commented Apr 30, 2024

mpskovvang commented Apr 30, 2024 • edited Loading

TheSpiritXIII commented Jul 3, 2024

cagataygurturk commented Jul 28, 2024

TheSpiritXIII commented Jul 29, 2024

TheSpiritXIII commented Jan 22, 2024 •

edited

Loading

bwplotka left a comment •

edited

Loading

mpskovvang commented Apr 30, 2024 •

edited

Loading