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

add additional managed namespaces #3

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

nbucci
Copy link
Contributor

@nbucci nbucci commented Nov 6, 2024

PR Type

Enhancement


Description

  • Introduced support for additional managed namespaces through new variable additional_managed_namespaces
  • Combined core and additional managed namespaces using distinct() to prevent duplicates
  • Updated role bindings to use the expanded namespace list
  • Added clarifying documentation for secret name generation behavior

Changes walkthrough 📝

Relevant files
Enhancement
main.tf
Extend namespace management with additional namespaces support

main.tf

  • Added managed_namespaces local variable that combines managed and
    additional managed namespaces
  • Added documentation for generate_name prefix in kubernetes_secret
  • Updated role binding to use combined managed namespaces list
  • +4/-1     
    Configuration changes
    variables.tf
    Add variable for additional managed namespaces                     

    variables.tf

  • Added new variable additional_managed_namespaces for specifying extra
    namespaces to manage
  • +6/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @sparkfabrik-ai-bot
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Role Binding Permissions:
    The role bindings are being created for multiple namespaces with deployment scale permissions. While this follows the principle of least privilege by only granting specific deployment scale permissions, ensure that the additional namespaces being added through var.additional_managed_namespaces are properly vetted and authorized to receive these permissions to prevent potential privilege escalation.

    ⚡ Recommended focus areas for review

    Potential Conflict
    The distinct() function is used to prevent duplicates between managed_namespaces and additional_managed_namespaces, but there's no validation to ensure the namespaces actually exist before being used in role bindings

    Resource Naming
    The generate_name prefix for kubernetes_secret_v1 could potentially create very long names if service_account_name is already long, which might hit Kubernetes name length limits

    @sparkfabrik-ai-bot
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Prevent unnecessary resource recreation by ignoring changes to generated names

    Consider adding a lifecycle block to ignore changes to the generate_name field to
    prevent unnecessary resource recreation.

    main.tf [42-46]

     metadata {
       generate_name = "${var.service_account_name}-"
       namespace     = local.final_namespace
       labels        = local.k8s_full_labels
     }
    +lifecycle {
    +  ignore_changes = [
    +    metadata[0].generate_name,
    +  ]
    +}
    Suggestion importance[1-10]: 9

    Why: Adding lifecycle block to ignore generate_name changes prevents unnecessary resource recreation, which is crucial for maintaining system stability and avoiding potential service disruptions.

    9
    Best practice
    Ensure generated resource names don't exceed Kubernetes length limits

    Consider adding a validation to check for the maximum length of the generated name
    to prevent potential truncation issues in Kubernetes.

    main.tf [44]

    -generate_name = "${var.service_account_name}-"
    +generate_name = substr("${var.service_account_name}-", 0, 57)
    Suggestion importance[1-10]: 8

    Why: Kubernetes has strict name length limits, and this suggestion helps prevent potential deployment failures by ensuring generated names stay within allowed limits.

    8
    Possible issue
    Add input validation to prevent empty or invalid namespace entries

    Consider adding input validation to ensure no empty strings or duplicate values are
    present in the managed namespaces lists before merging them.

    main.tf [10]

    -managed_namespaces = distinct(concat(var.managed_namespaces, var.additional_managed_namespaces))
    +managed_namespaces = distinct(compact(concat(var.managed_namespaces, var.additional_managed_namespaces)))
    Suggestion importance[1-10]: 7

    Why: Using compact() function to filter out empty strings is a good practice for input validation, helping prevent potential runtime errors from invalid namespace entries.

    7

    @nbucci nbucci force-pushed the platform/#3230-managed-namespaces branch from 39acc3e to c521488 Compare November 6, 2024 16:17
    @nbucci nbucci force-pushed the platform/#3230-managed-namespaces branch from c521488 to ada6663 Compare November 6, 2024 16:26
    @andypanix andypanix merged commit 69522cf into main Nov 6, 2024
    1 check passed
    @andypanix andypanix deleted the platform/#3230-managed-namespaces branch November 6, 2024 17:11
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants