fix: kafka controller does not panic on invalid kafka client config #3938

Cali0707 · 2024-06-20T20:06:49Z

Proposed Changes

Only defer closing the cluster admin if it is not nil
Check that the configs map exists before accessing a value in it

Release Note

The kafka controller does not panic on invalid kafka client configmaps anymore

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

Cali0707 · 2024-06-20T20:07:00Z

/cherry-pick release-1.14

knative-prow-robot · 2024-06-20T20:07:03Z

@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.

Cali0707 · 2024-06-20T20:07:10Z

/cherry-pick release-1.13

knative-prow-robot · 2024-06-20T20:07:11Z

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

In response to this:

/cherry-pick release-1.13

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 · 2024-06-20T20:07:18Z

/cc @creydr @pierDipi

codecov · 2024-06-20T20:11:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.24%. Comparing base (046eb4b) to head (ffd5126).
Report is 35 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3938   +/-   ##
=======================================
  Coverage   48.24%   48.24%           
=======================================
  Files         246      246           
  Lines       14500    14502    +2     
=======================================
+ Hits         6995     6997    +2     
  Misses       6799     6799           
  Partials      706      706

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

pierDipi · 2024-06-27T08:01:15Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+	if cg.Spec.Template.Spec.Configs.Configs == nil {
+		return fmt.Errorf("no consumer config supplied, unable to get bootstrap.servers")
+	}


This seems to be better suited for Validate so that it gets rejected from the beginning ?

pierDipi · 2024-06-27T08:01:25Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+	if cg.Spec.Template.Spec.Configs.Configs == nil {
+		return fmt.Errorf("no consumer config supplied, unable to get bootstrap.servers")
+	}


Same for this one?

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

Cali0707 · 2024-07-12T14:13:19Z

/cc @matzew

pierDipi

/lgtm

knative-prow · 2024-07-23T14:03:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [Cali0707,pierDipi]

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

matzew · 2024-07-23T14:03:40Z

control-plane/pkg/apis/internals/kafka/eventing/v1alpha1/consumer_group_validation.go

@@ -57,6 +57,9 @@ func (cts *ConsumerTemplateSpec) Validate(ctx context.Context) *apis.FieldError
 		cts.Spec.Delivery.Validate(specCtx).ViaField("delivery"),
 		cts.Spec.Subscriber.Validate(specCtx).ViaField("subscriber"),
 	)
+	if cts.Spec.Configs.Configs == nil {
+		err = err.Also(apis.ErrMissingField("spec.configs"))


can we test the missing config ?

pierDipi · 2024-07-23T14:03:55Z

/test upgrade-tests

Cali0707 · 2024-07-23T15:55:14Z

/test upgrade-tests

Cali0707 · 2024-07-23T17:24:29Z

/retest-required

Cali0707 · 2024-07-23T18:19:23Z

/test upgrade-tests

knative-prow-robot · 2024-07-23T21:06:05Z

@Cali0707: new pull request created: #4002

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.

knative-prow-robot · 2024-07-23T21:06:42Z

@Cali0707: #3938 failed to apply on top of branch "release-1.13":

Applying: fix: kafka controller does not panic on invalid kafka client config
Using index info to reconstruct a base tree...
M	control-plane/pkg/reconciler/consumergroup/consumergroup.go
Falling back to patching base and 3-way merge...
Auto-merging control-plane/pkg/reconciler/consumergroup/consumergroup.go
CONFLICT (content): Merge conflict in control-plane/pkg/reconciler/consumergroup/consumergroup.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix: kafka controller does not panic on invalid kafka client config
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.13

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 · 2024-07-24T15:02:02Z

@pierDipi is it worth backporting to 1.13 with the 1.15 release going out this week?

fix: kafka controller does not panic on invalid kafka client config

c038aba

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

knative-prow bot added area/control-plane size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 20, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2024

knative-prow bot requested review from creydr and Leo6Leo June 20, 2024 20:07

knative-prow bot requested a review from pierDipi June 20, 2024 20:07

pierDipi reviewed Jun 27, 2024

View reviewed changes

fix: consumergroup configs are validated as non-nil

ffd5126

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

knative-prow bot requested a review from matzew July 12, 2024 14:13

pierDipi approved these changes Jul 23, 2024

View reviewed changes

knative-prow bot assigned pierDipi Jul 23, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024

matzew reviewed Jul 23, 2024

View reviewed changes

knative-prow bot merged commit 3d45d70 into knative-extensions:main Jul 23, 2024
37 checks passed

knative-prow-robot mentioned this pull request Jul 23, 2024

[release-1.14] fix: kafka controller does not panic on invalid kafka client config #4002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: kafka controller does not panic on invalid kafka client config #3938

fix: kafka controller does not panic on invalid kafka client config #3938

Cali0707 commented Jun 20, 2024

Cali0707 commented Jun 20, 2024

knative-prow-robot commented Jun 20, 2024

Cali0707 commented Jun 20, 2024

knative-prow-robot commented Jun 20, 2024

Cali0707 commented Jun 20, 2024

codecov bot commented Jun 20, 2024 •

edited

Loading

pierDipi Jun 27, 2024

pierDipi Jun 27, 2024

Cali0707 commented Jul 12, 2024

pierDipi left a comment

knative-prow bot commented Jul 23, 2024

matzew Jul 23, 2024

pierDipi commented Jul 23, 2024

Cali0707 commented Jul 23, 2024

Cali0707 commented Jul 23, 2024

Cali0707 commented Jul 23, 2024

knative-prow-robot commented Jul 23, 2024

knative-prow-robot commented Jul 23, 2024

Cali0707 commented Jul 24, 2024

fix: kafka controller does not panic on invalid kafka client config #3938

fix: kafka controller does not panic on invalid kafka client config #3938

Conversation

Cali0707 commented Jun 20, 2024

Proposed Changes

Cali0707 commented Jun 20, 2024

knative-prow-robot commented Jun 20, 2024

Cali0707 commented Jun 20, 2024

knative-prow-robot commented Jun 20, 2024

Cali0707 commented Jun 20, 2024

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

pierDipi Jun 27, 2024

Choose a reason for hiding this comment

pierDipi Jun 27, 2024

Choose a reason for hiding this comment

Cali0707 commented Jul 12, 2024

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jul 23, 2024

matzew Jul 23, 2024

Choose a reason for hiding this comment

pierDipi commented Jul 23, 2024

Cali0707 commented Jul 23, 2024

Cali0707 commented Jul 23, 2024

Cali0707 commented Jul 23, 2024

knative-prow-robot commented Jul 23, 2024

knative-prow-robot commented Jul 23, 2024

Cali0707 commented Jul 24, 2024

codecov bot commented Jun 20, 2024 •

edited

Loading