Move pkg/certificates from control-protocol to networking

[ReToCode]

Motivation

• Currently Serving depends on control-protocol just for pkg/certificates
• The constants and parts of the code in control-protocol is needed in net-* implementations

Changes

• Move pkg/certificates from control-protocol to networking
• Update the dependencies accordingly
• Fixed lint warnings

Testing

• Testing in [TEST-ONLY] Full test set for https://github.com/knative/networking/pull/802 serving#13938.

Release Note
None

Docs
None

/kind cleanup

/cc @evankanderson, @nak3, @davidhadas, @dprotaso

Fixes #805

[codecov]

Codecov Report

Patch coverage: 50.86% and project coverage change: -11.76 ⚠️

Comparison is base (0dbe4f9) 94.59% compared to head (0a2e7a6) 82.84%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #802       +/-   ##
===========================================
- Coverage   94.59%   82.84%   -11.76%     
===========================================
  Files          41       45        +4     
  Lines        1259     1708      +449     
===========================================
+ Hits         1191     1415      +224     
- Misses         56      252      +196     
- Partials       12       41       +29     

Impacted Files	Coverage Δ
.../networking/v1alpha1/serverlessservice_defaults.go	0.00% <0.00%> (ø)
pkg/certificates/reconciler/reconciler.go	30.08% <30.08%> (ø)
pkg/certificates/reconciler/controller_impl.go	62.50% <62.50%> (ø)
pkg/certificates/reconciler/certificates.go	68.03% <68.03%> (ø)
pkg/certificates/reconciler/controller.go	92.85% <92.85%> (ø)
...apis/networking/v1alpha1/certificate_validation.go	100.00% <100.00%> (ø)
pkg/apis/networking/v1alpha1/domain_defaults.go	100.00% <100.00%> (ø)
pkg/apis/networking/v1alpha1/domain_validation.go	100.00% <100.00%> (ø)
pkg/apis/networking/v1alpha1/ingress_defaults.go	100.00% <100.00%> (ø)
pkg/apis/networking/v1alpha1/ingress_validation.go	100.00% <100.00%> (ø)
... and 3 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ReToCode]

I'd like to ignore lint errors to keep the code the same as in the origin and fix those in a new PR afterwards.

  Error: G101: Potential hardcoded credentials (gosec)
  Error: G101: Potential hardcoded credentials (gosec)
  Warning: var-naming: const LegacyFakeDnsName should be LegacyFakeDNSName (revive)
  Warning: var-naming: const FakeDnsName should be FakeDNSName (revive)
  Warning: var-naming: func parameter routingId should be routingID (revive)
  Warning: var-naming: func parameter routingId should be routingID (revive)
  Warning: var-naming: var routingId should be routingID (revive)
  Warning: var-naming: const secretRoutingId should be secretRoutingID (revive)
  Error: createCert - result cert is never used (unparam)
  Error: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
  Error: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)

Should be fine, as we have to update all usages anyway. FYI @davidhadas, @nak3

[nak3]

/lgtm
/approve
/hold for other reviewers.

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, ReToCode

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 [nak3]

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

[davidhadas]

@ReToCode
(1) Would it not be better to fix the lint and other issues in the original control-protocol repo and only then move to networking?
I think it is cleaner.
Consider any other PR that will be needed in networking until it is fixed - it will fail on Lint.

(2) As for changes needed in exported symbols that are being used as you correctly indicated elsewhere -changing such symbols may break stuff somewhere for someone at some code we do not control. In a way, if we can avoid making changes in exported symbols (e.g. ask Lint to not concern itself with this change) I would suggest to prefer this approach for exported symbols. Else, we can create a new correct export and leave the old export for a period of time as deprecated or something along this line.

[ReToCode]

This would be way more work (as this here is already done and tested) for something that is removed in control-protocol, also it seems that control-protocol has different lint settings, otherwise it would have failed there as well. But I get your point about the builds, then I'd prefer to go with fixing those in this PR in a separate commit.

Regarding the symbols. Agreed in general, but these are all just recently introduced by you for an undocumented alpha feature. We should be fine with changing them, right?

• LegacyFakeDnsName -> just recently renamed
• FakeDnsName -> just recently introduced
• routingID -> internal
• secretRoutingID -> internal

[davidhadas]

This would be way more work (as this here is already done and tested) for something that is removed in control-protocol, also it seems that control-protocol has different lint settings, otherwise it would have failed there as well. But I get your point about the builds, then I'd prefer to go with fixing those in this PR in a separate commit.

ok

Regarding the symbols. Agreed in general, but these are all just recently introduced by you for an undocumented alpha feature. We should be fine with changing them, right?

LegacyFakeDnsName -> just recently renamed
FakeDnsName -> just recently introduced
routingID -> internal
secretRoutingID -> internal

afaik, FakeDnsName is being used - this is why I did not change it.
All I did is deprecate it and replaced it with LegacyFakeDnsName to make people notice that we are making changes and moving away from the FakeDnsName.

[ReToCode]

Updated the PR with lint-fixes. I kept the public symbols and added lint-comments to ignore those.

[ReToCode]

/hold as I need to check if test failures in Serving are related or flakes.

[evankanderson]

Apparently, I never sent my review.

If you want to clean these in the future, I don't feel very strongly about adding more to this PR.

[evankanderson]

Not sure if we want to switch the pub and parentPriv to any like in x509.

[ReToCode]

If you want to clean these in the future, I don't feel very strongly about adding more to this PR.

@evankanderson I think I'd prefer to merge this one as closely to the original code from control-protocol and do a follow-up on the improvements. We also need to change all the usages of the code in serving and all net-* repos, then we'd also have better feedback if we break something in the dependant repos.

[ReToCode]

/unhold

@evankanderson gentle ping

[ReToCode]

Rebased here, rebased & retriggered the checking PR.

@evankanderson can you please take a look? ^^

[ReToCode]

Initially I intended to keep the code the same as in the source, but I'm also fine with fixing it in this PR so I updated the PR.

[dprotaso]

/lgtm

[nak3]

A few lint error happens. Will we fix another PR or fix here?

[ReToCode]

Fixed the lint issue and cherry-picked knative-extensions/control-protocol@06411c4 to be at the same state as control-protocol. If this is merged, I'll start to migrate all the dependants to networking so we can depreciate the code in control-protocol.

[nak3]

/lgtm
/hold

Thank you! /hold for test.

[ReToCode]

/unhold