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

E2E tests for channel: TLS key pair rotation #3406

Merged

Conversation

Leo6Leo
Copy link
Contributor

@Leo6Leo Leo6Leo commented Oct 17, 2023

Fixes #3374

Proposed Changes

  • Add the rekt test for keypair rotation for kafka sink

Issues Found

  • Path is not listed in the dataplane contract, so when probing request is sent, and receiver cannot navigate to the right path.
  • In the control-plane/config/eventing-kafka-broker/100-channel/100-kafka-channel.yaml, there is no custom resource definition for the addressable
  • Prober is using the channel service name. Instead we should use the channel name to be consistent with the kafka-broker prober.

Backporting

  • Midstream
  • Upstream

Release Note

Path is not listed in the data plane contract, so when a probing request is sent, and receiver cannot navigate to the right path. (#3406)

In the control-plane/config/eventing-kafka-broker/100-channel/100-kafka-channel.yaml, there is no custom resource definition for the addressable. (#3406)

Prober is using the channel service name. Instead, we should use the channel name to be consistent with the Kafka-broker prober. (#3406)

Docs

@knative-prow
Copy link

knative-prow bot commented Oct 17, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2023
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/control-plane area/test labels Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #3406 (939a589) into main (82261a3) will increase coverage by 0.04%.
Report is 6 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3406      +/-   ##
==========================================
+ Coverage   58.48%   58.52%   +0.04%     
==========================================
  Files          91       91              
  Lines        9319     9328       +9     
==========================================
+ Hits         5450     5459       +9     
  Misses       3433     3433              
  Partials      436      436              
Files Coverage Δ
control-plane/pkg/reconciler/channel/channel.go 69.79% <100.00%> (+0.05%) ⬆️
control-plane/pkg/reconciler/channel/controller.go 79.66% <100.00%> (+1.47%) ⬆️

Copy link
Contributor Author

@Leo6Leo Leo6Leo left a comment

Choose a reason for hiding this comment

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

@pierDipi The problem seems like to be: the data plane receiver cannot find the channel in the pathMap. See the logs

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Oct 18, 2023

This is the describe output for the channel. The data-plane is not ready, is because it cannot find anything in the pathMap, and the receiver returned 404 Not Found.

Name:         channel-hlsrtatzoker  channel-rekt-keypair-rotation                                                                                 
Namespace:    test-juolbwtq
Labels:       <none>
Annotations:  messaging.knative.dev/subscribable: v1
API Version:  messaging.knative.dev/v1beta1
Kind:         KafkaChannel
Metadata:
  Creation Timestamp:  2023-10-18T20:02:10Z
  Finalizers:
    kafkachannels.messaging.knative.dev
  Generation:  1
  Managed Fields:
    API Version:  messaging.knative.dev/v1beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:spec:
        .:
        f:numPartitions:
        f:replicationFactor:
    Manager:      e2e_new.test
    Operation:    Update
    Time:         2023-10-18T20:02:10Z
    API Version:  messaging.knative.dev/v1beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"kafkachannels.messaging.knative.dev":
    Manager:      kafka-controller
    Operation:    Update
    Time:         2023-10-18T20:02:10Z
    API Version:  messaging.knative.dev/v1beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:annotations:
          .:
          f:default.topic:
        f:conditions:
        f:observedGeneration:
    Manager:         kafka-controller
    Operation:       Update
    Subresource:     status
    Time:            2023-10-18T20:02:12Z
  Resource Version:  6455
  UID:               b3748489-f875-4e2e-84d2-11b3d2d1ba68
Spec:
  Num Partitions:      3
  Replication Factor:  1
  Retention Duration:  PT168H
Status:
  Annotations:
    default.topic:  knative-messaging-kafka.test-juolbwtq.channel-hlsrtatz
  Conditions:
    Last Transition Time:  2023-10-18T20:02:11Z
    Status:                Unknown
    Type:                  Addressable
    Last Transition Time:  2023-10-18T20:02:11Z
    Reason:                Config map knative-eventing/kafka-channel-channels-subscriptions updated
    Status:                True
    Type:                  ConfigMapUpdated
    Last Transition Time:  2023-10-18T20:02:11Z
    Status:                True
    Type:                  ConfigParsed
    Last Transition Time:  2023-10-18T20:02:11Z
    Status:                True
    Type:                  DataPlaneAvailable
    Last Transition Time:  2023-10-18T20:02:12Z
    Message:               status: NotReady
    Reason:                ProbeStatus
    Status:                False
    Type:                  ProbeSucceeded
    Last Transition Time:  2023-10-18T20:02:12Z
    Message:               status: NotReady
    Reason:                ProbeStatus
    Status:                False
    Type:                  Ready
    Last Transition Time:  2023-10-18T20:02:11Z
    Reason:                Topic knative-messaging-kafka.test-juolbwtq.channel-hlsrtatz created
    Status:                True
    Type:                  TopicReady
  Observed Generation:     1
Events:
  Type    Reason           Age   From                     Message
  ----    ------           ----  ----                     -------
  Normal  FinalizerUpdate  20m   kafkachannel-controller  Updated "channel-hlsrtatz" finalizers

@pierDipi
Copy link
Member

@Leo6Leo please gather also the "contract configmap" which is the kafka-channel-channels-subscriptions CM data

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Oct 19, 2023

"contract": {
  "ObjectMeta": {
    "Name": "kafka-channel-channels-subscriptions",
    "Namespace": "knative-eventing",
    "UID": "cec3db1b-4cac-4d9b-ba78-a2b58f3f29ab",
    "ResourceVersion": "29375",
    "Generation": 0,
    "CreationTimestamp": "2023-10-19 14:45:29 +0000 UTC",
    "DeletionTimestamp": null,
    "DeletionGracePeriodSeconds": null,
    "Labels": {},
    "Annotations": {},
    "OwnerReferences": [
      {
        "APIVersion": "kafka-controller",
        "Kind": "Update",
        "Name": "v1",
        "CreationTimestamp": "2023-10-19 14:45:29 +0000 UTC",
        "FieldsV1": {
          "f:binaryData": {
            ".": {},
            "f:data": {}
          }
        }
      }
    ]
  },
  "Data": {},
"BinaryData": {
    "data": [
      123,
      34,
      103,
      101,
      110,
...

@pierDipi

@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Oct 20, 2023

@pierDipi The path issue resolved yay!

But.... Another problem occurred:

 apiVersion: messaging.knative.dev/v1
        kind: Subscription
        metadata:
          name: subscription-iryrkaqz
          namespace: test-hxqaiukk
        spec:
          channel:
            apiVersion: null
            kind: null
            name: channel-crpyihmv
          subscriber:
            CACerts: |-
              -----BEGIN CERTIFICATE-----

The spec.channel.apiVersion and spec.channel.kind are null. I added it here but still no value.

I will take a look when I wake up tomorrow.

@Leo6Leo Leo6Leo marked this pull request as ready for review October 20, 2023 15:52
@knative-prow knative-prow bot requested a review from Cali0707 October 20, 2023 15:52
@Leo6Leo Leo6Leo changed the title [WIP] E2E tests for channel: TLS key pair rotation E2E tests for channel: TLS key pair rotation Oct 20, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2023
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

Hey @Leo6Leo great start! I found a couple things to clean up (mostly just leftover logs), and I had one questions where I'm curious why you made a change

control-plane/pkg/prober/prober.go Outdated Show resolved Hide resolved
control-plane/pkg/reconciler/channel/channel.go Outdated Show resolved Hide resolved
control-plane/pkg/reconciler/channel/channel.go Outdated Show resolved Hide resolved
@@ -672,6 +672,7 @@ func (r *Reconciler) getChannelContractResource(ctx context.Context, topic strin
Ingress: &contract.Ingress{
Host: receiver.Host(channel.GetNamespace(), channel.GetName()),
EnableAutoCreateEventTypes: feature.FromContext(ctx).IsEnabled(feature.EvenTypeAutoCreate),
Path: receiver.Path(channel.GetNamespace(), resources.MakeChannelServiceName(channel.GetName())),
Copy link
Member

Choose a reason for hiding this comment

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

@Leo6Leo I'm curious: why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Path is not specified in the dataplane contract. Consequently, when a probing request is transmitted, the receiver at the data-plane is unable to correctly navigate and locate the required resource. @Cali0707

control-plane/pkg/reconciler/channel/resources/service.go Outdated Show resolved Hide resolved
control-plane/pkg/reconciler/channel/resources/service.go Outdated Show resolved Hide resolved
}

// ValidateAddress validates the address retured by Address
func ValidateAddress(name string, validate addressable.ValidateAddress, timings ...time.Duration) feature.StepFn {
Copy link
Member

Choose a reason for hiding this comment

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

I think you might need to look at the change made by @creydr here: c5a0002 for this to work

Leo6Leo and others added 2 commits October 20, 2023 14:23
Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Oct 23, 2023

/retest-required

@pierDipi
Copy link
Member

/retest

@Leo6Leo Leo6Leo requested a review from pierDipi October 26, 2023 05:48
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023
@knative-prow
Copy link

knative-prow bot commented Oct 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Leo6Leo, 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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Oct 26, 2023

/retest-required

1 similar comment
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Oct 26, 2023

/retest-required

@knative-prow knative-prow bot merged commit 828b81e into knative-extensions:main Oct 26, 2023
21 of 22 checks passed
Leo6Leo added a commit to Leo6Leo/eventing-kafka-broker that referenced this pull request Nov 30, 2023
* Save work progress

* Expose the TLS port

* Adding the logger to see what is happening

* Java - Adding the debugging information

* Adding the path to the contract

* Comment out the certificate rotation test portion

* Resolve the source certificate not found issue

* Fix the issue in the test

* Update control-plane/pkg/prober/prober.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/resources/service.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/IngressProducerReconcilableStore.java

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Fix the inconsistent varable name

* Fix the failed build issue

* Remove the logger

* Run formatting

* Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Remove the logger

* Code gen

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Remove the uncessary code

* Fix the failing reconciler tests due to the missing newly added filed in the test

* Format fix

* Instead of using channel service name, we directly use channel name for Path

* Instead of using channel service name, we directly use channel name for Path

---------

Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
knative-prow bot pushed a commit that referenced this pull request Dec 5, 2023
#3406 (#3497)

* use the composite prober with the channel (#3252)

* using the composite prober for the channel

* edit channel_test.go file

* change channel_test.go

* done changes in V2 channel

* edit in controllerv2.go

* edit addresses

* 2nd edit addresses

* Update prober pod port

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update prober pod port

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update prober pod port for TLS

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update prober pod port for TLS

Co-authored-by: Calum Murray <cmurray@redhat.com>

---------

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>

* E2E tests for channel: TLS key pair rotation (#3406)

* Save work progress

* Expose the TLS port

* Adding the logger to see what is happening

* Java - Adding the debugging information

* Adding the path to the contract

* Comment out the certificate rotation test portion

* Resolve the source certificate not found issue

* Fix the issue in the test

* Update control-plane/pkg/prober/prober.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/resources/service.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/IngressProducerReconcilableStore.java

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Fix the inconsistent varable name

* Fix the failed build issue

* Remove the logger

* Run formatting

* Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Remove the logger

* Code gen

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Remove the uncessary code

* Fix the failing reconciler tests due to the missing newly added filed in the test

* Format fix

* Instead of using channel service name, we directly use channel name for Path

* Instead of using channel service name, we directly use channel name for Path

---------

Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Update the function name

---------

Co-authored-by: Rahul kumar <68837569+Rahul-Kumar-prog@users.noreply.github.com>
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
@Leo6Leo
Copy link
Contributor Author

Leo6Leo commented Dec 12, 2023

/cherry-pick release-1.12

@knative-prow-robot
Copy link
Contributor

@Leo6Leo: #3406 failed to apply on top of branch "release-1.12":

Applying: Save work progress
Applying: Expose the TLS port
Applying: Adding the logger to see what is happening
Applying: Java - Adding the debugging information
Applying: Adding the path to the contract
Applying: Comment out the certificate rotation test portion
Applying: Resolve the source certificate not found issue
Applying: Fix the issue in the test
Applying: Update control-plane/pkg/prober/prober.go
Applying: Update control-plane/pkg/reconciler/channel/channel.go
Applying: Update control-plane/pkg/reconciler/channel/channel.go
Applying: Update control-plane/pkg/reconciler/channel/resources/service.go
Applying: Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/IngressProducerReconcilableStore.java
Applying: Fix the inconsistent varable name
Applying: Fix the failed build issue
Applying: Remove the logger
Applying: Run formatting
Applying: Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java
Applying: Remove the logger
Applying: Code gen
Using index info to reconstruct a base tree...
M	data-plane/THIRD-PARTY.txt
Falling back to patching base and 3-way merge...
Auto-merging data-plane/THIRD-PARTY.txt
CONFLICT (content): Merge conflict in data-plane/THIRD-PARTY.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0020 Code gen
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.12

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/test-infra repository.

Leo6Leo added a commit to Leo6Leo/eventing-kafka-broker that referenced this pull request Dec 15, 2023
knative-extensions#3406 (knative-extensions#3497)

* use the composite prober with the channel (knative-extensions#3252)

* using the composite prober for the channel

* edit channel_test.go file

* change channel_test.go

* done changes in V2 channel

* edit in controllerv2.go

* edit addresses

* 2nd edit addresses

* Update prober pod port

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update prober pod port

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update prober pod port for TLS

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update prober pod port for TLS

Co-authored-by: Calum Murray <cmurray@redhat.com>

---------

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>

* E2E tests for channel: TLS key pair rotation (knative-extensions#3406)

* Save work progress

* Expose the TLS port

* Adding the logger to see what is happening

* Java - Adding the debugging information

* Adding the path to the contract

* Comment out the certificate rotation test portion

* Resolve the source certificate not found issue

* Fix the issue in the test

* Update control-plane/pkg/prober/prober.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update control-plane/pkg/reconciler/channel/resources/service.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/IngressProducerReconcilableStore.java

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Fix the inconsistent varable name

* Fix the failed build issue

* Remove the logger

* Run formatting

* Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

Co-authored-by: Calum Murray <cmurray@redhat.com>

* Remove the logger

* Code gen

* Update control-plane/pkg/reconciler/channel/channel.go

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Remove the uncessary code

* Fix the failing reconciler tests due to the missing newly added filed in the test

* Format fix

* Instead of using channel service name, we directly use channel name for Path

* Instead of using channel service name, we directly use channel name for Path

---------

Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Update the function name

---------

Co-authored-by: Rahul kumar <68837569+Rahul-Kumar-prog@users.noreply.github.com>
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
openshift-merge-bot bot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Dec 18, 2023
…ion knative-extensions#3406 (#923)

* Using the composite prober for the sink (knative-extensions#3267)

* Cherry-pick 57c4d1e

* Cherry-pick 82651c9

---------

Co-authored-by: Rahul kumar <68837569+Rahul-Kumar-prog@users.noreply.github.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
knative-prow bot pushed a commit that referenced this pull request Jan 16, 2024
#3406 (#3529)

* Cherry-pick 82651c9

* Update the variable name ValidateAddressible to ValidateAddressibleFn

* Cherry-pick f748a40

* Cherry-pick 57c4d1e

---------

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 area/test 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.

Eventing TLS: E2E tests for Channel TLS key pair rotation
4 participants