Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly parse includes outside the Configuration plugin #2063

Merged
merged 3 commits into from
Nov 22, 2024
Merged

fix: properly parse includes outside the Configuration plugin #2063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdcc01a
fix: properly parse includes outside the Configuration plugin
rustatian Nov 21, 2024
8c9aed5
chore: remove defer
rustatian Nov 21, 2024
642e83e
fix: expand envs for the root and included files
rustatian Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions internal/rpc/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ func NewClient(cfg string, flags []string) (*rpc.Client, error) {
return nil, err
}

// automatically inject ENV variables using ${ENV} pattern
expandEnvViper(v)

// override config Flags
if len(flags) > 0 {
for _, f := range flags {
Expand All @@ -48,6 +45,20 @@ func NewClient(cfg string, flags []string) (*rpc.Client, error) {
}
}

ver := v.Get(versionKey)
if ver == nil {
return nil, fmt.Errorf("rr configuration file should contain a version e.g: version: 3")
}

if _, ok := ver.(string); !ok {
return nil, fmt.Errorf("version should be a string: `version: \"3\"`, actual type is: %T", ver)
}

err = handleInclude(ver.(string), v)
if err != nil {
return nil, fmt.Errorf("failed to handle includes: %w", err)
}

// rpc.listen might be set by the -o flags or env variable
if !v.IsSet(rpcPlugin.PluginName) {
return nil, errors.New("rpc service not specified in the configuration. Tip: add\n rpc:\n\r listen: rr_rpc_address")
Expand Down
14 changes: 14 additions & 0 deletions internal/rpc/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,20 @@ func TestNewClient_SuccessfullyConnected(t *testing.T) {
defer func() { assert.NoError(t, c.Close()) }()
}

func TestNewClient_WithIncludes(t *testing.T) {
l, err := net.Listen("tcp", "127.0.0.1:6010")
assert.NoError(t, err)

defer func() { assert.NoError(t, l.Close()) }()

c, err := rpc.NewClient("test/include1/.rr.yaml", nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing required test configuration files

The test references a configuration file at test/include1/.rr.yaml, but this path doesn't exist in the repository. The only RPC-related configuration file found is at internal/rpc/test/config_rpc_conn_err.yaml, and the test directory structure is different from what's expected in the test.

  • internal/rpc/client_test.go: Update the test to use the correct path to the configuration file, likely should be internal/rpc/test/ instead of test/include1/
🔗 Analysis chain

Verify the referenced configuration files

Let's ensure the required configuration files exist and contain the expected structure.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for the existence and content of configuration files

echo "Checking for configuration files..."
fd -t f ".rr.yaml|.rr-include.yaml" test/include1/

echo -e "\nChecking file contents..."
for f in $(fd -t f ".rr.yaml|.rr-include.yaml" test/include1/); do
  echo -e "\nContents of $f:"
  cat "$f"
done

Length of output: 458

Script:

#!/bin/bash
# Let's first check if the test directory exists and locate any .rr.yaml files
echo "Checking for test directory and configuration files..."
fd -t d "test"

echo -e "\nSearching for .rr.yaml files in the entire repository..."
fd -t f ".rr.yaml"

echo -e "\nLooking for references to test/include1 in the codebase..."
rg "test/include1" -l

Length of output: 542


assert.NotNil(t, c)
assert.NoError(t, err)
Comment on lines +61 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add explicit validation of RPC configuration parsing

To better align with the PR objectives (fixing RPC plugin configuration includes), consider adding explicit validation of the parsed RPC configuration values.

Add assertions to verify that the RPC configuration was correctly parsed from the included file:

 	c, err := rpc.NewClient("test/include1/.rr.yaml", nil)
 
 	assert.NotNil(t, c)
 	assert.NoError(t, err)
+
+	// Verify that the RPC configuration was correctly parsed from the included file
+	cfg := c.Config() // Assuming there's a method to get the parsed config
+	assert.Equal(t, "tcp://127.0.0.1:6010", cfg.Listen, "RPC listen address should be loaded from included file")

Committable suggestion skipped: line range outside the PR's diff.


assert.NoError(t, c.Close())
}

func TestNewClient_SuccessfullyConnectedOverride(t *testing.T) {
l, err := net.Listen("tcp", "127.0.0.1:55554")
assert.NoError(t, err)
Expand Down
66 changes: 66 additions & 0 deletions internal/rpc/includes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package rpc

import (
"github.com/roadrunner-server/errors"
"github.com/spf13/viper"
)

const (
versionKey string = "version"
includeKey string = "include"
defaultConfigVersion string = "3"
prevConfigVersion string = "2.7"
)

func getConfiguration(path string) (map[string]any, string, error) {
v := viper.New()
v.SetConfigFile(path)
err := v.ReadInConfig()
if err != nil {
return nil, "", err
}

// get configuration version
ver := v.Get(versionKey)
if ver == nil {
return nil, "", errors.Str("rr configuration file should contain a version e.g: version: 2.7")
}

if _, ok := ver.(string); !ok {
return nil, "", errors.Errorf("type of version should be string, actual: %T", ver)
}

// automatically inject ENV variables using ${ENV} pattern
expandEnvViper(v)

return v.AllSettings(), ver.(string), nil
}
Comment on lines +15 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation and improve error handling.

Several improvements could make this function more robust:

  1. The path parameter should be validated before use
  2. Version validation could be more comprehensive
  3. The type assertion in the return statement could panic

Consider applying these improvements:

 func getConfiguration(path string) (map[string]any, string, error) {
+	if path == "" {
+		return nil, "", errors.Str("configuration path cannot be empty")
+	}
+
 	v := viper.New()
 	v.SetConfigFile(path)
 	err := v.ReadInConfig()
 	if err != nil {
 		return nil, "", err
 	}

 	// get configuration version
 	ver := v.Get(versionKey)
 	if ver == nil {
 		return nil, "", errors.Str("rr configuration file should contain a version e.g: version: 2.7")
 	}

 	if _, ok := ver.(string); !ok {
 		return nil, "", errors.Errorf("type of version should be string, actual: %T", ver)
 	}

+	version := ver.(string)
+	if version != defaultConfigVersion && version != prevConfigVersion {
+		return nil, "", errors.Errorf("unsupported configuration version: %s", version)
+	}

 	// automatically inject ENV variables using ${ENV} pattern
 	expandEnvViper(v)

-	return v.AllSettings(), ver.(string), nil
+	return v.AllSettings(), version, nil
 }

Committable suggestion skipped: line range outside the PR's diff.


func handleInclude(rootVersion string, v *viper.Viper) error {
// automatically inject ENV variables using ${ENV} pattern
// root config
expandEnvViper(v)

ifiles := v.GetStringSlice(includeKey)
if ifiles == nil {
return nil
}

for _, file := range ifiles {
config, version, err := getConfiguration(file)
if err != nil {
return err
}

if version != rootVersion {
return errors.Str("version in included file must be the same as in root")
}

// overriding configuration
for key, val := range config {
v.Set(key, val)
}
}

return nil
}
2 changes: 2 additions & 0 deletions internal/rpc/test/config_rpc_conn_err.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
version: "3"

rpc:
listen: tcp://127.0.0.1:55554
1 change: 1 addition & 0 deletions internal/rpc/test/config_rpc_empty.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
version: "3"
2 changes: 2 additions & 0 deletions internal/rpc/test/config_rpc_ok.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
version: "3"

rpc:
listen: tcp://127.0.0.1:55554
2 changes: 2 additions & 0 deletions internal/rpc/test/config_rpc_ok_env.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
version: "3"

rpc:
listen: ${RPC}
2 changes: 2 additions & 0 deletions internal/rpc/test/config_rpc_wrong.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
version: "3"

rpc:
$foo bar
5 changes: 5 additions & 0 deletions internal/rpc/test/include1/.rr-include.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
version: "3"

rpc:
listen: tcp://127.0.0.1:6010

11 changes: 11 additions & 0 deletions internal/rpc/test/include1/.rr.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
version: "3"

server:
command: "php app-with-domain-specific-routes.php"

logs:
level: debug
mode: development

include:
- test/include1/.rr-include.yaml
Loading