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

feat: Add ability to configure multiple wilcard domains #14543

Merged
merged 6 commits into from
Feb 7, 2024

Conversation

arsenetar
Copy link
Contributor

Fixes #14364

Proposed Changes

  • Update nscert reconciler to be able to create multiple wildcard certificates for any domain that is a wildcard
  • Update domain config to allow marking domains as having wildcard enable. The format of the change an implementation allows for prior usage without wildcard field to continue to function as it was.
  • Update references to config object as necessary to use new object format

Release Note

Add support for multiple wildcard certificate domains in domain config

@knative-prow knative-prow bot added area/API API objects and controllers area/networking size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 18, 2023
@knative-prow
Copy link

knative-prow bot commented Oct 18, 2023

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.

@dprotaso
Copy link
Member

/hold release is next week - will review after

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2023
@dprotaso
Copy link
Member

/ok-to-test
/retest

@knative-prow knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 17, 2023
Copy link
Member

@dprotaso dprotaso left a comment

Choose a reason for hiding this comment

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

I believe unit test failures are legit - I think you need to update the fixtures to set the default domain is a wildcard

pkg/reconciler/nscert/nscert.go Outdated Show resolved Hide resolved
pkg/reconciler/route/config/domain.go Outdated Show resolved Hide resolved
pkg/reconciler/route/config/domain.go Show resolved Hide resolved
internalConfig.Wildcard = true
}
c.Domains[k] = DomainConfig{
Selector: &LabelSelector{Selector: internalConfig.Selector},
Copy link
Member

Choose a reason for hiding this comment

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

nit: We could simplify LabelSelector now to be a type alias to map[string]string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could there are methods on the LabelSelector type that are used right now. LabelSelector is an exported type with exported functions so not sure we would want to change those.

@dprotaso
Copy link
Member

Hey @arsenetar just following up - next serving release is in two weeks.

@arsenetar
Copy link
Contributor Author

@dprotaso Been trying to find time to work on this... haven't had a chance to slot this work in.

- Update nscert reconciler to be able to create multiple wildcard
  certificates for any domain that is a wildcard
- Update domain config to allow marking domains as having wildcard
  enable.  The format of the change an implementation allows for prior
  usage without wildcard field to continue to function as it was.
- Update references to config object as necessary to use new object
  format
Change the boolean wildcard config to use a type field that could
support other usage later.  Update references and logic throughout.
Since this function bypasses the configuration loading it needs to set
the default domain to Type of wildcard.
@arsenetar
Copy link
Contributor Author

@dprotaso Took care of the quick changes, looking at the tests failures there seems to be a few things going on.

Found the one test fixture helper that bypassed the domain config loading function so that resolved some of the failures.

The test failure with two certs instead of one is due to it creating a new cert and not removing the old one since now there can technically be more than one. So that is a bug here if we want to keep the behavior similar to before, would need to modify the logic to remove any wildcard certs no longer in the list of wildcard domains.

Going to have to look at this failure more as:

   nscert_test.go:404: DNSNames should not be changed: (-want, +got) =   []string{
        - 	"*.subdomain.testns.svc.cluster.local",
        + 	"*.testns.svc.cluster.local",
          }

This might be the same as the other cleanup one... but not 100% sure at this point. I'll try to get back soon to rewriting the logic to make updates to remove the certs that are no longer matched by wildcard domains.

- Cleanup certs which no longer match a wildcard domains.
- Return on first error instead of continuing (similar to prior
  behavior)
Change findNamespaceCert to use the labeled domain to find matching cert
instead of the dnsNames in the cert.  This seems to be the intended
behavior to keep the cert associated with the entry in the domain config
updated instead of potentially creating another.
Copy link

codecov bot commented Jan 21, 2024

Codecov Report

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

Comparison is base (906d309) 85.91% compared to head (f1d3d2a) 85.90%.
Report is 39 commits behind head on main.

Files Patch % Lines
pkg/reconciler/nscert/nscert.go 74.28% 6 Missing and 3 partials ⚠️
pkg/reconciler/route/config/domain.go 70.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14543      +/-   ##
==========================================
- Coverage   85.91%   85.90%   -0.01%     
==========================================
  Files         197      197              
  Lines       14991    15018      +27     
==========================================
+ Hits        12879    12901      +22     
- Misses       1798     1800       +2     
- Partials      314      317       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arsenetar
Copy link
Contributor Author

/retest

@dprotaso
Copy link
Member

dprotaso commented Feb 7, 2024

Awesome thanks

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024
Copy link

knative-prow bot commented Feb 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arsenetar, dprotaso

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

The pull request process is described 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 knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024
@dprotaso
Copy link
Member

dprotaso commented Feb 7, 2024

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2024
@knative-prow knative-prow bot merged commit 87e6199 into knative:main Feb 7, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/networking lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple wildcard certificate domains
2 participants