fix: log name is updated as identifier instead of permify #1761
Conversation
WalkthroughThe changes involve a modification to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/cmd/log.go(2 hunks)
🔇 Additional comments (2)
pkg/cmd/log.go (2)
13-13: LGTM: Import statement for internal package.
The import is correctly added to access the internal.Identifier constant.
64-67: LGTM: Clean transition from hardcoded string to identifier constant.
The change from a hardcoded string to internal.Identifier improves maintainability while preserving the existing GCP handler functionality. The modification is minimal and focused on the specific concern raised in the PR.
| @@ -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 | |||
There was a problem hiding this comment.
💡 Codebase verification
Empty identifier needs validation and default value
The verification reveals a critical issue:
AccountIDis 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:
- Empty log names are invalid in GCP
- No validation exists for GCP's 512-character limit
- No URL-encoding is performed on the value
Locations needing attention:
internal/config/config.go: Add validation forAccountIDpkg/cmd/serve.go: Add default value and URL-encoding before settinginternal.Identifierinternal/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:
- The type and value of internal.Identifier
- 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.Identifieris initialized as an empty string ininternal/info.go- It's only set in
serve.gousingcfg.AccountIDwith 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:
cfg.AccountIDis properly validated before settinginternal.Identifier- 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
Summary by CodeRabbit