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

Jump to bottom

fix: log name is updated as identifier instead of permify #1761

Merged
merged 1 commit into from
Nov 6, 2024
Merged

fix: log name is updated as identifier instead of permify #1761

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion pkg/cmd/log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/Permify/sloggcp"
"github.com/agoda-com/opentelemetry-go/otelslog"

"github.com/Permify/permify/internal"
"github.com/Permify/permify/pkg/telemetry"
"github.com/Permify/permify/pkg/telemetry/logexporters"
)
Expand Down Expand Up @@ -60,7 +61,7 @@ func NewGCPHandler(headers map[string]string, level slog.Leveler) (slog.Handler,
}

// Initialize GCP-specific log handler
logName := "permify"
logName := internal.Identifier
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Empty identifier needs validation and default value

The verification reveals a critical issue:

  • AccountID is a simple string field in config without validation
  • No default value is set when empty
  • Used directly in GCP logging without sanitization

This could lead to GCP logging failures since:

  1. Empty log names are invalid in GCP
  2. No validation exists for GCP's 512-character limit
  3. No URL-encoding is performed on the value

Locations needing attention:

  • internal/config/config.go: Add validation for AccountID
  • pkg/cmd/serve.go: Add default value and URL-encoding before setting internal.Identifier
  • internal/info.go: Consider initializing with a safe default instead of empty string
🔗 Analysis chain

Verify internal.Identifier meets GCP requirements.

While using a constant is better than a hardcoded string, we should verify:

  1. The type and value of internal.Identifier
  2. Compliance with GCP log name requirements (must be less than 512 characters, URL-encoded)

Let's verify the internal.Identifier definition:

Empty identifier value could cause GCP logging issues

The verification reveals potential issues:

  • internal.Identifier is initialized as an empty string in internal/info.go
  • It's only set in serve.go using cfg.AccountID with a fallback check
  • Being used in multiple telemetry components (logging, tracing, metrics)

Since this is used as GCP log name, an empty string could cause logging initialization failures. The code should ensure:

  1. cfg.AccountID is properly validated before setting internal.Identifier
  2. A default fallback value is set when empty

Let's verify the AccountID validation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the definition and usage of internal.Identifier

# Search for the definition of Identifier
echo "Searching for Identifier definition:"
rg -p "^[[:space:]]*(?:var|const)[[:space:]]+Identifier[[:space:]]+" --type go

# Check if it's used elsewhere in the codebase
echo -e "\nChecking other usages of Identifier:"
rg "internal\.Identifier" --type go

Length of output: 928

Script:

#!/bin/bash
# Check AccountID validation and default value handling
echo "Checking AccountID validation:"
rg -A 5 "AccountID.*string" --type go

echo -e "\nChecking validation or default value logic:"
rg -B 3 -A 3 "cfg\.AccountID" --type go

Length of output: 1740

gcpHandler := sloggcp.NewGoogleCloudSlogHandler(context.Background(), projectId, logName, &slog.HandlerOptions{
Level: level,
})
Expand Down