Support a subset of annotations on the service resources#269
Support a subset of annotations on the service resources#269
Conversation
| # Ingress Resources | ||
| # Resources | ||
|
|
||
| This document covers how ingress resources work in relation to The ALB Ingress Controller. |
| return a, nil | ||
| } | ||
|
|
||
| // ParseServiceAnnotations validates and loads all the annotations provided into the Annotations struct. |
There was a problem hiding this comment.
This comment should mention it parses all annotations relevant to the service resource.
Also the annotations.ParseAnnotations should likely be refactored to annotations.ParseIngressAnnotations for consistency.
[or] you could combine these 2 functions as they're quite similar. Just needs to be smarter on which annotations to parse based on what's passed. I'm fine either way, as long as the intent of annotations.ParseServiceAnnotations vs annotations.ParseAnnotations is clear.
|
Minor changes requested. Spinning up a test cluster now to do some verification. |
|
The only issue i've hit when testing the controller doesn't fallback on the ingress resource as they should. For example, I've set my But in AWS, it's still using the traffic port as seen below.
If I add this setting to the service, it will be respected.
But after removal from the service resource, even though that setting is still in the ingress resource, it goes back to the default (traffic port).
Perhaps this is a small issue inside of the new |
|
Gotta be. That’s what I get for not writing a test function.
…On November 17, 2017 at 4:59:15 PM, joshrosso ***@***.***) wrote:
The only issue i've hit when testing the controller *doesn't* fallback on
the ingress resource as they should.
For example, I've set my healthcheck-port to 8880. In the ingress
resource.
$ kubectl get ing -n echoserver echoserver -o yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
annotations:
alb.ingress.kubernetes.io/healthcheck-interval-seconds: "30"
alb.ingress.kubernetes.io/healthcheck-port: "8880"
But in AWS, it's still using the traffic port as seen below.
[image: image]
<https://user-images.githubusercontent.com/6200057/32975028-02e716d8-cbb8-11e7-8187-04c65072d6b1.png>
If I add this setting to the service, it will be respected.
apiVersion: v1
kind: Service
metadata:
annotations:
alb.ingress.kubernetes.io/healthcheck-port: '8880'
[image: image]
<https://user-images.githubusercontent.com/6200057/32975040-34ad91ba-cbb8-11e7-8f9d-23630f37e2f5.png>
But after removal from the service resource, even though that setting is
still in the ingress resource, it goes back to the default (traffic port).
$ kubectl get -o yaml -n echoserver svc echoserver
apiVersion: v1
kind: Service
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"v1","kind":"Service","metadata":{"annotations":{},"name":"echoserver","namespace":"echoserver"},"spec":{"ports":[{"port":80,"protocol":"TCP","targetPort":8080}],"selector":{"app":"echoserver"},"type":"NodePort"}}
creationTimestamp: 2017-11-18T00:33:17Z
name: echoserver
namespace: echoserver
resourceVersion: "6811"
selfLink: /api/v1/namespaces/echoserver/services/echoserver
uid: 12541ce2-cbf8-11e7-8ebd-0aeebdf18292
spec:
[image: image]
<https://user-images.githubusercontent.com/6200057/32975064-6a96580c-cbb8-11e7-8162-ba1e981e3afd.png>
Perhaps this is a small issue inside of the new annotations.Join?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#269 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAfB8xYYsGwvsg76D8d8x1Xqb9Ce0gQYks5s3ivigaJpZM4QirOm>
.
|
|
Why are we not simply specifying the custom healthcheck per targetGroup inside the ingress file itself ? Why the need to use an annotation from the service definition? Wont this be so much cleaner and intuitive like this: |
|
@rahulsen agreed, but its not supported in the ingress spec, i know they are looking at extending it |
4 participants
This should allow users to specify custom healthchecks per service and resolve #93.