feat: Add ALB Ingress controller support #444
Conversation
dthomson25
changed the title
Codecov Report
@@ Coverage Diff @@
## master #444 +/- ##
==========================================
+ Coverage 84.76% 85.08% +0.31%
==========================================
Files 77 80 +3
Lines 7241 7448 +207
==========================================
+ Hits 6138 6337 +199
- Misses 798 803 +5
- Partials 305 308 +3
Continue to review full report at Codecov.
|
dthomson25
force-pushed
the
alb-ingress-controller
branch
from
25e07d5 to
8558098
Compare
dthomson25
force-pushed
the
alb-ingress-controller
branch
from
281b265 to
fcb1f77
Compare
ingress/ingress.go
Outdated
| switch ingress.Annotations["kubernetes.io/ingress.class"] { | ||
| case "aws-alb": | ||
| return c.syncALBIngress(ingress, rollouts) | ||
| case "nginx": |
There was a problem hiding this comment.
Does this mean that all Ingress configs must have the kubernetes.io/ingress.class annotation to work with traffic shaped rollouts? How would this work for clusters with multiple nginx ingresses with different names or don't use the default name?
In our large shared cluster we actually have multiple nginx ingress for different envs and service requirements, with this restriction all those would be excluded from using this feature.
There was a problem hiding this comment.
Hey, @cronik that's a great point! I'm going to add some command-line flags that allow you to specify which values of kubernetes.io/ingress.class are used for nginx and albs
There was a problem hiding this comment.
I also just saw that users can change their annotation prefix (i.e. alb.ingress.kubernetes.io) if they want so I'll add that functionality too
There was a problem hiding this comment.
Okay, I added the --alb-ingress-classes and --nginx-ingress-classes flags to address these usecases.
There was a problem hiding this comment.
would it also make sense to add an option like --default-ingress-class for cases where it is not annotated explicitly?
There was a problem hiding this comment.
I.E if there's no "kubernetes.io/ingress.class" annotation, have the flag dicate that the controller should consider it an NGINX or ALB ingress? Do the ingress controller support that behavior?
There was a problem hiding this comment.
Yeah, I don't know about ALB, but nginx-ingress has this behavior by default unless the ingress class name is explicitly set on the ingress controller.
https://kubernetes.github.io/ingress-nginx/user-guide/multiple-ingress/#multiple-ingress-nginx-controllers
https://kubernetes.github.io/ingress-nginx/user-guide/cli-arguments/
| arg | Description |
|---|---|
--ingress-class string |
Name of the ingress class this controller satisfies. The class of an Ingress object is set using the annotation "kubernetes.io/ingress.class". All ingress classes are satisfied if this parameter is left empty. |
Basically when --ingress-class is not set it acts as the default controller for all ingress.
Our cluster has multiple nginx ingress controllers, most ingress don't specify the ingress class annotation and get picked up by the default controller, when an ingress needs a specific class then the annotation is added to restrict to a specific one.
There was a problem hiding this comment.
I tested Jesse's solution made in another comment successful (--nginx-ingress-classes nginx --nginx-ingress-classes ''). Since there is no good way to pick a default value, I think it is better to leave it unset, and allow users to specify it themselves if needed.
cmd/rollouts-controller/main.go
Outdated
| command.Flags().StringArrayVar(&albIngressClasses, "alb-ingress-classes", defaultALBIngressClass, "Set the default Istio apiVersion that controller should look when manipulating VirtualServices.") | ||
| command.Flags().StringArrayVar(&nginxIngressClasses, "nginx-ingress-classes", defaultNGINXIngressClass, "Set the default Istio apiVersion that controller should look when manipulating VirtualServices.") |
There was a problem hiding this comment.
Copy and paste error. Still has the istio message.
Be sure to mention what we default to if unspecified.
There was a problem hiding this comment.
For @cronik's request regarding --default-ingress-class, I agree we need to solve that problem.
However, my question is: will this now solved if someone supplies the empty string? e.g.:
--nginx-ingress-classes nginx --nginx-ingress-classes ''
|
|
||
| Since the ALB Ingress controller allows users to configure the annotation prefix used by the Ingress controller, Rollouts can specify the optional `annotationPrefix` field. The Ingress uses that prefix instead of the default `alb.ingress.kubernetes.io` if the field set. | ||
|
|
||
| The Rollout adds another annotation called `rollouts.argoproj.io/managed-alb-actions` to the Ingress to help the controller manage the Ingresses. This annotation indicates which actions are being managed by Rollout objects (since multiple Rollouts can reference one Ingress). If a Rollout is deleted, the Argo Rollouts controller uses this annotation to see that this action is no longer managed, and it is reset to only the stable service with 100 weight. |
There was a problem hiding this comment.
If a Rollout is deleted, the Argo Rollouts controller uses this annotation to see that this action is no longer managed, and it is reset to only the stable service with 100 weight.
I wanted to call out that this behavior is such that we would be leaving around our annotation when the original ingress object did not have it in the beginning.
I think this is the correct behavior, since it is safer than deleting the annotation, which could cause downtime, but just wanted to point out that we could be leaving around leftover cruft.
Closes #421
Check out the alb.md for more information on the change.