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

Generalize Ingress creation, use wildcard cert with OCP apps domain #305

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Generalize Ingress creation, use wildcard cert with OCP apps domain #305

wants to merge 15 commits into from

Conversation

ThisIsntTheWay
Copy link
Contributor

@ThisIsntTheWay ThisIsntTheWay commented Feb 5, 2025 •
edited
Loading

Summary

  • Generalize ingress creation logic
  • Create separate ingresses that bundle FQDNs in the following way:
    • FQDN is part of an OCP clusters default apps domain and only has one subdomain (sub1.apps...)
    • FQDN is either not part of said domain or has 2 or more subdomains (sub2.sub1.apps...)
      • Bundled ingresses will have "letsencrypt" or "wildcard" added to their name according to their TLS config
    • Services that only have one FQDN will only create a single ingress with no extra name according to those rules
  • Use wildcard certs for the ingress bundling eligible FQDNs (empty TLS config)

Given an apps domain example.com:

❯ k get ing -o custom-columns="NAME:.metadata.name,HOSTS:.spec.rules[*].host"
NAME                               HOSTS
nextcloud-collabora-code-ingress   k-collabora.example.com
nextcloud-letsencrypt-ingress      next.nextcloud.example.com,past.nextcloud.example.com,presentcloud.my-domain.ch,nextcloud.my-domain.ch,future.tensecloud.my-domain.ch
nextcloud-wildcard-ingress         nextcloud.example.com,2-nextcloud.example.com

❯ k get ing nextcloud-letsencrypt-ingress  -o yaml | yq .spec.tls
- hosts:
    - next.nextcloud.example.com
    - past.nextcloud.example.com
    - presentcloud.my-domain.ch
    - nextcloud.my-domain.ch
    - future.tensecloud.my-domain.ch
  secretName: nextcloud-ingress-cert

❯ k get ing nextcloud-wildcard-ingress -o yaml | yq .spec.tls
- {}

This affects the following services:

  • Nextcloud
  • Keycloak
  • Collabora
  • Forgejo

Related PR in component-appcat: vshn/component-appcat#623

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.

@ThisIsntTheWay ThisIsntTheWay added the enhancement New feature or request label Feb 5, 2025
@ThisIsntTheWay ThisIsntTheWay self-assigned this Feb 5, 2025
@vshnbot vshnbot mentioned this pull request Feb 5, 2025
Open
2 tasks
@ThisIsntTheWay ThisIsntTheWay requested review from a team, Kidswiss, TheBigLee and wejdross and removed request for a team February 10, 2025 15:15
@ThisIsntTheWay ThisIsntTheWay marked this pull request as draft February 12, 2025 14:11
@ThisIsntTheWay ThisIsntTheWay force-pushed the change/ingress-tls branch 3 times, most recently from a58dfd8 to 80c9829 Compare February 26, 2025 10:39
@ThisIsntTheWay
Generalize ingress creation
f6fb4eb 
Previously, ingresses were (mostly) created using helm values.
With this change, ingresses are generated in the common package.
Meaning: Ingresses are now explicitly defined and no longer templated.

This also allows to properly handle OCP default apps wildcard certs
without the need of copy/pasting the same code for each service.
@ThisIsntTheWay ThisIsntTheWay marked this pull request as ready for review February 27, 2025 09:35
@ThisIsntTheWay ThisIsntTheWay requested a review from zugao February 27, 2025 09:36
@ThisIsntTheWay ThisIsntTheWay changed the title Add logic to identify apps domain Generalize Ingress creation, use wildcard cert with OCP apps domain Feb 27, 2025
zugao
zugao requested changes Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@zugao zugao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR will touch existing services. Make sure you check the crossplane-diff of this change.

pkg/comp-functions/functions/common/ingress.go
// Generate an Ingress containing a single FQDN using a TLS config as such:
// FQDN is one subdomain ON defaultAppsDomain (e.g. sub1.apps.cluster.com) -> Empty TLS config (uses wildcard cert on OCP).
// FQDN does not statisfy the former -> TLS config using a Let's Encrypt certificate.
func GenerateIngress(comp InfoGetter, svc *runtime.ServiceRuntime, ingressConfig IngressConfig) (*netv1.Ingress, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would definitely add some unit tests for this function, also for others as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added such in 4b1aa1e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zugao zugao zugao requested changes

@Kidswiss Kidswiss Awaiting requested review from Kidswiss Kidswiss was automatically assigned from vshn/schedar

@TheBigLee TheBigLee Awaiting requested review from TheBigLee TheBigLee was automatically assigned from vshn/schedar

@wejdross wejdross Awaiting requested review from wejdross wejdross was automatically assigned from vshn/schedar

Requested changes must be addressed to merge this pull request.

Assignees

@ThisIsntTheWay ThisIsntTheWay

Labels
enhancement New feature or request minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ThisIsntTheWay @zugao