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.

Sign up for GitHub

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

Jump to bottom

[common]: add ingress defaultBackend #106

Merged
merged 9 commits into from
Jul 27, 2023
Merged

[common]: add ingress defaultBackend #106

merged 9 commits into from
Jul 27, 2023

Conversation

TobyTheHutt
Copy link
Contributor

@TobyTheHutt TobyTheHutt commented Jul 26, 2023 •
edited
Loading

What this PR does:
This PR enables the possibility to define a default Backend for ingresses.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):
none

Notes for Reviewer:

Checklist:

  • Pull Request title in format [chart]: Changed Something
  • Updated documentation in the README.md.gotmpl file and executed helm-docs
  • Chart Version bumped
  • All commits are signed-off

@TobyTheHutt TobyTheHutt requested a review from a team as a code owner July 26, 2023 16:45
@TobyTheHutt TobyTheHutt requested a review from brjos July 26, 2023 16:45
@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 26, 2023
@TobyTheHutt TobyTheHutt force-pushed the feature/ingress_defaultBackend branch from 3069e13 to d52a7df Compare July 27, 2023 07:04
@TobyTheHutt
Added the ingress defaultBackend as an option
477897b 
Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt
Bump common Chart version
bfee6ce 
Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt
Updated common documentation
d78a693 
Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt
Regenerated documentation
338132f 
Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt
Corrected documentation generation
417fe3b 
Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt TobyTheHutt force-pushed the feature/ingress_defaultBackend branch from 7eb6031 to 417fe3b Compare July 27, 2023 07:11
@brjos
Copy link
Contributor

brjos commented Jul 27, 2023

Thanks for contributing @TobyTheHutt!

According to the ingress doc for DefaultBackends:
The defaultBackend is conventionally a configuration option of the Ingress controller and is not specified in your Ingress resources.

If you intend to create an ingress resource that is backed by a single Service, then you should assure, that no rules are set in the ingress resource, if I understand the doc correct:
You can also do this with an Ingress by specifying a default backend with no rules.

@TobyTheHutt
Copy link
Contributor Author

Hi @brjos
Thank you for reading the documentation to me! Seriously, I overlooked this part.
I agree with you that this modification would only be useful in very specific use cases, as shown in the examples of the page. How do we proceed with this PR?

@brjos
Copy link
Contributor

brjos commented Jul 27, 2023

I'd say: because you already invested time: why not just make sure that no rules can be created when $ingress.defaultBackend is defined; or alternatively: close the PR

@TobyTheHutt
Made defaultBackend of Ingress config exclusive
736ca49 
The defaultBackend config of the Ingress is now exclusive and cannot be
used together with Ingress rules.

Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt
Copy link
Contributor Author

@brjos I made the options exclusive, so that only one of rules or defaultBackend can be defined. Please check the PR again.

@TobyTheHutt
Modified ingress.ingress template
0458d51 
Added if/else conditions to either configure a defaultBackend or
backends with rules. TLS is not possible when defaultBackend is set.

Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt
Corrected ingress.ingress template
2463981 
conditional between `defaultBackend` and `rules` simplified.

Signed-off-by: Tobias Harnickell <[email protected]>
@TobyTheHutt TobyTheHutt force-pushed the feature/ingress_defaultBackend branch from 5823ca4 to 2463981 Compare July 27, 2023 12:07
brjos
brjos approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@brjos brjos left a comment

Choose a reason for hiding this comment

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

LGTM

@TobyTheHutt TobyTheHutt merged commit fc17db5 into master Jul 27, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brjos brjos brjos approved these changes

Assignees
No one assigned
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TobyTheHutt @brjos