Synchrony Ingress Path Incorrect about data-center-helm-charts

[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.

[errcode1202]

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

[bordenit]

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

[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.

[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.