-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add possibility to set a protocol #3648
Conversation
3ffc407
to
5005fc1
Compare
@@ -159,7 +159,8 @@ The following general annotations are applicable on the Ingress object: | |||
| `traefik.ingress.kubernetes.io/whitelist-source-range: "1.2.3.0/24, fe80::/16"` | A comma-separated list of IP ranges permitted for access (6). | | |||
| `ingress.kubernetes.io/whitelist-x-forwarded-for: "true"` | Use `X-Forwarded-For` header as valid source of IP for the white list. | | |||
| `traefik.ingress.kubernetes.io/app-root: "/index.html"` | Redirects all requests for `/` to the defined path. (4) | | |||
| `traefik.ingress.kubernetes.io/service-weights: <YML>` | Set ingress backend weights specified as percentage or decimal numbers in YAML. (5) | | |||
| `traefik.ingress.kubernetes.io/service-weights: <YML>` | Set ingress backend weights specified as percentage or decimal numbers in YAML. (5) | |||
| `ingress.kubernetes.io/protocol: <NAME>` | Set the protocol Traefik will use to communicate with pods |
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.
Needs a .
after pods
@@ -256,7 +257,7 @@ The following annotations are applicable on the Service object associated with a | |||
| `traefik.ingress.kubernetes.io/load-balancer-method: drr` | Override the default `wrr` load balancer algorithm. | | |||
| `traefik.ingress.kubernetes.io/max-conn-amount: 10` | Set a maximum number of connections to the backend.<br>Must be used in conjunction with the below label to take effect. | | |||
| `traefik.ingress.kubernetes.io/max-conn-extractor-func: client.ip` | Set the function to be used against the request to determine what to limit maximum connections to the backend by.<br>Must be used in conjunction with the above label to take effect. | | |||
| `traefik.ingress.kubernetes.io/session-cookie-name: <NAME>` | Manually set the cookie name for sticky sessions. | | |||
| `traefik.ingress.kubernetes.io/session-cookie-name: <NAME>` | Manually set the cookie name for sticky sessions. | |
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.
Does this need to be here?
Morning @dtomcej, requested changes applied :) |
@@ -159,7 +159,8 @@ The following general annotations are applicable on the Ingress object: | |||
| `traefik.ingress.kubernetes.io/whitelist-source-range: "1.2.3.0/24, fe80::/16"` | A comma-separated list of IP ranges permitted for access (6). | | |||
| `ingress.kubernetes.io/whitelist-x-forwarded-for: "true"` | Use `X-Forwarded-For` header as valid source of IP for the white list. | | |||
| `traefik.ingress.kubernetes.io/app-root: "/index.html"` | Redirects all requests for `/` to the defined path. (4) | | |||
| `traefik.ingress.kubernetes.io/service-weights: <YML>` | Set ingress backend weights specified as percentage or decimal numbers in YAML. (5) | | |||
| `traefik.ingress.kubernetes.io/service-weights: <YML>` | Set ingress backend weights specified as percentage or decimal numbers in YAML. (5) | |||
| `ingress.kubernetes.io/protocol: <NAME>` | Set the protocol Traefik will use to communicate with pods. |
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 thought ingress model is only for http/https protocol. this annotation allows users to set their own arbitrary protocol. some potential danger for operation engineers?🧐 Or some hacky users might abuse it?
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.
@yue9944882 The issue here is that h2c runs over http, but uses its own protocol. All of the other providers have the traefik.protocol
label, but the k8s provider does not. This allows users to specify h2c as a backend protocol so h2c will work as is does with other providers. Although your point about potential abuse is valid.
@SantoDE what do you think about instead of blindly assigning the value to protocol
, running it through a select/switch with acceptable protocols? http
/https
/h2c
so that someone can't add http://myinjectionsite.com/endpoint.php http
as the protocol ;)
Hey @yue9944882 , @dtomcej thanks for your concerns. I added a switch to only accept wanted protocols. :-) |
932ea0c
to
ed373cd
Compare
provider/kubernetes/kubernetes.go
Outdated
@@ -1029,3 +1040,8 @@ func getRateLimit(i *extensionsv1beta1.Ingress) *types.RateLimit { | |||
|
|||
return rateLimit | |||
} | |||
|
|||
func getProtocol(i *extensionsv1beta1.Ingress) string { |
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.
This function seems unused. I suppose we can just remove it?
provider/kubernetes/kubernetes.go
Outdated
protocol = allowedProtocolHTTPS | ||
case allowedProtocolH2C: | ||
protocol = allowedProtocolH2C | ||
default: |
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'm not too much of a fan of falling back to some default value for the case where the user entered invalid data. How about indicating an error and skipping the offending Ingress if the given annotation value is unsupported?
That's also how we seem to handle things in most other cases of the Kubernetes provider.
provider/kubernetes/kubernetes.go
Outdated
case allowedProtocolH2C: | ||
protocol = allowedProtocolH2C | ||
default: | ||
protocol = label.DefaultProtocol |
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.
Assuming we hit line 314 and have protocol be https
: Aren't we overwriting the value to http
again if the annotation is missing or empty (as the default of http
will be returned to getStringValue
, thereby causing our default switch case to apply)?
iHost("protocol"), | ||
iPaths(onePath(iPath("/valid"), iBackend("service1", intstr.FromInt(80))))), | ||
), | ||
), |
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.
How about adding the test case where we don't use port 443 (i.e., protocol is set to http
) but explicitly specify the https
protocol through the annotation?
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 added some more tests:)
Hey @dtomcej and @timoreimann , I addressed your feedback :) |
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
provider/kubernetes/kubernetes.go
Outdated
case label.DefaultProtocol: | ||
protocol = label.DefaultProtocol | ||
default: | ||
log.Errorf("Invalid protocol %s specified for Ingress %s - skipping", getStringValue(i.Annotations, annotationKubernetesProtocol, protocol), i.Name) |
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 think it's clearer (and, when in doubt, more correct because it's independent of the calling function's implementation) to use annotationKubernetesProtocol
instead of getStringValue(i.Annotations, annotationKubernetesProtocol, protocol)
.
provider/kubernetes/kubernetes.go
Outdated
case label.DefaultProtocol: | ||
protocol = label.DefaultProtocol | ||
default: | ||
log.Errorf("Invalid protocol %s specified for Ingress %s - skipping", getStringValue(i.Annotations, annotationKubernetesProtocol, protocol), i.Name) |
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.
Please include the namespace when describing the Ingress like we do it here.
provider/kubernetes/kubernetes.go
Outdated
case allowedProtocolH2C: | ||
protocol = allowedProtocolH2C | ||
case label.DefaultProtocol: | ||
protocol = label.DefaultProtocol |
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 switch statement can be simplified to:
protocol = getStringValue(i.Annotations, annotationKubernetesProtocol, protocol)
switch protocol {
case allowedProtocolHTTPS:
case allowedProtocolH2C:
case label.DefaultProtocol:
default:
log.Errorf(...)
}
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, thanks. 👍
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 👏
Thanks @SantoDE
58ed6c4
to
8548fa0
Compare
8548fa0
to
4c5ac16
Compare
What does this PR do?
Adds posibility to set a custom protocol for an ingress with the K8s provider
Motivation
Fixes #3641
More
Additional Notes
I'm official dumb and screwed the other pr :)