Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not build OIDC config unless enabled #4021

Merged

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Jul 30, 2024

Temporary fix for #3901 (this does not fix the underlying problem as far as I can tell, but will enable anyone not interested in OIDC to use 1.14+)

Proposed Changes

  • Do not load the OIDC config unless it is enabled
  • Refactor the receiver Main class to allow for redeploying the receiver verticles when the OIDC config changes
  • Set an annotation on the receiver pods to force k8s to update the files mounted from the config-features configmap, so that the receiver quickly picks up on changes to feature flags.

Release Note

The receiver components now successfully start on EKS. Special shoutout to @treyhyde for help debugging this.

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

/cc @pierDipi

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 30, 2024
@knative-prow knative-prow bot requested a review from pierDipi July 30, 2024 16:55
@Cali0707
Copy link
Member Author

/cherry-pick release-1.15

@knative-prow-robot
Copy link
Contributor

@Cali0707: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

@Cali0707
Copy link
Member Author

/cherry-pick release-1.14

@knative-prow-robot
Copy link
Contributor

@Cali0707: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.14

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-sigs/prow repository.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 31.42857% with 24 lines in your changes missing coverage. Please review.

Project coverage is 48.39%. Comparing base (135c294) to head (c002762).
Report is 5 commits behind head on main.

Files Patch % Lines
control-plane/pkg/reconciler/base/reconciler.go 46.66% 7 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/broker/broker.go 0.00% 0 Missing and 4 partials ⚠️
control-plane/pkg/reconciler/broker/controller.go 33.33% 1 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/channel/channel.go 0.00% 0 Missing and 2 partials ⚠️
control-plane/pkg/reconciler/channel/controller.go 33.33% 1 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/sink/controller.go 33.33% 1 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/sink/kafka_sink.go 0.00% 0 Missing and 2 partials ⚠️
control-plane/pkg/reconciler/trigger/trigger.go 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4021      +/-   ##
==========================================
- Coverage   48.48%   48.39%   -0.10%     
==========================================
  Files         244      244              
  Lines       14469    14511      +42     
==========================================
+ Hits         7016     7022       +6     
- Misses       6746     6778      +32     
- Partials      707      711       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Cali0707
Copy link
Member Author

/retest-required

Cali0707 added 2 commits July 30, 2024 20:31
Signed-off-by: Calum Murray <cmurray@redhat.com>
When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added area/control-plane size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 31, 2024
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

TODO (as a follow up): refactor the FileWatcher class to be able to either watch a single file or a directory. Because of how configmaps are mounted as files into pods this PR just watches the specific file associated with the OIDC feature flag. But, going forwards we will probably end up wanting to support more feature flags, so we should make the FileWatcher class more generic there

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

/retest-required

@Cali0707
Copy link
Member Author

/cc @pierDipi

This should be good to go now, I updated it to not redeploy the verticles (like we had discussed offline).

Now, we are using a listener + callback pattern for the OIDC Discovery stuff

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707 Cali0707 force-pushed the do-not-load-oidc-when-disabled branch from dcede6d to 123e6c1 Compare July 31, 2024 19:53
@Cali0707
Copy link
Member Author

/retest-required

1 similar comment
@Cali0707
Copy link
Member Author

Cali0707 commented Aug 1, 2024

/retest-required

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2024
@creydr
Copy link
Contributor

creydr commented Aug 8, 2024

/home/prow/go/src/knative.dev/eventing-kafka-broker is out of date. Please run hack/update-codegen.sh

@Cali0707 can you run it?

}

this.callbacks.add(callback);
return this.callbacks.size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC you are using this return value as some "id", which you later pass to "deregisterCallback()". This uses .set(id, null) to remove it.
So you do the following on an initial empty arraylist

  1. add a callback
  2. return as "callbackId" 1
  3. later want to deregister the callbackId 1, but actually you would need to call deregisterCallback(0)

sry, but I didn't find a way to explain it more complicated 🤦

tl;dr; is, that IIUC, you should return here this.callbacks.size() - 1, if you want to use this as the "callbackId"

Copy link
Contributor

@creydr creydr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this 💪 . Did a first round. But I probably need to run it locally too :D

TokenVerifier tokenVerifier = new TokenVerifierImpl(vertx, oidcDiscoveryConfig);
this.authenticationHandler = new AuthenticationHandler(tokenVerifier);
this.buildAuthHandler(oidcDiscoveryConfigListener.getOidcDiscoveryConfig());
this.oidcDiscoveryCallbackId = this.oidcDiscoveryConfigListener.registerCallback(this::buildAuthHandler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a quick note somewhere, that the FileWatcher runs the callbacks, when it gets started (in it's .run() ), that way we can be "sure", that this.authenticationHandler is not null (which is used in line 243)

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Aug 9, 2024

/cc @creydr

Thanks for taking a first look!

@knative-prow knative-prow bot requested a review from creydr August 9, 2024 14:00
@Cali0707
Copy link
Member Author

Cali0707 commented Aug 9, 2024

/test integration-tests

@Cali0707
Copy link
Member Author

Cali0707 commented Aug 9, 2024

/unhold

Seems like things are passing now, thanks for finding that off by one error @creydr !!

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2024
Copy link
Contributor

@creydr creydr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for fixing this @Cali0707
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2024
Copy link

knative-prow bot commented Aug 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, creydr

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:

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

@Cali0707
Copy link
Member Author

/test upgrade-tests

@Cali0707
Copy link
Member Author

/test upgrade-tests

Last failure looks like a flake, cert-manager failed to come ready

@knative-prow knative-prow bot merged commit 073bcd2 into knative-extensions:main Aug 12, 2024
36 of 37 checks passed
@knative-prow-robot
Copy link
Contributor

@Cali0707: #4021 failed to apply on top of branch "release-1.15":

Applying: fix: do not build OIDC config unless enabled
Using index info to reconstruct a base tree...
M	data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
Applying: feat: receiver redeploys verticles when oidc feature changes
Using index info to reconstruct a base tree...
M	data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
CONFLICT (content): Merge conflict in data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 feat: receiver redeploys verticles when oidc feature changes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

@knative-prow-robot
Copy link
Contributor

@Cali0707: #4021 failed to apply on top of branch "release-1.14":

Applying: fix: do not build OIDC config unless enabled
Using index info to reconstruct a base tree...
M	data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
Applying: feat: receiver redeploys verticles when oidc feature changes
Using index info to reconstruct a base tree...
M	data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
CONFLICT (content): Merge conflict in data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/main/Main.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 feat: receiver redeploys verticles when oidc feature changes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.14

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-sigs/prow repository.

Cali0707 added a commit to Cali0707/eventing-kafka-broker that referenced this pull request Aug 12, 2024
* fix: do not build OIDC config unless enabled

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: receiver redeploys verticles when oidc feature changes

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* cleanup: goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: do not re-deploy verticles

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: features config paths are now correct

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix java unit tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* address review comments

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
Cali0707 added a commit to Cali0707/eventing-kafka-broker that referenced this pull request Aug 12, 2024
* fix: do not build OIDC config unless enabled

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: receiver redeploys verticles when oidc feature changes

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* cleanup: goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: do not re-deploy verticles

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: features config paths are now correct

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix java unit tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* address review comments

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
Copy link

knative-prow bot commented Aug 12, 2024

@Cali0707: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit-tests_eventing-kafka-broker_main c002762 link unknown /test unit-tests
upgrade-tests_eventing-kafka-broker_main c002762 link unknown /test upgrade-tests
reconciler-tests-namespaced-broker_eventing-kafka-broker_main c002762 link unknown /test reconciler-tests-namespaced-broker
channel-reconciler-tests-sasl-plain_eventing-kafka-broker_main c002762 link unknown /test channel-reconciler-tests-sasl-plain

Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

knative-prow bot pushed a commit that referenced this pull request Aug 14, 2024
…4056)

* fix: do not build OIDC config unless enabled (#4021)

* fix: do not build OIDC config unless enabled

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: receiver redeploys verticles when oidc feature changes

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* cleanup: goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: do not re-deploy verticles

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: features config paths are now correct

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix java unit tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* address review comments

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: compilation errors

Signed-off-by: Calum Murray <cmurray@redhat.com>

* goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
knative-prow bot pushed a commit that referenced this pull request Aug 15, 2024
* fix: do not build OIDC config unless enabled



* feat: receiver redeploys verticles when oidc feature changes



* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s



* mvn spotless:apply



* cleanup: goimports



* fix: do not re-deploy verticles



* fix: features config paths are now correct



* fix java unit tests



* mvn spotless:apply



* address review comments



---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
matzew pushed a commit to matzew/eventing-kafka-broker that referenced this pull request Nov 6, 2024
…extensions#4021) (knative-extensions#4056)

* fix: do not build OIDC config unless enabled (knative-extensions#4021)

* fix: do not build OIDC config unless enabled

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: receiver redeploys verticles when oidc feature changes

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* cleanup: goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: do not re-deploy verticles

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: features config paths are now correct

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix java unit tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* address review comments

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: compilation errors

Signed-off-by: Calum Murray <cmurray@redhat.com>

* goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
matzew pushed a commit to matzew/eventing-kafka-broker that referenced this pull request Nov 6, 2024
knative-extensions#4055)

* fix: do not build OIDC config unless enabled



* feat: receiver redeploys verticles when oidc feature changes



* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s



* mvn spotless:apply



* cleanup: goimports



* fix: do not re-deploy verticles



* fix: features config paths are now correct



* fix java unit tests



* mvn spotless:apply



* address review comments



---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
openshift-merge-bot bot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Nov 7, 2024
* [release-1.14] fix: do not build OIDC config unless enabled (knative-extensions#4021) (knative-extensions#4056)

* fix: do not build OIDC config unless enabled (knative-extensions#4021)

* fix: do not build OIDC config unless enabled

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: receiver redeploys verticles when oidc feature changes

Signed-off-by: Calum Murray <cmurray@redhat.com>

* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* cleanup: goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: do not re-deploy verticles

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: features config paths are now correct

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix java unit tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

* mvn spotless:apply

Signed-off-by: Calum Murray <cmurray@redhat.com>

* address review comments

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>

* fix: compilation errors

Signed-off-by: Calum Murray <cmurray@redhat.com>

* goimports

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>

* usage of CopyOnWriteArrayList/AtomicRef

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* using concurrent hash map and atomic integer for ids

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
pierDipi pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Nov 22, 2024
* fix: do not build OIDC config unless enabled (knative-extensions#4021) (knative-extensions#4055)

* fix: do not build OIDC config unless enabled



* feat: receiver redeploys verticles when oidc feature changes



* feat: the control plane ensures receiver restarts

When config-features changes, the control plane
sets a annotation on the receiver pods so that
the configmap update is reconciled by k8s



* mvn spotless:apply



* cleanup: goimports



* fix: do not re-deploy verticles



* fix: features config paths are now correct



* fix java unit tests



* mvn spotless:apply



* address review comments



---------

Signed-off-by: Calum Murray <cmurray@redhat.com>

* usage of CopyOnWriteArrayList/AtomicRef

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* using concurrent hash map and atomic integer for ids

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/data-plane lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants