Provision client object if it does not exist #140
Conversation
|
Warning Rate limit exceeded@NickCao has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
""" WalkthroughThe updates modify the authorization logic by upgrading the authorizer to use a full client interface, enhancing error handling and resource creation for missing Client resources. Additionally, the metadata attribute getter now constructs a sanitized and hashed username to ensure a non-empty name in the context attributes. Configuration loading was updated to support provisioning settings propagated to the authorizer. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Plan: Pro
📒 Files selected for processing (2)
internal/authorization/basic.go(3 hunks)internal/authorization/metadata.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/authorization/basic.go (1)
api/v1alpha1/client_types.go (2)
Client(44-50)ClientSpec(28-30)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: deploy-kind
🔇 Additional comments (6)
internal/authorization/metadata.go (2)
5-6: LGTM! Appropriate imports for hash generation.The added imports support the SHA-256 hash generation logic for handling empty names.
57-60: Good fallback mechanism for empty names.The SHA-256 hash generation provides a deterministic, non-empty identifier when the name metadata is missing. This creates a stable mapping from username to resource name that supports the client provisioning logic in basic.go.
internal/authorization/basic.go (4)
8-9: LGTM! Necessary imports for client provisioning.The added imports support the enhanced error handling and client resource creation functionality.
Also applies to: 11-11
16-16: Good upgrade to full client interface.Upgrading from
client.Readertoclient.Clientenables the creation of missing client resources while maintaining all existing read capabilities.Also applies to: 20-21
31-31: LGTM! Updated to use the new client interface.The Exporter case correctly uses the upgraded client interface for the Get operation.
66-70: Authorization check logic is sound.The authorization check correctly validates that the user's name is in the client's usernames list. This will work properly once the previous issue with the client object reference is resolved.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🛑 Comments failed to post (1)
internal/authorization/metadata.go (1)
59-76: 🛠️ Refactor suggestion
Improve edge case handling and Kubernetes naming compliance.
The naming logic has potential issues that could generate invalid Kubernetes resource names:
- Double hyphens: Empty usernames or usernames with only invalid characters result in names like
"oidc--<hash>"or"oidc-----<hash>"- Invalid start/end characters: Kubernetes names must start and end with alphanumeric characters, but this logic could generate names ending with hyphens
- Dots in resource names: The regex allows dots, but some Kubernetes resources don't permit dots in names
Consider this improved implementation:
if name == "" { hash := sha256.Sum256([]byte(userInfo.GetName())) - sanitized := regexp.MustCompile("[^a-z0-9-.]+").ReplaceAllString( + sanitized := regexp.MustCompile("[^a-z0-9-]+").ReplaceAllString( strings.ToLower(userInfo.GetName()), "-", ) + + // Remove leading/trailing hyphens and collapse multiple hyphens + sanitized = regexp.MustCompile("^-+|-+$").ReplaceAllString(sanitized, "") + sanitized = regexp.MustCompile("-+").ReplaceAllString(sanitized, "-") + + // Ensure we have a valid base name + if sanitized == "" { + sanitized = "user" + } if len(sanitized) > 37 { sanitized = sanitized[:37] } + + // Ensure name doesn't end with hyphen after truncation + sanitized = strings.TrimRight(sanitized, "-") + if sanitized == "" { + sanitized = "user" + } name = strings.Join([]string{ "oidc", sanitized, hex.EncodeToString(hash[:3]), }, "-") }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if name == "" { hash := sha256.Sum256([]byte(userInfo.GetName())) // Sanitize: lowercase, allow only a–z, 0–9, and hyphens sanitized := regexp.MustCompile("[^a-z0-9-]+").ReplaceAllString( strings.ToLower(userInfo.GetName()), "-", ) // Remove leading/trailing hyphens and collapse multiple hyphens sanitized = regexp.MustCompile("^-+|-+$").ReplaceAllString(sanitized, "") sanitized = regexp.MustCompile("-+").ReplaceAllString(sanitized, "-") // Ensure we have a non-empty base if sanitized == "" { sanitized = "user" } // Truncate to max length if len(sanitized) > 37 { sanitized = sanitized[:37] } // Trim any trailing hyphens after truncation sanitized = strings.TrimRight(sanitized, "-") if sanitized == "" { sanitized = "user" } name = strings.Join([]string{ "oidc", sanitized, hex.EncodeToString(hash[:3]), }, "-") }🤖 Prompt for AI Agents
In internal/authorization/metadata.go around lines 59 to 76, the current username sanitization can produce invalid Kubernetes resource names due to double hyphens, invalid start/end characters, and dots in names. To fix this, update the regex to exclude dots, trim leading and trailing hyphens from the sanitized string, and replace consecutive hyphens with a single hyphen. Also, add a fallback to a default string if the sanitized username becomes empty after cleanup to avoid double hyphens in the final name. Ensure the final constructed name starts and ends with an alphanumeric character to comply with Kubernetes naming rules.
NickCao
force-pushed
the
provisioning
branch
2 times, most recently
from
bacede3 to
ec2dfc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/authorization/metadata_test.go (1)
39-42: Consider using require.NoError for cleaner test failure handling.While the current error handling works, you could make the test slightly cleaner by using testify's require package:
- if validation.IsDNS1123Subdomain(result) != nil { - t.Error(fmt.Sprintf("normalizing the name %s does not produce a valid RFC1123 subdomain, but %s", - testcase.input, result)) - } + require.NoError(t, validation.IsDNS1123Subdomain(result), + "normalizing %s should produce valid RFC1123 subdomain, got %s", testcase.input, result)This would also stop the test immediately on validation failure rather than continuing to check the exact output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/authorization/metadata.go(4 hunks)internal/authorization/metadata_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/authorization/metadata.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-go
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: tests
🔇 Additional comments (1)
internal/authorization/metadata_test.go (1)
11-48: Excellent test implementation with comprehensive coverage.This test function follows Go testing best practices with several strengths:
- Table-driven approach: Clean structure with clear input/output pairs
- Comprehensive edge cases: Covers simple names, email formats, multiple special characters, boundary characters, and length limits
- Dual validation: Tests both Kubernetes DNS-1123 compliance and exact output matching
- Clear error messages: Descriptive error reporting with context
The test cases effectively validate the normalization logic for usernames that will be used as Kubernetes identifiers.
Summary by CodeRabbit
New Features
Bug Fixes