NE-2117: Add httpsLogFormat and tcpLogFormat to API#2773
NE-2117: Add httpsLogFormat and tcpLogFormat to API#2773rohara wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @rohara! Some important instructions when contributing to openshift/api: |
|
@rohara: This pull request references NE-2117 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request introduces a new ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
openshift-ci
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/v1/types_ingress.go (1)
1522-1526:⚠️ Potential issue | 🟠 MajorClarify
httpLogFormatvshttpsLogFormatprecedence for HTTPS traffic.Line 1522 says
httpLogFormatapplies to terminated HTTPS, while Lines 1539-1542 sayhttpsLogFormatalso applies there. Please make the behavior explicit (precedence or mutually exclusive scope), otherwise HTTPS log formatting is ambiguous when both are set.Also applies to: 1539-1542
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/types_ingress.go` around lines 1522 - 1526, Update the comment text in operator/v1/types_ingress.go to explicitly resolve ambiguity between httpLogFormat and httpsLogFormat for terminated HTTPS: state that for TLS-terminating ingress connections (edge-terminated or reencrypt) httpsLogFormat takes precedence over httpLogFormat when both are specified, and otherwise httpLogFormat is used; apply this clarification in the existing comment block that starts "Note that this format only applies..." (referencing httpLogFormat) and in the later block that mentions httpsLogFormat so both places describe the same precedence rule and mutually consistent scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/features.go`:
- Around line 1024-1030: Add the feature-gate field annotation to the
AccessLogging struct fields so the fields are only accepted when the
FeatureGateHTTPSLogFormat gate is enabled: add the comment line
"+openshift:enable:FeatureGate=HTTPSLogFormat" immediately above the
TcpLogFormat, HttpLogFormat, and HttpsLogFormat field declarations (keeping the
existing "+optional" and the json `omitempty` tags intact) in the AccessLogging
type so the API server enforces the HTTPSLogFormat gate defined by
FeatureGateHTTPSLogFormat.
In `@operator/v1/types_ingress.go`:
- Line 1504: Fix the broken HAProxy doc links and the typo in the field docs by
replacing the malformed "http//docs.haproxy.org/2.8/configuration.html#8.2.2"
occurrences with "http://docs.haproxy.org/2.8/configuration.html#8.2.2" and
correct the misspelling "messsage" to "message" wherever they appear in the
ingress type comments (search for the literal strings "http//docs.haproxy.org"
and "messsage" in operator/v1/types_ingress.go to locate the affected struct
field docs and update them).
In `@operator/v1/zz_generated.swagger_doc_generated.go`:
- Around line 852-854: Update the source comment in operator/v1/types_ingress.go
for the AccessLogging fields so the docs are consistent: correct tcpLogFormat
and httpLogFormat HAProxy links to include "http://" (fix "http//" ->
"http://"), ensure httpLogFormat explicitly states it applies only to cleartext
HTTP connections (not edge/reencrypt HTTPS), and ensure httpsLogFormat describes
HTTPS requests for which the ingress controller terminates encryption
(edge-terminated or reencrypt) and does not overlap with httpLogFormat; after
editing the comments for the fields tcpLogFormat, httpLogFormat, and
httpsLogFormat regenerate zz_generated.swagger_doc_generated.go so the generated
docstrings reflect these fixes.
---
Outside diff comments:
In `@operator/v1/types_ingress.go`:
- Around line 1522-1526: Update the comment text in operator/v1/types_ingress.go
to explicitly resolve ambiguity between httpLogFormat and httpsLogFormat for
terminated HTTPS: state that for TLS-terminating ingress connections
(edge-terminated or reencrypt) httpsLogFormat takes precedence over
httpLogFormat when both are specified, and otherwise httpLogFormat is used;
apply this clarification in the existing comment block that starts "Note that
this format only applies..." (referencing httpLogFormat) and in the later block
that mentions httpsLogFormat so both places describe the same precedence rule
and mutually consistent scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 32770b05-8cde-4b22-ab82-5da94b479999
⛔ Files ignored due to path filters (3)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (12)
features.mdfeatures/features.gooperator/v1/types_ingress.gooperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
| FeatureGateHTTPSLogFormat = newFeatureGate("HTTPSLogFormat"). | ||
| reportProblemsToJiraComponent("Networking/router"). | ||
| contactPerson("rohara"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/1832"). | ||
| enable(inDevPreviewNoUpgrade(), inTechPreviewNoUpgrade()). | ||
| mustRegister() |
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 HTTPSLogFormat gate is registered in features.go, but the corresponding fields in the AccessLogging struct lack the required field-level annotation. Without +openshift:enable:FeatureGate=HTTPSLogFormat markers on TcpLogFormat, HttpLogFormat, and HttpsLogFormat fields in operator/v1/types_ingress.go, the API will accept these fields regardless of the gate status.
Add the annotation to each field:
// +openshift:enable:FeatureGate=HTTPSLogFormat
// +optional
TcpLogFormat string `json:"tcpLogFormat,omitempty"`
(Similarly for HttpLogFormat and HttpsLogFormat.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/features.go` around lines 1024 - 1030, Add the feature-gate field
annotation to the AccessLogging struct fields so the fields are only accepted
when the FeatureGateHTTPSLogFormat gate is enabled: add the comment line
"+openshift:enable:FeatureGate=HTTPSLogFormat" immediately above the
TcpLogFormat, HttpLogFormat, and HttpsLogFormat field declarations (keeping the
existing "+optional" and the json `omitempty` tags intact) in the AccessLogging
type so the API server enforces the HTTPSLogFormat gate defined by
FeatureGateHTTPSLogFormat.
| // 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 http//... (missing :), and Line 1531 has messsage typo. These will propagate into generated API docs.
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 Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/v1/types_ingress.go` at line 1504, Fix the broken HAProxy doc links
and the typo in the field docs by replacing the malformed
"http//docs.haproxy.org/2.8/configuration.html#8.2.2" occurrences with
"http://docs.haproxy.org/2.8/configuration.html#8.2.2" and correct the
misspelling "messsage" to "message" wherever they appear in the ingress type
comments (search for the literal strings "http//docs.haproxy.org" and "messsage"
in operator/v1/types_ingress.go to locate the affected struct field docs and
update them).
| "tcpLogFormat": "tcpLogFormat specifies the format of the log message for a TCP request.\n\nIf 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\n\nNote that this format only applies to TCP connections. It only affects the log format for TLS passthrough connections.", | ||
| "httpLogFormat": "httpLogFormat specifies the format of the log message for an HTTP request.\n\nIf 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\n\nNote that this format only applies to cleartext HTTP connections and to secure HTTP 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.", | ||
| "httpsLogFormat": "httpsLogFormat specifies the format of the log messsage for an HTTPS request.\n\nIf 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\n\nNote 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.", |
There was a problem hiding this comment.
The new AccessLogging docs are internally inconsistent.
As written, httpLogFormat still says it applies to edge/reencrypt traffic, so the public API docs now say both httpLogFormat and httpsLogFormat govern HTTPS requests. tcpLogFormat and httpLogFormat also have malformed HAProxy links (http//...). Please fix the source comments in operator/v1/types_ingress.go and regenerate this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/v1/zz_generated.swagger_doc_generated.go` around lines 852 - 854,
Update the source comment in operator/v1/types_ingress.go for the AccessLogging
fields so the docs are consistent: correct tcpLogFormat and httpLogFormat
HAProxy links to include "http://" (fix "http//" -> "http://"), ensure
httpLogFormat explicitly states it applies only to cleartext HTTP connections
(not edge/reencrypt HTTPS), and ensure httpsLogFormat describes HTTPS requests
for which the ingress controller terminates encryption (edge-terminated or
reencrypt) and does not overlap with httpLogFormat; after editing the comments
for the fields tcpLogFormat, httpLogFormat, and httpsLogFormat regenerate
zz_generated.swagger_doc_generated.go so the generated docstrings reflect these
fixes.
|
@rohara: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Add both httpsLogFormat and tcpLogFormat to ingress API. These log-format strings belong in AccessLogging struct and will provide the ability to customize both HTTPS and TCP log formats when applicable.