telegraf icon indicating copy to clipboard operation
telegraf copied to clipboard

OPC UA listener input. Support for nsu Identifier and OPC naming rules.

Open alexmc1510 opened this issue 1 year ago • 9 comments

Use Case

The current configuration of inputs.opcua_listener is not following the real naming structure of OPC UA and the current configuration, based on NamespaceIndex is a risk because the index can change on server side after a restart as you can see in the link provided.

Current conguration of a node: nodes = [ {name="namespaces", namespace="0", identifier_type="i", identifier="2255"}, ]

Expected behavior

I would propose a configuration string like the following when accessing by NamespaceIndex (for the ones who still want to use the legacy method). nodes = [ {name="namespaces", NamespaceIndex="0", IdentifierType="i", Identifier="2255"}, ]

I would propose a configuration string like the following when accessing by Namespace (for the ones who want to use the new and secure method). nodes = [ {name="namespaces", Namespace="http://opcfoundation.org/UA/", IdentifierType="i", Identifier="2255"}, ]

Actual behavior

The behavior in the end will be the same with the only benefit of having the namespace change under control.

Additional info

Clear explanation of the behavior. https://documentation.unified-automation.com/uasdkhp/1.4.1/html/_l2_ua_node_ids.html

alexmc1510 avatar Dec 08 '23 09:12 alexmc1510

Hi @alexmc1510,

As proposed, I believe that is a breaking change. As you have switched Namespace to mean a namespace versus the index it means today. Am I reading this correctly?

I think adding a new option for a specific namespace is fine, but we need to leave the existing config options and behavior the same.

Thanks

powersj avatar Dec 08 '23 15:12 powersj

Hello, exactly, the configuration as it is done today is right but maybe in future improvements, another option for including namespace instead of namespaceIndex would be fine according to the explanation and justification included in the link I added to my answer. The index can change if something happens at server side so the complete NodeId is different.

Regards

El vie, 8 dic 2023, 16:03, Joshua Powers @.***> escribió:

Hi @alexmc1510 https://github.com/alexmc1510,

As proposed, I believe that is a breaking change. As you have switched Namespace to mean a namespace versus the index it means today. Am I reading this correctly?

I think adding a new option for a specific namespace is fine, but we need to leave the existing config options and behavior the same.

Thanks

— Reply to this email directly, view it on GitHub https://github.com/influxdata/telegraf/issues/14417#issuecomment-1847335481, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZBUXFBCWTCYMIJ6UN56YDYIMT3DAVCNFSM6AAAAABAMMRRRSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBXGMZTKNBYGE . You are receiving this because you were mentioned.Message ID: @.***>

alexmc1510 avatar Dec 08 '23 15:12 alexmc1510

OK, I would suggest updating your expected behavior then. Effectively it sounds like you are asking for and NamespaceURL or NamespaceString to get added and to leave the Namespace that exists.

powersj avatar Dec 08 '23 15:12 powersj

Hello,

No no. NamespaceIndex and Namespace are referring to the same info if you have a look at the explanation provided in the link. The problem is that NamespaceIndex is different depending on the server and Namespace, treated as string is really unique identifier. If you refer to the Namespace using an index, the index is dependant on the server and it can vary....

El vie, 8 dic 2023, 16:30, Joshua Powers @.***> escribió:

OK, I would suggest updating your expected behavior then. Effectively it sounds like you are asking for and NamespaceURL or NamespaceString to get added and to leave the Namespace that exists.

— Reply to this email directly, view it on GitHub https://github.com/influxdata/telegraf/issues/14417#issuecomment-1847386998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZBUXECD7RFGI64CQJPNWDYIMXCZAVCNFSM6AAAAABAMMRRRSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBXGM4DMOJZHA . You are receiving this because you were mentioned.Message ID: @.***>

alexmc1510 avatar Dec 08 '23 15:12 alexmc1510

I am aware of what OPCUA defines these. However, in Telegraf we have already defined "namespace" to mean the ID:

namespace - OPC UA namespace of the node (integer value 0 thru 3)

We cannot, and will not, change this definition to avoid breaking existing users.

powersj avatar Dec 08 '23 16:12 powersj

I am not go programmer but after having a look, it seems it is necessary to modify: plugins/common/opcua/input/input_client.go

// NodeID returns the OPC UA node id

func (tag *NodeSettings) NodeID() string {
	return "ns=" + tag.Namespace + ";" + tag.IdentifierType + "=" + tag.Identifier
}

I have checked goopcua and it is compatible with ExtendedNodeId:

"nsu=" + tag.NamespaceString + ";" + tag.IdentifierType + "=" + tag.Identifier

How do you suggest to implement it without generating a breaking change?

Thanks

alexmc1510 avatar Dec 15 '23 17:12 alexmc1510

As you mentioned in your original report, this is the current configuration of a node:

nodes = [ {name="namespaces", namespace="0", identifier_type="i", identifier="2255"}, ]

We need to differentiate the namespace ID we have today with a string namespace, so introduce a new field:

namespaceString - OPC UA namespace of the node (string)

Does that work?

powersj avatar Dec 18 '23 14:12 powersj

Hello @powersj , do you have a manual on how to upgrade/update existing plugins? Guidelines, forks...etc?

alexmc1510 avatar Jan 05 '24 12:01 alexmc1510

Take a look at: https://github.com/influxdata/telegraf/tree/master/docs/developers

In general, the short version is: fork the repo, make your change/add a test/update docs, verify it works, and create a PR. Of course, we don't want to see breaking changes as I've pointed out. We also have a number of tests and linters that you can run locally to verify everything matches our style.

Hope that helps!

powersj avatar Jan 05 '24 14:01 powersj