koli / kong-ingress

[DEPRECATED] A Kubernetes Ingress for Kong
Other
93 stars 14 forks source link

Configure Strip URI parameter for a set of API Objects via Ingress Annotations #24

Closed paniagua closed 6 years ago

paniagua commented 6 years ago

The strip_uri property determines whether or not to strip a matching prefix from the upstream URI to be requested. The default value is true but it might not be the desire behavior in all cases.

This PR enables an Ingress Resource to be annotated as

...
annotations:
  ingress.kubernetes.io/strip-uri: "[false | true]"

The controller then reads this annotation and uses its value while creating the different API objects via Kong Admin.

sandromello commented 6 years ago

Thank you for your contribution, by the end of this week I will review your PR.

sandromello commented 6 years ago

The ingress.kubernetes.io annotation is used in oficial Kubernetes ingresses (gce and nginx), using the same name could cause conflicts in the future when launching or switching between ingresses.

I think a better approach could be handled adding a namespaced name more related to kong and also indicating the api path, in this case: kong.apis/strip-uri, what do you think about that?

Related reference: https://github.com/kubernetes/ingress-nginx/blob/master/docs/annotations.md

indrekj commented 6 years ago

The related reference says The intention is to ensure the maximum amount of compatibility between different implementations.. Doesn't this mean it should use the same keys as other ingress controller when possible to make switching between ingress controllers easier?

@sandromello what do you think about making strip-uri default value to false instead? I don't think any other ingress controllers currently strip the uri by default.

paniagua commented 6 years ago

I agree with @indrekj - by using the same keys as other ingress controllers it would make it easier for existing systems to move to kong. Specially, while adding more keys to configure the Apis.

In the current state this PR sets strip-uri to false unless it is explicitly set to true via annotations, following @indrekj suggestion to set up the default value to false.

@sandromello please let me know your thoughts.

sandromello commented 6 years ago

Indeed you are right, we could reuse kubernetes annotations, there're ingress implementations that follows this pattern also.

Regarding the strip-uri, I understand this is not a common default configuration and specifying an annotation for each ingress resource for not stripping the URI may be undesirable. My concerns are with changing the default behaviour and breaking things when updating the ingress controller and also sticking with the same defaults as Kong does.

I think this matter could be handled in another issue where we could provide a global configuration which defines how to create routes. Please take a look at issue #27

indrekj commented 6 years ago

27 seems also a good approach

sandromello commented 6 years ago

We could proceed with the merge after changing the strip-uri default to true. Let's keep the kong defaults for now and later we could work on #27.

paniagua commented 6 years ago

Updated the PR so if the annotation cannot be parsed then the default value from kong (true) is preserved.

sandromello commented 6 years ago

Thanks for the contribution! It's important to mention that the new version has a breaking change and it will only work in version 1.7 of Kubernetes.