Skip to content

Bump all Knative go-deps to version 1.10#2207

Merged
openshift-merge-robot merged 1 commit intoopenshift-knative:mainfrom
ReToCode:bump-code-1-30
Aug 11, 2023
Merged

Bump all Knative go-deps to version 1.10#2207
openshift-merge-robot merged 1 commit intoopenshift-knative:mainfrom
ReToCode:bump-code-1-30

Conversation

@ReToCode
Copy link
Copy Markdown
Contributor

@ReToCode ReToCode commented Aug 3, 2023

Proposed Changes

  • Bump all Knative go-deps to version 1.10
  • Update hack/patches/005-conversion-webhook.patch to reflect changes in 1.10
  • Drop hack/patches/013-rekt-test-add-service-port-name.patch because it is already in 1.10
  • Update hack/patches/remove_eventing_ns_manifest.patch to reflect changes in 1.10
  • Use subdir for ingress manifests based on Change file structure for ingresses knative/operator#1355

@openshift-ci openshift-ci bot requested review from alanfx and mgencur August 3, 2023 07:54
@openshift-ci openshift-ci bot added the approved label Aug 3, 2023
@openshift-ci openshift-ci bot requested review from matzew, pierDipi and skonto August 3, 2023 07:56
@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 3, 2023

@pierDipi Hm, this does not work anymore, as Addressable with CACerts is no longer assignable to URL.

@nak3
Copy link
Copy Markdown
Contributor

nak3 commented Aug 3, 2023

@skonto @nak3 are webhook patches ok like this? Outcome: https://github.com/openshift-knative/serverless-operator/pull/2207/files#diff-4ade17300ba49da3fe4bcf7ff0e67471ae89f4938d3d926895c0efaa7943c41bR44

Okay to me but I guess Stavros is working on #2148 so defer to him.

@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 4, 2023

error=Get "https://10.0.128.94:10250/containerLogs/ci-op-p6d50wjt/upstream-e2e-aws-ocp-413-serverless-e2e/test": x509: certificate signed by unknown authority

/retest

@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 4, 2023

/retest

@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 7, 2023

/retest

1 similar comment
@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 7, 2023

/retest

@@ -49,7 +49,7 @@ spec:
spec:
containers:
- name: heartbeats
image: registry.ci.openshift.org/openshift/knative-v1.8:knative-eventing-test-heartbeats
image: ko://knative.dev/eventing/cmd/heartbeats
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that's a wrong reference here, we don't do the ko:// this should be proper in midstream....

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 8, 2023

In the job:
pull-ci-openshift-knative-serverless-operator-main-4.13-operator-e2e-aws-ocp-413

I see:

  - Match errors -
        0 - Kn-Namespace header not found
        1 - Kn-Namespace header not found
        2 - Kn-Namespace header not found
        3 - Kn-Namespace header not found

https://prow.ci.openshift.org/log?container=test&id=1688863651936604160&job=pull-ci-openshift-knative-serverless-operator-main-4.13-operator-e2e-aws-ocp-413

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 8, 2023

@mgencur is that above Kn-Namespace related to this?
#2195

(or some env var, for "istio" (or not istio)?

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 8, 2023

/test 4.13-upstream-e2e-kafka-aws-ocp-413

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 8, 2023

@mgencur is that above Kn-Namespace related to this?
#2195

I don't think it's related. The test is obviously updated in this PR so it expects the KnHeader. All the other tests are passing (channel-related ones etc. which expect the KnHeader as well). So, the Knative Forwarder component should be forwarding the header correctly. I also checked the image of Ping source adapter and it's version 1.10 which is OK.
I don't know why this error happens.

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 8, 2023

@matzew I'd probably try to remove this line to see if the problem is in the forwarder. If the test passes the problem is in forwarder, if it still fails it must be in PingSource.

@creydr
Copy link
Copy Markdown
Contributor

creydr commented Aug 8, 2023

In the PingSource we add since 1.9 the Kn-Namespace header (openshift-knative/eventing#265). Not sure, if this header gets forwarded in the forwarder too

@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 9, 2023

Trying without the forwarder to see if this is the issue.

/hold

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 9, 2023

+++ b/go.mod
@@ -184,8 +184,8 @@ replace (
 	knative.dev/eventing-kafka-broker => github.com/openshift-knative/eventing-kafka-broker v0.25.1-0.20230803130431-bf8f4e608e85
 	knative.dev/hack => knative.dev/hack v0.0.0-20230417170854-f591fea109b3
 	knative.dev/networking => knative.dev/networking v0.0.0-20230419144338-e5d04e805e50
- 	knative.dev/reconciler-test => knative.dev/reconciler-test v0.0.0-202[30](https://github.com/openshift-knative/serverless-operator/actions/runs/5806025450/job/15738118955?pr=2207#step:10:31)808072333-158379f56e0b
 	knative.dev/pkg => knative.dev/pkg v0.0.0-20230418073056-dfad48eaa5d0
+	knative.dev/reconciler-test => knative.dev/reconciler-test v0.0.0-20230808072333-158379f56e0b
 	knative.dev/serving => github.com/openshift-knative/serving v0.10.1-0.20230721070749-1af735104[33](https://github.com/openshift-knative/serverless-operator/actions/runs/5806025450/job/15738118955?pr=2207#step:10:34)0
 )

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 9, 2023

OK. So it seems the issue is in forwarder...

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 9, 2023

Yeah. Looking at the logs from the forwarder:

Received event:

{"level":"info","ts":"2023-08-08T11:29:00.203Z","logger":"eventshub.event logger","caller":"logger_vent/logger.go:26","msg":"Event: \n-- EventInfo --\n--- Kind: Received ---\n--- Event ---\nContext Attributes,\n  specversion: 1.0\n  type: dev.knative.sources.ping\n  source: /apis/v1/namespaces/test-znrhfwtv/pingsources/pingsource-hdicxhyy\n  id: 7edb6ea3-31ea-4dbb-a5d6-62aaa9ea5bff\n  time: 2023-08-08T11:29:00.081538462Z\n\n--- HTTP headers ---\n  X-Request-Id: 56a28edf-d5bb-4e49-bd1c-9a0dd08b9b1b\n  X-B3-Spanid: d0d1dcc37b90469a\n  X-B3-Traceid: d626695bb5c8ad09af015327faa16bb3\n  K-Proxy-Request: activator\n  X-B3-Sampled: 1\n  User-Agent: Go-http-client/1.1\n  Host: sink-bvacvnzb.test-znrhfwtv.svc.cluster.local\n  Content-Length: 0\n  Traceparent: 00-d626695bb5c8ad09af015327faa16bb3-d0d1dcc37b90469a-01\n  Accept-Encoding: gzip\n  Kn-Namespace: test-znrhfwtv\n  X-Forwarded-Proto: http\n  Forwarded: for=10.131.0.23;proto=http\n  X-Forwarded-For: 10.131.0.23, 10.129.2.23\n\n--- Origin: '127.0.0.1:44260' ---\n--- Observer: 'sink-bvacvnzb' ---\n--- Time: 2023-08-08 11:29:00.203898196 +0000 UTC m=+106.857147156 ---\n--- Sequence: 0 ---\n--- Sent Id:  ' ---\n--------------------\n"}

Sent/forwarded event:

{"level":"info","ts":"2023-08-08T11:29:00.212Z","logger":"eventshub.event logger","caller":"logger_vent/logger.go:26","msg":"Event: \n-- EventInfo --\n--- Kind: Sent ---\n--- Event ---\nContext Attributes,\n  specversion: 1.0\n  type: dev.knative.sources.ping\n  source: /apis/v1/namespaces/test-znrhfwtv/pingsources/pingsource-hdicxhyy\n  id: 7edb6ea3-31ea-4dbb-a5d6-62aaa9ea5bff\n  time: 2023-08-08T11:29:00.081538462Z\n\n--- HTTP headers ---\n  Ce-Id: 7edb6ea3-31ea-4dbb-a5d6-62aaa9ea5bff\n  Ce-Source: /apis/v1/namespaces/test-znrhfwtv/pingsources/pingsource-hdicxhyy\n  Ce-Specversion: 1.0\n  Ce-Type: dev.knative.sources.ping\n  Ce-Time: 2023-08-08T11:29:00.081538462Z\n\n--- Origin: 'sink-bvacvnzb' ---\n--- Observer: 'sink-bvacvnzb' ---\n--- Time: 2023-08-08 11:29:00.212682216 +0000 UTC m=+106.865931174 ---\n--- Sequence: 0 ---\n--- Sent Id:  '7edb6ea3-31ea-4dbb-a5d6-62aaa9ea5bff ---\n--------------------\n"}

The kn-namespace header is not forwarded.

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 9, 2023

I will work on the fix for the forwarder.

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 9, 2023

Would this be acceptable? knative-extensions/reconciler-test#572
I still need to test it with real deployment but I think it should work.

@ReToCode
Copy link
Copy Markdown
Contributor Author

ReToCode commented Aug 9, 2023

Would this be acceptable? knative-extensions/reconciler-test#572

Deferring this to Eventing folks @matzew , @creydr . Just out of curiosity, is there a specific reason the forwarder filters headers in the first place?

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 9, 2023

Deferring this to Eventing folks. Just out of curiosity, is there a specific reason the forwarder filters headers in the first place?

No specific reason. Forwarding of headers was just not implemented, probably by mistake.

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 9, 2023

No specific reason. Forwarding of headers was just not implemented, probably by mistake.

should it just be transparent? forward, as is - and not edit/manipulate the actual req?

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Aug 9, 2023

Apparently, something like this works as well: knative-extensions/reconciler-test#575

@@ -1 +1,2 @@
knative.dev/reconciler-test/cmd/eventshub: registry.ci.openshift.org/openshift/knative-eventing-test-eventshub:knative-nightly
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it right that you still use knative-nightly here? Should that be v1.10?
I think you said nightly cannot be used anymore. Just curious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I think my comments were only related to the code deps from main branch because of a conflict with the changes to Adressable upstream. We should be fine with that image (I think..., let's see).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK. Anyway, the backports helped for multiarch builds here: 3863526
We should probably use this image as well here so that we have the multiarch image from the beginning and no need to patch later like in the commit above.
Also, the version will change with each release so we probably need to create a template for this file and generate the resulting file automatically. But this can be done in a follow-up. I'd suggest using the quay image in this file for both eventshub and heartbeats. The latest available heartbeats image at quay is v1.9 though: quay.io/openshift-knative/eventing/heartbeats:v1.9 That's because the image was moved from test images to core images in 1.10 and the github action is not building it (this needs a fix!)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, it's probably better to use quay images later, not in this PR. There's more work to be done. Let's keep using registry.ci in this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 11, 2023

/test ?

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Aug 11, 2023

@matzew: The following commands are available to trigger required jobs:

  • /test 4.10-images
  • /test 4.10-operator-e2e-aws-ocp-410
  • /test 4.10-ui-tests
  • /test 4.13-aws-ovn-images
  • /test 4.13-azure-images
  • /test 4.13-gcp-images
  • /test 4.13-hypershift-images
  • /test 4.13-images
  • /test 4.13-operator-e2e-aws-ocp-413
  • /test 4.13-osd-images
  • /test 4.13-single-node-images
  • /test 4.13-ui-tests
  • /test 4.13-vsphere-images
  • /test ocp4.14-lp-interop-images
  • /test unit-test

The following commands are available to trigger optional jobs:

  • /test 4.10-e2e-kitchensink-ocp-410
  • /test 4.10-upgrade-kitchensink-ocp-410
  • /test 4.10-upgrade-tests-aws-ocp-410
  • /test 4.10-upstream-e2e-aws-ocp-410
  • /test 4.10-upstream-e2e-kafka-aws-ocp-410
  • /test 4.10-upstream-e2e-mesh-aws-ocp-410
  • /test 4.13-e2e-kitchensink-ocp-413
  • /test 4.13-upgrade-kitchensink-ocp-413
  • /test 4.13-upgrade-tests-aws-ocp-413
  • /test 4.13-upstream-e2e-aws-ocp-413
  • /test 4.13-upstream-e2e-kafka-aws-ocp-413
  • /test 4.13-upstream-e2e-mesh-aws-ocp-413

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-knative-serverless-operator-main-4.10-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-aws-ovn-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-azure-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-gcp-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-hypershift-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-operator-e2e-aws-ocp-413
  • pull-ci-openshift-knative-serverless-operator-main-4.13-osd-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-single-node-images
  • pull-ci-openshift-knative-serverless-operator-main-4.13-upgrade-tests-aws-ocp-413
  • pull-ci-openshift-knative-serverless-operator-main-4.13-upstream-e2e-aws-ocp-413
  • pull-ci-openshift-knative-serverless-operator-main-4.13-upstream-e2e-kafka-aws-ocp-413
  • pull-ci-openshift-knative-serverless-operator-main-4.13-vsphere-images
  • pull-ci-openshift-knative-serverless-operator-main-ocp4.14-lp-interop-images
  • pull-ci-openshift-knative-serverless-operator-main-unit-test
Details

In response to this:

/test ?

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.

@matzew
Copy link
Copy Markdown
Member

matzew commented Aug 11, 2023

It passed. testing it again

/test 4.13-operator-e2e-aws-ocp-413

Copy link
Copy Markdown
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

/hold until the mentioned e2e test passes

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Aug 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, ReToCode

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@ReToCode
Copy link
Copy Markdown
Contributor Author

Yay 🎉

/unhold

@ReToCode
Copy link
Copy Markdown
Contributor Author

/unhold

@openshift-merge-robot openshift-merge-robot merged commit 20fdaf8 into openshift-knative:main Aug 11, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Aug 11, 2023

/test remaining-required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants