feat/add support for hostPort in container #2763
Conversation
|
|
|
@bmiguel-teixeira could you please sign the EasyCLA ? |
|
Hi @pavolloffay CLA signed. The PR is now ready to review. Additionally, i added a quick e2e for the ports fields, however it looks like all requests are getting mutated/converted to v1alpha1 model even though the storage version is v1beta. Looks like i caught the project in the transition to the v1beta1 version. I am unsure how you guys want to handle it? Cheers, |
|
Related to #2180 |
|
@bmiguel-teixeira you're gonna get slow responses from maintainers this week bc of Kubecon in Paris. I believe we'll want to include the feature to both versions since we're still releasing alpha. |
|
v1alpah1 updated. e2e now passing. No rush. Enjoy kubecon |
|
@bmiguel-teixeira please sign the CLA |
|
fixed missing tests. CLA signed. Had some trouble with my commit signature. Cheers |
| @@ -291,6 +291,16 @@ type OpenTelemetryCollectorSpec struct { | |||
| DeploymentUpdateStrategy appsv1.DeploymentStrategy `json:"deploymentUpdateStrategy,omitempty"` | |||
| } | |||
|
|
|||
| // PortsSpec defines the OpenTelemetryCollector's container/service ports additional specifications. | |||
| type PortsSpec struct { | |||
There was a problem hiding this comment.
This ends up being a breaking change, so maybe I was wrong when I said it should live in the alpha api. @open-telemetry/operator-maintainers can you confirm?
There was a problem hiding this comment.
Is it? Looks like we're just adding fields to the CRDs. I'm personally a bit worried that we're reinventing the wheel with this PortSpec definition, and the community has converged on some standard for this "ContainerPort and ServicePort in one" concept.
There was a problem hiding this comment.
I put my comment in the wrong spot.
Changing from Ports []v1.ServicePort json:"ports,omitempty" to Ports []PortsSpec json:"ports,omitempty" is the breaking change I was referring to.
There was a problem hiding this comment.
hi @swiatekm-sumo
While I aggree that mixing values for different reasons can be messy, the current ports definition already present in the spec is used both to populate the Service object and the ContainerPorts object.
We could do a dedicated new field or something, but then we would have multiples places for settings being used in also multiples places.
Putting in the same port context seemed the cleanest place.
There was a problem hiding this comment.
@TylerHelmuth Can you elaborate on how this is a breaking change? the interface/spec with the client remains the same, minus the new field hostPort.
There was a problem hiding this comment.
@bmiguel-teixeira I agree with your approach here. My point was that this sounds like a problem other operators also had to solve, and I'm wondering if there's some de-facto standard way of doing it.
@TylerHelmuth PortsSpec embeds ServicePort, so this doesn't look like a breaking change from the perspective of a CRD user. Can you describe a specific situation where this would break someone?
There was a problem hiding this comment.
I misinterpreted what ServicePort represented, I didn't realize it was a full struct. I agree this is being done in a non-breaking way.
There was a problem hiding this comment.
yeah I looked around and didn't see anything from the obvious sources...
|
Was literally just looking for this feature to handle daemonset targeting, thanks! |
|
@bmiguel-teixeira could you rebase when you get a chance? |
Signed-off-by: Bruno Teixeira <[email protected]>
|
hey @jaronoff97 Rebased & all checks passing. Cheers |
|
hey @swiatekm-sumo Ty |
|
@bmiguel-teixeira thank you very much for your contribution!! This will be included in the next release. |
Signed-off-by: Bruno Teixeira <[email protected]>
Signed-off-by: Bruno Teixeira <[email protected]>
Description:
This PR adds supports for defining additional behavior on the ports section. More specifically, I added the
hostPortfield that maps to the equivalent field in the containerPort.This allows the container to bind the hostPort when running in daemonset mode.
Link to tracking Issue(s):
Related to #2180
Testing:
Documentation: