Add token path to Agent config Command block #958

sean-breen · 2025-01-07T13:30:54Z

Proposed changes

Allow the data plane key to be loaded from a file by adding a new configuration option token-path under the Command section of the agent config file. Also refactored some code to create new functions addTransportCredentials and validateTokenFile which are used when creating the gRPC credentials.

If both token and token-path are set:
- token-path will take priority
- token field will be used as a fallback when the token-path file does not exist or is inaccessible

command:
  server:
    host: "127.0.0.1"             # Command server host
    port: 8888                    # Command server port
  tls:
    server_name: test-local-server
    ca: /tmp/ca.pem
    cert: /tmp/cert.pem
    key: /tmp/key.pem
    generate_self_signed_cert: true
  auth:
    token: "1a2b3c4d"    
    token-path: "path/to/secret-token"    # Path to a file containing the authentication token for the command sever

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING document
I have run make install-tools and have attached any dependency changes to this pull request
If applicable, I have added tests that prove my fix is effective or that my feature works
If applicable, I have checked that any relevant tests pass after adding my changes
If applicable, I have updated any relevant documentation (README.md)
If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

dhurley · 2025-01-09T16:15:41Z

internal/grpc/grpc.go

-		)
-	} else {
+	if err != nil {
+		slog.Error("Unable to add transport credentials to gRPC dial options", "error", err)


This error log is misleading since we are adding the default transport credentials. Maybe it should be Unable to get transport credentials from agent configuration, adding default transport credentials to gRPC dial options

dhurley · 2025-01-09T16:15:52Z

internal/grpc/grpc.go

-		)
-	} else {
+	if err != nil {
+		slog.Error("Unable to add transport credentials to gRPC dial options", "error", err)
 		slog.Debug("Adding default transport credentials to gRPC dial options")


Maybe remove the debug log

dhurley · 2025-01-09T16:47:32Z

internal/grpc/grpc.go

 	return opts
 }

+func validateTokenFile(path string) (string, error) {


Suggested change

func validateTokenFile(path string) (string, error) {

func retrieveTokenFromFile(path string) (string, error) {

might be a better description for what the function is doing

dhurley · 2025-01-09T16:48:14Z

internal/grpc/grpc.go

+}
+
+func addPerRPCCredentials(agentConfig *config.Config, resourceID string, opts []grpc.DialOption) []grpc.DialOption {
+	key := agentConfig.Command.Auth.Token


Suggested change

key := agentConfig.Command.Auth.Token

token := agentConfig.Command.Auth.Token

would token be a better name for this variable?

dhurley · 2025-01-09T16:49:51Z

internal/grpc/grpc.go

+		return "", errors.New("token file path is empty")
+	}
+
+	slog.Debug("Reading dataplane key from file", "path", path)


Suggested change

slog.Debug("Reading dataplane key from file", "path", path)

slog.Debug("Reading token from file", "path", path)

Should be consistent is our naming. I think referring to it as token is better than dataplane key

dhurley · 2025-01-09T16:51:22Z

internal/grpc/grpc.go

 	return opts
 }

+func validateTokenFile(path string) (string, error) {
+	if path == "" {
+		slog.Error("Token file path is empty")


If you are return the error then there is no need to also log it here since it will be logged by the function calling it

dhurley · 2025-01-09T16:52:17Z

internal/grpc/grpc.go

+
+	if keyVal == "" {
+		slog.Error("failed to load token, please check agent configuration")
+		return "", errors.New("failed to load token, please check agent configuration")


Suggested change

return "", errors.New("failed to load token, please check agent configuration")

return "", errors.New("failed to retrieve token, token file is empty")

dhurley · 2025-01-13T15:39:51Z

internal/grpc/grpc.go

+		slog.Error("Unable to read token from file", "error", err)
+		return "", err


Suggested change

slog.Error("Unable to read token from file", "error", err)

return "", err

return "", fmt.Errorf("unable to read token from file %w", err)

dhurley · 2025-01-13T15:44:08Z

internal/grpc/grpc_test.go

+	}
+}
+
+func Test_getTransportCredentials(t *testing.T) {


Just wondering if the struct array is needed since there is only one test case

Not really needed, but I was going to leave it there in case of expansion in the future.

* modify alpine package name: nginx-agent-3.0.0_1234 -> nginx-agent-3.0.0.1234 * protoc-gen update

* update config defaults and format

sean-breen added 4 commits January 6, 2025 15:22

support reading token from file via config

1a17cd5

remove empty file

c4b962d

simplify token validation and add unit tests

d68fe7e

add unit tests for transport credentials funtions

1b2b029

github-actions bot added the chore Pull requests for routine tasks label Jan 7, 2025

sean-breen marked this pull request as ready for review January 8, 2025 14:33

sean-breen requested a review from a team as a code owner January 8, 2025 14:33

sean-breen changed the title ~~Draft: Add token path to Agent config~~ Add token path to Agent config Jan 8, 2025

sean-breen changed the title ~~Add token path to Agent config~~ Add token path to Agent config Command block Jan 8, 2025

dhurley reviewed Jan 9, 2025

View reviewed changes

sean-breen added 5 commits January 10, 2025 11:13

address PR feedback

a43cb53

proto updates

2ecf102

fix function name

6872207

fix lint error: lll

2c30d2c

add missing PR feedback

2ee40ed

sean-breen requested a review from dhurley January 10, 2025 11:30

dhurley reviewed Jan 13, 2025

View reviewed changes

sean-breen added 2 commits January 14, 2025 13:07

remove error log message

3cbf80a

fix unit test

95403c9

sean-breen requested review from dhurley, aphralG and oliveromahony January 14, 2025 17:04

sean-breen and others added 5 commits January 15, 2025 15:09

Fix apk test package naming (#961)

b36e5fd

* modify alpine package name: nginx-agent-3.0.0_1234 -> nginx-agent-3.0.0.1234 * protoc-gen update

Update agent config defaults and format (#959)

36f9e3c

* update config defaults and format

Add config apply request queue (#949)

81a70e1

add unit tests for transport credentials funtions

e096397

fix test name

df3b793

sean-breen added the v3.x Issues and Pull Requests related to the major version v3 label Jan 15, 2025

sean-breen added 2 commits January 16, 2025 09:50

fix error message

a44ca08

fix lint error: lll

8d61593

sean-breen added 2 commits January 16, 2025 15:27

modify error messages

459c0d5

remove error logging and modify messages

f26c45e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add token path to Agent config Command block #958

Add token path to Agent config Command block #958

sean-breen commented Jan 7, 2025 •

edited

Loading

dhurley Jan 9, 2025

dhurley Jan 9, 2025

dhurley Jan 9, 2025

dhurley Jan 9, 2025

dhurley Jan 9, 2025

dhurley Jan 9, 2025

dhurley Jan 9, 2025

dhurley Jan 13, 2025

dhurley Jan 13, 2025

sean-breen Jan 14, 2025

	func validateTokenFile(path string) (string, error) {
	func retrieveTokenFromFile(path string) (string, error) {

	key := agentConfig.Command.Auth.Token
	token := agentConfig.Command.Auth.Token

	slog.Debug("Reading dataplane key from file", "path", path)
	slog.Debug("Reading token from file", "path", path)

	return "", errors.New("failed to load token, please check agent configuration")
	return "", errors.New("failed to retrieve token, token file is empty")

		slog.Error("Unable to read token from file", "error", err)
		return "", err

	slog.Error("Unable to read token from file", "error", err)
	return "", err
	return "", fmt.Errorf("unable to read token from file %w", err)

Add token path to Agent config Command block #958

Are you sure you want to change the base?

Add token path to Agent config Command block #958

Conversation

sean-breen commented Jan 7, 2025 • edited Loading

Proposed changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-breen commented Jan 7, 2025 •

edited

Loading