Skip to content
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

feat(services) add Ingress for cluster sync #583

Merged
merged 3 commits into from
Apr 14, 2022

Conversation

FabianHardt
Copy link
Contributor

@FabianHardt FabianHardt commented Apr 14, 2022

What this PR does / why we need it:

This is an alternative pull request to #573 . This one uses the default Ingress feature of K8s to create OpenShift routes. I just need to add Ingress option for cluster communication as well, that's why I edited the matching service templates. I need this for automatic creation of OpenShift routes as I need them for cluster to cluster communication between CP and DP.
I also had to remove an unnecessary if statement, because ingress.path already has a default value in values.yml. I need the option to be able to leave the ingress.path blank.

Special notes for your reviewer:

As I said, this is an alternative to #573 and I would be happy if we could find a common solution here.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@FabianHardt FabianHardt marked this pull request as ready for review April 14, 2022 18:46
@FabianHardt FabianHardt requested a review from a team as a code owner April 14, 2022 18:46
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does including an empty path do in your case?

It's unclear why we had the if clause given that all of our configuration sets the field by default in values.yaml and v1 Ingresses without it were invalid. However, they were invalid because pathType was absent, not because path was. Not including path is valid, but a bit unclear since you can't really issue an HTTP request without one.

Best guess is that the original template was somehow relevant to extensions/v1beta1 Ingress if anything, but can't really test that anymore for lack of a readily available test instance that supports it. That's old and not of clear use there either, so not too worried about it.

@rainest rainest merged commit 5a68481 into Kong:main Apr 14, 2022
@FabianHardt
Copy link
Contributor Author

Thank's for the quick merge!
With this possibility to leave the path empty, I'm able to get auto generated OpenShift routes in passthrough mode as described here. I tired it, but if path attribute is not empty, OpenShift doesn't generate a passthrough route automatically.
Combined with new Ingress resources for cluster and clustertelemetry I'm able to deploy my existing OpenShift hybrid mode setup completely with this chart. Everything works fine now, thanks!
If you're interested in an OpenShift example values.yml please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants