feat: add kubernetes connection proxy for http check#2839
feat: add kubernetes connection proxy for http check#2839yashmehrotra wants to merge 3 commits intomasterfrom
Conversation
WalkthroughIntroduces Kubernetes-aware HTTP checks enabling HTTP requests to route through Kubernetes API proxies, allowing internal cluster service access without external exposure. Changes include new Kubernetes connection field on HTTPCheck struct, HTTP checker implementation for Kubernetes transport injection and URL rewriting, schema definitions for multi-provider Kubernetes connections (EKS, GKE, CNRM, kubeconfig), and test fixtures. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
checks/http.go (1)
79-104: Fix fmt.Errorf formatting at line 98 to unblock vet/Go test.Line 98 uses
%swith*connection.KubernetesConnection(a pointer type), which causes SA5009. Use%vinstead to properly format the pointer.🐛 Proposed fix
-if err != nil { -return nil, fmt.Errorf("failed to instantiate k8s client (%s): %w", check.Kubernetes, err) -} +if err != nil { +return nil, fmt.Errorf("failed to instantiate k8s client (%v): %w", check.Kubernetes, err) +}
🤖 Fix all issues with AI agents
In `@checks/http.go`: - Around line 199-205: Remove the trailing whitespace flagged by the linter in the Kubernetes hydration block: edit the block that checks check.Kubernetes (the if check.Kubernetes != nil { ... } section that calls ctx.WithKubernetesConnection and check.Kubernetes.Populate) and trim any extra spaces at the end of the closing brace/line so there is no trailing whitespace left. - Around line 110-121: The rewritten K8s proxy URL drops query parameters; update the logic that sets check.URL (where parsedURL, port, svc, ns are used) to preserve parsedURL.RawQuery (and optionally Fragment) when rebuilding the URL: after constructing the path portion (using strings.TrimPrefix(parsedURL.Path, "/")), append the original RawQuery prefixed with "?" if non-empty so query parameters are forwarded to the proxied endpoint; ensure the resulting check.URL still uses k8s.Config.Host and the same namespace/service/port composition. - Around line 413-417: The code currently logs the full URL via logger.Infof("Check url is %s", check.URL) which may expose sensitive query params; update the logging to use debug level and redact query parameters before logging: in the flow after c.generateHTTPRequest(...) (referencing generateHTTPRequest and check.URL) parse the URL, strip or replace RawQuery with a placeholder (e.g., "<redacted>") or remove it entirely, then call logger.Debugf with the sanitized URL instead of logger.Infof; ensure no full query string is emitted anywhere in the info-level logs. In `@config/deploy/manifests.yaml`: - Around line 4488-4816: The http.kubernetes schema is missing the serviceAccount field used for proxy auth; update the kubernetes block in this diff to add a serviceAccount property (name: serviceAccount, description: "ServiceAccount specifies the service account whose token should be fetched", type: string) in the same places other Kubernetes connection schemas include it (e.g., alongside other valueFrom/serviceAccount usages and under kubernetes properties), ensuring the symbol "kubernetes" in the HTTP connection schema now contains "serviceAccount" so CRD validation accepts manifests that supply that token. In `@config/schemas/health_http.schema.json`: - Around line 180-214: Add descriptive documentation entries to the GKEConnection schema to clarify the difference between "project" and "projectID": update the "properties" for GKEConnection to include a "description" for "projectID" stating it is the required unique GCP project identifier used for API operations, and a "description" for "project" stating it is an optional human-readable/display name for the project; keep "projectID" in the "required" array and leave "project" optional. In `@fixtures/k8s/http-kubernetes-proxy.yaml`: - Around line 1-10: The fixture file named by metadata.name "http-check" is not following the test naming convention so the suite won't run it; rename the file to end with _pass.yaml (e.g., http-kubernetes-proxy_pass.yaml) so it is executed, and ensure the kubernetes: {} entry (the empty KubernetesConnection) either relies on in-cluster/default credentials or is populated explicitly: update the fixture or test documentation to state that the hydrate path calls Populate(ctx.Context, true) on the KubernetesConnection and that an empty config must resolve to in-cluster/default auth (or provide the required fields in the fixture) so runtime Populate(...) does not fail. 🧹 Nitpick comments (1)
config/schemas/health_http.schema.json (1)
372-392: Consider adding JSON schema validation to ensure at least one Kubernetes connection method is specified.The
KubernetesConnectiondefinition has no required fields, allowing an empty object{}to pass schema validation. While thePopulate()method in the duty package validates the configuration at runtime (seechecks/http.goline 201), adding a schema-level constraint such asoneOfacross the connection methods orminProperties: 1would provide earlier validation feedback to users.If this flexibility is intentional to support in-cluster defaults or other connection modes, no changes are required.
| parsedURL, err := url.Parse(check.URL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing check url[%s]: %w", check.URL, err) | ||
| } | ||
| | ||
| port := lo.CoalesceOrEmpty(parsedURL.Port(), lo.Ternary(parsedURL.Scheme == "https", "443", "80")) | ||
| parts := strings.Split(parsedURL.Hostname(), ".") | ||
| if len(parts) < 2 { | ||
| return nil, fmt.Errorf("check host[%s] is invalid. Use `service.namespace` format", parsedURL.Hostname()) | ||
| } | ||
| svc, ns := parts[0], parts[1] | ||
| check.URL = fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, strings.TrimPrefix(parsedURL.Path, "/")) |
There was a problem hiding this comment.
Preserve query parameters when rewriting the K8s proxy URL.
Line 121 rebuilds the URL but drops RawQuery, so checks with query params will hit the wrong endpoint.
🔧 Proposed fix
-check.URL = fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, strings.TrimPrefix(parsedURL.Path, "/")) +path := strings.TrimPrefix(parsedURL.EscapedPath(), "/") +proxyURL := fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, path) +if parsedURL.RawQuery != "" { +proxyURL += "?" + parsedURL.RawQuery +} +check.URL = proxyURL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parsedURL, err := url.Parse(check.URL) | |
| if err != nil { | |
| return nil, fmt.Errorf("error parsing check url[%s]: %w", check.URL, err) | |
| } | |
| port := lo.CoalesceOrEmpty(parsedURL.Port(), lo.Ternary(parsedURL.Scheme == "https", "443", "80")) | |
| parts := strings.Split(parsedURL.Hostname(), ".") | |
| if len(parts) < 2 { | |
| return nil, fmt.Errorf("check host[%s] is invalid. Use `service.namespace` format", parsedURL.Hostname()) | |
| } | |
| svc, ns := parts[0], parts[1] | |
| check.URL = fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, strings.TrimPrefix(parsedURL.Path, "/")) | |
| parsedURL, err := url.Parse(check.URL) | |
| if err != nil { | |
| return nil, fmt.Errorf("error parsing check url[%s]: %w", check.URL, err) | |
| } | |
| port := lo.CoalesceOrEmpty(parsedURL.Port(), lo.Ternary(parsedURL.Scheme == "https", "443", "80")) | |
| parts := strings.Split(parsedURL.Hostname(), ".") | |
| if len(parts) < 2 { | |
| return nil, fmt.Errorf("check host[%s] is invalid. Use `service.namespace` format", parsedURL.Hostname()) | |
| } | |
| svc, ns := parts[0], parts[1] | |
| path := strings.TrimPrefix(parsedURL.EscapedPath(), "/") | |
| proxyURL := fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, path) | |
| if parsedURL.RawQuery != "" { | |
| proxyURL += "?" + parsedURL.RawQuery | |
| } | |
| check.URL = proxyURL |
🤖 Prompt for AI Agents
In `@checks/http.go` around lines 110 - 121, The rewritten K8s proxy URL drops query parameters; update the logic that sets check.URL (where parsedURL, port, svc, ns are used) to preserve parsedURL.RawQuery (and optionally Fragment) when rebuilding the URL: after constructing the path portion (using strings.TrimPrefix(parsedURL.Path, "/")), append the original RawQuery prefixed with "?" if non-empty so query parameters are forwarded to the proxied endpoint; ensure the resulting check.URL still uses k8s.Config.Host and the same namespace/service/port composition. | if check.Kubernetes != nil { | ||
| *ctx = ctx.WithKubernetesConnection(*check.Kubernetes) | ||
| if _, _, err := check.Kubernetes.Populate(ctx.Context, true); err != nil { | ||
| return nil, nil, oops, results.Invalidf("failed to hydrate kubernetes connection: %v", err) | ||
| } | ||
| | ||
| } |
There was a problem hiding this comment.
Remove trailing whitespace to satisfy lint.
Lint flags unnecessary trailing whitespace at Line 205; please trim it to keep CI green.
🧰 Tools
🪛 GitHub Check: lint
[failure] 205-205:
unnecessary trailing newline (whitespace)
🤖 Prompt for AI Agents
In `@checks/http.go` around lines 199 - 205, Remove the trailing whitespace flagged by the linter in the Kubernetes hydration block: edit the block that checks check.Kubernetes (the if check.Kubernetes != nil { ... } section that calls ctx.WithKubernetesConnection and check.Kubernetes.Populate) and trim any extra spaces at the end of the closing brace/line so there is no trailing whitespace left. | request, err := c.generateHTTPRequest(ctx, check, connection) | ||
| if err != nil { | ||
| return results.ErrorMessage(oops.Wrap(err)) | ||
| } | ||
| logger.Infof("Check url is %s", check.URL) |
There was a problem hiding this comment.
Avoid logging full URLs at info level.
Line 417 logs the complete URL; query params often carry tokens. Prefer debug-level logging with query redaction.
🔐 Suggested change
-logger.Infof("Check url is %s", check.URL) +if ctx.IsDebug() { +if u, err := url.Parse(check.URL); err == nil { +u.RawQuery = "" +ctx.Debugf("Check url is %s", u.String()) +} else { +ctx.Debugf("Check url is %s", check.URL) +} +}🤖 Prompt for AI Agents
In `@checks/http.go` around lines 413 - 417, The code currently logs the full URL via logger.Infof("Check url is %s", check.URL) which may expose sensitive query params; update the logging to use debug level and redact query parameters before logging: in the flow after c.generateHTTPRequest(...) (referencing generateHTTPRequest and check.URL) parse the URL, strip or replace RawQuery with a placeholder (e.g., "<redacted>") or remove it entirely, then call logger.Debugf with the sanitized URL instead of logger.Infof; ensure no full query string is emitted anywhere in the info-level logs. | kubernetes: | ||
| description: Kubernetes connection to use for the HTTP request | ||
| properties: | ||
| cnrm: | ||
| properties: | ||
| clusterResource: | ||
| type: string | ||
| clusterResourceNamespace: | ||
| type: string | ||
| gke: | ||
| properties: | ||
| cluster: | ||
| type: string | ||
| connection: | ||
| description: ConnectionName of the connection. It'll be used to populate the endpoint and credentials. | ||
| type: string | ||
| credentials: | ||
| properties: | ||
| name: | ||
| type: string | ||
| value: | ||
| type: string | ||
| valueFrom: | ||
| properties: | ||
| configMapKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| helmRef: | ||
| properties: | ||
| key: | ||
| description: Key is a JSONPath expression used to fetch the key from the merged JSON. | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| secretKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| serviceAccount: | ||
| description: ServiceAccount specifies the service account whose token should be fetched | ||
| type: string | ||
| type: object | ||
| type: object | ||
| endpoint: | ||
| type: string | ||
| project: | ||
| type: string | ||
| projectID: | ||
| type: string | ||
| skipTLSVerify: | ||
| description: Skip TLS verify | ||
| type: boolean | ||
| zone: | ||
| type: string | ||
| required: | ||
| - cluster | ||
| - projectID | ||
| - zone | ||
| type: object | ||
| required: | ||
| - clusterResource | ||
| - clusterResourceNamespace | ||
| - gke | ||
| type: object | ||
| connection: | ||
| description: Connection name to populate kubeconfig | ||
| type: string | ||
| eks: | ||
| properties: | ||
| accessKey: | ||
| properties: | ||
| name: | ||
| type: string | ||
| value: | ||
| type: string | ||
| valueFrom: | ||
| properties: | ||
| configMapKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| helmRef: | ||
| properties: | ||
| key: | ||
| description: Key is a JSONPath expression used to fetch the key from the merged JSON. | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| secretKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| serviceAccount: | ||
| description: ServiceAccount specifies the service account whose token should be fetched | ||
| type: string | ||
| type: object | ||
| type: object | ||
| assumeRole: | ||
| type: string | ||
| cluster: | ||
| type: string | ||
| connection: | ||
| description: ConnectionName of the connection. It'll be used to populate the endpoint, accessKey and secretKey. | ||
| type: string | ||
| endpoint: | ||
| type: string | ||
| region: | ||
| type: string | ||
| secretKey: | ||
| properties: | ||
| name: | ||
| type: string | ||
| value: | ||
| type: string | ||
| valueFrom: | ||
| properties: | ||
| configMapKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| helmRef: | ||
| properties: | ||
| key: | ||
| description: Key is a JSONPath expression used to fetch the key from the merged JSON. | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| secretKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| serviceAccount: | ||
| description: ServiceAccount specifies the service account whose token should be fetched | ||
| type: string | ||
| type: object | ||
| type: object | ||
| sessionToken: | ||
| properties: | ||
| name: | ||
| type: string | ||
| value: | ||
| type: string | ||
| valueFrom: | ||
| properties: | ||
| configMapKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| helmRef: | ||
| properties: | ||
| key: | ||
| description: Key is a JSONPath expression used to fetch the key from the merged JSON. | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| secretKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| serviceAccount: | ||
| description: ServiceAccount specifies the service account whose token should be fetched | ||
| type: string | ||
| type: object | ||
| type: object | ||
| skipTLSVerify: | ||
| description: Skip TLS verify when connecting to aws | ||
| type: boolean | ||
| required: | ||
| - cluster | ||
| type: object | ||
| gke: | ||
| properties: | ||
| cluster: | ||
| type: string | ||
| connection: | ||
| description: ConnectionName of the connection. It'll be used to populate the endpoint and credentials. | ||
| type: string | ||
| credentials: | ||
| properties: | ||
| name: | ||
| type: string | ||
| value: | ||
| type: string | ||
| valueFrom: | ||
| properties: | ||
| configMapKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| helmRef: | ||
| properties: | ||
| key: | ||
| description: Key is a JSONPath expression used to fetch the key from the merged JSON. | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| secretKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| serviceAccount: | ||
| description: ServiceAccount specifies the service account whose token should be fetched | ||
| type: string | ||
| type: object | ||
| type: object | ||
| endpoint: | ||
| type: string | ||
| project: | ||
| type: string | ||
| projectID: | ||
| type: string | ||
| skipTLSVerify: | ||
| description: Skip TLS verify | ||
| type: boolean | ||
| zone: | ||
| type: string | ||
| required: | ||
| - cluster | ||
| - projectID | ||
| - zone | ||
| type: object | ||
| kubeconfig: | ||
| properties: | ||
| name: | ||
| type: string | ||
| value: | ||
| type: string | ||
| valueFrom: | ||
| properties: | ||
| configMapKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| helmRef: | ||
| properties: | ||
| key: | ||
| description: Key is a JSONPath expression used to fetch the key from the merged JSON. | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| secretKeyRef: | ||
| properties: | ||
| key: | ||
| type: string | ||
| name: | ||
| type: string | ||
| required: | ||
| - key | ||
| type: object | ||
| serviceAccount: | ||
| description: ServiceAccount specifies the service account whose token should be fetched | ||
| type: string | ||
| type: object | ||
| type: object | ||
| type: object |
There was a problem hiding this comment.
Add serviceAccount to the HTTP Kubernetes connection schema.
The new http.kubernetes block omits serviceAccount, which is present in the Kubernetes connection schema elsewhere in this CRD. If users configure serviceAccount for proxy auth, CRD validation will reject otherwise valid manifests.
🔧 Proposed fix
kubernetes: description: Kubernetes connection to use for the HTTP request properties: cnrm: properties: clusterResource: type: string @@ kubeconfig: properties: name: type: string value: type: string valueFrom: properties: configMapKeyRef: properties: key: type: string name: type: string required: - key type: object helmRef: properties: key: description: Key is a JSONPath expression used to fetch the key from the merged JSON. name: type: string required: - key type: object secretKeyRef: properties: key: type: string name: type: string required: - key type: object serviceAccount: description: ServiceAccount specifies the service account whose token should be fetched type: string type: object type: object + serviceAccount: + description: ServiceAccount when enabled will allow access to KUBERNETES env vars + type: boolean type: object🤖 Prompt for AI Agents
In `@config/deploy/manifests.yaml` around lines 4488 - 4816, The http.kubernetes schema is missing the serviceAccount field used for proxy auth; update the kubernetes block in this diff to add a serviceAccount property (name: serviceAccount, description: "ServiceAccount specifies the service account whose token should be fetched", type: string) in the same places other Kubernetes connection schemas include it (e.g., alongside other valueFrom/serviceAccount usages and under kubernetes properties), ensuring the symbol "kubernetes" in the HTTP connection schema now contains "serviceAccount" so CRD validation accepts manifests that supply that token. | "GKEConnection": { | ||
| "properties": { | ||
| "connection": { | ||
| "type": "string" | ||
| }, | ||
| "endpoint": { | ||
| "type": "string" | ||
| }, | ||
| "credentials": { | ||
| "$ref": "#/$defs/EnvVar" | ||
| }, | ||
| "skipTLSVerify": { | ||
| "type": "boolean" | ||
| }, | ||
| "project": { | ||
| "type": "string" | ||
| }, | ||
| "projectID": { | ||
| "type": "string" | ||
| }, | ||
| "zone": { | ||
| "type": "string" | ||
| }, | ||
| "cluster": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "additionalProperties": false, | ||
| "type": "object", | ||
| "required": [ | ||
| "projectID", | ||
| "zone", | ||
| "cluster" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'project[ID]*' --type=go config/ | grep -i gke | head -100Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
rg -n 'GKEConnection' --type=go -A 10 | head -150Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
fd . -name "*.go" | xargs grep -l "GKEConnection\|projectID.*project[^ID]" | head -10Repository: flanksource/canary-checker
Length of output: 300
🏁 Script executed:
rg 'GKE' --type=go -B2 -A10 | head -150Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
find . -name "*.go" -type f | xargs grep -l "GKE" | head -20Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
rg '"project"' config/schemas/ -A2 -B2Repository: flanksource/canary-checker
Length of output: 12227
🏁 Script executed:
cat config/schemas/health_http.schema.json | grep -n "GKE" -A 35 -B 2Repository: flanksource/canary-checker
Length of output: 2853
🏁 Script executed:
cat config/schemas/health_http.schema.json | grep -n "GCP\|project" | head -50Repository: flanksource/canary-checker
Length of output: 146
🏁 Script executed:
find . -name "*.go" -type f -exec grep -l "projectID\|gke\|GKE" {} \; 2>/dev/null | head -20Repository: flanksource/canary-checker
Length of output: 121
🏁 Script executed:
rg 'type.*GKE' -A 20 --type=goRepository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
find . -path "*/pkg/*" -o -path "*/internal/*" | grep -E "\.go$|gke" | head -20Repository: flanksource/canary-checker
Length of output: 543
🏁 Script executed:
rg 'projectID' --type=go -B3 -A3 | head -100Repository: flanksource/canary-checker
Length of output: 2136
🏁 Script executed:
rg '"projectID"' . -A2 -B2 | head -100Repository: flanksource/canary-checker
Length of output: 3710
🏁 Script executed:
cat ./api/context/functions.go | grep -i "gke\|project" -B2 -A2 | head -50Repository: flanksource/canary-checker
Length of output: 2298
🏁 Script executed:
find . -name "*.go" -type f -exec grep -l "GKE\|gke\|kubernetes.*connection" {} \; | head -10Repository: flanksource/canary-checker
Length of output: 131
🏁 Script executed:
rg 'zone.*cluster' --type=go -B5 -A5 | head -100Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
grep -n "GKE\|project\|zone\|cluster" ./checks/http.go | head -100Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
grep -n "type.*HTTP\|GKE" ./checks/http.go -A 30 | head -200Repository: flanksource/canary-checker
Length of output: 1109
🏁 Script executed:
rg 'struct|GKE' ./checks/http.go -A 20 | head -150Repository: flanksource/canary-checker
Length of output: 1488
🏁 Script executed:
find . -name "*.go" -type f | xargs grep -l "type.*GKEConnection" 2>/dev/nullRepository: flanksource/canary-checker
Length of output: 54
🏁 Script executed:
find . -path "*/v1/*" -name "*.go" | xargs grep -l "GKE\|gke" 2>/dev/null | head -10Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
rg 'type GKEConnection|GKEConnection struct' --type=go -A 20Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
find . -name "*generate*" -o -name "*gen*" | grep -E "\.(sh|go|yaml|json)$" | head -20Repository: flanksource/canary-checker
Length of output: 143
🏁 Script executed:
rg 'project.*projectID|projectID.*project' config/schemas/ -B2 -A2 | head -80Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
cat ./config/schemas/health_http.schema.json | jq '.["$defs"]["GKEConnection"]' 2>/dev/nullRepository: flanksource/canary-checker
Length of output: 609
Add descriptions to clarify the distinction between project and projectID.
Both properties serve intentional, different purposes: projectID is the required unique GCP project identifier needed for API operations, while project is an optional field for the project display name. Add field descriptions to the schema to document this distinction for users.
🤖 Prompt for AI Agents
In `@config/schemas/health_http.schema.json` around lines 180 - 214, Add descriptive documentation entries to the GKEConnection schema to clarify the difference between "project" and "projectID": update the "properties" for GKEConnection to include a "description" for "projectID" stating it is the required unique GCP project identifier used for API operations, and a "description" for "project" stating it is an optional human-readable/display name for the project; keep "projectID" in the "required" array and leave "project" optional. | apiVersion: canaries.flanksource.com/v1 | ||
| kind: Canary | ||
| metadata: | ||
| name: http-check | ||
| spec: | ||
| schedule: "@every 30s" | ||
| http: | ||
| - name: k8s-check | ||
| kubernetes: {} | ||
| url: http://httpbin.default/status/404 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # First, find where KubernetesConnection is defined rg -n "type KubernetesConnection" -A 10 -B 2 -g '*.go'Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for HTTP check handler that processes kubernetes config rg -n "func.*HTTP|type.*HTTP" -g '*.go' | head -30 rg -n "kubernetes\s*\{" -A 5 -B 2 -g '*.go' | head -50Repository: flanksource/canary-checker
Length of output: 2168
🏁 Script executed:
#!/bin/bash # Look for InClusterConfig or default config handling rg -n "InClusterConfig|BuildConfigFromFlags" -A 5 -B 2 -g '*.go'Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Read the HTTPCheck struct definition sed -n '117,200p' api/v1/checks.goRepository: flanksource/canary-checker
Length of output: 3747
🏁 Script executed:
#!/bin/bash # Read the hydrate function sed -n '190,250p' checks/http.goRepository: flanksource/canary-checker
Length of output: 1886
🏁 Script executed:
#!/bin/bash # Search for kubernetes field in HTTPCheck or HTTP type rg -n "kubernetes\s*\*|Kubernetes\s*\*" -B 2 -A 2 -g '*.go' api/v1/checks.goRepository: flanksource/canary-checker
Length of output: 400
🏁 Script executed:
#!/bin/bash # Find KubernetesConnection definition rg -n "type KubernetesConnection" -A 20 -g '*.go'Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Find Populate method for KubernetesConnection rg -n "func.*KubernetesConnection.*Populate|func \(.*\*KubernetesConnection.*Populate" -A 15 -g '*.go'Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for KubernetesConnection in connection package fd -type f -name '*.go' -path '*/connection/*' | head -20Repository: flanksource/canary-checker
Length of output: 240
🏁 Script executed:
#!/bin/bash # Search for imports of KubernetesConnection rg -n "import|KubernetesConnection" checks/http.go | head -20Repository: flanksource/canary-checker
Length of output: 141
🏁 Script executed:
#!/bin/bash # Search for connection package imports rg -n "\".*connection\"" -g '*.go' | grep -i import | head -10Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Look for Populate method across the codebase rg -n "\.Populate\(" checks/http.go -B 3 -A 3Repository: flanksource/canary-checker
Length of output: 364
🏁 Script executed:
#!/bin/bash # Read the import section of checks/http.go head -30 checks/http.goRepository: flanksource/canary-checker
Length of output: 748
🏁 Script executed:
#!/bin/bash # Search for Populate method definitions in the codebase rg -n "func.*Populate.*\{" -A 10 -g '*.go' | head -50Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for github imports that might contain KubernetesConnection rg -n "github.com.*connection|\".*duty" -g '*.go' | head -20Repository: flanksource/canary-checker
Length of output: 1533
🏁 Script executed:
#!/bin/bash # Check if KubernetesConnection is defined in the canary-checker codebase rg -n "type KubernetesConnection|KubernetesConnection" api/v1/ -A 5 | head -40Repository: flanksource/canary-checker
Length of output: 2700
🏁 Script executed:
#!/bin/bash # Look at api/v1/checks.go imports to see where connection comes from head -50 api/v1/checks.go | grep -A 30 "^import"Repository: flanksource/canary-checker
Length of output: 932
🏁 Script executed:
#!/bin/bash # Search for where KubernetesConnection is used in context rg -n "WithKubernetesConnection" -B 2 -A 5 -g '*.go'Repository: flanksource/canary-checker
Length of output: 1850
🏁 Script executed:
#!/bin/bash # Search for duty package to understand if it's a dependency grep -n "duty" go.mod | head -20Repository: flanksource/canary-checker
Length of output: 165
🏁 Script executed:
#!/bin/bash # Look for other usages of KubernetesConnection with empty/minimal configs in fixtures find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "kubernetes:" | head -10Repository: flanksource/canary-checker
Length of output: 488
🏁 Script executed:
#!/bin/bash # Search for tests that use kubernetes: {} config rg -n "kubernetes\s*:\s*\{\}" -B 3 -A 3 -g '*.yaml' -g '*.yml'Repository: flanksource/canary-checker
Length of output: 401
🏁 Script executed:
#!/bin/bash # Look at other kubernetes check fixtures to see how they handle kubernetes config cat fixtures/k8s/kubernetes-check-inline-kubeconfig.yamlRepository: flanksource/canary-checker
Length of output: 1277
🏁 Script executed:
#!/bin/bash # Search for tests that use the http-kubernetes-proxy fixture rg -n "http-kubernetes-proxy" -B 3 -A 3 -g '*.go'Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for any test or documentation about Populate behavior with empty config rg -n "Populate.*true|kubernetes.*Populate" -B 5 -A 5 -g '*.go' checks/Repository: flanksource/canary-checker
Length of output: 589
🏁 Script executed:
#!/bin/bash # Search for tests that reference this fixture file rg -n "http-kubernetes-proxy|fixtures/k8s" -g '*_test.go' -lRepository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Look for any test files in the fixtures directory ls -la fixtures/k8s/ | grep -i testRepository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for integration or fixture tests find . -name "*test*.go" -o -name "*_test.go" | xargs grep -l "fixtures/k8s" | head -5Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for how Populate is used in other checks with KubernetesConnection rg -n "Populate\(" checks/ -B 3 -A 3Repository: flanksource/canary-checker
Length of output: 2678
🏁 Script executed:
#!/bin/bash # Look at the kubernetes resource check to see how it handles KubernetesConnection sed -n '70,100p' checks/kubernetes_resource.goRepository: flanksource/canary-checker
Length of output: 1011
🏁 Script executed:
#!/bin/bash # Look for documentation or comments about empty kubernetes config rg -n "in-cluster|InClusterConfig|default.*kubernetes" -i -g '*.go' checks/ api/ | head -20Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Search for CI/test configurations that might run fixtures find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "http-kubernetes-proxy" 2>/dev/nullRepository: flanksource/canary-checker
Length of output: 54
🏁 Script executed:
#!/bin/bash # Look for test runner configuration ls -la | grep -E "Makefile|\.github|\.gitlab|\.circleci|\.travis"Repository: flanksource/canary-checker
Length of output: 190
🏁 Script executed:
#!/bin/bash # Check if there's a test or run script that uses fixtures find . -name "run_test.go" -o -name "*_test.go" | xargs grep -l "fixtures" 2>/dev/null | head -3Repository: flanksource/canary-checker
Length of output: 115
🏁 Script executed:
#!/bin/bash # Look at test/run_test.go to see how fixtures are used head -100 test/run_test.goRepository: flanksource/canary-checker
Length of output: 2571
🏁 Script executed:
#!/bin/bash # Check the exact filename of the fixture ls -la fixtures/k8s/http-kubernetes-proxy*Repository: flanksource/canary-checker
Length of output: 157
🏁 Script executed:
#!/bin/bash # Look for any references to http-kubernetes-proxy in test or CI configuration rg -r "http-kubernetes-proxy" .github Makefile 2>/dev/nullRepository: flanksource/canary-checker
Length of output: 866
🏁 Script executed:
#!/bin/bash # Check the Makefile for test targets grep -n "test\|fixtures" Makefile | head -20Repository: flanksource/canary-checker
Length of output: 637
🏁 Script executed:
#!/bin/bash # Look at the fixture directory to see naming patterns ls -la fixtures/k8s/ | grep -E "pass|fail"Repository: flanksource/canary-checker
Length of output: 1153
🏁 Script executed:
#!/bin/bash # Look for any documentation or comments about Populate in local codebase rg -n "Populate.*empty|empty.*kubernetes|in.?cluster" -i -g '*.go' -g '*.md'Repository: flanksource/canary-checker
Length of output: 615
🏁 Script executed:
#!/bin/bash # Check git history or comments about why this fixture doesn't follow naming convention git log --oneline -20 -- fixtures/k8s/http-kubernetes-proxy.yaml 2>/dev/null || echo "No git history available"Repository: flanksource/canary-checker
Length of output: 93
🏁 Script executed:
#!/bin/bash # Verify the fixture content one more time and check if similar fixtures exist cat fixtures/k8s/http-kubernetes-proxy.yaml ls -la fixtures/k8s/ | grep httpRepository: flanksource/canary-checker
Length of output: 659
🏁 Script executed:
#!/bin/bash # Check if there's a specific reason this fixture isn't named with _pass/_fail git show cf9d3b9:fixtures/k8s/http-kubernetes-proxy.yaml 2>/dev/null || echo "Cannot show commit"Repository: flanksource/canary-checker
Length of output: 278
🏁 Script executed:
#!/bin/bash # Search for any documentation about kubernetes empty config or in-cluster defaults rg -n "kubernetes.*\{\}|empty.*config|in.?cluster" -i docs/ README* 2>/dev/null | head -20Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Look for how other external checks handle empty configs (e.g., AWS, S3) rg -n "AWSConnection\(\)|S3Connection\(\)" -B 2 -A 2 -g '*.go'Repository: flanksource/canary-checker
Length of output: 52
🏁 Script executed:
#!/bin/bash # Final check: confirm what happens if Populate fails - look at error handling grep -n "Invalidf\|failed to hydrate" checks/http.goRepository: flanksource/canary-checker
Length of output: 1397
🏁 Script executed:
#!/bin/bash # Check if any HTTP checks actually use kubernetes config in passing tests rg -n "kubernetes:" fixtures/k8s/ -g '*.yaml' -B 2 -A 2Repository: flanksource/canary-checker
Length of output: 4047
Fixture must follow test naming convention to be executed.
This fixture doesn't end with _pass.yaml, _pass.yml, _fail.yaml, or _fail.yml, so it won't be run by the test suite. Rename it accordingly (e.g., http-kubernetes-proxy_pass.yaml) to ensure the kubernetes: {} configuration is tested.
Additionally, the hydrate function will call Populate(ctx.Context, true) on the non-nil empty KubernetesConnection, which may fail at runtime depending on the duty package's behavior. Confirm that empty config uses in-cluster/default credentials or explicitly document the expected behavior.
🤖 Prompt for AI Agents
In `@fixtures/k8s/http-kubernetes-proxy.yaml` around lines 1 - 10, The fixture file named by metadata.name "http-check" is not following the test naming convention so the suite won't run it; rename the file to end with _pass.yaml (e.g., http-kubernetes-proxy_pass.yaml) so it is executed, and ensure the kubernetes: {} entry (the empty KubernetesConnection) either relies on in-cluster/default credentials or is populated explicitly: update the fixture or test documentation to state that the hydrate path calls Populate(ctx.Context, true) on the KubernetesConnection and that an empty config must resolve to in-cluster/default auth (or provide the required fields in the fixture) so runtime Populate(...) does not fail. There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@checks/http.go`: - Around line 95-122: The Kubernetes proxy rewrite in the if check.Kubernetes block currently rebuilds check.URL with fmt.Sprintf and drops parsedURL.RawQuery and proper path escaping; update the logic that computes check.URL (after obtaining parsedURL and port/svc/ns) to construct a url.URL (or otherwise append the original parsedURL.RawQuery and use url.PathEscape/ResolveReference to preserve path escaping) so the final check.URL includes the original parsedURL.RawQuery and an appropriately escaped path; ensure this change is applied where check.URL is set (reference parsedURL, k8s.Config.Host, svc, ns, port) and that the rest.TransportFor/k8s client usage remains unchanged. - Around line 412-416: The current logger.Infof call prints the full check.URL which may include sensitive query/userinfo; instead create a redacted URL string (e.g., parse check.URL with url.Parse, clear u.User and u.RawQuery and/or mask u.Path as needed, then use u.String()) and pass that redacted value to logger.Infof; update the logging at the site where generateHTTPRequest is called (the logger.Infof("Check url is %s", check.URL) line) to log the sanitized URL variable rather than check.URL. 🧹 Nitpick comments (2)
fixtures/k8s/http-kubernetes-proxy.yaml (1)
6-10: Clarify expected HTTP status in the fixture.
If this fixture is meant to represent a healthy check, the 404 endpoint will fail by default unlessresponseCodesincludes 404. Consider switching to a 200 endpoint or addingresponseCodes: [404]if failure is intentional.config/deploy/manifests.yaml (1)
4487-4816: Consider enforcing or documenting a single Kubernetes provider choice.
The schema allowscnrm,eks,gke,kubeconfig, andconnectionto be set together, which can lead to ambiguous resolution. Consider documenting precedence or adding a schema constraint (e.g., one-of/validation) to ensure only one provider is selected.
| if check.Kubernetes != nil { | ||
| k8s, err := ctx.Kubernetes() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to instantiate k8s client (%s): %w", check.Kubernetes, err) | ||
| } | ||
| | ||
| k8srt, err := rest.TransportFor(k8s.Config) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get transport config for k8s: %w", err) | ||
| } | ||
| | ||
| client.Use(func(rt nethttp.RoundTripper) nethttp.RoundTripper { | ||
| return k8srt | ||
| }) | ||
| | ||
| parsedURL, err := url.Parse(check.URL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing check url[%s]: %w", check.URL, err) | ||
| } | ||
| | ||
| port := lo.CoalesceOrEmpty(parsedURL.Port(), lo.Ternary(parsedURL.Scheme == "https", "443", "80")) | ||
| parts := strings.Split(parsedURL.Hostname(), ".") | ||
| if len(parts) < 2 { | ||
| return nil, fmt.Errorf("check host[%s] is invalid. Use `service.namespace` format", parsedURL.Hostname()) | ||
| } | ||
| svc, ns := parts[0], parts[1] | ||
| check.URL = fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, strings.TrimPrefix(parsedURL.Path, "/")) | ||
| } |
There was a problem hiding this comment.
Preserve query parameters when proxying via the Kubernetes API.
The rewrite drops RawQuery (and path escaping), so any checks that rely on query strings will be sent without them.
🛠️ Proposed fix
-check.URL = fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, strings.TrimPrefix(parsedURL.Path, "/")) +proxyPath := strings.TrimPrefix(parsedURL.EscapedPath(), "/") +proxyURL := fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, proxyPath) +if parsedURL.RawQuery != "" { +proxyURL = proxyURL + "?" + parsedURL.RawQuery +} +check.URL = proxyURL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if check.Kubernetes != nil { | |
| k8s, err := ctx.Kubernetes() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to instantiate k8s client (%s): %w", check.Kubernetes, err) | |
| } | |
| k8srt, err := rest.TransportFor(k8s.Config) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get transport config for k8s: %w", err) | |
| } | |
| client.Use(func(rt nethttp.RoundTripper) nethttp.RoundTripper { | |
| return k8srt | |
| }) | |
| parsedURL, err := url.Parse(check.URL) | |
| if err != nil { | |
| return nil, fmt.Errorf("error parsing check url[%s]: %w", check.URL, err) | |
| } | |
| port := lo.CoalesceOrEmpty(parsedURL.Port(), lo.Ternary(parsedURL.Scheme == "https", "443", "80")) | |
| parts := strings.Split(parsedURL.Hostname(), ".") | |
| if len(parts) < 2 { | |
| return nil, fmt.Errorf("check host[%s] is invalid. Use `service.namespace` format", parsedURL.Hostname()) | |
| } | |
| svc, ns := parts[0], parts[1] | |
| check.URL = fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, strings.TrimPrefix(parsedURL.Path, "/")) | |
| } | |
| if check.Kubernetes != nil { | |
| k8s, err := ctx.Kubernetes() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to instantiate k8s client (%s): %w", check.Kubernetes, err) | |
| } | |
| k8srt, err := rest.TransportFor(k8s.Config) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get transport config for k8s: %w", err) | |
| } | |
| client.Use(func(rt nethttp.RoundTripper) nethttp.RoundTripper { | |
| return k8srt | |
| }) | |
| parsedURL, err := url.Parse(check.URL) | |
| if err != nil { | |
| return nil, fmt.Errorf("error parsing check url[%s]: %w", check.URL, err) | |
| } | |
| port := lo.CoalesceOrEmpty(parsedURL.Port(), lo.Ternary(parsedURL.Scheme == "https", "443", "80")) | |
| parts := strings.Split(parsedURL.Hostname(), ".") | |
| if len(parts) < 2 { | |
| return nil, fmt.Errorf("check host[%s] is invalid. Use `service.namespace` format", parsedURL.Hostname()) | |
| } | |
| svc, ns := parts[0], parts[1] | |
| proxyPath := strings.TrimPrefix(parsedURL.EscapedPath(), "/") | |
| proxyURL := fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s/proxy/%s", k8s.Config.Host, ns, svc, port, proxyPath) | |
| if parsedURL.RawQuery != "" { | |
| proxyURL = proxyURL + "?" + parsedURL.RawQuery | |
| } | |
| check.URL = proxyURL | |
| } |
🤖 Prompt for AI Agents
In `@checks/http.go` around lines 95 - 122, The Kubernetes proxy rewrite in the if check.Kubernetes block currently rebuilds check.URL with fmt.Sprintf and drops parsedURL.RawQuery and proper path escaping; update the logic that computes check.URL (after obtaining parsedURL and port/svc/ns) to construct a url.URL (or otherwise append the original parsedURL.RawQuery and use url.PathEscape/ResolveReference to preserve path escaping) so the final check.URL includes the original parsedURL.RawQuery and an appropriately escaped path; ensure this change is applied where check.URL is set (reference parsedURL, k8s.Config.Host, svc, ns, port) and that the rest.TransportFor/k8s client usage remains unchanged. | request, err := c.generateHTTPRequest(ctx, check, connection) | ||
| if err != nil { | ||
| return results.ErrorMessage(oops.Wrap(err)) | ||
| } | ||
| logger.Infof("Check url is %s", check.URL) |
There was a problem hiding this comment.
Avoid logging full URLs with query parameters.
URLs can contain secrets (query tokens, paths). Logging them at info level risks leakage.
🛡️ Proposed fix (redact userinfo/query before logging)
-logger.Infof("Check url is %s", check.URL) +if parsed, err := url.Parse(check.URL); err == nil { +parsed.User = nil +parsed.RawQuery = "" +logger.Infof("Check url is %s", parsed.String()) +} else { +logger.Infof("Check url is %s", check.URL) +}🤖 Prompt for AI Agents
In `@checks/http.go` around lines 412 - 416, The current logger.Infof call prints the full check.URL which may include sensitive query/userinfo; instead create a redacted URL string (e.g., parse check.URL with url.Parse, clear u.User and u.RawQuery and/or mask u.Path as needed, then use u.String()) and pass that redacted value to logger.Infof; update the logging at the site where generateHTTPRequest is called (the logger.Infof("Check url is %s", check.URL) line) to log the sanitized URL variable rather than check.URL. 1 participant
Fixes: flanksource/duty#1654
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.