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

Ingress Trait to support an array on the path #5981

Open
cfitzw opened this issue Dec 6, 2024 · 7 comments
Open

Ingress Trait to support an array on the path #5981

cfitzw opened this issue Dec 6, 2024 · 7 comments
Assignees
Labels
good first issue Does not require full understanding of the codebase kind/feature New feature or request
Milestone

Comments

@cfitzw
Copy link
Contributor

cfitzw commented Dec 6, 2024

Requirement

Would like to set multiple paths and/or aliases for an integration.

Problem

Currently unable to utilize the ingress trait on an integration to set multiple paths for the ingress to consume.

It appears this is possible/supported via kubernetes documentation:
https://kubernetes.io/docs/concepts/services-networking/ingress/#examples

Proposal

Allow and array on ingress.path ([]string):
https://camel.apache.org/camel-k/2.5.x/apis/camel-k.html#_camel_apache_org_v1_trait_IngressTrait

Open questions

No response

@cfitzw cfitzw added the kind/feature New feature or request label Dec 6, 2024
@squakez
Copy link
Contributor

squakez commented Dec 7, 2024

Thanks for the feature request. Are you planning to work on this by any chance?

@squakez squakez added the good first issue Does not require full understanding of the codebase label Dec 7, 2024
@cfitzw
Copy link
Contributor Author

cfitzw commented Dec 11, 2024

@squakez - I would very much like to try -- I'll take a walk through the developer's guide and see if I can work through how to implement the changes.

@cfitzw
Copy link
Contributor Author

cfitzw commented Dec 11, 2024

Good progress so far, documentation has been excellent. I should be able to get a PR submitted in the coming days.

@squakez squakez added this to the 2.6.0 milestone Dec 12, 2024
@squakez
Copy link
Contributor

squakez commented Dec 12, 2024

Good progress so far, documentation has been excellent. I should be able to get a PR submitted in the coming days.

Excellent. Just let us know if you have any doubt or you need any help!

@cfitzw
Copy link
Contributor Author

cfitzw commented Dec 13, 2024

TL;DR - I think this is bubbling up into a bigger issue.

@squakez I have a working build (Draft PR #5996) , allowing a string array on the ingress.path, however, I have some doubts...

  1. This would become a breaking change == any integration having an ingress.path=<string> set would break.
    Possibly some code could be implemented to auto-convert string into array... but maybe not worth the effort based on below.
  2. This isn't fully solving the problem... IMO, the IngressTrait should follow the APIs hierarchy, starting from here:
    https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#ingressspec-v1-networking-k8s-io
    This would allow any number of IngressRules (which has it's own sub-structure), meaning, you could specify multiple hosts, and for those hosts, multiple paths, which may also have their own associated pathType.

Best visual example of the hierarchy (from https://v1-30.docs.kubernetes.io/docs/concepts/services-networking/ingress/#hostname-wildcards):

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-wildcard-host
spec:
  rules:
  - host: "foo.bar.com"
    http:
      paths:
      - pathType: Prefix
        path: "/bar"
        backend:
          service:
            name: service1
            port:
              number: 80
  - host: "*.foo.com"
    http:
      paths:
      - pathType: Prefix
        path: "/foo"
        backend:
          service:
            name: service2
            port:
              number: 80

Please let me know your thoughts.

@squakez
Copy link
Contributor

squakez commented Dec 14, 2024

First of all thanks for the effort to put on a draft @cfitzw, really appreciated. About the specific problems you're mentioning:

  1. Indeed, we cannot transform the usage of path as it introduce a compatibility problem. What we can do, however, is to introduce a new one, ie, paths that will hold the new logic. Eventually we can deprecate path in favor of paths if we see it makes sense.
  2. If the goal of the feature is to support multiple paths and hosts, then, we may rework entirely the feature and allow the user to introduce a given syntax, eg, -t ingress.rules=foo.bar.com/bar -t ingress.rules=*.foo.com/foo which would translate into the Ingress structure you've highlighted.

I am not entirely sure what exactly you need to support, but I think we can separate this issue to work both on 1 and, if that makes sense, on 2, providing a different parameter for each feature we want.

@cfitzw
Copy link
Contributor Author

cfitzw commented Dec 16, 2024

@squakez -- thank you for the feedback.

I've implemented number 1 and committed to the PR.

In regards to number 2, this is not a desired need at-the-moment, so let's focus on adding support for number 1 and this can become a new issue/PR at a later point-in-time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Does not require full understanding of the codebase kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants