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

Feat/adding kube proxy and fixing core dns #114

Merged
merged 6 commits into from
May 9, 2024

Conversation

venkatamutyala
Copy link
Contributor

@venkatamutyala venkatamutyala commented May 9, 2024

PR Type

Enhancement, Bug fix


Description

  • Consolidated all EKS addons into a single Terraform file (addons.tf), improving maintainability.
  • Introduced kube_proxy addon configuration to the EKS setup.
  • Updated coredns addon configuration to reflect new version requirements.
  • Added and updated several variables in variables.tf to support the latest versions and new components.

Changes walkthrough 📝

Relevant files
Configuration changes
addon_coredns.tf
Remove CoreDNS resource configuration                                       

addon_coredns.tf

  • Removed the coredns resource configuration.
+0/-15   
variables.tf
Update and Add Variables for EKS Components                           

variables.tf

  • Updated default versions for csi_driver_version and coredns_version.
  • Added new variable kube_proxy_version.
  • Updated eks_version default value.
  • +9/-3     
    Enhancement
    addons.tf
    Consolidate Addons and Add Kube Proxy Configuration           

    addons.tf

  • Consolidated all addons into a single file.
  • Added kube_proxy addon configuration.
  • Updated coredns addon configuration.
  • +25/-0   
    Tests
    main.tf
    Update Test Configuration with Kube Proxy Version               

    tests/main.tf

    • Added kube_proxy_version variable to the test configuration.
    +1/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @venkatamutyala venkatamutyala marked this pull request as ready for review May 9, 2024 18:22
    @codiumai-pr-agent-free codiumai-pr-agent-free bot added documentation Improvements or additions to documentation bug_fix labels May 9, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (2085fec)

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented May 9, 2024

    PR Review 🔍

    (Review updated until commit 9b719ef)

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple changes across different configuration files, including the addition of new resources and updating existing configurations. The changes are not overly complex but require a good understanding of Terraform and AWS EKS components to ensure they are correct and do not introduce regressions.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Version Mismatch: The kube_proxy_version is set to "v1.27.10-eksbuild.2" in variables.tf, but the test configuration in tests/main.tf uses "v1.28.6-eksbuild.2". This discrepancy could lead to inconsistencies and should be aligned.

    Dependency Management: The depends_on attribute for both coredns and kube_proxy resources depends solely on [module.node_pool]. If there are other dependencies that affect the initialization or configuration of these addons, they should be explicitly included to avoid potential runtime issues.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Maintainability
    Reduce redundancy by using a dynamic block for conflict resolution settings.

    Consider using a dynamic block for resolve_conflicts_on_create and
    resolve_conflicts_on_update to reduce redundancy and improve maintainability. This
    approach allows you to define these properties in one place and reuse them across
    different resources.

    addons.tf [61-77]

    -resolve_conflicts_on_create = "OVERWRITE"
    -resolve_conflicts_on_update = "OVERWRITE"
    +dynamic "conflict_resolution" {
    +  for_each = ["coredns", "kube_proxy"]
    +  content {
    +    resolve_conflicts_on_create = "OVERWRITE"
    +    resolve_conflicts_on_update = "OVERWRITE"
    +  }
    +}
     
    Best practice
    Improve flexibility by conditionally setting the service_account_role_arn.

    It is recommended to handle the service_account_role_arn with a conditional check rather
    than setting it to null directly. This can help in future configurations where a specific
    role might be needed based on certain conditions.

    addons.tf [64-79]

    -service_account_role_arn = null
    +service_account_role_arn = var.use_custom_service_account ? var.custom_service_account_role_arn : null
     
    Possible issue
    Add validation to ensure node pools are not empty before deployment.

    To avoid potential issues with zero node pools, consider adding a validation to ensure
    that var.node_pools is not empty before deploying the addons.

    addons.tf [66-81]

     count = length(var.node_pools) > 0 ? 1 : 0
    +validation {
    +  condition     = length(var.node_pools) > 0
    +  error_message = "At least one node pool must be specified."
    +}
     
    Bug
    Align the default kube_proxy_version with the EKS version to ensure compatibility.

    Ensure the default version of kube_proxy_version matches the EKS version to avoid
    compatibility issues. The default set in variables.tf does not match the eks_version set
    in tests/main.tf.

    variables.tf [20]

    -default = "v1.27.10-eksbuild.2"
    +default = "v1.28.6-eksbuild.2"
     
    Enhancement
    Update the default eks_version to maintain consistency with test configurations.

    Update the eks_version default value to match the latest version used in the test
    configurations to maintain consistency across your Terraform configurations.

    variables.tf [68]

    -default = "1.27"
    +default = "1.28"
     

    Copy link

    sonarqubecloud bot commented May 9, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @codiumai-pr-agent-free codiumai-pr-agent-free bot added Bug fix and removed documentation Improvements or additions to documentation labels May 9, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (9b719ef)

    Copy link
    Contributor

    Persistent review updated to latest commit 9b719ef

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Best practice
    Use a dynamic block for conditional module dependencies.

    It's recommended to use a dynamic block for depends_on when referencing modules,
    especially when the dependency is conditional based on a variable like var.node_pools.
    This ensures that the dependency is handled correctly during the Terraform plan and apply
    phases.

    addons.tf [78]

    -depends_on = [module.node_pool]
    +dynamic "depends_on" {
    +  for_each = length(var.node_pools) > 0 ? [module.node_pool] : []
    +  content {
    +    depends_on = [module.node_pool]
    +  }
    +}
     
    Enhancement
    Improve the robustness of conditional resource creation.

    To avoid potential issues with resource creation based on the count of node pools,
    consider using a more explicit conditional check or handling the scenario where
    var.node_pools might be undefined or null.

    addons.tf [79]

    -count = length(var.node_pools) > 0 ? 1 : 0
    +count = can(length(var.node_pools)) && length(var.node_pools) > 0 ? 1 : 0
     
    Bug
    Align the default kube_proxy_version with the eks_version to ensure compatibility.

    The default value for kube_proxy_version is set to "v1.27.10-eksbuild.2", which is
    inconsistent with the eks_version of "1.28" specified in tests/main.tf. Aligning these
    versions can prevent compatibility issues.

    variables.tf [20]

    -default = "v1.27.10-eksbuild.2"
    +default = "v1.28.6-eksbuild.2"
     
    Security
    Use separate IAM roles for different EKS addons to adhere to the principle of least privilege.

    Reusing the IAM role aws_iam_role.eks_addon_ebs_csi_role for the coredns addon might lead
    to permission issues if the role's policies are not properly scoped for both coredns and
    ebs_csi. Consider creating a separate IAM role for coredns to ensure least privilege
    access control.

    addons.tf [64]

    -service_account_role_arn = aws_iam_role.eks_addon_ebs_csi_role.arn
    +service_account_role_arn = aws_iam_role.eks_addon_coredns_role.arn
     
    Possible issue
    Review and potentially diversify conflict resolution strategies for different EKS addons.

    Using the same resolve_conflicts_on_create and resolve_conflicts_on_update settings for
    both coredns and kube_proxy addons might not be suitable for all scenarios. Review if
    different strategies might be needed based on the specific requirements of each addon.

    addons.tf [61-76]

     resolve_conflicts_on_create = "OVERWRITE"
    -resolve_conflicts_on_update = "OVERWRITE"
    +resolve_conflicts_on_update = "PRESERVE"
     

    @venkatamutyala venkatamutyala merged commit a2e524c into main May 9, 2024
    7 of 9 checks passed
    @venkatamutyala venkatamutyala deleted the feat/adding-kube-proxy-and-fixing-core-dns branch May 9, 2024 22:35
    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