-
Notifications
You must be signed in to change notification settings - Fork 828
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
prow-build-cluster: deploy AWS Load Balancer Controller #4874
Conversation
Signed-off-by: Marko Mudrinić <[email protected]>
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
We could tweak the values for the Helm chart.
# AWS Load Balancer Controller (ALB/NLB integration). | ||
resource "helm_release" "aws_lb_controller" { | ||
name = "aws-load-balancer-controller" | ||
namespace = "kube-system" |
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.
I usually put the ALB load balancer controller into a less privileged namespace.
name = "serviceAccount.annotations.eks\\.amazonaws\\.com/role-arn" | ||
value = module.aws_load_balancer_controller_irsa.iam_role_arn | ||
} | ||
|
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.
I'd also set:
resources{}
priorityClassName
image.repository
podDisruptionBudget.minAvailable
and explicitly manage via Terraform:
image.tag
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.
Do you have recommended values for:
resources{}
priorityClassName
podDisruptionBudget.minAvailable
(I guess 1 since we have 2 replicas)
and explicitly manage via Terraform:
image.tag
Do we really need to do so? Doesn't we get that implicitly since we bind the chart version? Or they can keep the same chart version but bump the app version?
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.
For resources, try:
resources:
limits:
memory: 256Mi
requests:
cpu: 200m
memory: 128Mi
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.
What priority classes do we use in the existing cluster (on GCP)? I'd use one of them if we have some, and skip if we don't.
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.
The image tag thing isn't essential. Setting it puts drift control even more in Terraform's hands.
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.
On most projects like this, I'd pin image tags.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ameukam, xmudrii The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
As stated in #4686, our EKS build cluster should have ability to use ALB. This PR ensures that by deploying and configuring AWS Load Balancer Controller.
This change is already reconciled and tested.
/assign @ameukam