Synchrony Ingress Path Incorrect

[bordenit]

The ingress path for synchrony is incorrect.

- path: "{{ trimSuffix "/" .Values.ingress.path }}/synchrony"

This should not rely on the .Values.ingress.path as it then nests the synchrony path inside the Confluence contextPath and/or puts synchrony at the same base path, which usually users have to authenticate to with SSO. I fixed it manually by just changing this path to /synchrony in the ingress, as I already have a context path for Confluence.

The path in the ingress should be changed to /synchrony if Confluence is using a contextPath already. But, ideally the ingress for Synchrony should be broken down in the same way the ingress is for Confluence where a host and a path are both parameters. There will also be a different requirement for ingress depending on whether the user uses or does not use a contextPath with Confluence.

User Case 1 - User uses a contextPath with Confluence

• The same ingress can be used for both Confluence and Synchrony by putting both Confluence and Synchrony at different contextPaths. Confluence would route to {{ .Values.ingress.host }}/confluence and and Synchrony would route to {{ .Values.ingress.host }}/synchrony for example.

Use Case 2 - User does not use a contextPath with Confluence

• The same ingress can't be used for both Confluence and Synchrony as a different ingress will be used for a second host used by Synchronys ingress to avoid SSO collisions with the baseURL. Synchrony would route to {{ .Values.sychrony.ingress.host }} and Confluence would route to {{ .Values.ingress.host }} where they don't have any chance of colliding.

This would create a conditional requirement to create a second ingress specifically for synchrony if a contextPath is not used as well.

I can work on a PR if you think creating a conditional for second ingress and breaking down synchrony into host and path options is feasible.

[bordenit]

On second thought, it will probably always make sense just to make two ingresses.... One for confluence and one for synchrony.

[badgersow]

I agree that we shouldn't force people to have Confluence and Synchrony on the same host or with the same context path. I think it's OK to always create a separate ingress for Synchrony - I don't like the idea of having a conditional ingress, as it adds complexity. We can introduce new values Values.synchrony.ingress.host and Values.synchrony.ingress.path. They can by default resolve to what they are now (confluence.host and confluence.path/synchrony), but the user can override them if they want. This gives the user more flexibility, but at the same time we have sensible default values and we don't force additional configuration on the user. Finally, I see no problem of having multiple ingresses, because Confluence does it now anyway and it works well: there is a separate ingress for /setup endpoints because they need bigger timeout.

[bordenit]

Yes, @badgersow makes perfect sense. I use a kubectl patch with our current deployment to fix the ingress, but looking forward to eliminating the need for that patch.

[bordenit]

Just checking back to see if you would like me to submit a PR for this. When using context path the bug fix is:

''' kubectl patch ingress -n jira-datacenter jira-datacenter --type=json -p '[{"op": "replace", "path": "/spec/rules/0/http/paths/1/path", "value":"/synchrony"}]' '''

[badgersow]

@nanux Do we want to accept the contribution here or this ticket is on our roadmap already?

[nghazali]

@bordenit , we would be happy to accept the contribution.

[errcode1202]

Hi @bordenit just wondering if you've had time to look into submitting a PR for this yet?

[bordenit]

No I have not had time. If someone else wants to pick it up feel free. The fix is really just to make sure the synchrony path is correct if context path is used. It should not append the synchrony path to the end of the context path, but be its own path entirely.

[errcode1202]

All good @bordenit, we'll look into getting this tee'd up on our end.

[bordenit]

@errcode1202 I through something together, but will wait to create PR until my other PR is merged. I just removed the syncrhony ingress from values.yaml, and updated the other files to use /synchrony context path appended to the ingress.host variable. I think that kind of makes sense, since less than .01% of people are probably going to have an external synchrony service. Maybe it can be just an automatic thing that is set in the background that user never needs to configure.