Skip to content

Refactor collector ingest input management#25502

Open
thll wants to merge 26 commits intomasterfrom
refactor/collector-ingest-input-management
Open

Refactor collector ingest input management#25502
thll wants to merge 26 commits intomasterfrom
refactor/collector-ingest-input-management

Conversation

@thll
Copy link
Copy Markdown
Contributor

@thll thll commented Mar 30, 2026

Decouples the collector ingest input from the collectors config lifecycle. Previously, the input was fully managed by the config — created, started, stopped, and deleted as part of config saves. Now the input is a regular, user-manageable input that can be independently configured, started, stopped, and deleted without affecting the collectors config or collector behavior.

The collectors config page becomes the external endpoint configuration (hostname + port pushed to collectors) plus lifecycle thresholds. Input management moves to the standard inputs page, with a convenience bootstrap on first setup and status visibility on the settings page.

Collectors always receive their endpoint configuration regardless of input state. If no input is running, collectors fail to connect and retry — their persistent queues are preserved and drain when an input comes back up. This avoids data loss from transient input restarts or accidental deletions.

/nocl

thll and others added 6 commits March 30, 2026 17:18
- Decouple collector ingest input lifecycle from collectors config
- Remove enabled toggle and inputId linkage from IngestEndpointConfig
- Expose transport config fields via allowlist in CollectorIngestHttpTransport
- Extract CollectorIngestInputService from CollectorsConfigResource
- Add separate GET /collectors/config/inputs endpoint for input IDs
- Add create_input flag to PUT /collectors/config for onboarding bootstrap
- Reject input creation in cloud environments
- Simplify OpAmpService supplier to non-Optional backed by getOrDefault()
- DEV migration: clean http sub-object and backfill input config defaults
- Rewrite CollectorsSettings UI with Formik, extract IngestEndpointStatus
- Add useCollectorInputIds hook for separate input ID fetching

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Gate create-input checkbox on inputs:create + input_types:create permissions
- Map backend validation error keys to Formik field names so threshold
  errors surface inline on the correct TimeUnitInput fields
- Remove unused Section styled component
- Fix stale test: heading 'HTTP' → 'Ingest Endpoint', create_input
  expectation true → false for configured state, mock useInputMutations
- Drop transport config overrides from createInput() and migration —
  only bind_address and port need explicit values, rest uses generic defaults

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace handcrafted Subject mock and SecurityTestUtils setup with
the declarative @WithAuthorization annotation and extension, matching
the pattern from EnrollmentTokenResourceTest.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deleted inputs are filtered out silently via isSuccess check instead
of showing a toast notification via defaultOnError.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@thll
Copy link
Copy Markdown
Contributor Author

thll commented Mar 31, 2026

@kroepke, @bernd, I'd like to get your opinion on the following matter.

This PR contains code to handle a scenario where a collector input exists but a user can't see it because they don't have the required permissions. However, with the Reader role granting inputs:read permission, the code is practically unreachable at the moment. Or am I missing something here?

I'm inclined to remove that code, even though it may eventually become relevant, should we ever implement entity sharing for inputs. But even if that moment ever comes, the code would need re-testing because it never triggered. What do you think?

@kroepke
Copy link
Copy Markdown
Member

kroepke commented Mar 31, 2026

@kroepke, @bernd, I'd like to get your opinion on the following matter.

This PR contains code to handle a scenario where a collector input exists but a user can't see it because they don't have the required permissions. However, with the Reader role granting inputs:read permission, the code is practically unreachable at the moment. Or am I missing something here?

I'm inclined to remove that code, even though it may eventually become relevant, should we ever implement entity sharing for inputs. But even if that moment ever comes, the code would need re-testing because it never triggered. What do you think?

There are a few places in the code that still have generic INPUTS_READ permission checks without any input id. Those would also break if we added input sharing. Perhaps for clarity we should do the permission check, even if it is unreachable at the moment, so that we have a better chance of seeing it if we ever change the behavior?
Adding a small comment to that effect is probably the nicest way of leaving a breadcrumb, as well :)

@thll
Copy link
Copy Markdown
Contributor Author

thll commented Mar 31, 2026

@kroepke, @bernd, I'd like to get your opinion on the following matter.
This PR contains code to handle a scenario where a collector input exists but a user can't see it because they don't have the required permissions. However, with the Reader role granting inputs:read permission, the code is practically unreachable at the moment. Or am I missing something here?
I'm inclined to remove that code, even though it may eventually become relevant, should we ever implement entity sharing for inputs. But even if that moment ever comes, the code would need re-testing because it never triggered. What do you think?

There are a few places in the code that still have generic INPUTS_READ permission checks without any input id. Those would also break if we added input sharing. Perhaps for clarity we should do the permission check, even if it is unreachable at the moment, so that we have a better chance of seeing it if we ever change the behavior? Adding a small comment to that effect is probably the nicest way of leaving a breadcrumb, as well :)

OK, thanks for the feedback. I added some comments and left the code in.

@thll thll marked this pull request as ready for review March 31, 2026 09:35
@thll thll requested review from bernd and kroepke March 31, 2026 09:35
…ingest-input-management

# Conflicts:
#	graylog2-web-interface/src/components/collectors/settings/CollectorsSettings.test.tsx
#	graylog2-web-interface/src/components/collectors/settings/CollectorsSettings.tsx
.collectorExpirationThreshold(effectiveExpiration)
.build();

if (Boolean.TRUE.equals(request.createInput())) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can createInput just be a boolean instead of the boxed version?

thll and others added 11 commits April 9, 2026 11:26
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Show error toast on input detail fetch failure instead of silently
treating errors as absent inputs. Extract PortMismatchAlert into its
own component file.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thll and others added 4 commits April 10, 2026 11:58
…ingest-input-management

# Conflicts:
#	graylog2-server/src/main/java/org/graylog/collectors/CollectorsConfigService.java
#	graylog2-server/src/main/java/org/graylog/collectors/input/transport/CollectorIngestHttpTransport.java
#	graylog2-server/src/test/java/org/graylog/collectors/CollectorCaServiceTest.java
#	graylog2-server/src/test/java/org/graylog/collectors/opamp/auth/AgentTokenServiceTest.java
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@thll
Copy link
Copy Markdown
Contributor Author

thll commented Apr 10, 2026

I've added some stuff to help the user avoid the pitfall of inadvertently configuring a different port on the input than on the collector settings page.

The generic config dialog for the collectors input now shows a description explaining the purpose of the input and the relationship to the collectors settings. The help text for the port field in that dialog mentions the configured port of the collectors settings page.

The collector settings page makes it more explicit that users are configuring the external endpoint. When inputs exist with a different port, a notification box (yay!) appears that let's the user know. It's informational, and not a warning, because this is a valid scenario.

Also fixed some stale state when navigating between tabs or between input table and settings page.

@thll thll requested a review from kroepke April 10, 2026 15:15
Copy link
Copy Markdown
Member

@kroepke kroepke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, much nicer to use.
i've flagged some minor fully qualified imports in tests, but nothing serious.

Comment on lines +66 to +67
final var input1 = mock(org.graylog2.inputs.Input.class);
final var input2 = mock(org.graylog2.inputs.Input.class);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be an import

when(messageInput.asMap()).thenReturn(Map.of());
when(messageInputFactory.create(any(InputCreateRequest.class), anyString(), isNull(), anyBoolean()))
.thenReturn(messageInput);
final var input = mock(org.graylog2.inputs.Input.class);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import

@Test
void portDescriptionIncludesConfiguredPort() {
final var config = CollectorsConfig.builder()
.http(new org.graylog.collectors.IngestEndpointConfig("host", 14401))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants