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

Add string manipulation functions to domain template #14903

Open
braunsonm opened this issue Feb 15, 2024 · 16 comments
Open

Add string manipulation functions to domain template #14903

braunsonm opened this issue Feb 15, 2024 · 16 comments
Labels
area/API API objects and controllers kind/feature Well-understood/specified features, ready for coding.

Comments

@braunsonm
Copy link

braunsonm commented Feb 15, 2024

/area API

Describe the feature

I have an environment where multiple working tenants are deployed in the same cluster and would like to format my domains like so:

myfunc-project.dev.example.com
myfunc-project.prod.example.com

Where myfunc is deployed in the namespace project-dev, project-prod, etc. Right now in order to do this automatically I would need to create a domain template with multiple if/else blocks matching the namespace to a specific domain.

{{if eq .Namespace "project-dev"}}
{{.Name}}-project.dev.example.com
{{else if eq .Namespace "project-prod"}}
{{.Name}}-project.prod.example.com
{{else if eq .Namespace "project2-dev"}}
{{.Name}}-project2.dev.example.com
{{else if eq .Namespace "project2-prod"}}
{{.Name}}-project2.prod.example.com
{{else}}
{{.Name}}.example.com
{{end}}

I know using annotations is possible but unfortunately func deploy does not allow setting labels or annotations, and it would be nice if this was handled for the user automatically by the platform operators.

I would like to be able to simply remove the -<environment> suffix from my namespace and use it in the domain. I'm proposing adding some string manipulation functions to the template. Specifically

funcs := map[string]any{
    "contains":  strings.Contains,
    "hasPrefix": strings.HasPrefix,
    "hasSuffix": strings.HasSuffix,
    "replace": strings.Replace}

Which would allow a template of

{{if hasSuffix .Namespace "-dev"}}
{{.Name}}-{{ replace .Namespace "-dev" "" 1}}.dev.example.com
{{end}}

Which is much more manageable.

I think it would be as simple as adding .Funcs(funcs) to this line: https://github.com/knative/serving/blob/main/pkg/reconciler/route/domains/domains.go#L115

If accepted, I wouldn't mind making a PR for this. Thanks!

@braunsonm braunsonm added the kind/feature Well-understood/specified features, ready for coding. label Feb 15, 2024
@knative-prow knative-prow bot added the area/API API objects and controllers label Feb 15, 2024
@dprotaso
Copy link
Member

dprotaso commented Mar 5, 2024

It seems like you want a different template per namespace.

Maybe extending the config to support that then trying to do this with templates would the easier approach. This would mirror already what we're doing with domain name selection.

@braunsonm
Copy link
Author

@dprotaso that would work too.

Not sure what you mean about the domain name selection though, as far as I know that only selects on labels not based on the namespace.

@dprotaso
Copy link
Member

dprotaso commented Mar 7, 2024

This would mirror already what we're doing with domain name selection.

You're right we do domain selection based on labels from the knative service resource. But now with the new config it's easy enough to extend it to have something like a namespaceSelector

@braunsonm
Copy link
Author

Up to you. I think adding namespace selectors to the domain template and domains configuration is a more elegant solution. Even more so if the configurations were consistent formats where you could match labels or namespaces. It would be more user friendly.

But string manipulation would certainly be easier to implement.

Copy link

github-actions bot commented Jun 6, 2024

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 6, 2024
@braunsonm
Copy link
Author

/remove-lifecycle stale

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 6, 2024
@dprotaso
Copy link
Member

dprotaso commented Jun 6, 2024

I'm going to close this out but feel free to create a new feature request for namespaceSelector in the domain selection

@dprotaso dprotaso closed this as completed Jun 6, 2024
@braunsonm
Copy link
Author

braunsonm commented Jun 6, 2024

I'm looking more at the label selectors in the domain configmap and I'm not sure it would be a good solution after all for the example I gave. The configuration would still have to be pretty huge and repetitive.

{{.Name}}-project1.dev.example.com: |
  namespaceSelector:
    name: project1-dev
{{.Name}}-project1.test.example.com: |
  namespaceSelector:
    name: project1-test
{{.Name}}-project2.dev.example.com: |
  namespaceSelector:
    name: project2-dev

Whereas some string manipulation functions would reduce this kind of repetitive configuration considerably. Is there a reason you don't think that is a good idea? @dprotaso or do you have a better suggestion on a way forward?

@dprotaso
Copy link
Member

dprotaso commented Jun 6, 2024

what's {{.Name}} in that example?

The config-domain is just the base domain selection.

project1.dev.example.com: |
  namespaceSelector:
    name: project1-dev
project1.test.example.com: |
  namespaceSelector:
    name: project1-test
project2.dev.example.com: |
  namespaceSelector:
    name: project2-dev

And then pair it with the domain template - https://github.com/knative/networking/blob/41aa2087242da0af0b1fee6721882fc0aed1b87e/config/config-network.yaml#L109

You could then make your domain template the following

"{{.Name}}-{{.Domain}}"

This would give you isolation since you've codified the namespace into the base domain

@braunsonm
Copy link
Author

braunsonm commented Jun 6, 2024

@dprotaso yea that would work but would be even more verbose than the current solution. So I don't see it as a real improvement.

Edit: It's also more difficult to handle the else case that I listed above unless you built a very long if statement with all the namespaces names in it to decide if the pattern should be {{.Name}}-{{.Domain}} or {{.Name}}.{{.Domain}}

@dprotaso
Copy link
Member

dprotaso commented Jun 6, 2024

difficult to handle the else case that I listed above

I believe the way it works is we always pick the longest selector - so you could add an empty selector and it would be your fallback which would act like your else

@braunsonm
Copy link
Author

braunsonm commented Jun 6, 2024

The else would need to be in the domain template not the domain configmap (note that dot instead of a dash). So I'm not sure if that would work.

So where are we at with this issue? Is this not going to be accepted to Knative? If so I don't think I intend on making a new feature request with your suggestion as I mentioned it is actually more verbose than the current workaround.

@dprotaso
Copy link
Member

dprotaso commented Jun 6, 2024

I'll re-open the issue and see if there's more input from the other maintainers

I'm leaning towards your idea of adding the funcs

cc @ReToCode @skonto

@dprotaso dprotaso reopened this Jun 6, 2024
@ReToCode
Copy link
Member

ReToCode commented Jun 7, 2024

Hm I'm a bit hesitent about adding more dynamics in general. This increases the (already quite high) complexity in various places while the value added is limited (in my personal opinion).
For example the domain pattern in #14903 (comment) also ends up adding new challenges to create certificates. There were good reasons for ingress solutions (OpenShift for example) to rely on default domains with just one subdomain (like *.apps.mycluster.com) with the ability to overwrite this for specific services.

Copy link

github-actions bot commented Sep 6, 2024

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 6, 2024
@braunsonm
Copy link
Author

braunsonm commented Sep 6, 2024

Remains an issue, not able to have a subdomain per namespace without pretty large if/else blocks. I'm open to other solutions if preferred.

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers kind/feature Well-understood/specified features, ready for coding.
Projects
None yet
Development

No branches or pull requests

3 participants