Hmm why would index be -1 here? Maybe store -1 as store as

const INVALID_INDEX = -1

Added the index constant, but now the function has been deduped.

https://github.com/elastic/kibana/pull/250280/changes#diff-7ea88fb880abbd507bb63081807f1dd4017aa68aabbb8f3f11e803999afc1eb2R110

https://github.com/elastic/kibana/pull/250280/changes#diff-bd2dda36254f5eb14f4c4fb82802df30bb7d7c0448afce5504ae078c3361b325R113

Nit: Can we derive input from newPolicy? I wonder if remove input and create a util to extract from newPolicy?

My bad, did not notice the input was still in the params and unused. It has been removed

It looks like this function is really about updating vars based on their scope, not just a “target.”
Some ideas to make this clearer:

• Rename target → scope throughout.
• Rename updatePolicyVarsAtTarget → updatePolicyVarsByScope or updatePackagePolicyVarsScope.

Add a brief comment explaining the two var scopes:

• Package scope: policy.vars (global to the package)
• Input/stream scope: policy.inputs[].streams[].vars

Thanks for bringing this also to my attention, I found a way deduplicate updating vars, server side and client side.

https://github.com/elastic/kibana/pull/250280/changes#diff-7ea88fb880abbd507bb63081807f1dd4017aa68aabbb8f3f11e803999afc1eb2R110

https://github.com/elastic/kibana/pull/250280/changes#diff-bd2dda36254f5eb14f4c4fb82802df30bb7d7c0448afce5504ae078c3361b325R113

Changed updatePolicyVarsAtTarget -> updatePolicyVarsByScope

Rename target → scope throughout.

I changed mode to scope, but target makes sense to me. I dedicated most of the code review changes on this comment 😅

The target is there mostly for the inputs to return the index of the enabled input. For consistency I would like to keep this as target

const scope = getCredentialStorageScope(packageInfo);
const { target } = resolveVarTarget(policy, scope);

The name detectStorageMode is a bit unclear about what’s being checked. It seems this function is really determining where connector credentials are stored—package/global vs input/stream level.
Maybe consider a name like getConnectorVarScope or getConnectorCredentialScope to make the intent clearer.

changed to updatePolicyVarsByScope

const updatePolicyVarsByScope = (
  policy: NewPackagePolicy,
  updatedVars: PackagePolicyConfigRecord,
  packageInfo: PackageInfo
): NewPackagePolicy => {
  const scope = getCredentialStorageScope(packageInfo);
  const { target } = resolveVarTarget(policy, scope);

  return applyVarsAtTarget(policy, updatedVars, target);
};

Storing -1 as const INVALID_INDEX or INDEX_NOT_FOUND

Same as previous change

• https://github.com/elastic/kibana/pull/250280/changes#diff-bd2dda36254f5eb14f4c4fb82802df30bb7d7c0448afce5504ae078c3361b325R128

I noticed there are quite a few for/map loops here (O(n) operations) to access or update vars both on the FE and BE. Wondering if there’s a way to optimize some of these lookups to O(1), e.g., by indexing vars by key or using a lookup map. This could improve performance especially in case there is scale in the increase fields for each cloud provider and processing Cloud connector API request.

I was going to come back to this a create a helper, thanks for pointing this out.

I created a new function in the var_accessor called findFirstVarEntry to reduce the repeating for-loop to get the first var that matches any key.

const tenantIdVar = findFirstVarEntry(vars, tenantIdKeys);
const clientIdVar = findFirstVarEntry(vars, clientIdKeys);
const azureCredentials = findFirstVarEntry(vars, connectorIdKeys);

I need to give some thought about replacing the for-loop though.

varKeys is typically 2-4 items (credential field names like ['role_arn', 'aws.credentials.role_arn'])

Building a lookup map would add complexity and the performance difference is small for these arrays

nit: suggestion on the naming getCredentialVariablesScope

😅 I am not going to change this since its a :nit, I just changed it to getCredentialStorageScope

With that model It will never be possible to have multiple enabled input with cloud connector variables? I do not think a package exists with that currently

For Cloud Connectors, I don't see how it is possible, ATM, to have multiple enabled inputs with each having their own access credentials. We can only have one cloud connector id per policy.

I think there is a potential future case where a multi-cloud integration, such as Sentinel Cloud, might need to support Cloud Connector. Sentinel Cloud integrations already enable AWS, GCP, and Azure inputs with credentials. While this is not something we need to account for right now, it’s possible that if this use case becomes relevant in the future, we would need to refactor the Cloud Connector model to better support multi-cloud enabled inputs .