NE-2117: Add httpsLogFormat and tcpLogFormat to API #2773
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1020,4 +1020,12 @@ var ( | |||||
| enhancementPR("https://github.com/openshift/enhancements/pull/1910"). | ||||||
| enable(inDevPreviewNoUpgrade(), inTechPreviewNoUpgrade()). | ||||||
| mustRegister() | ||||||
|
|
||||||
| FeatureGateHTTPSLogFormat = newFeatureGate("HTTPSLogFormat"). | ||||||
| reportProblemsToJiraComponent("Networking/router"). | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
| contactPerson("rohara"). | ||||||
| productScope(ocpSpecific). | ||||||
| enhancementPR("https://github.com/openshift/enhancements/pull/1832"). | ||||||
| enable(inDevPreviewNoUpgrade(), inTechPreviewNoUpgrade()). | ||||||
|
Contributor
There was a problem hiding this comment. As a note, we are asking for the corresponding enhancement to be merged before tech preview. Is there anything blocking the enhancement at this time? Could this feature start in dev preview only instead?
Contributor
There was a problem hiding this comment. @yuqi-zhang, this feature poses low risk of breaking other features or tests (tech preview or otherwise). It only involves OpenShift router and the Ingress Operator, and no other components. In light of that, what is the advantage of making the feature DPNU instead of TPNU? |
||||||
| mustRegister() | ||||||
|
Comment on lines
+1024
to
+1030
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
types_file="$(fd '^types_ingress\.go$' | head -n1)"
if [[ -z "${types_file}" ]]; then
echo "types_ingress.go not found" >&2
exit 1
fi
echo "== AccessLogging definition from ${types_file} =="
python - "$types_file" <<'PY'
from pathlib import Path
import re, sys
text = Path(sys.argv[1]).read_text()
match = re.search(r'type\s+AccessLogging\s+struct\s*\{.*?\n\}', text, re.S)
if not match:
raise SystemExit("AccessLogging struct not found")
print(match.group(0))
PY
echo
echo "== Related feature-gate annotations and log-format fields =="
rg -n -C2 'HTTPSLogFormat|httpsLogFormat|tcpLogFormat|\+openshift:.*FeatureGate' "$types_file"Repository: openshift/api Length of output: 167 🏁 Script executed: # Find where AccessLogging is defined
rg -n 'type\s+AccessLogging' --type go
# Also search for the feature gate in context
rg -n 'HTTPSLogFormat' --type go
# Look for any ingress-related API types files
fd 'types.*ingress' --type fRepository: openshift/api Length of output: 263 🏁 Script executed: # Get the AccessLogging struct definition and surrounding context
sed -n '1490,1550p' operator/v1/types_ingress.goRepository: openshift/api Length of output: 2592 🏁 Script executed: # Search for FeatureGate annotations in the ingress types file
rg -n 'FeatureGate' operator/v1/types_ingress.go -B2 -A2Repository: openshift/api Length of output: 39 🏁 Script executed: # Search for FeatureGate pattern across operator/v1/
rg -n 'FeatureGate' operator/v1/ -B1 -A1 | head -50
# Also check if there are any gated fields using a different pattern
rg -n '\+openshift:' operator/v1/types_ingress.go | head -20Repository: openshift/api Length of output: 3591 🏁 Script executed: # Find the Ingress CRD in generated manifests
rg -n 'name: ingress' operator/v1/zz_generated.featuregated-crd-manifests.yaml -A 50 | head -100
# Also search for any field-level feature gate examples
rg -n '\+openshift.*FeatureGate' . --type go -B2 -A2 | head -60Repository: openshift/api Length of output: 6179 🏁 Script executed: # Check type-level annotations for AccessLogging
sed -n '1488,1495p' operator/v1/types_ingress.goRepository: openshift/api Length of output: 257 Add feature gate annotation to the new log-format fields. The Add the annotation to each field: (Similarly for 🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment.
I suggest naming the feature gate "IngressControllerHTTPSLogFormat" in #2773 (comment); if you implement that suggestion, then you will need to add There was a problem hiding this comment.
To summarize the corrected guidance: if the feature gate is renamed to
|
||||||
| ) | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1495,13 +1495,29 @@ type AccessLogging struct { | |||||||||||||||||||||||
| // +required | ||||||||||||||||||||||||
| Destination LoggingDestination `json:"destination"` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // tcpLogFormat specifies the format of the log message for a TCP | ||||||||||||||||||||||||
| // request. | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. the indentation seems off. Are these mixing tabs and spaces? Could you fix the indentation in this file?
Comment on lines
+1498
to
+1499
Contributor
There was a problem hiding this comment. It might be useful to say "TCP connection" and explain that there may be separate log messages for the TCP connection (for the public_ssl frontend) and for the HTTP connection (for the fe_sni frontend):
Suggested change
|
||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // If this field is empty, log messages use the implementation's default | ||||||||||||||||||||||||
| // TCP log format. For HAProxy's default TCP log format, see the | ||||||||||||||||||||||||
| // HAProxy documentation: | ||||||||||||||||||||||||
| // http//docs.haproxy.org/2.8/configuration.html#8.2.2 | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Fix broken HAProxy doc links and typo in field docs. Line 1504 and Line 1520 use Suggested doc fix- // http//docs.haproxy.org/2.8/configuration.html#8.2.2
+ // http://docs.haproxy.org/2.8/configuration.html#8.2.2
...
- // http//docs.haproxy.org/2.8/configuration.html#8.2.3
+ // http://docs.haproxy.org/2.8/configuration.html#8.2.3
...
- // httpsLogFormat specifies the format of the log messsage for an HTTPS
+ // httpsLogFormat specifies the format of the log message for an HTTPSAlso applies to: 1520-1520, 1531-1531 🤖 Prompt for AI AgentsThere was a problem hiding this comment.
Failed to handle agent chat message. Please try again. |
||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // Note that this format only applies to TCP connections. It only affects | ||||||||||||||||||||||||
| // the log format for TLS passthrough connections. | ||||||||||||||||||||||||
|
Comment on lines
+1506
to
+1507
Contributor
There was a problem hiding this comment. We talked about this today, but I'll note here: This log format applies to TLS reencrypt and edge-terminated routes as well, because for edge-terminated and reencrypt routes, the client connection first goes to the public_ssl frontend (which logs the TCP connection) and then gets forwarded to the fe_sni frontend (which logs the HTTP session). |
||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // +kubebuilder:validation:MinLength=1 | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. For validations such as these, please have their corresponding requirements explicit in the godoc (example: |
||||||||||||||||||||||||
| // +kubebuilder:validation:MaxLength=1024 | ||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||
| TcpLogFormat string `json:"tcpLogFormat,omitempty"` | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. As noted by coderabbit in https://github.com/openshift/api/pull/2773/changes#r2963291054, the new fields should have featuregate markers. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // httpLogFormat specifies the format of the log message for an HTTP | ||||||||||||||||||||||||
| // request. | ||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // If this field is empty, log messages use the implementation's default | ||||||||||||||||||||||||
| // HTTP log format. For HAProxy's default HTTP log format, see the | ||||||||||||||||||||||||
| // HAProxy documentation: | ||||||||||||||||||||||||
| // http//docs.haproxy.org/2.8/configuration.html#8.2.3 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // Note that this format only applies to cleartext HTTP connections | ||||||||||||||||||||||||
| // and to secure HTTP connections for which the ingress controller | ||||||||||||||||||||||||
|
|
@@ -1512,6 +1528,24 @@ type AccessLogging struct { | |||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||
| HttpLogFormat string `json:"httpLogFormat,omitempty"` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // httpsLogFormat specifies the format of the log messsage for an HTTPS | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. typo, |
||||||||||||||||||||||||
| // request. | ||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // If this field is empty, log messages use the implementation's default | ||||||||||||||||||||||||
| // HTTPS log format. For HAProxy's default HTTPS log format, see the | ||||||||||||||||||||||||
| // HAProxy documentation: | ||||||||||||||||||||||||
| // http://docs.haproxy.org/2.8/configuration.html#8.2.4 | ||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // Note that this format only applies to HTTPS connections for which the | ||||||||||||||||||||||||
| // ingress controller terminates encryption (that is, edge-terminated or | ||||||||||||||||||||||||
| // reencrypt connections). It does not affect the log format for TLS | ||||||||||||||||||||||||
| // passthrough connections. | ||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
| // +kubebuilder:validation:MinLength=1 | ||||||||||||||||||||||||
| // +kubebuilder:validation:MaxLength=1024 | ||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||
| HttpsLogFormat string `json:"httpsLogFormat,omitempty"` | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // httpCaptureHeaders defines HTTP headers that should be captured in | ||||||||||||||||||||||||
| // access logs. If this field is empty, no headers are captured. | ||||||||||||||||||||||||
| // | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.