-
Notifications
You must be signed in to change notification settings - Fork 3
[DEV-14578] Migrade k8s ingress controller to nginx ingress controller #527
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
Conversation
| podAnnotations: | ||
| linkerd.io/inject: ${var.enable_service_mesh ? "enabled" : "false"} | ||
| annotations: | ||
| linkerd.io/inject: ${var.enable_service_mesh ? "\"enabled\"" : "\"disabled\""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linkerd annotation moved from pods to deployment level
High Severity
The linkerd.io/inject annotation was moved from controller.podAnnotations to controller.annotations. For linkerd sidecar injection to work, this annotation must be applied to pods, not to the Deployment resource. Using annotations instead of podAnnotations will prevent linkerd from injecting its sidecar proxy into the nginx ingress controller pods, breaking service mesh functionality when enable_service_mesh is true.
| httpPort: | ||
| enable: ${local.enableHttp} | ||
| httpsPort: | ||
| enable: ${local.backend_port == "https" ? true : false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTPS port disabled when using ACM certificate
High Severity
The httpsPort.enable value is set based on backend_port == "https", but when ACM is used (var.acm_arn != ""), backend_port is set to "http". This causes httpsPort.enable to be false when using ACM, which would disable the HTTPS listener entirely. ACM terminates SSL at the load balancer and forwards to HTTP backend - the HTTPS port should still be enabled for client connections.
| service.beta.kubernetes.io/aws-load-balancer-scheme: internal | ||
| service.beta.kubernetes.io/aws-load-balancer-subnets: "${var.internal_elb_use_public_subnets ? join(", ", local.environment_public_subnet_ids) : join(", ", local.environment_private_subnet_ids)}" | ||
| EOT | ||
| ) : ""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate YAML annotation keys in loadbalancer configuration
Medium Severity
When internal_elb is true, the loadbalancer_config and acm_loadbalancer_config generate YAML with duplicate annotation keys. For example, aws-load-balancer-scheme is set to "internet-facing" unconditionally, then conditionally set to "internal" again. Similarly, aws-load-balancer-type appears twice with "external". Duplicate YAML keys cause undefined parsing behavior and make the configuration fragile.
Additional Locations (1)
| httpsPort: | ||
| enable: ${local.backend_port == "https" ? true : false} | ||
| ${local.lb_config} | ||
| image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing image registry configuration for nginx ingress
High Severity
The nginx ingress controller image registry configuration has been completely removed. The old code explicitly configured controller.image.registry and admissionWebhooks.patch.image.registry to use ${var.image_registry}/registry.k8s.io. Without this configuration, the nginx ingress controller will attempt to pull images from public registries instead of the customer's private registry. This will cause image pull failures in air-gapped or private registry environments where var.image_registry is set.
ashmuck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Note
Migrates ingress configuration and streamlines load balancer setup across AWS and Azure.
ingress-nginxwithnginx-ingressvalues inapplication.tfandazure/*, updating controller annotations and service port fields (httpPort/httpsPort) and removing legacy blocks (rbac/admission webhook patches)aws-load-balancer-controllerand refactor NLB/ALB annotations via locals (loadbalancer_config,acm_loadbalancer_config), settingnlb-target-type: ip, scheme, subnets, and security group annotationscontroller.annotations(linkerd.io/injecttoggled to "enabled"/"disabled")nginx-ingressand update private DNS/internal LB annotations under the new keyWritten by Cursor Bugbot for commit 49e8658. This will update automatically on new commits. Configure here.