OCPBUGS-84491: Set terminationMessagePolicy on gateway proxy containers#1428
OCPBUGS-84491: Set terminationMessagePolicy on gateway proxy containers#1428gcs278 wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@gcs278: This pull request references Jira Issue OCPBUGS-84491, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
📝 WalkthroughWalkthroughThe PR replaces the previous HPA-focused config builder with a new 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/controller/gatewayclass/controller_test.go (1)
186-196:⚠️ Potential issue | 🟠 MajorSort the expected GatewayClass names before serializing JSON.
gatewayclassesConfigpreserves caller order, butbuildGatewayClassesConfigmarshals a Go map (line 287 of istio_sail_installer.go), which causesjson.Marshalto sort keys lexicographically. The multi-class case at Line 697 passes"openshift-default", "openshift-internal", "openshift-custom", which produces JSON keys in a different order than the production code's alphabetically sorted output.🔧 Proposed fix
+import "sort" + func gatewayclassesConfig(config string, gatewayclasses ...string) json.RawMessage { + sort.Strings(gatewayclasses) return json.RawMessage(fmt.Appendf(nil, `{%s}`, strings.Join(func() []string { var result []string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/gatewayclass/controller_test.go` around lines 186 - 196, The test helper gatewayclassesConfig preserves caller order, but buildGatewayClassesConfig marshals a map which results in lexicographically sorted keys; to make tests match production, sort the gatewayclasses slice before building the JSON in gatewayclassesConfig so the serialized keys are in alphabetical order (e.g., apply a sort.Strings(gatewayclasses) step inside gatewayclassesConfig) so multi-class cases like the test passing "openshift-default", "openshift-internal", "openshift-custom" produce the same key order as buildGatewayClassesConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/operator/controller/gatewayclass/controller_test.go`:
- Around line 186-196: The test helper gatewayclassesConfig preserves caller
order, but buildGatewayClassesConfig marshals a map which results in
lexicographically sorted keys; to make tests match production, sort the
gatewayclasses slice before building the JSON in gatewayclassesConfig so the
serialized keys are in alphabetical order (e.g., apply a
sort.Strings(gatewayclasses) step inside gatewayclassesConfig) so multi-class
cases like the test passing "openshift-default", "openshift-internal",
"openshift-custom" produce the same key order as buildGatewayClassesConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e96c66c0-732d-460d-96d0-f1693667cb2b
📒 Files selected for processing (5)
pkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio_olm.gopkg/operator/controller/gatewayclass/istio_sail_installer.gotest/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
gcs278
force-pushed
the
OCPBUGS-84491-termination-message-policy
branch
from
c411daf to
4a97fc4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/controller/gatewayclass/controller_test.go (1)
187-200: ⚡ Quick winBuild the fixture from structured data instead of hand-assembling JSON.
Sorting the input slice and concatenating strings makes this helper side-effectful and easy to drift from
buildGatewayClassesConfig. A smallmap[string]json.RawMessage+json.Marshalhere would mirror production, remove the manual sort, and avoid future JSON-shape mismatches.♻️ Proposed fix
func gatewayclassesConfig(config string, gatewayclasses ...string) json.RawMessage { - sort.Strings(gatewayclasses) - return fmt.Appendf(nil, `{%s}`, strings.Join(func() []string { - var result []string - - for _, name := range gatewayclasses { - result = append(result, fmt.Sprintf(`"%s":%s`, name, config)) - } - - return result - }(), ",")) + payload := make(map[string]json.RawMessage, len(gatewayclasses)) + for _, name := range gatewayclasses { + payload[name] = json.RawMessage(config) + } + b, _ := json.Marshal(payload) + return b }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/gatewayclass/controller_test.go` around lines 187 - 200, The helpers gatewayclassesConfig and gatewayclassConfig should build JSON from structured data instead of hand-assembling strings: replace gatewayclassesConfig's manual sort/concatenation with constructing a map[string]json.RawMessage (or map[string]interface{}) populated by iterating gatewayclasses and using json.RawMessage of gatewayclassConfig, then json.Marshal the map; change gatewayclassConfig to return a structured value (map or struct) or json.RawMessage built via json.Marshal rather than a formatted string so the test fixture mirrors buildGatewayClassesConfig and avoids ordering/shape drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/controller/gatewayclass/controller_test.go`:
- Around line 187-200: The helpers gatewayclassesConfig and gatewayclassConfig
should build JSON from structured data instead of hand-assembling strings:
replace gatewayclassesConfig's manual sort/concatenation with constructing a
map[string]json.RawMessage (or map[string]interface{}) populated by iterating
gatewayclasses and using json.RawMessage of gatewayclassConfig, then
json.Marshal the map; change gatewayclassConfig to return a structured value
(map or struct) or json.RawMessage built via json.Marshal rather than a
formatted string so the test fixture mirrors buildGatewayClassesConfig and
avoids ordering/shape drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eeb1b9d7-ee49-4780-8807-96c392a0fb42
📒 Files selected for processing (5)
pkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio_olm.gopkg/operator/controller/gatewayclass/istio_sail_installer.gotest/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/util_gatewayapi_test.go
- pkg/operator/controller/gatewayclass/istio_olm.go
- pkg/operator/controller/gatewayclass/istio_sail_installer.go
gcs278
force-pushed
the
OCPBUGS-84491-termination-message-policy
branch
from
4a97fc4 to
65ca117
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/util_gatewayapi_test.go`:
- Around line 830-836: The test currently only verifies terminationMessagePolicy
for all containers but doesn't assert the presence of the istio-proxy container;
update the check to first ensure a container with Name == "istio-proxy" exists
in dep.Spec.Template.Spec.Containers, fail/return false if not found, and then
verify that container's TerminationMessagePolicy equals
corev1.TerminationMessageFallbackToLogsOnError (or, alternatively, locate the
istio-proxy container by iterating dep.Spec.Template.Spec.Containers and assert
c.Name == "istio-proxy" before checking c.TerminationMessagePolicy); reference
the existing loop and symbols c.Name, c.TerminationMessagePolicy, and
corev1.TerminationMessageFallbackToLogsOnError when implementing this change.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1700a141-8bcd-4cf5-a05f-d26b7328c988
📒 Files selected for processing (5)
pkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio_olm.gopkg/operator/controller/gatewayclass/istio_sail_installer.gotest/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/gateway_api_test.go
|
hypershift flake techpreview is failing because #1408 needs to merge |
|
#1408 has merged. /test e2e-aws-operator-techpreview |
|
change lgtm, thanks @gcs278 let's wait now for verified label |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Add a deployment overlay to the gatewayclass configuration that sets terminationMessagePolicy=FallbackToLogsOnError on the istio-proxy container, satisfying the OCP platform policy enforced by the termination-message-policy monitor test. https://redhat.atlassian.net/browse/OCPBUGS-84491 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gcs278
force-pushed
the
OCPBUGS-84491-termination-message-policy
branch
from
65ca117 to
da0bcdf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/controller/gatewayclass/istio_sail_installer.go`:
- Around line 265-278: The overlay currently sets
deployment.spec.template.spec.containers to a single-entry list (using
gatewayProxyContainerName), which will replace the entire containers list under
Strategic Merge Patch and drop other containers; fix it by removing this partial
"containers" entry from the overlay or by replacing it with a targeted patch
that modifies only the istio-proxy container's terminationMessagePolicy (e.g.,
use a JSONPatch/JSON6902 or a strategic-merge entry that includes the container
"name" and full container spec), referencing the
deployment.spec.template.spec.containers overlay and gatewayProxyContainerName
so you either supply all containers in that list or use a patch that updates
only terminationMessagePolicy for the container named by
gatewayProxyContainerName.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4c9190ad-d24a-4277-a60d-ea81a9f44b3c
📒 Files selected for processing (6)
pkg/operator/controller/gatewayclass/controller.gopkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio_olm.gopkg/operator/controller/gatewayclass/istio_sail_installer.gotest/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/gateway_api_test.go
|
/retest-required |
|
@gcs278: The following test 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. |
|
/lgtm |
|
Update - we may get this for free in https://github.com/istio/istio/pull/60102/changes#diff-25656c65a83f75b949b39ee2750b8b72d0936e4aece66265979cee93a3326892 (we don't need this PR if that merges) |
openshift-ci
Bot
added
the
do-not-merge/hold
5 participants
Summary
terminationMessagePolicy=FallbackToLogsOnErroron theistio-proxycontainer, satisfying the OCP platform policy enforced by thetermination-message-policymonitor test.buildHorizontalPodAutoscalerConfigtobuildGatewayClassesConfigsince it now builds all per-gatewayclass overlays (HPA + deployment).ensureGatewayObjectSuccessto verify the policy is applied.Test plan
go test ./pkg/operator/controller/gatewayclass/...)TestGatewayAPIverifiesterminationMessagePolicy=FallbackToLogsOnErroron the gateway proxy deployment🤖 Generated with Claude Code