Add reconciler logic for DomainMapping AutoTLS by tcnghia · Pull Request #10467 · knative/serving

tcnghia · 2021-01-03T03:09:28Z

Proposed Changes

Inject in a CertificateInformer
Add reconcile logic to handle ACME challenge and Secret propagation into Ingress

Release Note

Add AutoTLS support for DomainMapping

knative-prow-robot · 2021-01-03T03:09:33Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2021-01-03T03:14:08Z

Codecov Report

Merging #10467 (1aa845b) into master (915172b) will increase coverage by 0.05%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master   #10467      +/-   ##
==========================================
+ Coverage   88.02%   88.07%   +0.05%     
==========================================
  Files         187      187              
  Lines        8808     8849      +41     
==========================================
+ Hits         7753     7794      +41     
- Misses        814      816       +2     
+ Partials      241      239       -2

Impacted Files	Coverage Δ
pkg/reconciler/domainmapping/controller.go	`0.00% <0.00%> (ø)`
pkg/reconciler/domainmapping/reconciler.go	`92.92% <100.00%> (+3.58%)`	⬆️
pkg/autoscaler/statforwarder/leases.go	`76.97% <0.00%> (-1.44%)`	⬇️
pkg/reconciler/configuration/configuration.go	`88.28% <0.00%> (+3.90%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 915172b...1aa845b. Read the comment docs.

tcnghia · 2021-01-03T08:12:15Z

/test pull-knative-serving-istio-stable-no-mesh-tls

tcnghia · 2021-01-03T17:07:40Z

/test pull-knative-serving-istio-stable-no-mesh-tls

vagababov · 2021-01-03T20:43:11Z

pkg/reconciler/domainmapping/reconciler.go

+	annotationValue := dm.Annotations[networking.DisableAutoTLSAnnotationKey]
+	disabledByAnnotation, err := strconv.ParseBool(annotationValue)
+	if annotationValue != "" && err != nil {
+		// validation should've caught an invalid value here.
+		// if we have one anyways, assume not disabled and log a warning.
+		logger.Warnf("Invalid annotation value for %q. Value: %q",
+			networking.DisableAutoTLSAnnotationKey, annotationValue)


this looks like a method on DM itself?

Sure, I'll follow up.

vagababov · 2021-01-03T20:44:18Z

pkg/reconciler/domainmapping/reconciler.go

+// Check that our Reconciler implements CertificateAccessor
+var _ networkaccessor.CertificateAccessor = (*Reconciler)(nil)
+
+// GetNetworkingClient implements networking.CertificateAccessor


this is unused?

It is used here https://github.com/knative/serving/pull/10467/files/76b30367936cb4cfdbca4446239dbde73b9fa473#diff-ebdda38d8b796c4f5e3b7e5ed54460483d22b3fd1d273de3b91a739c93aa59d6R168

I don't see a call to GetNetworkingClient there.

Hm, it's passed down... not sure we should do pass reconciler down. Rather we should pass down the networking client.

I can follow up with a change to fix route and domainmapping reconciler both.

vagababov · 2021-01-03T20:44:26Z

pkg/reconciler/domainmapping/reconciler.go

+	return r.netclient
+}
+
+// GetCertificateLister implements networking.CertificateAccessor


It is used here https://github.com/knative/serving/pull/10467/files/76b30367936cb4cfdbca4446239dbde73b9fa473#diff-ebdda38d8b796c4f5e3b7e5ed54460483d22b3fd1d273de3b91a739c93aa59d6R168

tcnghia · 2021-01-03T22:20:01Z

/assign @vagababov @YoussB

tcnghia · 2021-01-03T22:20:33Z

/assign @julz

Co-authored-by: Julian Friedman <julz.friedman@uk.ibm.com>

tcnghia · 2021-01-05T01:25:47Z

/test pull-knative-serving-istio-stable-no-mesh

tcnghia · 2021-01-05T03:18:02Z

/hold

tcnghia · 2021-01-05T08:02:25Z

/unhold
(tested using latest version of net-http01)

@julz @vagababov I've addressed most of your feedbacks, except for a few that I will follow up.

julz

/lgtm
/hold so @vagababov can have a look and in case you want to do the little suggested refactor (but Im fine if you prefer this way or want to do it separately).

pkg/reconciler/domainmapping/reconciler.go

knative-prow-robot · 2021-01-05T08:18:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: julz, tcnghia

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/reconciler/OWNERS~~ [tcnghia]

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

pkg/reconciler/domainmapping/reconciler.go

pkg/reconciler/domainmapping/reconciler_test.go

Co-authored-by: Victor Agababov <vagababov@gmail.com>

vagababov · 2021-01-05T18:30:50Z

/lgtm
/unhold

mattmoor · 2021-01-05T19:07:58Z

pkg/reconciler/domainmapping/reconciler.go

 	domainmappingreconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1alpha1/domainmapping"
 	"knative.dev/serving/pkg/reconciler/domainmapping/config"
 	"knative.dev/serving/pkg/reconciler/domainmapping/resources"
+	routeresources "knative.dev/serving/pkg/reconciler/route/resources"


Can we move this somewhere common?

I'll follow up with that as well.

Got an issue with a DomainMapping:

party knative.party Unknown NewObservedGenFailure

Looking at the logs of the kcert:

Warning InternalError 3m23s (x17 over 8m51s) certificate-controller Service "knative.party" is invalid: metadata.name: Invalid value: "knative.party": a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name', or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')```

I believe you need to update net-http01, let's discuss in slack

mattmoor

/lgtm

Inject in a certificate lister for domainmapping controller

5d73642

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 3, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 3, 2021

knative-prow-robot requested review from markusthoemmes and whaught January 3, 2021 03:09

knative-prow-robot added area/API API objects and controllers approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 3, 2021

tcnghia force-pushed the dm-tls-controller branch from 1cc4246 to 5059cc8 Compare January 3, 2021 03:11

tcnghia force-pushed the dm-tls-controller branch 2 times, most recently from 2af2bd4 to 1508951 Compare January 3, 2021 03:32

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2021

tcnghia force-pushed the dm-tls-controller branch from 1508951 to 114be33 Compare January 3, 2021 03:34

Add Certificate / ACME challenge logic

c674cdf

tcnghia force-pushed the dm-tls-controller branch from 114be33 to c674cdf Compare January 3, 2021 04:38

tcnghia added 2 commits January 2, 2021 21:28

Run hack/update-codegen.sh

c48e0dd

Fix lint comment

76b3036

tcnghia marked this pull request as ready for review January 3, 2021 07:44

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2021

vagababov reviewed Jan 3, 2021

View reviewed changes

knative-prow-robot assigned vagababov and YoussB Jan 3, 2021

knative-prow-robot assigned julz Jan 3, 2021

tcnghia and others added 3 commits January 4, 2021 12:50

Update pkg/reconciler/domainmapping/reconciler.go

dd9e65d

Co-authored-by: Julian Friedman <julz.friedman@uk.ibm.com>

Address PR feedback

258644c

Run hack/update-codegen.sh

4ea8346

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2021

julz approved these changes Jan 5, 2021

View reviewed changes

pkg/reconciler/domainmapping/reconciler.go Outdated Show resolved Hide resolved

pkg/reconciler/domainmapping/reconciler.go Outdated Show resolved Hide resolved

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 5, 2021

Address PR feedback

6220801

tcnghia force-pushed the dm-tls-controller branch from 265eab7 to 6220801 Compare January 5, 2021 16:56

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2021

vagababov reviewed Jan 5, 2021

View reviewed changes

Address PR feedbacks

8e8b427

vagababov reviewed Jan 5, 2021

View reviewed changes

pkg/reconciler/domainmapping/reconciler_test.go Outdated Show resolved Hide resolved

Update pkg/reconciler/domainmapping/reconciler_test.go

5593262

Co-authored-by: Victor Agababov <vagababov@gmail.com>

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 5, 2021

Fix styling issues

1aa845b

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2021

mattmoor reviewed Jan 5, 2021

View reviewed changes

knative-prow-robot assigned mattmoor Jan 5, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2021

knative-prow-robot merged commit 160a16a into knative:master Jan 5, 2021

julz mentioned this pull request Apr 15, 2021

Add julz to API Approvers knative/community#573

Merged

Conversation

tcnghia commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Jan 3, 2021

Uh oh!

codecov bot commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tcnghia commented Jan 3, 2021

Uh oh!

tcnghia commented Jan 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcnghia commented Jan 3, 2021

Uh oh!

tcnghia commented Jan 3, 2021

Uh oh!

tcnghia commented Jan 5, 2021

Uh oh!

tcnghia commented Jan 5, 2021

Uh oh!

tcnghia commented Jan 5, 2021

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knative-prow-robot commented Jan 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vagababov commented Jan 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tcnghia commented Jan 3, 2021 •

edited

Loading

codecov bot commented Jan 3, 2021 •

edited

Loading