NE-2115: Implement httpsLogFormat and tcpLogFormat for Ingress Operator#1274
NE-2115: Implement httpsLogFormat and tcpLogFormat for Ingress Operator#1274rohara wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rohara: This pull request references NE-2115 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.20.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. |
|
/assign @rikatz |
| RouterTCPLogFormatEnvName = "ROUTER_TCP_LOG_FORMAT" | ||
| RouterHTTPLogFormatEnvName = "ROUTER_HTTP_LOG_FORMAT" | ||
| RouterHTTPSLogFormatEnvName = "ROUTER_HTTPS_LOG_FORMAT" |
There was a problem hiding this comment.
Need to run gofmt on this.
| } | ||
|
|
||
| if len(accessLogging.TcpLogFormat) > 0 { | ||
| env = append(env, corev1.EnvVar{Name: RouterTCPLogFormatEnvName, Value: fmt.Sprintf("%q", accessLogging.TcpLogFormat)}) |
There was a problem hiding this comment.
Where do we validate the format for these?
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
/reopen |
|
@rohara: Reopened this PR. 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 kubernetes-sigs/prow repository. |
|
@rohara: This pull request references NE-2115 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. |
openshift-ci
Bot
removed
the
lifecycle/rotten
|
[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 |
📝 WalkthroughWalkthroughThis change refactors the router's access logging format configuration by replacing a single syslog format constant with three protocol-specific logging format constants: TCP, HTTP, and HTTPS. The deployment configuration logic is updated to conditionally append these format environment variables when their corresponding values are non-empty. The ✨ 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: 3
🧹 Nitpick comments (1)
test/e2e/operator_test.go (1)
2364-2366: Use distinct per-protocol sentinels to make these e2e checks meaningful.Because TCP/HTTP/HTTPS formats are set to the same payload, these tests can still pass if the wrong field is wired for HTTPS requests. Give each format a unique prefix and assert the HTTPS-specific one in the log scan.
Example tightening
- TcpLogFormat: "8192" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 120), - HttpLogFormat: "8192" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 120), - HttpsLogFormat: "8192" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 120), + TcpLogFormat: "TCP8192" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 120), + HttpLogFormat: "HTTP8192" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 120), + HttpsLogFormat: "HTTPS8192" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 120),- TcpLogFormat: "0480" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 40), - HttpLogFormat: "0480" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 40), - HttpsLogFormat: "0480" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 40), + TcpLogFormat: "TCP0480" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 40), + HttpLogFormat: "HTTP0480" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 40), + HttpsLogFormat: "HTTPS0480" + strings.Repeat("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789@@@@@@@=", 40),And update the scanner checks to match
HTTPS8192.../HTTPS0480....Also applies to: 2508-2510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/operator_test.go` around lines 2364 - 2366, The three log format strings (TcpLogFormat, HttpLogFormat, HttpsLogFormat) currently use identical payloads so tests won't detect protocol-specific wiring; change each to include a distinct per-protocol sentinel prefix (e.g., "TCP8192...", "HTTP8192...", "HTTPS8192..." or similar unique markers) in the test data and then update the log-scanner assertions that check the captured logs (the scanner/checks referencing the HTTPS-specific payload) to assert against the HTTPS-specific sentinel (and analogous asserts for TCP/HTTP where applicable) so the e2e checks verify the correct field is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/controller/ingress/deployment_test.go`:
- Around line 633-635: The test currently sets TcpLogFormat, HttpLogFormat and
HttpsLogFormat but only asserts the legacy ROUTER_SYSLOG_FORMAT env var; update
the assertions to also check the protocol-specific env vars
ROUTER_TCP_LOG_FORMAT, ROUTER_HTTP_LOG_FORMAT and ROUTER_HTTPS_LOG_FORMAT match
the values in TcpLogFormat, HttpLogFormat and HttpsLogFormat respectively
(alongside the existing check for ROUTER_SYSLOG_FORMAT). Locate the assertions
around the deployment test (the test that builds env vars from
TcpLogFormat/HttpLogFormat/HttpsLogFormat in deployment_test.go) and add three
new equality assertions verifying each ROUTER_*_LOG_FORMAT env value equals the
corresponding struct field.
In `@pkg/operator/controller/ingress/deployment.go`:
- Around line 57-60: Update the failing tests in deployment_test.go to stop
using the old hardcoded "ROUTER_SYSLOG_FORMAT" and instead reference the new
constants RouterTCPLogFormatEnvName, RouterHTTPLogFormatEnvName, and
RouterHTTPSLogFormatEnvName (or remove the obsolete assertions) where those env
var names are asserted at the former lines; locate the assertions that compare
env var names or expected env entries and replace the literal string with the
appropriate constant (RouterTCPLogFormatEnvName for TCP cases,
RouterHTTPLogFormatEnvName for HTTP cases, RouterHTTPSLogFormatEnvName for HTTPS
cases) so tests align with the updated deployment.go constants.
- Around line 913-921: The code references missing fields TcpLogFormat and
HttpsLogFormat on the AccessLogging struct causing build failures; update the
vendored OpenShift API so AccessLogging includes these fields (or vendor a newer
version of github.com/openshift/api/operator/v1 that defines TcpLogFormat and
HttpsLogFormat), then run go mod tidy && go mod vendor to refresh vendor files;
after vendoring, rebuild to ensure the references to AccessLogging.TcpLogFormat
and AccessLogging.HttpsLogFormat (and their usage with RouterTCPLogFormatEnvName
and RouterHTTPSLogFormatEnvName in deployment.go) compile cleanly.
---
Nitpick comments:
In `@test/e2e/operator_test.go`:
- Around line 2364-2366: The three log format strings (TcpLogFormat,
HttpLogFormat, HttpsLogFormat) currently use identical payloads so tests won't
detect protocol-specific wiring; change each to include a distinct per-protocol
sentinel prefix (e.g., "TCP8192...", "HTTP8192...", "HTTPS8192..." or similar
unique markers) in the test data and then update the log-scanner assertions that
check the captured logs (the scanner/checks referencing the HTTPS-specific
payload) to assert against the HTTPS-specific sentinel (and analogous asserts
for TCP/HTTP where applicable) so the e2e checks verify the correct field is
used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d3ec0f95-aa7a-4893-8959-0888321e5028
📒 Files selected for processing (3)
pkg/operator/controller/ingress/deployment.gopkg/operator/controller/ingress/deployment_test.gotest/e2e/operator_test.go
| TcpLogFormat: "%ci:%cp [%t] %ft %b/%s %Tw/%Tc/%Tt %B %ts", | ||
| HttpLogFormat: "%ci:%cp [%t] %ft %b/%s %B %bq %HM %HU %HV", | ||
| HttpsLogFormat: "%ci:%cp [%t] %ft %b/%s %B %bq %HM %HU %HV %sslv/%sslc", |
There was a problem hiding this comment.
Add assertions for the new protocol-specific log format env vars.
The test now sets TcpLogFormat and HttpsLogFormat, but it still validates only legacy ROUTER_SYSLOG_FORMAT and never checks ROUTER_TCP_LOG_FORMAT / ROUTER_HTTPS_LOG_FORMAT (or ROUTER_HTTP_LOG_FORMAT). This leaves the new wiring unverified.
✅ Suggested assertion update
- {"ROUTER_SYSLOG_FORMAT", true, `"%ci:%cp [%t] %ft %b/%s %B %bq %HM %HU %HV"`},
+ {RouterTCPLogFormatEnvName, true, `"%ci:%cp [%t] %ft %b/%s %Tw/%Tc/%Tt %B %ts"`},
+ {RouterHTTPLogFormatEnvName, true, `"%ci:%cp [%t] %ft %b/%s %B %bq %HM %HU %HV"`},
+ {RouterHTTPSLogFormatEnvName, true, `"%ci:%cp [%t] %ft %b/%s %B %bq %HM %HU %HV %sslv/%sslc"`},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/ingress/deployment_test.go` around lines 633 - 635,
The test currently sets TcpLogFormat, HttpLogFormat and HttpsLogFormat but only
asserts the legacy ROUTER_SYSLOG_FORMAT env var; update the assertions to also
check the protocol-specific env vars ROUTER_TCP_LOG_FORMAT,
ROUTER_HTTP_LOG_FORMAT and ROUTER_HTTPS_LOG_FORMAT match the values in
TcpLogFormat, HttpLogFormat and HttpsLogFormat respectively (alongside the
existing check for ROUTER_SYSLOG_FORMAT). Locate the assertions around the
deployment test (the test that builds env vars from
TcpLogFormat/HttpLogFormat/HttpsLogFormat in deployment_test.go) and add three
new equality assertions verifying each ROUTER_*_LOG_FORMAT env value equals the
corresponding struct field.
| RouterTCPLogFormatEnvName = "ROUTER_TCP_LOG_FORMAT" | ||
| RouterHTTPLogFormatEnvName = "ROUTER_HTTP_LOG_FORMAT" | ||
| RouterHTTPSLogFormatEnvName = "ROUTER_HTTPS_LOG_FORMAT" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if tests still reference the old ROUTER_SYSLOG_FORMAT constant
echo "=== Searching for old ROUTER_SYSLOG_FORMAT references ==="
rg -n 'ROUTER_SYSLOG_FORMAT' pkg/operator/controller/ingress/
echo ""
echo "=== Searching for new format env var references in tests ==="
rg -n 'ROUTER_(TCP|HTTP|HTTPS)_LOG_FORMAT' pkg/operator/controller/ingress/deployment_test.goRepository: openshift/cluster-ingress-operator
Length of output: 521
🏁 Script executed:
#!/bin/bash
# Check if ROUTER_SYSLOG_FORMAT constant still exists in deployment.go
echo "=== Checking for ROUTER_SYSLOG_FORMAT definition in deployment.go ==="
rg -n 'ROUTER_SYSLOG_FORMAT' pkg/operator/controller/ingress/deployment.go
echo ""
echo "=== Getting context around test references (line 752) ==="
sed -n '745,760p' pkg/operator/controller/ingress/deployment_test.go | cat -n
echo ""
echo "=== Checking full constant definitions in deployment.go ==="
rg -n 'RouterTCPLogFormatEnvName|RouterHTTPLogFormatEnvName|RouterHTTPSLogFormatEnvName|ROUTER_SYSLOG_FORMAT' pkg/operator/controller/ingress/deployment.goRepository: openshift/cluster-ingress-operator
Length of output: 1841
Test files must be updated to use the new environment variable names.
The code now uses RouterTCPLogFormatEnvName, RouterHTTPLogFormatEnvName, and RouterHTTPSLogFormatEnvName constants, but test files in pkg/operator/controller/ingress/deployment_test.go still reference the old hardcoded string "ROUTER_SYSLOG_FORMAT" at lines 465, 752, and 972. Update these test cases to use the new environment variable names or remove references to the old format variable that no longer exists in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/ingress/deployment.go` around lines 57 - 60, Update
the failing tests in deployment_test.go to stop using the old hardcoded
"ROUTER_SYSLOG_FORMAT" and instead reference the new constants
RouterTCPLogFormatEnvName, RouterHTTPLogFormatEnvName, and
RouterHTTPSLogFormatEnvName (or remove the obsolete assertions) where those env
var names are asserted at the former lines; locate the assertions that compare
env var names or expected env entries and replace the literal string with the
appropriate constant (RouterTCPLogFormatEnvName for TCP cases,
RouterHTTPLogFormatEnvName for HTTP cases, RouterHTTPSLogFormatEnvName for HTTPS
cases) so tests align with the updated deployment.go constants.
| if len(accessLogging.TcpLogFormat) > 0 { | ||
| env = append(env, corev1.EnvVar{Name: RouterTCPLogFormatEnvName, Value: fmt.Sprintf("%q", accessLogging.TcpLogFormat)}) | ||
| } | ||
| if len(accessLogging.HttpLogFormat) > 0 { | ||
| env = append(env, corev1.EnvVar{Name: RouterSyslogFormatEnvName, Value: fmt.Sprintf("%q", accessLogging.HttpLogFormat)}) | ||
| env = append(env, corev1.EnvVar{Name: RouterHTTPLogFormatEnvName, Value: fmt.Sprintf("%q", accessLogging.HttpLogFormat)}) | ||
| } | ||
| if len(accessLogging.HttpsLogFormat) > 0 { | ||
| env = append(env, corev1.EnvVar{Name: RouterHTTPSLogFormatEnvName, Value: fmt.Sprintf("%q", accessLogging.HttpsLogFormat)}) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify TcpLogFormat and HttpsLogFormat fields exist in the AccessLogging struct
# Search for field definitions in the vendored API types
echo "=== Checking AccessLogging struct for new fields ==="
rg -n -A 50 'type AccessLogging struct' vendor/github.com/openshift/api/operator/v1/types_ingress.go | head -80
echo ""
echo "=== Searching for TcpLogFormat field ==="
rg -n 'TcpLogFormat' vendor/github.com/openshift/api/operator/v1/
echo ""
echo "=== Searching for HttpsLogFormat field ==="
rg -n 'HttpsLogFormat' vendor/github.com/openshift/api/operator/v1/Repository: openshift/cluster-ingress-operator
Length of output: 2670
🏁 Script executed:
#!/bin/bash
# Get the complete AccessLogging struct definition
echo "=== Complete AccessLogging struct definition ==="
rg -n -A 100 'type AccessLogging struct' vendor/github.com/openshift/api/operator/v1/types_ingress.go | grep -A 100 'type AccessLogging'Repository: openshift/cluster-ingress-operator
Length of output: 4864
🏁 Script executed:
#!/bin/bash
# Verify the code references the non-existent fields
echo "=== Code snippet at lines 913-921 in deployment.go ==="
sed -n '913,921p' pkg/operator/controller/ingress/deployment.go
echo ""
echo "=== Verify these are field accesses on accessLogging ==="
sed -n '910,925p' pkg/operator/controller/ingress/deployment.goRepository: openshift/cluster-ingress-operator
Length of output: 1429
Update the vendored OpenShift API dependency to include TcpLogFormat and HttpsLogFormat fields in the AccessLogging struct.
The current vendored AccessLogging struct at vendor/github.com/openshift/api/operator/v1/types_ingress.go:1492-1550 only defines Destination, HttpLogFormat, HTTPCaptureHeaders, HTTPCaptureCookies, and LogEmptyRequests fields. The code at lines 913-919 attempts to access TcpLogFormat and HttpsLogFormat fields which do not exist, causing a compilation failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/ingress/deployment.go` around lines 913 - 921, The
code references missing fields TcpLogFormat and HttpsLogFormat on the
AccessLogging struct causing build failures; update the vendored OpenShift API
so AccessLogging includes these fields (or vendor a newer version of
github.com/openshift/api/operator/v1 that defines TcpLogFormat and
HttpsLogFormat), then run go mod tidy && go mod vendor to refresh vendor files;
after vendoring, rebuild to ensure the references to AccessLogging.TcpLogFormat
and AccessLogging.HttpsLogFormat (and their usage with RouterTCPLogFormatEnvName
and RouterHTTPSLogFormatEnvName in deployment.go) compile cleanly.
|
@rohara: The following tests failed, say
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. |
7 participants
Add support for httpsLogFormat and tcpLogFormat. These allow users to define a custom log format for both HTTPS and TCP traffic, which configured appropriately in OpenShift Router.