Skip to content

Conversation

@hamzabouissi
Copy link
Contributor

@hamzabouissi hamzabouissi commented Oct 20, 2025

PR Type

Enhancement


Description

  • Replace node port configuration with LoadBalancer and externalIPs approach

  • Add support for internal nginx-ingress service deployment

  • Introduce node affinity selectors for load balancer pod placement

  • Remove lifecycle hooks and minReadySeconds from nginx deployments


Diagram Walkthrough

flowchart LR
  A["node_ports config"] -->|replaced| B["LoadBalancer with externalIPs"]
  B --> C["public_loadbalancer_ips"]
  B --> D["private_loadbalancer_ips"]
  E["nginx.internal.enabled"] -->|controls| F["Internal ClusterIP service"]
  G["nodeAffinity"] -->|selects| H["Dedicated LB nodes"]
Loading

File Walkthrough

Relevant files
Documentation
README.md
Update documentation for nginx configuration changes         

README.md

  • Removed documentation for node_ports configuration parameters
  • Added documentation for new nginx.internal.enabled boolean parameter
  • Updated parameter table to reflect simplified nginx service
    configuration
+1/-5     
Enhancement
application-nginx-public.yaml
Refactor public nginx service for external IPs and internal support

templates/application-nginx-public.yaml

  • Changed service type from conditional NodePort/LoadBalancer to static
    LoadBalancer
  • Added externalIPs configuration using public_loadbalancer_ips.public
    values
  • Introduced internal service configuration block with ClusterIP type
    and private IPs
  • Added node affinity requirement for use-as-public-lb label selector
  • Removed minReadySeconds (180s) and lifecycle.preStop sleep hooks
+17/-19 
application-nginx-glueops-oauth2.yaml
Refactor oauth2 nginx service for external IPs and internal support

templates/glueops-platform-ingress/application-nginx-glueops-oauth2.yaml

  • Changed service type from conditional NodePort/LoadBalancer to static
    LoadBalancer
  • Added externalIPs configuration using private_loadbalancer_ips.public
    values
  • Introduced internal service configuration block with ClusterIP type
    and private IPs
  • Added node affinity requirement for use-as-lb label selector
  • Removed minReadySeconds (180s) and lifecycle.preStop sleep hooks
Configuration changes
values.yaml
Replace node ports config with internal service flag         

values.yaml

  • Removed entire node_ports configuration section including enabled flag
    and port mappings
  • Added nginx.internal.enabled boolean parameter (default: false) for
    internal service control
  • Simplified nginx configuration structure by removing NodePort-specific
    settings
+2/-11   

@codiumai-pr-agent-free
Copy link
Contributor

codiumai-pr-agent-free bot commented Oct 20, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR integrates GlueKube functionality by transitioning from NodePort-based services to LoadBalancer services with external IPs and node affinity controls. The changes enable dedicated load balancer nodes and support for internal/private load balancing configurations.

  • Removes NodePort configuration in favor of LoadBalancer with external IPs
  • Adds node affinity rules to schedule ingress controllers on dedicated LB nodes
  • Introduces internal load balancer support configuration

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
values.yaml Removes node_ports configuration block and adds nginx.internal.enabled flag
templates/glueops-platform-ingress/application-nginx-glueops-platform-oauth2.yaml Replaces NodePort config with LoadBalancer using external IPs, adds node affinity for use-as-lb nodes, removes AWS NLB-specific timing configurations
templates/application-nginx-public.yaml Replaces NodePort config with LoadBalancer using external IPs, adds node affinity for use-as-public-lb nodes
README.md Updates documentation to reflect removal of node_ports parameters and addition of nginx.internal.enabled

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

{{- end }}
type: "LoadBalancer"
externalIPs: [{{ .Values.public_loadbalancer_ips.public | join ", " }}]
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

Missing required values definition. The code references .Values.public_loadbalancer_ips.public but this value is not defined in values.yaml, which will cause template rendering to fail.

Suggested change
externalIPs: [{{ .Values.public_loadbalancer_ips.public | join ", " }}]
externalIPs: [{{ .Values.public_loadbalancer_ips.public | default (list) | join ", " }}]

Copilot uses AI. Check for mistakes.
{{- end }}
type: "LoadBalancer"
externalIPs: [{{ .Values.private_loadbalancer_ips.public | join ", " }}]
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

Missing required values definition. The code references .Values.private_loadbalancer_ips but this value is not defined in values.yaml, which will cause template rendering to fail.

Copilot uses AI. Check for mistakes.
@codiumai-pr-agent-free
Copy link
Contributor

codiumai-pr-agent-free bot commented Oct 20, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Removing graceful shutdown hooks risks downtime

The suggestion warns that removing minReadySeconds and preStop hooks from NGINX
deployments risks reintroducing service interruptions during rolling updates, as
these hooks were intended to handle slow AWS NLB target deregistration.

Examples:

templates/application-nginx-public.yaml [120-123]
          updateStrategy:
            rollingUpdate:
              maxSurge: 1
              maxUnavailable: 0
templates/glueops-platform-ingress/application-nginx-glueops-platform-oauth2.yaml [120-123]
          updateStrategy:
            rollingUpdate:
              maxSurge: 1
              maxUnavailable: 0

Solution Walkthrough:

Before:

# templates/application-nginx-public.yaml
...
controller:
  updateStrategy:
    rollingUpdate:
      maxSurge: 1
      maxUnavailable: 0
  minReadySeconds: 180
  lifecycle:
    preStop:
      exec:
        command: ["/bin/sh", "-c", "sleep 240; /wait-shutdown"]
  metrics:
    enabled: true
...

After:

# templates/application-nginx-public.yaml
...
controller:
  updateStrategy:
    rollingUpdate:
      maxSurge: 1
      maxUnavailable: 0
  # minReadySeconds and lifecycle hooks are removed.
  metrics:
    enabled: true
...
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies the removal of critical graceful shutdown mechanisms (minReadySeconds and preStop hooks), which could reintroduce downtime during rolling updates with AWS NLBs.

High
  • Update

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