feat(inputs.aliyuncms): Refactor common parts to plugins/common #18280

ZPascal · 2026-01-29T06:16:01Z

Description

This PR adds the Aliyun common library to Telegraf.

Checklist

No AI generated code was used in this PR
I have read the contributing guidelines
I have signed the CLA
I have added tests to cover my changes
All new and existing tests pass

Related issues

related to #18253

ZPascal · 2026-01-29T06:16:51Z

FYI: @srebhan This PR integrates the Aliyun common library.

telegraf-tiger · 2026-01-29T06:45:17Z

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

srebhan

Thanks for the PR @ZPascal! I have some comments in the code. Furthermore, I would love to see inputs.aliyuncms to be migrated to this package in this PR without change of functionality.

srebhan · 2026-01-30T14:33:31Z

plugins/common/aliyun/auth.go

+	var (
+		roleSessionExpiration = 3600
+		sessionExpiration     = 3600
+	)


Please use

Suggested change

var (

roleSessionExpiration = 3600

sessionExpiration = 3600

)

roleSessionExpiration := 3600

sessionExpiration := 3600

srebhan · 2026-01-30T14:35:17Z

plugins/common/aliyun/auth_test.go

+func TestGetCredentials(t *testing.T) {
+	tests := []struct {
+		name        string
+		config      CredentialConfig
+		expectError bool
+	}{
+		{
+			name: "with access key credentials",
+			config: CredentialConfig{
+				AccessKeyID:     "test-key-id",
+				AccessKeySecret: "test-key-secret",
+			},
+			expectError: false,
+		},
+		{
+			name: "with access key and STS token",
+			config: CredentialConfig{
+				AccessKeyID:       "test-key-id",
+				AccessKeySecret:   "test-key-secret",
+				AccessKeyStsToken: "test-sts-token",
+			},
+			expectError: false,
+		},
+		{
+			name: "with role ARN",
+			config: CredentialConfig{
+				AccessKeyID:     "test-key-id",
+				AccessKeySecret: "test-key-secret",
+				RoleArn:         "acs:ram::123456:role/test-role",
+				RoleSessionName: "test-session",
+			},
+			expectError: false,
+		},
+		{
+			name: "with RSA keypair",
+			config: CredentialConfig{
+				PublicKeyID: "test-public-key-id",
+				PrivateKey:  "-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----",
+			},
+			expectError: true,
+		},
+		{
+			name: "with ECS RAM role name",
+			config: CredentialConfig{
+				RoleName: "test-ecs-role",
+			},
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cred, err := GetCredentials(tt.config)
+
+			if tt.expectError {
+				if err == nil {
+					require.NotNil(t, cred)
+				}
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, cred)
+			}
+		})
+	}
+}


Please split the tests into TestGetCredentialsSuccess and TestGetCredentialsFail to simplify the code and to provide context on what to expect for the reader.

srebhan · 2026-01-30T14:36:16Z

plugins/common/aliyun/auth_test.go

+	if err != nil {
+		require.Contains(t, err.Error(), "failed to retrieve credential")
+	} else {
+		require.NotNil(t, cred)
+	}


No! You should know what you expect from that call! Either this should return an error, then check for it, or it doesn't!

srebhan · 2026-01-30T14:40:11Z

plugins/common/aliyun/discovery.go

+	var (
+		err      error
+		data     map[string]interface{}
+		lastData map[string]interface{}
+	)


None of these is required to be declared outside of the go-routine, is it? Please use implicit declarations where possible and limit the scope of variables to a minimum to avoid unintended reuse!

srebhan · 2026-01-30T14:40:42Z

plugins/common/aliyun/discovery.go

+			case <-dt.done:
+				return


Wouldn't using a cancel-able context be better here?

srebhan · 2026-01-30T15:00:19Z

plugins/common/aliyun/discovery_test.go

+	req = "string"
+	require.NotNil(t, req)
+
+	req = 123
+	require.NotNil(t, req)


What does this test?

srebhan · 2026-01-30T15:01:25Z

plugins/common/aliyun/discovery_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rpcReq, err := getRPCReqFromDiscoveryRequest(tt.req)
+			if tt.expectError {
+				require.Error(t, err)
+				require.Nil(t, rpcReq)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, rpcReq)
+			}
+		})
+	}


Please split into a test that should succeed and one that should fail to simplify code and to provide context to the reader.

Same for other cases where you have if expectedError else constructs in tests!

srebhan · 2026-01-30T15:01:50Z

plugins/common/aliyun/discovery_test.go

+	var req DiscoveryRequest
+
+	req = &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}}


Please use implicit declarations where possible

Suggested change

var req DiscoveryRequest

req = &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}}

req := &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}}

srebhan · 2026-01-30T15:03:01Z

plugins/common/aliyun/regions.go

Is there a way to get the regions from the SDK? I don't want to maintain that list in telegraf as it is likely to be changed without anyone from Aliyun notifying us...

srebhan · 2026-01-30T15:03:33Z

plugins/common/aliyun/regions_test.go

I don't think this test does anything useful...

feat: Add the aliyun common lib

284bef1

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jan 29, 2026

srebhan changed the title ~~feat: Add the aliyun common lib~~ feat(inputs.aliyuncms): Refactor common parts to plugins/common Jan 30, 2026

telegraf-tiger bot added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jan 30, 2026

srebhan reviewed Jan 30, 2026

View reviewed changes

srebhan self-assigned this Jan 30, 2026

		var req DiscoveryRequest

		req = &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}}

feat(inputs.aliyuncms): Refactor common parts to plugins/common #18280

Are you sure you want to change the base?

feat(inputs.aliyuncms): Refactor common parts to plugins/common #18280

Conversation

ZPascal commented Jan 29, 2026 • edited by srebhan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Related issues

Uh oh!

ZPascal commented Jan 29, 2026

Uh oh!

telegraf-tiger bot commented Jan 29, 2026

Artifact URLs

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ZPascal commented Jan 29, 2026 •

edited by srebhan

Loading