docs(e2e): add GMP sidecar injection example script and test - #2042
docs(e2e): add GMP sidecar injection example script and test#2042bwplotka wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end test and a helper bash script to demonstrate and verify the injection of GMP sidecars into a Kubernetes deployment. The review feedback highlights several improvement opportunities in the bash script, including enabling pipefail for safer error handling, validating extracted cluster variables, using declarative kubectl apply for the ConfigMap, and avoiding the touch command in a distroless environment. Additionally, the Go test should be updated to clean up the created ConfigMap to prevent resource leaks.
0fbd8c4 to
e4539f0
Compare
bwplotka
changed the title
bf4d04e to
d9c3f6e
Compare
| PORT=80 | ||
|
|
||
| # Extract labels. | ||
| CLUSTER_NAME=$(kubectl -n gmp-system get configmap/collector -o jsonpath='{.data.config\.yaml}' | grep 'cluster:' | awk '{print $2}' | tr -d '"') |
There was a problem hiding this comment.
Any particular reason you changed this parsing from yq to grep/awk/tr? Seems more brittle and less readable to pass it through so many tools.
There was a problem hiding this comment.
AI decided there's no extra dependency to install, which is some benefit (:
There was a problem hiding this comment.
Assuming awk is available and grep has consistent behavior, etc.
We already have yq as a dependency of the project.
https://github.com/GoogleCloudPlatform/prometheus-engine/blob/main/tools/go.mod#L252
| @@ -0,0 +1,124 @@ | |||
| // Copyright 2024 Google LLC | |||
There was a problem hiding this comment.
2026?
Usually addlicense should handle this correctly.
|
| # Images need to be updated periodically. | ||
| DISTROLESS_IMAGE=gke.gcr.io/gke-distroless/bash:gke_distroless_20260220.00_p0@sha256:828371616edc2c38e36868e2f8c992df37e484df72670f148de59867dfdd2490 | ||
| PROMETHEUS_IMAGE=gke.gcr.io/prometheus-engine/prometheus:v2.53.5-gmp.4-gke.0@sha256:6f349dc0be36c8a61be183254f1126c9935f5332daa96c481f7e0e1b20fe0513 | ||
| CONFIG_RELOADER_IMAGE=gke.gcr.io/prometheus-engine/config-reloader:v0.18.0-gke.2@sha256:b41862ee7ee3e9f24112ccdb0e53060085af1a8347054a7dbcff04467d3e1e9c |
There was a problem hiding this comment.
Maybe we should integrate this into our regular scanning/bumping. Or reference images from the manifests? 🤔
There was a problem hiding this comment.
Yea, I was thinking to do this in separate step, but we should
Adds a bash script example for injecting GMP sidecars (prometheus and config-reloader) into an existing Kubernetes Deployment via strategic merge patch, alongside an E2E test verifying the injection. The naming uses 'example-deployment' and 'example-service'. TAG=agy CONV=bbab9ab7-4203-4ea3-a69b-2c40577629c2
d9c3f6e to
f0cef33
Compare
| PROJECT_ID=$(kubectl -n gmp-system get configmap/collector -o jsonpath='{.data.config\.yaml}' | yq '.global.external_labels.project_id') | ||
|
|
||
|
|
||
| if [[ -z "${CLUSTER_NAME}" || -z "${LOCATION}" || -z "${PROJECT_ID}" ]]; then |
There was a problem hiding this comment.
If any of these label keys are missing in the ConfigMap, yq outputs the literal string "null" rather than an empty string, which bypasses the -z check. We should check for "null" explicitly as well:
| if [[ -z "${CLUSTER_NAME}" || -z "${LOCATION}" || -z "${PROJECT_ID}" ]]; then | |
| if [[ -z "${CLUSTER_NAME}" || "${CLUSTER_NAME}" == "null" || -z "${LOCATION}" || "${LOCATION}" == "null" || -z "${PROJECT_ID}" || "${PROJECT_ID}" == "null" ]]; then |
| if err := kubeClient.Create(ctx, deployment); err != nil { | ||
| t.Fatalf("error creating example-deployment: %s", err) | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
If the test times out and ctx expires, these deferred deletions will fail with context deadline exceeded, leaving the deployment and ConfigMap behind in the test cluster. Consider creating a fresh background context with a short timeout here for cleanup operations.
| // 3. Verify sidecars are injected and ConfigMap is created. | ||
| if err := wait.PollUntilContextCancel(ctx, 2*time.Second, true, func(ctx context.Context) (bool, error) { | ||
| cm := &corev1.ConfigMap{} | ||
| if getErr := kubeClient.Get(ctx, client.ObjectKey{Name: "example-deployment", Namespace: "default"}, cm); getErr != nil { |
There was a problem hiding this comment.
To keep this consistent with the cleanup logic above and avoid hardcoded strings, consider referencing deployment.Name and deployment.Namespace here and on line 100:
| if getErr := kubeClient.Get(ctx, client.ObjectKey{Name: "example-deployment", Namespace: "default"}, cm); getErr != nil { | |
| if getErr := kubeClient.Get(ctx, client.ObjectKey{Name: deployment.Name, Namespace: deployment.Namespace}, cm); getErr != nil { |
| return hasProm && hasReloader, nil | ||
| }); err != nil { | ||
| t.Fatalf("failed to verify injected sidecars or config map: %s", err) | ||
| } |
There was a problem hiding this comment.
This polling loop confirms that the Deployment spec in the API server was patched with the sidecars, but it doesn't verify that the pods actually start up cleanly at runtime without crashing or failing image pulls. Consider waiting for the deployment pods to reach ready status after this check so the test catches runtime failures or config unmarshalling errors.
3 participants
Adds a bash script example for injecting GMP sidecars (prometheus and config-reloader) into an existing Kubernetes Deployment via strategic merge patch, alongside an E2E test verifying the injection.
The naming has been updated to use
example-deploymentandexample-serviceinstead ofunreliable.TAG=agy
CONV=bbab9ab7-4203-4ea3-a69b-2c40577629c2