feat: Add ability to configure multiple wilcard domains #14543

arsenetar · 2023-10-18T20:23:39Z

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 · 2023-10-18T20:23:50Z

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 · 2023-10-20T14:44:55Z

/hold release is next week - will review after

dprotaso · 2023-11-17T19:25:17Z

/ok-to-test
/retest

dprotaso

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

pkg/reconciler/route/config/domain.go

dprotaso · 2023-11-17T21:14:48Z

pkg/reconciler/route/config/domain.go

+			internalConfig.Wildcard = true
+		}
+		c.Domains[k] = DomainConfig{
+			Selector: &LabelSelector{Selector: internalConfig.Selector},


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

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 · 2024-01-12T15:52:07Z

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

arsenetar · 2024-01-12T16:36:55Z

@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 · 2024-01-19T04:47:18Z

@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.

codecov · 2024-01-21T09:18:22Z

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 · 2024-02-01T02:44:09Z

/retest

dprotaso · 2024-02-07T21:09:39Z

Awesome thanks

/lgtm
/approve

knative-prow · 2024-02-07T21:09:49Z

[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:

~~OWNERS~~ [dprotaso]

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

dprotaso · 2024-02-07T21:22:26Z

/hold cancel

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 bot requested review from jsanin-vmw and xtreme-vikram-yadav October 18, 2023 20:23

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

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

dprotaso reviewed Nov 17, 2023

View reviewed changes

dprotaso mentioned this pull request Nov 22, 2023

feat: Filter on istio gateways based on ingress annotations knative-extensions/net-istio#1137

Closed

arsenetar added 3 commits January 19, 2024 03:13

fix: Correct issue with generated deepcopy file.

78d7c89

refactor: Change wildcard domain to use type field

7bd1abb

Change the boolean wildcard config to use a type field that could support other usage later. Update references and logic throughout.

arsenetar force-pushed the as/multi-wildcard branch from 1b4d377 to 7bd1abb Compare January 19, 2024 04:13

fix: Correct test fixture function for domainConfig

90ce623

Since this function bypasses the configuration loading it needs to set the default domain to Type of wildcard.

arsenetar added 2 commits January 21, 2024 08:40

refactor(nscert): Change cert cleanup behavior

93c3983

- Cleanup certs which no longer match a wildcard domains. - Return on first error instead of continuing (similar to prior behavior)

knative-prow bot assigned dprotaso Feb 7, 2024

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

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024

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 bot merged commit 87e6199 into knative:main Feb 7, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to configure multiple wilcard domains #14543

feat: Add ability to configure multiple wilcard domains #14543

arsenetar commented Oct 18, 2023

knative-prow bot commented Oct 18, 2023

dprotaso commented Oct 20, 2023

dprotaso commented Nov 17, 2023

dprotaso left a comment

dprotaso Nov 17, 2023

arsenetar Dec 6, 2023

dprotaso commented Jan 12, 2024

arsenetar commented Jan 12, 2024

arsenetar commented Jan 19, 2024

codecov bot commented Jan 21, 2024 •

edited

arsenetar commented Feb 1, 2024

dprotaso commented Feb 7, 2024

knative-prow bot commented Feb 7, 2024

dprotaso commented Feb 7, 2024

feat: Add ability to configure multiple wilcard domains #14543

feat: Add ability to configure multiple wilcard domains #14543

Conversation

arsenetar commented Oct 18, 2023

Proposed Changes

knative-prow bot commented Oct 18, 2023

dprotaso commented Oct 20, 2023

dprotaso commented Nov 17, 2023

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Nov 17, 2023

Choose a reason for hiding this comment

arsenetar Dec 6, 2023

Choose a reason for hiding this comment

dprotaso commented Jan 12, 2024

arsenetar commented Jan 12, 2024

arsenetar commented Jan 19, 2024

codecov bot commented Jan 21, 2024 • edited

Codecov Report

arsenetar commented Feb 1, 2024

dprotaso commented Feb 7, 2024

knative-prow bot commented Feb 7, 2024

dprotaso commented Feb 7, 2024

codecov bot commented Jan 21, 2024 •

edited