serving icon indicating copy to clipboard operation
serving copied to clipboard

feat(domainmapping): Add wildcard domain claim support

Open arsenetar opened this issue 1 year ago • 7 comments

Fixes #14688

Proposed Changes

Update domain claims used for domainmapping to support wildcard. This allows domain claims like *.example.com to be used to match domains like foo.example.com and bar.example.com. Additionally, this allows breaking out nested subdomains into separate claims assigned to different namespaces. For example, *.foo.example.com and *.example.com can both exist. The more specific one will be the one that owns the specified domain (in absence of a specific domainclaim as those always take priority). This allows for additional flexibility.

Release Note

Add support for wildcard DomainClaims

arsenetar avatar Feb 13 '24 04:02 arsenetar

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arsenetar Once this PR has been reviewed and has the lgtm label, please assign cali0707 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

knative-prow[bot] avatar Feb 13 '24 04:02 knative-prow[bot]

Hi @arsenetar. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow[bot] avatar Feb 13 '24 04:02 knative-prow[bot]

Still need to update to add additional tests.

arsenetar avatar Feb 13 '24 04:02 arsenetar

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (2125772) 85.77% compared to head (6e252a0) 85.69%.

Files Patch % Lines
pkg/reconciler/domainmapping/reconciler.go 42.10% 17 Missing and 5 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14881      +/-   ##
==========================================
- Coverage   85.77%   85.69%   -0.09%     
==========================================
  Files         198      198              
  Lines       15153    15187      +34     
==========================================
+ Hits        12998    13014      +16     
- Misses       1829     1844      +15     
- Partials      326      329       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 13 '24 04:02 codecov[bot]

Ah this will need some other way of either indicating wildcard or processing since *.example.com is an invalid object name. I forgot these use the name of the claim directly. Have options:

  • Add a item in the spec to indicate wildcard or could even just have an entry for subDomains with a special value * for wildcard. Subdomain list would allow specifying specific subdomains to match only if desired.
  • Allow a domain claim of example.com to match all subdomains (logic in this PR still allows other subdomains to be given to other claims if desired). This is slightly less clear although maybe not entirely unintuitive. This effectively makes every claim a "wildcard".

@braunsonm Since you were also interested in this not sure if one of these options would align better with how you wanted to use this.

arsenetar avatar Feb 13 '24 04:02 arsenetar

I'm fine with either, but with option 2 wouldn't that make it no longer possible to have a cluster domain claim that matches a single domain? If you make them all wildcardable that might be undesirable to others who are already using this feature.

Another option (if allowed) could be any domains prefixed with a . allow wildcard subdomains. So .mydomain.com allows all subdomains of mydomain.com whereas without the prefix is an exact match.

braunsonm avatar Feb 13 '24 13:02 braunsonm

@braunsonm .mydomain.com has the same issue as starting with * as the name needs to start with alphanumeric.

arsenetar avatar Feb 13 '24 21:02 arsenetar

Yeah the way this feature was implemented it uses name conflicts in the cluster scope to ensure there are no duplicates.

If we wanted to support wild cards then we'd have to modify the existing ClusterDomainClaim resource and introduce controllers to try to ensure the same guarantee.

I'm going to close this out - if this is something you're interested in pursuing then we'd probably want to write up a feature track document.

dprotaso avatar Mar 26 '24 21:03 dprotaso