feat: add support for multiple forward and http in globaInputs#1867
feat: add support for multiple forward and http in globaInputs#1867ben-dov wants to merge 3 commits intofluent:masterfrom
Conversation
|
@ben-dov Thanks for the PR! Would you please add some tests for this functionality? I realize tests didn't exist prior to this PR, but we're trying to improve our testing coverage. |
|
hey @joshuabaird, |
There was a problem hiding this comment.
Pull request overview
Adds support in the Fluentd Kubernetes resources generated by Fluent Operator to expose multiple forward and http globalInputs in a single Fluentd deployment, avoiding Kubernetes port name collisions while keeping the first port name backward-compatible.
Changes:
- Update Fluentd container port generation to suffix non-first
forward/httpport names with the port number (e.g.,forward-25224). - Update Fluentd Service port generation to use the same naming scheme and target the corresponding named container ports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/operator/sts.go | Generates unique container port names for multiple forward/http global inputs (keeps first as forward/http). |
| pkg/operator/fluentd-service.go | Generates unique Service port names/TargetPorts matching the container port names for multiple forward/http global inputs. |
| if input.Http != nil { | ||
| httpPort := *input.Http.Port | ||
| if httpPort == 0 { | ||
| httpPort = DefaultHttpPort | ||
| } |
There was a problem hiding this comment.
input.Http.Port is optional (omitempty). Dereferencing it directly can panic when port is omitted. Please align with makeFluentdPorts by nil-checking and applying DefaultHttpPort when unset (and/or keep the 0 check if you want to be defensive).
| @@ -60,28 +59,38 @@ | |||
| forwardPort = DefaultForwardPort | |||
| } | |||
There was a problem hiding this comment.
input.Forward.Port is an optional pointer in the API (omitempty). Dereferencing it directly can panic when the user omits port, and it also makes the defaulting behavior inconsistent with makeFluentdPorts (which checks for nil). Consider using the same nil-check + default logic here before building forwardName/ServicePort.
What this PR does / why we need it:
There are cases were we would like to add several forward or http global inputs to the same fluent deployment.
for example, separate processing logic depending on tag prefix.
Which issue(s) this PR fixes:
Fixes #1833
Does this PR introduced a user-facing change?
Additional information
The PR does not include an breaking changes, since the first iteration of forward/http port name stays the same in the container and service of fluentd. The code can be further simplified by removing the backward compatibility logic.