fix: OCPBUGS-93456: changes NodePortPublishingStrategy.Port#9002
fix: OCPBUGS-93456: changes NodePortPublishingStrategy.Port#9002mehabhalodiya wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-critical
|
@mehabhalodiya: This pull request references Jira Issue OCPBUGS-93456, which is invalid:
Comment 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. |
📝 WalkthroughWalkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mehabhalodiya 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 |
openshift-ci
Bot
added
the
area/testing
… to *int32 With this fix, omitempty no longer drops port: 0 during serialization. Signed-off-by: mehabhalodiya <mehabhalodiya@gmail.com>
![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)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
5950-5961: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover omitted ports separately from explicit
0.
Port: ptr.To(int32(0))does not exercise the newnilpath. Add a table case withPort: nilto verify omitted ports are also accepted as dynamically assigned.As per coding guidelines, “Unit test any code changes and additions.”
🤖 Prompt for 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. In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 5950 - 5961, Extend the table-driven tests around the “port 0 for dynamic assignment” case with a separate NodePortPublishingStrategy case whose Port is nil. Verify that an omitted port is accepted for dynamic assignment, while retaining the existing explicit zero-port coverage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 5950-5961: Extend the table-driven tests around the “port 0 for
dynamic assignment” case with a separate NodePortPublishingStrategy case whose
Port is nil. Verify that an omitted port is accepted for dynamic assignment,
while retaining the existing explicit zero-port coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6a2d7d9c-9aaa-44d9-8af6-e3d48de1414f
⛔ Files ignored due to path filters (3)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (13)
api/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gohypershift-operator/controllers/hostedcluster/network_policies_test.gotest/e2e/util/util.go
10bc860 to
33bf9a3
Compare
mehabhalodiya
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9002 +/- ##
==========================================
+ Coverage 43.79% 44.11% +0.31%
==========================================
Files 772 772
Lines 96037 96109 +72
==========================================
+ Hits 42061 42399 +338
+ Misses 51061 50767 -294
- Partials 2915 2943 +28
... and 19 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Now I have all the evidence needed. Here is the final analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe In Tekton's reconciler ( Key evidence this is infrastructure-only and not code-related:
Recommendations
Evidence
|
|
@mehabhalodiya: 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. |
|
/uncc |
| @@ -1102,7 +1102,7 @@ type NodePortPublishingStrategy struct { | |||
| // port is the port of the NodePort service. If <=0, the port is dynamically | |||
There was a problem hiding this comment.
We should also document what a nil value here means with the new type.
|
@csrwng @enxebre @mehabhalodiya was this API change proposed somewhere? I see no reference to a discussion in the Jira, etc. I haven't reviewed this yet, but generally I'm wary of API changes framed as bugfixes, and especially without an up front API design review by the team /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
| // assigned when the service is created. | ||
| // +optional | ||
| Port int32 `json:"port,omitempty"` | ||
| Port *int32 `json:"port,omitempty"` |
There was a problem hiding this comment.
What is the expected behaviour of this field if the field is omitted? E.g. in the future when this is a nil pointer, what would you expect to happen?
There was a problem hiding this comment.
This is what Claude is suggesting to me (new area, took this bug as an opportunity to learn) -
When
portis omitted (nil pointer), the behavior is the same asport: 0— Kubernetes dynamically assigns a NodePort when the service is created.All consumers already handle this correctly:
- KAS/OAuth/Konnectivity service controllers: check
strategy.NodePort.Port != nilbefore using the value — when nil, they skip settingportSpec.NodePort, so Kubernetes auto-assigns.- Ignition service controllers: same nil guard — skip setting the port, let Kubernetes choose.
- Port range validation: uses
ptr.Deref(svc.NodePort.Port, 0)and only validates whenport > 0— nil defaults to 0 which is excluded from range checks.The reason this field needs a pointer is not to distinguish nil from 0 semantically (they're equivalent), but to fix a serialization bug: with the previous
int32+omitempty, Go's JSON encoder droppedport: 0from the serialized output. When the reconciler read a HostedCluster withport: 0, serialized it back (now missing the field), and calledclient.Update(), the CEL ruleself.services == oldSelf.servicesrejected the update as "Services is immutable" — creating a permanent deadlock where the reconciler couldn't even add a finalizer.With
*int32+omitempty, onlynilis omitted; an explicitport: 0is preserved in the JSON, so the round-trip is stable and the immutability check passes.
I've updated the godoc to: "If not specified or set to 0, the port is dynamically assigned when the service is created." (not yet pushed, will do)
There was a problem hiding this comment.
The immutability of services implemented by this CEL rule makes this round tripping issue a genuine problem.
Walking through the examples and possible solutions.
Today, an unstructured client can set port: 0. There is no restriction on this and the API server accepts. A structured client would see this as int32, the omitempty causes the port field to be dropped. This breaks immutability.
If we instead changed the field to drop the omitempty. An unstructured client may not set port at all. But when a structured client reads the value, they'll see a 0 int32 value, but without omit empty, they'll add port: 0 to the object. This breaks immutability.
We could prevent the 0 value from being committed (+kubebuilder:validation:XValidation:=1). In the future, no 0 value would be allowed and when the key is omitted, the behaviour is the same as setting 0 today - dynamic allocation. There is no explicit 0 option and there's only one way in the API to represent dynamic assignment. This avoid the round trip issue without changing the port field to a pointer. This is favourable, but it does not account for existing resources. Any resource that exists today would need a manual fix from an unstructured client. This may be acceptable because, AFAICT, any resource that has 0 today would never have a finalizer added and therefore would not progress to become an actual cluster. Users would probably have already deleted this resource? This IS a breaking change, but a breaking change that prevents an otherwise invalid/useless object I think we can permit.
Or we continue to allow the 0 value as an explicit "I want a dynamically allocated port". If we want to do that, we must fix the round trip, and AFAICT the only way to do that is to change the field to *int32 as laid out in this PR. It does correctly maintain whatever the unstructured client applied and would solve the immutability issue. The cons of changing this to a pointer is that we do not control every usage of this field. There will be end users out there who build tooling atop our client libraries. Those who write to this field will need to update their code to account for the change, the compiler will complain if they don't. Those who read this field will be exposed to NPEs as a result of this change, that's a pretty nasty change and not something the compiler would necessarily complain about, so this could break their code in the wild.
So, do we, or do we not want to allow unstructured users to explicitly set port: 0?
6 participants
The fix changes
NodePortPublishingStrategy.Portfromint32to*int32across 16 files, soomitemptyno longer dropsport: 0during serialization. This breaks the deadlock where the reconciler could never add its finalizer because the webhook rejected the round-tripped object as having mutated the immutable Services field.What this PR does / why we need it:
Root cause:
NodePortPublishingStrategy.Portwas declared asint32withjson:"port,omitempty". Go'somitemptyomits zero-valued integers, so when the UI creates a HostedCluster withnodePort.port: 0(dynamic assignment), the reconciler'sclient.Update()serializes the struct without the port field. The webhook then rejects the update because Services appears to have changed — permanent deadlock, no finalizer can ever be added.Fix: Changed
Portfromint32to*int32in hostedcluster_types.go:1105. With a pointer,omitemptyonly omits when the pointer isnil(field truly absent), not when the value is0(explicitly set to zero for dynamic assignment). This follows the API convention: "The only reason to use a pointer in a CRD is when the zero value is a valid, distinct user choice."Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-93456
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests