Unified kuadrant - Add TLSPolicy and DNSPolicy #416

mikenairn · 2024-02-08T11:24:49Z

Adds the DNSPolicy api and controllers from MGC
Adds the TLSPolicy api and controllers from MGC
Removes the policy-controller
Adds a new dependency for DNS support dns-operator

As much as possible the DNS and TLS policy code has not changed from what is currently in MGC, but has been modified to fit in with the kuadrant operator code base. The only noticeable difference is the status of the polices is now different, as it now shows a "Status" instead of "Ready":

kubectl get dnspolicy -A -o wide                                                                                                                                                                                                                           
NAMESPACE                NAME       STATUS     TARGETREFKIND   TARGETREFNAME   AGE                                                                             
multi-cluster-gateways   prod-web   Accepted   Gateway         prod-web        27m

Verification

make local-setup
kubectl get deployments -n kuadrant-system
NAME                                    READY   UP-TO-DATE   AVAILABLE   AGE
authorino-operator                      1/1     1            1           7m3s
authorino-webhooks                      1/1     1            1           7m3s
dns-operator-controller-manager         1/1     1            1           7m3s
kuadrant-operator-controller-manager    1/1     1            1           3m48s
limitador-operator-controller-manager   1/1     1            1           7m3s

Run though DNS and TLS user guides:

codecov · 2024-02-08T11:27:17Z

Codecov Report

Merging #416 (6a2c94d) into main (f982a02) will decrease coverage by 0.25%.
The diff coverage is 80.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #416      +/-   ##
==========================================
- Coverage   81.15%   80.90%   -0.25%     
==========================================
  Files          40       54      +14     
  Lines        3210     4452    +1242     
==========================================
+ Hits         2605     3602     +997     
- Misses        404      562     +158     
- Partials      201      288      +87

Flag	Coverage Δ
integration	`72.14% <78.82%> (+1.96%)`	⬆️
unit	`28.75% <4.82%> (-6.85%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	`71.42% <ø> (ø)`
api/v1beta2 (u)	`90.90% <ø> (ø)`
pkg/common (u)	`89.87% <100.00%> (+0.27%)`	⬆️
pkg/istio (u)	`73.91% <ø> (-1.25%)`	⬇️
pkg/log (u)	`94.73% <ø> (ø)`
pkg/reconcilers (u)	`80.99% <ø> (+0.82%)`	⬆️
pkg/rlptools (u)	`79.45% <ø> (ø)`
controllers (i)	`77.79% <79.39%> (+0.51%)`	⬆️

Files	Coverage Δ
controllers/gateway_eventmapper.go	`85.00% <100.00%> (+3.75%)`	⬆️
pkg/common/common.go	`97.84% <ø> (ø)`
pkg/common/errors.go	`100.00% <100.00%> (ø)`
pkg/common/gatewayapi_utils.go	`84.83% <100.00%> (+0.17%)`	⬆️
pkg/common/k8s_utils.go	`97.56% <100.00%> (+0.26%)`	⬆️
controllers/tlspolicy_certmanager.go	`96.92% <96.92%> (ø)`
controllers/dnspolicy_status.go	`86.36% <86.36%> (ø)`
controllers/tlspolicy_status.go	`86.36% <86.36%> (ø)`
controllers/kuadrant_status.go	`71.29% <72.22%> (-7.60%)`	⬇️
pkg/multicluster/target.go	`94.28% <94.28%> (ø)`
... and 10 more

... and 7 files with indirect coverage changes

Makefile

api/v1alpha1/dnspolicy_types.go

api/v1alpha1/tlspolicy_types.go

bundle.Dockerfile

maleck13 · 2024-02-08T15:09:30Z

So I have done a scan of the code and other files. All looks as expected. I will try run it locally to verify. But I don't think we need to spend too much time on this PR as it is basically just code moving over.

@alexsnaps @eguzki what are your thoughts here?

maleck13 · 2024-02-08T15:12:54Z

@eguzki would like thoughts on the code coverage. We have focused more on the integration tests than unit tests. So although the code bot says its not covered, it is likely covered by an integratation rather than unit. What is the expectation here for kuadrant operator. Do we need to up the unit coverage before merge, or are we happy to merge knowing the coverage is not reflecting the integration tests?

mikenairn · 2024-02-08T15:49:26Z

@eguzki would like thoughts on the code coverage. We have focused more on the integration tests than unit tests. So although the code bot says its not covered, it is likely covered by an integratation rather than unit. What is the expectation here for kuadrant operator. Do we need to up the unit coverage before merge, or are we happy to merge knowing the coverage is not reflecting the integration tests?

I'm looking into improving the coverage, there are still tests that have not been added. Also looks like it is including integration tests in the coverage report, lack of TLS tests are probably bringing it down a good bit.

alexsnaps · 2024-02-09T09:11:37Z

I wouldn't over sweat the Codecov stuff… If you know it's covered by integration tests, that's good enough to me certainly

didierofrivia · 2024-02-12T17:50:15Z

It's looking good to me, I like how your organize and match the code within the common and helper functions and the controller logic. I wouldn't bother for the test coverage just yet. Will try locally and wait for when it's ready for review

.github/workflows/update-stored-dependencies-version.yaml

* Remove policy-controller * Add kuadrant-dns-operator dependency * Move DNSPolicy api and controller from MGC * patch kuadrant-dns deployment labels * Update image build GH workflows

Add TLSPolicy and controller, moved from MGC

Update status condition logic to use Accepted instead of Ready

Update the dnspolicy controllers and tests to align better with the current kuadrant operator structure.

* import ordering and grouping * add certmanager scheme * typo in variable name (tlsPolicyBaseReconiler -> tlsPolicyBaseReconiler) * add tlspolicy crd and generate manifests * add missing crd to bundle * add missing tlspolicy sample * add missing gateway.networking.k8s.io/policy label * update CRD printcolumns to match kuadrant polices

* add tls controller tests * Return empty array for GetRulesHostnames * Update certmanager version to closer match the version of certmanager we are running v1.7.1 -> v1.12.1 * requeue tlspolicy on TargetNotFound errors

* Remove unnecessary common functions * Return empty array for GetRulesHostnames * Add additional unit tests for k8s utils

Update dns-operator version v0.1.0

mikenairn · 2024-02-20T13:16:14Z

I've successfully checked that the TLS and DNS records are created correctly. Also, I've checked that the TLS user guide is working on Linux... on macOS one needs to have running docker-mac-net-connect since docker for mac does not expose container networks directly on the macOS host, might want to add a note to the user guides.

I can't get a port forward option to work with the TLS user guide, so i think saying you should use docker-mac-net-connect on a mac somewhere in the guides will have to do. I don't however have any clue how it works or what is involved with that so can someone give me a sentence to add?

* Add dns-operator to update-stored-dependencies-version workflow * Update dns policy sample * Removed Pointer test helper in favour of using ptr.To from * Undo hard coded images k8s.io/utils/ptr. * Set dnspolicy as owner of dnsrecords * Add additional certmanager tests

* Add dnspolicy overview and reference docs * Add tlspolicy overview and reference docs

maleck13 · 2024-02-20T14:13:54Z

Maybe we add just that for now as a dependency in the docs?
actually it is already there https://docs.kuadrant.io/getting-started-single-cluster/ so we are prob good to go?
https://docs.kuadrant.io/getting-started-multi-cluster/

didierofrivia

🎖️

mikenairn · 2024-02-20T15:50:14Z

Follow on tasks:

mikenairn mentioned this pull request Feb 8, 2024

Unified kuadrant - Remove TLSPolicy and DNSPolicy Kuadrant/multicluster-gateway-controller#785

Merged