feat: parse connector id from tool parameters map

[yuye-aws]

Description

Parse connector id from previous_node_inputs into the parameters map

{
  "id": "create_connector_tool",
  "type": "create_tool",
  "previous_node_inputs": {
    "create_connector": "connector_id"
  },
  "user_inputs": {
    "parameters": {},
    "name": "ConnectorTool",
    "type": "ConnectorTool"
  }
}

Related Issues

Resolves #845

Check List

• New functionality includes testing.
• New functionality has been documented.
• [ ] API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• [ ] Public documentation issue/PR created. Already documented

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[yuye-aws]

Not sure what are the expected UTs and ITs in the PR.

[codecov]

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.55%. Comparing base (60458a6) to head (52d68df).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rg/opensearch/flowframework/workflow/ToolStep.java	84.61%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #846      +/-   ##
============================================
+ Coverage     77.38%   77.55%   +0.17%     
- Complexity      963      966       +3     
============================================
  Files            97       97              
  Lines          4536     4531       -5     
  Branches        423      422       -1     
============================================
+ Hits           3510     3514       +4     
+ Misses          845      835      -10     
- Partials        181      182       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dbwiddis]

Thanks for the PR! I see you copied what we already did for model ID which works great. I wonder if there's a more efficient way to do it but we can refactor later. :)

Not sure what are the expected UTs and ITs in the PR.

For UT: Looks like our existing unit tests just confirm that we generate a tool step. We could update those to include things in the map and verify that they exist in the MLToolSpec object in the output... which we probably should have already done. ☹️

But this is one of those things probably better tested in an integ test (or API Spec test). Can you add a new test method to FlowFrameworkRestApiIT that tests a Tool (or tools) with inputs from both a connector and a model? You'd basically:

• create a template in src/test/resources/template with appropriate JSON (probably need to create an agent with those tool(s))
• copy testSearchWorkflows() and change it to execute your template
• provision the template
• call an API to verify the output (in this case you'd probably get the workflow ID after provisioning, use that to query the workflow state to get the resource ID of the agent, then use the agent API to query the agent and make sure it has the connector ID in the JSON

From the standpoint of this PR, doing a manual test and showing the output here is probably sufficient as we have a plan to eventually test all our templates via api spec and that will eventually cover this... but any help you can provide improving out UT/IT would be appreciated.

[yuye-aws]

For UT: Looks like our existing unit tests just confirm that we generate a tool step. We could update those to include things in the map and verify that they exist in the MLToolSpec object in the output... which we probably should have already done. ☹️

Updated

[yuye-aws]

Thanks for the PR! I see you copied what we already did for model ID which works great. I wonder if there's a more efficient way to do it but we can refactor later. :)

It should be a good idea to iterate over a set of [CONNECTOR_ID, MODEL_ID, AGENT_ID]. Hoping that the code can get merged before 2.17, we can refactor the code later in 2.18.

[dbwiddis]

LGTM for now, please make the one tweak on workflow data param I requested!

[yuye-aws]

• call an API to verify the output (in this case you'd probably get the workflow ID after provisioning, use that to query the workflow state to get the resource ID of the agent, then use the agent API to query the agent and make sure it has the connector ID in the JSON

Not sure how to accomplish this step. It takes me some while to get the agent id.

[dbwiddis]

Not sure how to accomplish this step. It takes me some while to get the agent id.

This one's probably better handled by an api spec test
CC @JunweiD

[yuye-aws]

Not sure how to accomplish this step. It takes me some while to get the agent id.

This one's probably better handled by an api spec test CC @JunweiD

Just figured out. Pushing the code now.

[yuye-aws]

@dbwiddis Can you help review the IT code? Thanks!

[yuye-aws]

Not sure whether the current CI error is attributed to this PR. Can someone take a look?

[dbwiddis]

Still LGTM with a few suggestions.

[owaiskazi19]

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

[yuye-aws]

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

I can add an IT. Before that, can you tell me its difference between FlowFrameworkRestApiIT? I do not wish to implement duplicated ITs.

[owaiskazi19]

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

I can add an IT. Before that, can you tell me its difference between FlowFrameworkRestApiIT? I do not wish to implement duplicated

FlowFrameworkSecureRestApiIT is for security enabled integ tests and FlowFrameworkRestApiIT is for security disabled integ tests.

[yuye-aws]

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

I can add an IT. Before that, can you tell me its difference between FlowFrameworkRestApiIT? I do not wish to implement duplicated

FlowFrameworkSecureRestApiIT is for security enabled integ tests and FlowFrameworkRestApiIT is for security disabled integ tests.

FlowFrameworkSecureRestApiIT is mainly about access control. Not sure if you expect to see a similar method like testCreateAndProvisionConnectorToolAgentFrameworkWorkflow.

[owaiskazi19]

FlowFrameworkSecureRestApiIT is mainly about access control. Not sure if you expect to see a similar method like testCreateAndProvisionConnectorToolAgentFrameworkWorkflow.

So since you are adding a new template file createconnector-createconnectortool-createflowagent.json. We can add a new secured integ tests for create and provision with access control.

[dbwiddis]

FlowFrameworkSecureRestApiIT is mainly about access control. Not sure if you expect to see a similar method like testCreateAndProvisionConnectorToolAgentFrameworkWorkflow.

Does the ML Commons client connector API behave differently with/without the security plugin installed? If so, it's appropriate.

[yuye-aws]

Does the ML Commons client connector API behave differently with/without the security plugin installed? If so, it's appropriate.

No. I don't think so. Connector is accessing remote services. Security plugin does not matter.

[dbwiddis]

No. I don't think so. Connector is accessing remote services. Security plugin does not matter.

Then I don't think a security integ test is needed. @owaiskazi19 do you feel differently?

[owaiskazi19]

No. I don't think so. Connector is accessing remote services. Security plugin does not matter.

Then I don't think a security integ test is needed. @owaiskazi19 do you feel differently?

Agreed. @yuye-aws can you resolve the other comments then and we can get this in?

[yuye-aws]

@dbwiddis @owaiskazi19 This PR has been updated according to your comments. Can you help review again?

[yuye-aws]

There seems to be a flakey test. Need to rerun this workflow.

[dbwiddis]

There seems to be a flakey test. Need to rerun this workflow.

Yeah, macos multinode runs into memory issues sometimes (3 nodes in docker + deploying a local model), will rerun, but sometimes we just merge if that's the only failure.

[dbwiddis]

LGTM and I've checked all requested changes and resolved the conversations.