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

[Security solution] Update default Bedrock api url #176090

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Feb 1, 2024

Summary

It seems Amazon Bedrock changed the behavior of the default service endpoint. Previously the bedrock endpoint supported "inference requests for models" (in our case, the invoke API). Now that responsibility lies with bedrock-runtime.

I noticed this behavior when I was testing with Bedrock today. The last time i tested with it was early this week and the bedrock endpoint still worked at that time. I cannot find any announcement or release note pointing to this change. This likely means existing connectors will become broken with this error. In order to help our user fix the error, I enhanced the error message for this situation with a recommendation for how to resolve:

Screenshot 2024-02-01 at 11 26 28 AM

I tried to included a markdown link to the endpoint docs, but since the actions test view renders plaintext, not markdown, I did not include the link:
Screenshot 2024-02-01 at 11 25 43 AM

I also updated the documentation link for the url to go to the most relevant page, https://docs.aws.amazon.com/general/latest/gr/bedrock.html

To test

  1. Add these credentials to your dev yml: https://p.elstc.co/paste/Ow6RfYMA#khj+NK0IF2bp-1RhNciATkDRUgJLEAT+uaTQ4T1Lo+L
  2. Either test with the Stack Management > Connector > Test connector view or by sending a message to Security AI Assistant
  3. Send a message with the connector named Old Bedrock Endpoint, notice the enhanced error message
  4. Send a message with the connector named New Bedrock Endpoint, notice the API working

@stephmilovic stephmilovic added release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.13.0 Team:Security Generative AI Security Generative AI v8.12.2 labels Feb 1, 2024
@stephmilovic stephmilovic requested review from a team as code owners February 1, 2024 18:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

github-actions bot commented Feb 1, 2024

A documentation preview will be available soon.

Help us out by validating the Buildkite preview and reporting issues here.
Please also be sure to double check all images to ensure they are correct in the preview.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

Everything looks good, but left a question about the multi-line error message


// @ts-expect-error expects an axios error as the parameter
expect(connector.getResponseErrorMessage(err)).toEqual(
`API Error: The requested operation is not recognized by the service.
Copy link
Member

Choose a reason for hiding this comment

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

Seems slightly weird to have an multi-line error message - or at least non-standard. I would guess that in some environments (writing to the Kibana log) these will get removed/replaced - for instance, in the server log connector, we replace "\n" with "; ".

Does it need to be multi-line?

Copy link
Contributor Author

@stephmilovic stephmilovic Feb 1, 2024

Choose a reason for hiding this comment

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

It doesn't need to, it just looks nicer in the assistant. If you look at the screenshots in the description, you can see it puts it all in one line anyways on the Test connector err message and it looks fine when that happens, so it shouldn't matter. I have a comment about it in the code. Is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect it will be fine, but as it's not what we typically do (AFAIK), there could be some unintended side effects - I would guess in UX's. Have you checked what this looks like in the Kibana logs as displayed in cloud log viewers? I don't >think< it will split this into multiple log documents ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so either, but will deploy this branch to double check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One document, keeps the space:

Screenshot 2024-02-02 at 10 11 24 AM

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM - I'll let Steph roll the dice on the multi-line error message! 🤞🏻

@stephmilovic stephmilovic added ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment labels Feb 2, 2024
@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Thank you! :)

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Feb 2, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackConnectors 520.7KB 520.7KB -20.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
stackConnectors 43.2KB 43.2KB +8.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stephmilovic stephmilovic merged commit 3a4ad77 into elastic:main Feb 2, 2024
18 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 2, 2024
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 2, 2024
…176176)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[Security solution] Update default Bedrock api url
(#176090)](#176090)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Steph
Milovic","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-02T17:43:33Z","message":"[Security
solution] Update default Bedrock api url
(#176090)","sha":"3a4ad7725a28429f25acd3ce630fb43f45ecde1e","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:
SecuritySolution","ci:cloud-deploy","ci:cloud-redeploy","v8.13.0","Team:Security
Generative AI","v8.12.2"],"title":"[Security solution] Update default
Bedrock api
url","number":176090,"url":"https://github.com/elastic/kibana/pull/176090","mergeCommit":{"message":"[Security
solution] Update default Bedrock api url
(#176090)","sha":"3a4ad7725a28429f25acd3ce630fb43f45ecde1e"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/176090","number":176090,"mergeCommit":{"message":"[Security
solution] Update default Bedrock api url
(#176090)","sha":"3a4ad7725a28429f25acd3ce630fb43f45ecde1e"}},{"branch":"8.12","label":"v8.12.2","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Steph Milovic <[email protected]>
@mistic
Copy link
Member

mistic commented Feb 7, 2024

This PR didn't make it on time to the latest build candidate of v8.12.1. Updating the labels.

@mistic mistic removed the v8.12.1 label Feb 7, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Feb 7, 2024
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment release_note:fix Team:Security Generative AI Security Generative AI Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.12.2 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants