Add support for sending events to HTTPS endpoints in Eventshub by Rahul-Kumar-prog · Pull Request #526 · knative-extensions/reconciler-test

Rahul-Kumar-prog · 2023-05-16T19:17:14Z

Signing-off by : Rahul Kumar rahulkumarrsde@gmail.com

Add support for sending events to HTTPS endpoints

Release Note

Add support for sending events to HTTPS endpoints

Docs

knative-prow · 2023-05-16T19:17:16Z

@Rahul-Kumar-prog: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

Details

In response to this:

Signing-off by : Rahul Kumar rahulkumarrsde@gmail.com

Add support for sending events to HTTPS endpoints

/kind

Fixes #6840

Release Note
Docs

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.

knative-prow · 2023-05-16T19:17:24Z

Hi @Rahul-Kumar-prog. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

pierDipi

we need to solve the requirement number 2 on the issue.
This is just for an idea, in my PR I added another env variable to enforce TLS and the sender would check and enforce TLS regardless of the "is HTTPS sink", see #527

In addition, we need functions to configure the sender with CA Certs, similar to https://github.com/knative-sandbox/reconciler-test/blob/7e6edebb371797610b06b117670ba9bd22b6dbcc/pkg/eventshub/options.go#L62-L94

Rahul-Kumar-prog · 2023-05-19T18:09:15Z

@pierDipi
Let me explain myself whether I understand or not.

step i need to create a function that configure the sender with CA certs.
step is i need to enforce TLS sender would check and enforce TLS regardless of the "is HTTPS sink"
Right and
One question : i have to do both function implementation in sender.go file? Right!

pierDipi · 2023-05-22T09:20:08Z

@pierDipi Let me explain myself whether I understand or not.

step i need to create a function that configure the sender with CA certs.

step is i need to enforce TLS sender would check and enforce TLS regardless of the "is HTTPS sink"
Right and
One question : i have to do both function implementation in sender.go file? Right!

No, I sent you the reference on where to do point 1 (options.go), point 2 is in sender.go

For point 1, we're trying to offer a way to use the new feature you have implemented, when we implement a new feature, we need to think about "how users will use such feature?" and "how are users using similar features?"; that's what point 2 is trying to address, does that make sense?

vishal-chdhry · 2023-05-25T18:10:12Z

Hi @Rahul-Kumar-prog! Do you need any help, are the requirements clear to you?

Rahul-Kumar-prog · 2023-05-26T06:29:17Z

Hey @vishal-chdhry yes I need help. I will contact with you on slack for some doubts. Please have a look

pierDipi · 2023-05-29T08:16:38Z

@Rahul-Kumar-prog do you have any updates on this?

pierDipi · 2023-05-29T08:18:20Z

How can we help you?

Rahul-Kumar-prog · 2023-05-29T13:30:31Z

Hey @pierDipi i have talk to vishal and I got solution just little more time and I will be commit with the solution.

pierDipi · 2023-05-29T15:12:09Z

Thank you both!

pierDipi · 2023-06-06T12:14:17Z

pkg/eventshub/options.go

 }

+func StartsenderTLS(sinkSvc string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "CACerts"), func(ctx context.Context, envs map[string]string) error {


EventGeneratorsEnv set to CACerts is not a valid generator, we need to set this to sender

pierDipi · 2023-06-06T12:14:50Z

pkg/eventshub/options.go


+func StartsenderTLS(sinkSvc string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "CACerts"), func(ctx context.Context, envs map[string]string) error {
+		envs["SINK"] = "https://" + network.GetServiceHostname(sinkSvc, environment.FromContext(ctx).Namespace())


Is SINK env variable enough to make an HTTPS request? do we need CA certs?

pierDipi · 2023-06-06T12:15:10Z

pkg/eventshub/options.go

+		if u == nil {
+			return fmt.Errorf("resource %v named %s is not addressable", gvr, name)
+		}
+		envs["SINK"] = "https://" + u.String()


Is SINK env variable enough to make an HTTPS request? do we need CA certs?

pierDipi · 2023-06-06T12:15:30Z

pkg/eventshub/options.go

+
+func StartSenderURLTLS(sink string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "CACerts"), func(ctx context.Context, envs map[string]string) error {
+		envs["SINK"] = "https://" + sink


Is SINK env variable enough to make an HTTPS request? do we need CA certs?

ohh i miss this sorry?

No need to be sorry

pierDipi · 2023-06-06T12:16:16Z

pkg/eventshub/options.go

+func StartsenderTLS(sinkSvc string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "CACerts"), func(ctx context.Context, envs map[string]string) error {
+		envs["SINK"] = "https://" + network.GetServiceHostname(sinkSvc, environment.FromContext(ctx).Namespace())
+		return nil
+	})
+}
+
+func StartSenderToResourceTLS(gvr schema.GroupVersionResource, name string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "CACerts"), func(ctx context.Context, envs map[string]string) error {
+		u, err := k8s.Address(ctx, gvr, name)
+		if err != nil {
+			return err
+		}
+		if u == nil {
+			return fmt.Errorf("resource %v named %s is not addressable", gvr, name)
+		}
+		envs["SINK"] = "https://" + u.String()
+		return nil
+	})
+}
+
+func StartSenderURLTLS(sink string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "CACerts"), func(ctx context.Context, envs map[string]string) error {
+		envs["SINK"] = "https://" + sink
+		return nil
+	})
+}


For each new *TLS function we need to also set ENFORCE_TLS to true so that eventshub can enforce TLS

var startsender EventsHubOption = envAdditive(EventGeneratorsEnv, "sender") var StartSendertls EventsHubOption = compose(startsender,envAdditive(EnforceTLS, "True"))

like this ?

Have you tried it? the current startSender is different from what you posted above, so I'm not sure that would work

To each compose function we need to add

envAdditive(EnforceTLS, "true")

as argument

i made a commit recently for these changes have you check those @pierDipi

I saw it, I don't think it's correct

pierDipi · 2023-06-06T12:17:36Z

pkg/eventshub/sender/sender.go

+	server := &https.Server{addr: ":8080", Handler: handler}
+	serverTLS := &http.Server{
+		Addr: ":8443",
+		TLSConfig: &tls.Config{
+			MinVersion: tls.VersionTLS12,
+		},
+		Handler: handler,
+	}
+
+	var httpErr error
+	go func() {
+		httpErr = server.ListenAndServe()
+	}()
+	var httpsErr error
+	if env.EnforceTLS {
+		go func() {
+			httpsErr = serverTLS.ListenAndServe("/etc/tls/certificates/tls.crt", "/etc/tls/certifcates/tls.key")
+		}()
+		defer serverTLS.Close()
+	}
+
+	if httpErr != nil {
+		return fmt.Errorf("error while starting the HTTP server: %w", httpErr)
+	}
+	if httpsErr != nil {
+		return fmt.Errorf("error while starting the HTTPS server: %w", httpsErr)
+	}


I don't get why we need the servers, this is a sender, which means it acts as HTTP(s) client

pierDipi · 2023-06-06T12:18:10Z

pkg/eventshub/sender/sender.go

+	}
+
+	httpClient := &nethttp.Client{}
+	if isHTTPSSink(env.Sink) {


Suggested change

if isHTTPSSink(env.Sink) {

if env.EnforceTLS {

pierDipi · 2023-06-06T12:18:57Z

pkg/eventshub/sender/sender.go

+			if isHTTPSSink(env.Sink) {
+				if _, err := httpClient.Do(req); err != nil {
+					logging.FromContext(ctx).Error(zap.Error(err))
+					return false, nil
+				}
+			} else if _, err := nethttp.DefaultClient.Do(req); err != nil {


Suggested change

if isHTTPSSink(env.Sink) {

if _, err := httpClient.Do(req); err != nil {

logging.FromContext(ctx).Error(zap.Error(err))

return false, nil

}

} else if _, err := nethttp.DefaultClient.Do(req); err != nil {

if _, err := httpClient.Do(req); err != nil {

pierDipi · 2023-06-06T12:19:13Z

pkg/eventshub/sender/sender.go

+		return fmt.Errorf("error while starting the HTTPS server: %w", httpsErr)
+	}
+
+	httpClient := &nethttp.Client{}


Suggested change

httpClient := &nethttp.Client{}

httpClient := nethttp.DefaultClient

Rahul-Kumar-prog · 2023-06-07T10:06:06Z

@pierDipi have a look.

pierDipi · 2023-06-12T12:09:58Z

pkg/eventshub/options.go

+var Startsender EventsHubOption = envAdditive(EventGeneratorsEnv, "sender")
+


Suggested change

var Startsender EventsHubOption = envAdditive(EventGeneratorsEnv, "sender")

We don't need this, there is already StartSender

pierDipi · 2023-06-12T12:13:44Z

pkg/eventshub/options.go

+// StartSendertls starts the sender in the eventshub with TLS enforcement.
+// It requires cert-manager operator to be able to create TLS Certificate.const
+// To get the CA certificate used you can use GetCAcerts
+var StartSendertls EventsHubOption = compose(Startsender, envAdditive(EnforceTLS, "true"))


Suggested change

// StartSendertls starts the sender in the eventshub with TLS enforcement.

// It requires cert-manager operator to be able to create TLS Certificate.const

// To get the CA certificate used you can use GetCAcerts

var StartSendertls EventsHubOption = compose(Startsender, envAdditive(EnforceTLS, "true"))

// StartSenderTLS starts the sender in the eventshub with TLS enforcement.

func StartSenderTLS(sinkSvc string, caCerts *caCerts) EventsHubOption {

return compose(StartSender(sinkSvc), envAdditive(EnforceTLS, "true"), func(ctx context.Context, envs map[string]string) error {

if caCerts != nil {

envs["CA_CERTS"] = *caCerts

}

return nil

})

}

This is the idea for StartSenderTLS function

pierDipi · 2023-06-12T12:16:35Z

pkg/eventshub/options.go


+func StartSenderTLS(sinkSvc string) EventsHubOption {
+	return compose(envAdditive(EventGeneratorsEnv, "sender"), func(ctx context.Context, envs map[string]string) error {
+		envs["CACerts"] = "https://" + network.GetServiceHostname(sinkSvc, environment.FromContext(ctx).Namespace())


I left a comment earlier to challenge CA certs set to a URL, if we set CACerts env variabe to a URL, would that work for the updated code you've implmented in pkg/eventshub/sender/sender.go? is that the expected behavior?

I think so. what you say?

how would that work if CACerts is not used anywhere?

given the PR change:

// CACert is the certificate for enabling HTTPS in Sink URL CACerts string `envconfig:"CA_CERTS"` // EnforceTLS is used to enforce TLS. EnforceTLS bool `envconfig:"ENFORCE_TLS" default:"false"`

I don't see where CACerts is used, nor how setting CACerts to an HTTP address has a meaning.

In addition, the PR is setting CACerts to a URL that should be set as SINK.

See this comment: #526 (comment)

@pierDipi You mean in startsendertls function I have to set env as sink right ?
If we set env as a sink then how can start sender tls with Cacerts ?

pierDipi · 2023-06-16T08:46:17Z

@Rahul-Kumar-prog this is blocking progress on bunch of other tasks, so we need to move forward.

are the task and comments clear?
by when can we get a solution done?
if you don't have capacity because you're busy, that's ok, we can have other contributors taking your initial great work and bring it to completion, but please be clear about that

Rahul-Kumar-prog · 2023-06-18T17:45:38Z

Sorry @pierDipi i get sick past few days. I understand your comments. Give me some time I will push a commit with the solution/changes.

And again sorry for blocking

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-06-30T08:13:31Z

@Rahul-Kumar-prog I've sent a PR to your branch Rahul-Kumar-prog#1, please merge it

pierDipi · 2023-06-30T14:36:45Z

/ok-to-test

pierDipi

/lgtm
/approve

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-07-04T13:33:47Z

/lgtm
/approve

knative-prow · 2023-07-04T13:33:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, Rahul-Kumar-prog

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:

~~OWNERS~~ [pierDipi]

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

Manual update since there are a few braking chanages with knative-extensions/reconciler-test#526 first commit upgrade deps and vendor, the rest fixes the issues. --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Add support for sending events to HTTPS endpoints in Eventshub

82ddf68

knative-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 16, 2023

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 16, 2023

pierDipi mentioned this pull request May 17, 2023

Support receiving events to HTTPS endpoints in eventshub #527

Merged

pierDipi reviewed May 19, 2023

View reviewed changes

add Enforce TLS support

774e58d

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2023

pierDipi reviewed Jun 6, 2023

View reviewed changes

modify Enforce TLS support

c2a1c4a

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2023

Changes in Enforce TLS support

e7498e2

pierDipi reviewed Jun 12, 2023

View reviewed changes

Rahul-Kumar-prog force-pushed the Rahulkumar6840 branch 2 times, most recently from 81ac3b1 to e7498e2 Compare June 21, 2023 18:26

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2023

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 21, 2023

Merge branch 'main' into Rahulkumar6840

10a3ac4

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2023

Implement all StartSenderTLS options and TLS enforcement

5fe6ab0

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi mentioned this pull request Jun 30, 2023

Implement all StartSenderTLS options and TLS enforcement Rahul-Kumar-prog/reconciler-test#1

Merged

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2023

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 30, 2023

pierDipi approved these changes Jun 30, 2023

View reviewed changes

knative-prow bot assigned pierDipi Jun 30, 2023

knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 30, 2023

Avoid using URI resolver since it doesn't work

e091333

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi force-pushed the Rahulkumar6840 branch from 85315e7 to e091333 Compare July 4, 2023 12:58

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2023

Merge branch 'main' into sending-events-tls

6c12b5c

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi force-pushed the Rahulkumar6840 branch from c15f9fc to 6c12b5c Compare July 4, 2023 13:12

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

knative-prow bot merged commit 4c1226f into knative-extensions:main Jul 4, 2023

This was referenced Jul 4, 2023

Upgrade rekt to latest knative/eventing#7076

Merged

Eventing TLS: support sending events to HTTPS endpoints in eventshub knative/eventing#6840

Closed

	httpClient := &nethttp.Client{}
	httpClient := nethttp.DefaultClient

		var Startsender EventsHubOption = envAdditive(EventGeneratorsEnv, "sender")

Conversation

Rahul-Kumar-prog commented May 16, 2023 • edited by pierDipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow bot commented May 16, 2023

Uh oh!

knative-prow bot commented May 16, 2023

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

Rahul-Kumar-prog commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierDipi commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishal-chdhry commented May 25, 2023

Uh oh!

Rahul-Kumar-prog commented May 26, 2023

Uh oh!

pierDipi commented May 29, 2023

Uh oh!

pierDipi commented May 29, 2023

Uh oh!

Rahul-Kumar-prog commented May 29, 2023

Uh oh!

pierDipi commented May 29, 2023

Uh oh!

pierDipi Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rahul-Kumar-prog commented Jun 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Rahul-Kumar-prog commented May 16, 2023 •

edited by pierDipi

Loading

Rahul-Kumar-prog commented May 19, 2023 •

edited

Loading

pierDipi commented May 22, 2023 •

edited

Loading

pierDipi Jun 6, 2023 •

edited

Loading

pierDipi Jun 12, 2023 •

edited

Loading

pierDipi Jun 12, 2023 •

edited

Loading

pierDipi Jun 20, 2023 •

edited

Loading

Rahul-Kumar-prog Jun 20, 2023 •

edited

Loading

pierDipi commented Jun 16, 2023 •

edited

Loading

Rahul-Kumar-prog commented Jun 18, 2023 •

edited

Loading