Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

websocket: new generic integration #9926

Merged
merged 10 commits into from
Jul 17, 2024
Merged

Conversation

muskan-agarwal26
Copy link
Contributor

Proposed commit message

This makes the Filebeat Websocket input available as an integration package.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

screencapture-127-0-0-1-5602-app-integrations-detail-websocket-0-1-0-overview-2024-05-21-15_39_24
Screenshot 2024-05-21 154613

1. This makes the Filebeat Websocket input available as an integration package.
@muskan-agarwal26 muskan-agarwal26 changed the title New Generic Integration websocket: New Generic Integration May 21, 2024
@muskan-agarwal26 muskan-agarwal26 changed the title websocket: New Generic Integration websocket: new generic integration May 21, 2024
@ShourieG ShourieG added the Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] label May 21, 2024
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@ShourieG ShourieG added New Integration Issue or pull request for creating a new integration package. integration Label used for meta issues tracking each integration Crest labels May 21, 2024
packages/websocket/_dev/build/docs/README.md Outdated Show resolved Hide resolved
packages/websocket/_dev/build/docs/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please go fmt this code.

@efd6
Copy link
Contributor

efd6 commented May 21, 2024

/test

@ShourieG
Copy link
Contributor

ShourieG commented Jun 7, 2024

@muskan-agarwal26, any updates on this ?

@muskan-agarwal26
Copy link
Contributor Author

@muskan-agarwal26, any updates on this ?

Yes, We are addressing the comments and will update the PR soon.

1. Changed the title as suggested in Readme and manifest.
2. Added support for state, regexp, redact and auth support in the hbs file.
3. Added another scenario for system test.
packages/websocket/_dev/build/docs/README.md Outdated Show resolved Hide resolved
packages/websocket/img/websocket_configuration.png Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reverse the order of function declarations. The convention in Go code is caller before callee.

}
}

conn, err := upgrader.Upgrade(w, r, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
conn, err := upgrader.Upgrade(w, r, nil)
upgrader := websocket.Upgrader{
CheckOrigin: func(r *http.Request) bool { return true },
}
conn, err := upgrader.Upgrade(w, r, nil)

packages/websocket/manifest.yml Outdated Show resolved Hide resolved
@kcreddy
Copy link
Contributor

kcreddy commented Jun 18, 2024

/test

1. Remove default value of URL from manifest.
2. Reverse the order of function declarations.
3. Added copyright header.
4. Removed note from readme and update screenshot.
@muskan-agarwal26 muskan-agarwal26 requested a review from efd6 June 18, 2024 13:05
@efd6
Copy link
Contributor

efd6 commented Jun 18, 2024

/test

Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

@muskan-agarwal26 looks like one of the system test is failing as its unable to find any hits.

@kcreddy
Copy link
Contributor

kcreddy commented Jun 20, 2024

/test

Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

After 8.15, we should also plan to add config option url_program as per elastic/beats#39997

packages/websocket/_dev/test/system/test-get-config.yml Outdated Show resolved Hide resolved
@ShourieG
Copy link
Contributor

ShourieG commented Jul 1, 2024

Hi @muskan-agarwal26, could you resolve the system test issues and some unresolved suggestions so we can merge this ?

@muskan-agarwal26 muskan-agarwal26 requested a review from kcreddy July 8, 2024 10:05
@kcreddy
Copy link
Contributor

kcreddy commented Jul 8, 2024

/test

@kcreddy
Copy link
Contributor

kcreddy commented Jul 16, 2024

@ShourieG, can you please check why the system tests failing inside the CI? They are successful locally.

Could be same issue inside CI here: #10472

@ShourieG
Copy link
Contributor

/test

@ShourieG
Copy link
Contributor

/test

1 similar comment
@ShourieG
Copy link
Contributor

/test

@ShourieG
Copy link
Contributor

/test

@ShourieG
Copy link
Contributor

ShourieG commented Jul 17, 2024

The issue with system tests seems two fold:

  1. The service deployer on linux systems takes longer to deploy the mock service but the current implementation does not wait for the service to be available in the agent network and returns earlier causing the websocket input to hit an unreachable instance of the server.

  2. Currently the websocket input lacks a retry mechanism causing the input to immediately fail after the 1st unsuccessful attempt.

The fix needs to come from both ends but from the input side the earliest would be 8.15.1. The service deployer fix could potentially come earlier but that depends on the team priorities. Right now for websocket integrations, system tests might need to be disabled.

NOTE: The delay for the mock service seems to be an issue on some linux docker systems only and hence went unnoticed until now as mac machines don't seem to suffer from this.

@ShourieG
Copy link
Contributor

/test

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @muskan-agarwal26

@ShourieG
Copy link
Contributor

ShourieG commented Jul 17, 2024

UPDATE: The solution was as simple as adding a health check to the mock-service container. Thanks a lot @chrisberkhout for noticing this and @jsoriano for verifying in realtime. cc: @kcreddy. Tests are now passing. :)

Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

LGTM

@ShourieG ShourieG merged commit 9b26696 into elastic:main Jul 17, 2024
5 checks passed
@elasticmachine
Copy link

Package websocket - 0.1.0 containing this change is available at https://epr.elastic.co/search?package=websocket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Crest Integration:websocket Custom Websocket logs integration Label used for meta issues tracking each integration New Integration Issue or pull request for creating a new integration package. Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants