search

knative-extensions / control-protocol

Control protocol to enable interaction between control plane and data plane without redeploy
Apache License 2.0
2 stars 26 forks source link

Certificate controller to only watch secrets with a label #225

Closed nader-ziada closed 1 year ago

nader-ziada commented 1 year ago

Changes

/kind enhancement

Release Note

Certificate controller to only watch secrets with a label
nader-ziada commented 1 year ago

/retest

nader-ziada commented 1 year ago

/retest

codecov[bot] commented 1 year ago

Codecov Report

Merging #225 (b0cb9e3) into main (b3d2d77) will decrease coverage by 9.76%. The diff coverage is 35.39%.

:exclamation: Current head b0cb9e3 differs from pull request most recent head 86ed6d3. Consider uploading reports for the commit 86ed6d3 to get more accurate results

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
- Coverage   82.66%   72.89%   -9.77%     
==========================================
  Files          21       23       +2     
  Lines        1090     1380     +290     
==========================================
+ Hits          901     1006     +105     
- Misses        148      317     +169     
- Partials       41       57      +16
Impacted Files Coverage Δ
pkg/certificates/reconciler/reconciler.go 27.96% <27.96%> (ø)
pkg/certificates/reconciler/controller_impl.go 62.50% <62.50%> (ø)
pkg/certificates/reconciler/controller.go 92.85% <100.00%> (+0.54%) :arrow_up:
pkg/certificates/reconciler/certificates.go 69.52% <0.00%> (+0.89%) :arrow_up:
pkg/network/base_connection.go 89.28% <0.00%> (+2.67%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

nader-ziada commented 1 year ago

/cc @nak3 @dprotaso

skonto commented 1 year ago

Cool we hit a problem with this downstream and were planning to fix it. :)

dprotaso commented 1 year ago

Imports modify a global var - and I think it will cause the non filter informer to run.

You can confirm but that’s why I think we need to not use that reconciler

On Tue, Nov 1, 2022 at 14:39 Nader Ziada @.***> wrote:

@.**** commented on this pull request.

In pkg/certificates/reconciler/controller_impl.go https://github.com/knative-sandbox/control-protocol/pull/225#discussion_r1010774099 :

+ +import (

  • context "context"
  • fmt "fmt"
  • reflect "reflect"
  • strings "strings"
  • zap "go.uber.org/zap"
  • corev1 "k8s.io/api/core/v1"
  • watch "k8s.io/apimachinery/pkg/watch"
  • informersv1 "k8s.io/client-go/informers/core/v1"
  • scheme "k8s.io/client-go/kubernetes/scheme"
  • v1 "k8s.io/client-go/kubernetes/typed/core/v1"
  • record "k8s.io/client-go/tools/record"
  • client "knative.dev/pkg/client/injection/kube/client"
  • secretreconciler "knative.dev/pkg/client/injection/kube/reconciler/core/v1/secret"

I'm only pulling in the secretreconciler to use the reconciler.go file which doesn't import the secret informer and allows me to pass in the the filtered lister defined in the above step.

Do you prefer if I have a new implementation of the reconciler here as well?

— Reply to this email directly, view it on GitHub https://github.com/knative-sandbox/control-protocol/pull/225#discussion_r1010774099, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAERATMTPS3SNUR4WIAGCTWGFPUHANCNFSM6AAAAAARR3PZEM . You are receiving this because you were mentioned.Message ID: @.***>

nader-ziada commented 1 year ago

Imports modify a global var - and I think it will cause the non filter informer to run. You can confirm but that’s why I think we need to not use that reconciler

As far as I can tell from running the tests, even if the regular secret informer is running, its not the one used by the controller. The filtered informer is the one used by the controller which is the main reason I had to do the specific controller impl so I can pass in the informer I create. If I can't use the reconciler package from the pkg repo at all, then it would be a much bigger change which is fine I guess, but I didn't want to make this work completely different than everything else.

I will try to make a different reconciler and see how that goes

nader-ziada commented 1 year ago

@dprotaso added reconciler as well

dprotaso commented 1 year ago

even if the regular secret informer is running

Just having the regular informer running will cause a leak since it will fill it's cache with secrets

dprotaso commented 1 year ago

/lgtm /approve

knative-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nader-ziada

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](https://github.com/knative-sandbox/control-protocol/blob/main/OWNERS)~~ [dprotaso] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
nader-ziada commented 1 year ago

/retest

nader-ziada commented 1 year ago

/retest

dprotaso commented 1 year ago

/lgtm

rachitchauhan43 commented 1 year ago

/cherry-pick release-1.7

knative-prow-robot commented 1 year ago

@rachitchauhan43: only knative-sandbox org members may request cherry picks. You can still do the cherry-pick manually.

In response to [this](https://github.com/knative-sandbox/control-protocol/pull/225#issuecomment-1428563527): >/cherry-pick release-1.7 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
evankanderson commented 1 year ago

/cherry-pick release-1.8 /cherry-pick release-1.7

knative-prow-robot commented 1 year ago

@evankanderson: new pull request created: #253

In response to [this](https://github.com/knative-sandbox/control-protocol/pull/225#issuecomment-1428599549): >/cherry-pick release-1.8 >/cherry-pick release-1.7 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
evankanderson commented 1 year ago

Cherry-picking to 1.7 and 1.8 due to reports here: https://cloud-native.slack.com/archives/C04LMU0AX60/p1674692609842299

evankanderson commented 1 year ago

/cherry-pick release-1.7

knative-prow-robot commented 1 year ago

@evankanderson: new pull request created: #254

In response to [this](https://github.com/knative-sandbox/control-protocol/pull/225#issuecomment-1428609896): >/cherry-pick release-1.7 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.