fix(outputs.iotdb): Handle paths that contain illegal characters #14519

giovanni-bellini-argo · 2024-01-02T16:15:29Z

Summary

Support for IoTDB ILLEGAL_PATH error when tags contains an illegal character, explained more in detail in issue.

Checklist

No AI generated code was used in this PR

Related issues

resolves #14518

telegraf-tiger · 2024-01-02T16:15:35Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

powersj

Thanks for the PR!

Looking at the IoTBD this does appear to be the suggested way of handling special characters in the path.

Can you break out the regex checking into a new function and please add tests for that function with both cases including special characters? My only concern with these types of changes is future changes down the road or corner cases :)

Thanks!

srebhan · 2024-01-03T08:05:29Z

@giovanni-bellini-argo please also sign the CLA!

giovanni-bellini-argo · 2024-01-03T09:13:45Z

!signed-cla

powersj

Can you confirm my understanding after playing with this a bit more: A path is made up of three elements:

root - reserved word
storage group
child - one or more items separated by periods

The storage group can only have:

The storage group name can only be characters, numbers and underscores.

I think this is what you are currently testing for. However, the child elements seem to allow a greater set of elements. This means the following are valid without escaping them.

root.foo.3
root.foo.()

Is that your understanding as well? I've been trying this with version 0.13.0, so that may play into this as well.

giovanni-bellini-argo · 2024-01-03T15:47:14Z

We are testing on V1.2.2 and could not create child nodes with any other characters outside of underscores and alphanumeric characters.

even tho the documentation states otherwise

powersj · 2024-01-03T16:03:52Z

Thanks!

We are testing on V1.2.2 and could not create child nodes with any other characters outside of underscores and alphanumeric characters.

I am glad we are trying on different versions. Unfortunately, having different behavior on different versions does mean we would need to make this opt-in as well. We cannot be breaking existing users on versions that do not run into this.

How we typically handle this is add a config option, something like:

# Mode to sanitize tags
# By default, tags are not sanitized. Possible options:
#   1.2 - only allows alpha/numeric and underscores, and non-numeric values. Otherwise, 
#         backticks are applied
sanitize_tag = ""

Then if that value is set to "1.2", you can apply your sanitization function.

I would still like to see a specific test as well that only tests the sanitize function with a various test cases.

Does that make sense?

powersj · 2024-01-03T17:26:11Z

I've also asked over in the iotdb repo for guidance on valid characters:

apache/iotdb#11840

Co-authored-by: SeanGaluzzi <SeanGaluzzi@users.noreply.github.com>

giovanni-bellini-argo · 2024-01-05T07:50:05Z

a list of updates:

added the option as per your request and made some researches: from version 1.x.x they completely changed the handling of nodes names which can apparently take only letters, numbers and underscores but can't be composed of only numbers.
made some more checks to throw an error if root (which is a reserved kayword) is used and if ` is misused
added the specific tests cases.

powersj · 2024-01-05T19:04:11Z

Thanks for the updates and doing that research. Almost there!

I do not see your name in our CLA signed sheet. Can you please sign it?
Looks like there a couple in lint issues:

plugins/outputs/iotdb/iotdb.go:277:28  gosimple  S1007: should use raw string (`...`) with regexp.Compile to avoid having to escape twice
plugins/outputs/iotdb/iotdb.go:310:5   revive    var-naming: don't use underscores in Go names; var tag_value should be tagValue

And a test issue with your new tests, specifically: TestTagsHandling/treat_tags_as_device_IDs
Please update the sample config and README with the new config option for sanitize_tag.

Happy to help with any of these updates, let me know if you do want assistence.

giovanni-bellini-argo · 2024-01-07T19:19:51Z

as per the CLA i did sign it on google (first time doing this so i cheched out online and found most use the google db) i did also read your guide about contributions and followed the link to sign the CLA on your site but the form has some iessues:

much appreciated your help.

- fixed lint issues - fixed failing tests - added documentation

powersj · 2024-01-11T21:28:56Z

Hey @giovanni-bellini-argo,

That seems to be your browser saying it is blocked by your content security policy. Can you please try a different browser?

Thanks!

giovanni-bellini-argo · 2024-01-12T07:45:33Z

@powersj u seem to be right XD.
signed it

powersj

Sorry for the delay, thanks again for the re-work, I think we are nearly there!

plugins/outputs/iotdb/README.md

plugins/outputs/iotdb/iotdb.go

plugins/outputs/iotdb/sample.conf

plugins/outputs/iotdb/iotdb_test.go

powersj

Thanks for working through this!

srebhan

Thanks for this nice PR @giovanni-bellini-argo! I have some small comments, which moving the regexp-compilation out of the Write() path being the most important one...

plugins/outputs/iotdb/README.md

plugins/outputs/iotdb/iotdb.go

srebhan · 2024-01-18T17:27:39Z

plugins/outputs/iotdb/iotdb.go

+		matchUnsopportedCharacter, _ := regexp.Compile("[^0-9a-zA-Z_:@#${}\x60]")
+
+		regex := []*regexp.Regexp{matchUnsopportedCharacter}
+		regexArray = append(regexArray, regex...)
+
+	// from version 1.x.x IoTDB changed the allowed keys in nodes
+	case "1.0", "1.1", "1.2", "1.3":
+		matchUnsopportedCharacter, _ := regexp.Compile("[^0-9a-zA-Z_\x60]")
+		matchNumericString, _ := regexp.Compile(`^\d+$`)


Same here, use MustCompile and initialize regexArray in Init()!

why initializing regexArray in Init()?
u think it would be better to include it in the struct for future changes?

and i would also delete those useless variables and directly append the regex to the array.

regexArray := []*regexp.Regexp{} // array of compiled regex patterns switch s.SanitizeTags { case "0.13": regex := []*regexp.Regexp{ regexp.MustCompile("[^0-9a-zA-Z_:@#${}\x60]"), } regexArray = append(regexArray, regex...) // from version 1.x.x IoTDB changed the allowed keys in nodes case "1.0", "1.1", "1.2", "1.3": regex := []*regexp.Regexp{ regexp.MustCompile("[^0-9a-zA-Z_\x60]"), regexp.MustCompile(`^\d+$`), } regexArray = append(regexArray, regex...) default: return tag, nil }

the idea is to make it easy to undertand and modify

plugins/outputs/iotdb/sample.conf

srebhan

Thanks for your update @giovanni-bellini-argo! Please try to compile the regular-expressions only once as suggested in the code! If you need help, I can provide a patch...

plugins/outputs/iotdb/iotdb.go

srebhan

Looks good @giovanni-bellini-argo! I would just un-export the member variable and fix the typos and we are good to go. Tried to do it in my suggestions to ease your life but no sure I got all of them... :-)

plugins/outputs/iotdb/iotdb.go

giovanni-bellini-argo · 2024-01-23T07:43:02Z

done everything, thanks for the patience and help 😎

srebhan

Thank you for your work and patience with my requests @giovanni-bellini-argo! :-)

telegraf-tiger · 2024-01-23T15:22:58Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

) Co-authored-by: SeanGaluzzi <SeanGaluzzi@users.noreply.github.com> Co-authored-by: SeanGaluzzi <sean.galuzzi@argo.consulting> (cherry picked from commit 4c1d8e3)

…luxdata#14519) Co-authored-by: SeanGaluzzi <SeanGaluzzi@users.noreply.github.com> Co-authored-by: SeanGaluzzi <sean.galuzzi@argo.consulting>

Update iotdb.go to support paths that contain illegal characters

159a7e7

powersj reviewed Jan 2, 2024

View reviewed changes

powersj changed the title ~~Update iotdb.go to support paths that contain illegal characters~~ fix(outputs.iotdb): Handle paths that contain illegal characters Jan 2, 2024

powersj added the waiting for response waiting for response from contributor label Jan 2, 2024

powersj mentioned this pull request Jan 2, 2024

IoTDB device_id [ILLEGAL_PATH(509)] Exception occurred: insertRecord failed #14518

Closed

Merge branch 'influxdata:master' into IoTDB_fix

d75cbf0

telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jan 3, 2024

srebhan assigned powersj Jan 3, 2024

srebhan added fix pr to fix corresponding bug area/iot New plugins or features relating to IoT monitoring plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Jan 3, 2024

giovanni-bellini-argo added 2 commits January 3, 2024 09:16

added function to check for the validity of tags

a9f88b1

updated tests

3b99da0

bug fix: exclusion of some special characters

1955db9

powersj reviewed Jan 3, 2024

View reviewed changes

validation based on provisioned version and added separate test function

54e2eaf

Co-authored-by: SeanGaluzzi <SeanGaluzzi@users.noreply.github.com>

🔨 Lint and Test fixes

c9bfd16

- fixed lint issues - fixed failing tests - added documentation

powersj reviewed Jan 17, 2024

View reviewed changes

syntax fixes

01eb1fb

powersj approved these changes Jan 18, 2024

View reviewed changes

powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jan 18, 2024

powersj assigned srebhan and unassigned powersj Jan 18, 2024

srebhan reviewed Jan 18, 2024

View reviewed changes

giovanni-bellini-argo added 2 commits January 19, 2024 15:58

syntax fixes

6e9d7c3

changed regexp.Compile to MustCompile

c8eeabb

srebhan reviewed Jan 22, 2024

View reviewed changes

plugins/outputs/iotdb/iotdb.go Outdated Show resolved Hide resolved

plugins/outputs/iotdb/iotdb.go Outdated Show resolved Hide resolved

plugins/outputs/iotdb/iotdb.go Outdated Show resolved Hide resolved

initialization of regex in Init function

c64790c

srebhan reviewed Jan 22, 2024

View reviewed changes

typos fix and un-export of sanityRegex

bb3bae2

srebhan approved these changes Jan 23, 2024

View reviewed changes

srebhan merged commit 4c1d8e3 into influxdata:master Jan 23, 2024
26 checks passed

github-actions bot added this to the v1.29.3 milestone Jan 23, 2024

Porkepix mentioned this pull request Jan 30, 2024

telegraf 1.29.3 Homebrew/homebrew-core#161265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(outputs.iotdb): Handle paths that contain illegal characters #14519

fix(outputs.iotdb): Handle paths that contain illegal characters #14519

giovanni-bellini-argo commented Jan 2, 2024

telegraf-tiger bot commented Jan 2, 2024

powersj left a comment

srebhan commented Jan 3, 2024

giovanni-bellini-argo commented Jan 3, 2024

powersj left a comment

giovanni-bellini-argo commented Jan 3, 2024 •

edited

Loading

powersj commented Jan 3, 2024

powersj commented Jan 3, 2024

giovanni-bellini-argo commented Jan 5, 2024

powersj commented Jan 5, 2024

giovanni-bellini-argo commented Jan 7, 2024 •

edited

Loading

powersj commented Jan 11, 2024

giovanni-bellini-argo commented Jan 12, 2024

powersj left a comment

powersj left a comment

srebhan left a comment

srebhan Jan 18, 2024

giovanni-bellini-argo Jan 19, 2024

srebhan left a comment

srebhan left a comment

giovanni-bellini-argo commented Jan 23, 2024

srebhan left a comment

telegraf-tiger bot commented Jan 23, 2024

Artifact URLs

fix(outputs.iotdb): Handle paths that contain illegal characters #14519

fix(outputs.iotdb): Handle paths that contain illegal characters #14519

Conversation

giovanni-bellini-argo commented Jan 2, 2024

Summary

Checklist

Related issues

telegraf-tiger bot commented Jan 2, 2024

powersj left a comment

Choose a reason for hiding this comment

srebhan commented Jan 3, 2024

giovanni-bellini-argo commented Jan 3, 2024

powersj left a comment

Choose a reason for hiding this comment

giovanni-bellini-argo commented Jan 3, 2024 • edited Loading

powersj commented Jan 3, 2024

powersj commented Jan 3, 2024

giovanni-bellini-argo commented Jan 5, 2024

powersj commented Jan 5, 2024

giovanni-bellini-argo commented Jan 7, 2024 • edited Loading

powersj commented Jan 11, 2024

giovanni-bellini-argo commented Jan 12, 2024

powersj left a comment

Choose a reason for hiding this comment

powersj left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan Jan 18, 2024

Choose a reason for hiding this comment

giovanni-bellini-argo Jan 19, 2024

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

giovanni-bellini-argo commented Jan 23, 2024

srebhan left a comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Jan 23, 2024

Artifact URLs

giovanni-bellini-argo commented Jan 3, 2024 •

edited

Loading

giovanni-bellini-argo commented Jan 7, 2024 •

edited

Loading