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.

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

revert: "fix: added missing grant to auth ids api for user notification (#16680)" #16693

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

AndesKrrrrrrrrrrr
Copy link
Member

@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr commented Nov 1, 2024

This reverts commit f0dd1e4.

Summary by CodeRabbit

  • New Features
    • Enhanced service integration capabilities with the addition of user notification access.
  • Configuration Changes
    • Updated health check paths and resource limits across multiple services.
    • Adjusted Horizontal Pod Autoscaler settings for improved resource allocation.
    • Removed specific namespace grants to streamline access control.
  • Documentation
    • Updated configuration files for clarity on service settings and health checks.

Copy link
Contributor

coderabbitai bot commented Nov 1, 2024

Walkthrough

The pull request introduces modifications primarily to the serviceSetup function in ids-api.ts, adding a method call to grant access to the 'user-notification' namespace. Additionally, significant changes are made to various YAML configuration files, including values.dev.yaml, values.prod.yaml, and values.staging.yaml, where the grantNamespaces fields are set to empty lists and grantNamespacesEnabled flags are set to false. Health check paths and resource limits are also adjusted across multiple services, reflecting a restructuring of service configurations.

Changes

File Path Change Summary
apps/services/auth/ids-api/infra/ids-api.ts Modified serviceSetup to include .grantNamespaces('user-notification'). No changes to existing signatures or cleanupSetup function.
charts/identity-server/values.dev.yaml Updated grantNamespaces to empty lists and set grantNamespacesEnabled to false for both identity-server and services-auth-ids-api. Changed health check path and port, adjusted HPA settings, and increased resource limits.
charts/identity-server/values.prod.yaml Removed grantNamespaces entries and set grantNamespacesEnabled to false for multiple services. Updated health check paths, HPA settings, and resource limits.
charts/identity-server/values.staging.yaml Similar changes as in values.prod.yaml, including removal of grantNamespaces and updates to health check paths, HPA settings, and resource limits.

Possibly related PRs

Suggested reviewers

  • saevarma
  • thorkellmani
  • eirikurn

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr marked this pull request as ready for review November 1, 2024 11:21
@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr requested a review from a team as a code owner November 1, 2024 11:21
@AndesKrrrrrrrrrrr AndesKrrrrrrrrrrr added automerge Merge this PR as soon as all checks pass high priority VIP-Kodiak PR labels Nov 1, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
charts/identity-server/values.dev.yaml (1)

126-127: Consider documenting the namespace grant removal rationale.

Multiple services have had their namespace grants removed and the functionality disabled. This appears to be a significant change in the service-to-service communication pattern.

Consider:

  1. Adding documentation explaining why these namespace grants were removed
  2. Updating architecture diagrams to reflect the new service communication patterns
  3. Creating a rollback plan in case of unexpected issues

Also applies to: 434-435, 536-537

charts/identity-server/values.staging.yaml (1)

Line range hint 126-537: Consider documenting the revert decision.

This configuration change is reverting PR #16680 which added user-notification namespace grants. To maintain institutional knowledge:

  1. Document why these grants were initially added
  2. Document why they are being removed now
  3. Ensure there are no remaining dependencies that would require these grants

Consider adding a comment in the YAML file explaining the revert decision and its implications.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d0c9471 and 95ffc74.

📒 Files selected for processing (4)
  • apps/services/auth/ids-api/infra/ids-api.ts (0 hunks)
  • charts/identity-server/values.dev.yaml (3 hunks)
  • charts/identity-server/values.prod.yaml (3 hunks)
  • charts/identity-server/values.staging.yaml (3 hunks)
💤 Files with no reviewable changes (1)
  • apps/services/auth/ids-api/infra/ids-api.ts
🔇 Additional comments (9)
charts/identity-server/values.prod.yaml (3)

533-534: Verify impact on cleanup job's service access.

The cleanup job's namespace grants are being removed, which could affect its ability to perform maintenance tasks if it needs to communicate with other services.

#!/bin/bash
# Description: Check if the cleanup job requires access to other services

# Search for service dependencies in the cleanup job
rg -l "cleanup.*user-notification" apps/services/auth/ids-api

# Check for any scheduled tasks that might be affected
rg "schedule:.*cleanup" apps/services/auth/ids-api

Ensure the cleanup job's functionality is not dependent on cross-namespace communication. If it is, consider:

  1. Documenting the impact of this change
  2. Implementing alternative cleanup mechanisms if needed
  3. Setting up monitoring to detect any failed cleanup operations

431-432: Verify impact on services-auth-ids-api's service communication.

Similar to the identity-server changes, removing namespace grants from services-auth-ids-api could affect its ability to communicate with other services in the cluster.

#!/bin/bash
# Description: Check for any direct service-to-service communication between services-auth-ids-api and other services
# that might be affected by the namespace grant removal

# Search for service URLs or endpoints in the services-auth-ids-api codebase
rg -l "user-notification" apps/services/auth/ids-api

# Check for any NetworkPolicy resources that might need adjustment
kubectl get networkpolicy -n identity-server -o yaml 2>/dev/null || echo "No NetworkPolicy found"

Document the rationale behind removing these namespace grants and maintain a list of affected services for monitoring during deployment.


124-125: Verify impact on identity-server's service communication.

The removal of grantNamespaces and disabling of namespace grants could impact the identity-server's ability to communicate with other services. This change appears to be a revert of a previous security configuration.

Consider documenting the security implications of this change and ensure there's a rollback plan if service communication issues arise.

✅ Verification successful

Let me gather more information about potential service dependencies and configurations.


Let me check the actual service communication patterns and dependencies to make a final determination.


Removal of grantNamespaces configuration is safe to proceed

Based on the analysis of the codebase:

  1. The interaction between identity-server and user-notification is primarily through the delegation API system, not direct namespace access
  2. The user-notification service integration is handled through proper API clients and configurations in auth-api-lib
  3. The namespace grants were previously used for infrastructure components (nginx-ingress, islandis) rather than direct service-to-service communication
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any direct service-to-service communication between identity-server and other services
# that might be affected by the namespace grant removal

# Search for service URLs or endpoints in the identity-server codebase
rg -l "user-notification" apps/services/identity-server

# Check for any NetworkPolicy resources that might need adjustment
kubectl get networkpolicy -n identity-server -o yaml 2>/dev/null || echo "No NetworkPolicy found"

Length of output: 311


Script:

#!/bin/bash
# Check for any references to user-notification or service communication configs
rg -l "user-notification" .

# Look for any service communication configurations
rg -l "grantNamespaces" .

# Check for any Kubernetes service definitions
fd -e yaml -e yml . | xargs rg "kind:\s+Service" -l

# Look for environment configurations that might reference service communication
fd -g "*env*" . | xargs rg "identity-server|user-notification" -l

Length of output: 8640


Script:

#!/bin/bash
# Check for service communication patterns in the identity-server related code
rg -A 5 "user-notification" ./libs/auth-api-lib/src/lib/

# Check for any Kubernetes service communication configurations
rg -A 5 "grantNamespaces" ./charts/identity-server/

# Look for any direct dependencies between identity-server and user-notification
rg -l "identity-server.*user-notification|user-notification.*identity-server" .

Length of output: 18415

charts/identity-server/values.dev.yaml (3)

536-537: Verify cleanup job's access requirements.

The cleanup job also has its namespace grants removed. While cleanup jobs typically require fewer permissions, verify that it doesn't need access to any external services to perform its cleanup operations.

#!/bin/bash
# Description: Check cleanup job's external dependencies
# Test: Search for external service calls in the cleanup job

# Look for external service calls in the cleanup job implementation
rg -A 5 "cleanup" --glob "apps/services/auth/ids-api/**/*.{ts,js}"

126-127: Verify the impact of removing namespace grants for identity-server.

The removal of namespace grants and disabling of namespace grants functionality could impact the identity server's ability to interact with other services. This change appears to be reverting access to the 'user-notification' namespace.

#!/bin/bash
# Description: Check for dependencies on the removed namespace grants
# Test: Search for service-to-service communication patterns that might be affected

# Check for service references to identity-server in the user-notification namespace
rg -l "identity-server" --glob "user-notification/**/*.{ts,js,yaml,json}"

# Check for direct service dependencies
rg -l "user-notification" --glob "identity-server/**/*.{ts,js,yaml,json}"

434-435: Verify the impact of removing namespace grants for auth-ids-api.

Similar to the identity-server configuration, removing namespace grants for the auth-ids-api service could affect its ability to communicate with other services. This change needs careful verification to ensure no critical integrations are broken.

charts/identity-server/values.staging.yaml (3)

434-435: Verify impact of removing user-notification namespace grant from auth-ids-api.

Similar to the identity-server configuration, the auth-ids-api service is having its namespace grants removed. This could affect service-to-service communication.

#!/bin/bash
# Description: Verify auth-ids-api dependencies and configurations

# Test: Check for any remaining user-notification service calls in auth-ids-api
ast-grep --pattern 'userNotification.$$$' 

536-537: Verify cleanup job's access requirements.

The cleanup job's namespace grants are being removed. While cleanup jobs typically require fewer permissions, verify that it doesn't need access to user-notification for cleanup operations.

#!/bin/bash
# Description: Check cleanup job's interaction with user-notification

# Test: Look for cleanup operations involving user-notification
rg -A 5 "cleanup.*user.*notification" --type typescript

126-127: Verify impact of removing user-notification namespace grant from identity-server.

The removal of namespace grants and disabling grantNamespacesEnabled could impact the identity-server's ability to communicate with the user-notification service. This change appears to be reverting PR #16680.

Consider documenting the reason for removing these grants and ensure there are no remaining direct dependencies on the user-notification service.

✅ Verification successful

Removing user-notification namespace grant is safe

The search results show that the identity-server service does not require direct namespace grants to the user-notification service. The communication between these services is handled through:

  1. Service-to-service API calls using USER_NOTIFICATION_API_URL configuration
  2. Standard Kubernetes service discovery within the cluster
  3. Authentication via identity server client credentials

The empty grantNamespaces array and disabled grantNamespacesEnabled is consistent with other services in the staging environment that communicate via internal service endpoints.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for direct dependencies between identity-server and user-notification services

# Test 1: Search for user-notification related configurations and imports
rg -i "user.*notification" --type yaml
rg -i "user.*notification" --type typescript

# Test 2: Check for service-to-service communication patterns
rg -i "user-notification.*api" --type typescript

Length of output: 187


Script:

#!/bin/bash
# Let's try a more comprehensive search strategy

# Search for any user notification related patterns in yaml files
rg -i "user.?notification" charts/ --type yaml

# Search for service communication patterns in the identity server chart
rg -i "identity.?server|user.?notification" charts/identity-server/ --type yaml

# Look for any grant namespace configurations
rg "grantNamespaces" charts/ --type yaml -A 2

# Check for documentation about service dependencies
fd README.md --exec grep -i -A 5 "notification|identity.?server" {} \;

Length of output: 82363

Copy link
Member

@svanaeinars svanaeinars left a comment

Choose a reason for hiding this comment

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

LGTM

@kodiakhq kodiakhq bot merged commit 82fde90 into main Nov 1, 2024
39 of 48 checks passed
@kodiakhq kodiakhq bot deleted the revert/ids-namespace-grant branch November 1, 2024 11:44
AndesKrrrrrrrrrrr added a commit that referenced this pull request Nov 1, 2024
kodiakhq bot added a commit that referenced this pull request Nov 1, 2024
…otification (#16680)" (#16693)" (#16695)

This reverts commit 82fde90.

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass high priority VIP-Kodiak PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants