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 support for wildcard hostname in VirutalServer #2939

Merged
merged 41 commits into from
Aug 29, 2022
Merged

Conversation

shaun-nx
Copy link
Contributor

@shaun-nx shaun-nx commented Aug 18, 2022

Proposed changes

This change updates the VirutalServer CRD to add support for a wildcard hostname.
This change also makes updates to ExternalDNS to allow a DNSEndpoint type to be created when specifying a wildcard hostname in a VirtualServer object.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added the enhancement Pull requests for new features/feature enhancements label Aug 18, 2022
@shaun-nx shaun-nx changed the title Feat/wildcardhostname Add support for wildcard hostname in VirutalServer Aug 18, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #2939 (3ae9c3b) into main (a8c2590) will increase coverage by 0.03%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #2939      +/-   ##
==========================================
+ Coverage   52.28%   52.32%   +0.03%     
==========================================
  Files          58       58              
  Lines       15965    15970       +5     
==========================================
+ Hits         8348     8357       +9     
+ Misses       7337     7335       -2     
+ Partials      280      278       -2     
Impacted Files Coverage Δ
internal/externaldns/sync.go 36.95% <50.00%> (ø)
pkg/apis/configuration/validation/virtualserver.go 94.38% <100.00%> (+0.02%) ⬆️
internal/k8s/configuration.go 95.76% <0.00%> (+0.36%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaun-nx shaun-nx marked this pull request as ready for review August 23, 2022 15:41
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Aug 24, 2022
func validateHost(host string, fieldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if host == "" {
return append(allErrs, field.Required(fieldPath, ""))
}

for _, msg := range validation.IsDNS1123Subdomain(host) {
allErrs = append(allErrs, field.Invalid(fieldPath, host, msg))
if !strings.HasPrefix(host, wildcardHost) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also validate the wildcard host - we can use validation.IsWildcardDNS1123Subdomain

see here for reference

@shaun-nx shaun-nx requested a review from ciarams87 August 24, 2022 14:05
Copy link
Contributor

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

🚀

service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "tcp"
service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: "*"
service.beta.kubernetes.io/aws-load-balancer-type: "nlb"
service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: "ip"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor one but these 2 annotations can be appended to old list instead of replacing them

@@ -51,7 +51,7 @@ spec:
{{% table %}}
|Field | Description | Type | Required |
| ---| ---| ---| --- |
|``host`` | The host (domain name) of the server. Must be a valid subdomain as defined in RFC 1123, such as ``my-app`` or ``hello.example.com``. Wildcard domains like ``*.example.com`` are not allowed. The ``host`` value needs to be unique among all Ingress and VirtualServer resources. See also [Handling Host and Listener Collisions](/nginx-ingress-controller/configuration/handling-host-and-listener-collisions). | ``string`` | Yes |
|``host`` | The host (domain name) of the server. Must be a valid subdomain as defined in RFC 1123, such as ``my-app`` or ``hello.example.com``. When using a wildcard domain like ``*.example.com`` the domain must be contained in double quotes. The ``host`` value needs to be unique among all Ingress and VirtualServer resources. See also [Handling Host and Listener Collisions](/nginx-ingress-controller/configuration/handling-host-and-listener-collisions). | ``string`` | Yes |
Copy link
Contributor

Choose a reason for hiding this comment

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

The ``host`` value needs to be unique: do we allow two VS resources with same wildcard host? eg. both with *.something.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll check if that's possible or not.

Copy link
Contributor Author

@shaun-nx shaun-nx Aug 29, 2022

Choose a reason for hiding this comment

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

We don't allow two VS resources to be deployed with the same wildcard hostname. Output below is what I got when I tried this:

Spec:
  Host:  *.example.com
  Routes:
    Action:
      Pass:  tea
    Path:    /tea
    Action:
      Pass:  coffee
    Path:    /coffee
  Tls:
    Secret:  cafe-secret
  Upstreams:
    Name:     tea
    Port:     80
    Service:  tea-svc
    Name:     coffee
    Port:     80
    Service:  coffee-svc
Status:
  Message:  Host is taken by another resource
  Reason:   Rejected
  State:    Warning
Events:
  Type     Reason    Age   From                      Message
  ----     ------    ----  ----                      -------
  Warning  Rejected  13s   nginx-ingress-controller  Host is taken by another resource

Copy link
Contributor

@vepatel vepatel left a comment

Choose a reason for hiding this comment

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

approved but left couple comments!

@shaun-nx shaun-nx merged commit e96b68e into main Aug 29, 2022
@shaun-nx shaun-nx deleted the feat/wildcardhostname branch August 29, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants