-
Notifications
You must be signed in to change notification settings - Fork 163
[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ import ( | |||
| "fmt" | ||||
| "net/http" | ||||
| "net/http/httptest" | ||||
| "net/url" | ||||
| "testing" | ||||
| "time" | ||||
|
|
||||
|
|
@@ -52,7 +53,69 @@ func TestEventHandler(t *testing.T) { | |||
| t.Fatalf("failed to create memory storage") | ||||
| } | ||||
|
|
||||
| eventServer := server.NewEventServer("127.0.0.1:56789", logf.Log, k8sClient, true) | ||||
| httpScheme := "http" | ||||
|
|
||||
| eventServerTests := []struct { | ||||
| name string | ||||
| isHttpEnabled bool | ||||
| url string | ||||
| }{ | ||||
| { | ||||
| name: "http scheme is enabled", | ||||
| isHttpEnabled: true, | ||||
| }, { | ||||
| name: "http scheme is disabled", | ||||
| isHttpEnabled: false, | ||||
| }, | ||||
| } | ||||
| for _, eventServerTest := range eventServerTests { | ||||
| t.Run(eventServerTest.name, func(t *testing.T) { | ||||
|
|
||||
| eventServer := server.NewEventServer("127.0.0.1:56789", logf.Log, k8sClient, true, eventServerTest.isHttpEnabled) | ||||
|
|
||||
| stopCh := make(chan struct{}) | ||||
| go eventServer.ListenAndServe(stopCh, eventMdlw, store) | ||||
| requestsReceived := 0 | ||||
| rcvServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||||
| requestsReceived = requestsReceived + 1 | ||||
| req = r | ||||
| w.WriteHeader(200) | ||||
| })) | ||||
| defer rcvServer.Close() | ||||
| defer close(stopCh) | ||||
|
|
||||
| providerKey := types.NamespacedName{ | ||||
| Name: fmt.Sprintf("provider-%s", randStringRunes(5)), | ||||
| Namespace: namespace, | ||||
| } | ||||
| provider = ¬ifyv1.Provider{ | ||||
| ObjectMeta: metav1.ObjectMeta{ | ||||
| Name: providerKey.Name, | ||||
| Namespace: providerKey.Namespace, | ||||
| }, | ||||
| Spec: notifyv1.ProviderSpec{ | ||||
| Type: "generic", | ||||
| Address: rcvServer.URL, | ||||
| }, | ||||
| } | ||||
|
|
||||
| webhook_url, err := url.Parse(provider.Spec.Address) | ||||
|
|
||||
| g.Expect(err).ToNot(HaveOccurred()) | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| if eventServerTest.isHttpEnabled { | ||||
| g.Expect(webhook_url.Scheme).To(Equal(httpScheme)) | ||||
| g.Expect(requestsReceived).To(Equal(1)) | ||||
| } else { | ||||
| g.Expect(webhook_url.Scheme).ToNot(Equal(httpScheme)) | ||||
| g.Expect(requestsReceived).To(Equal(0)) | ||||
| } | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| }) | ||||
| } | ||||
|
|
||||
| eventServer := server.NewEventServer("127.0.0.1:56789", logf.Log, k8sClient, true, true) | ||||
|
gunishmatta marked this conversation as resolved.
Outdated
|
||||
|
|
||||
| stopCh := make(chan struct{}) | ||||
| go eventServer.ListenAndServe(stopCh, eventMdlw, store) | ||||
|
|
||||
|
|
@@ -77,6 +140,9 @@ func TestEventHandler(t *testing.T) { | |||
| Address: rcvServer.URL, | ||||
| }, | ||||
| } | ||||
|
|
||||
| g.Expect(err).ToNot(HaveOccurred()) | ||||
|
|
||||
| g.Expect(k8sClient.Create(context.Background(), provider)).To(Succeed()) | ||||
| g.Eventually(func() bool { | ||||
| var obj notifyv1.Provider | ||||
|
|
@@ -173,6 +239,7 @@ func TestEventHandler(t *testing.T) { | |||
| res, err := http.Post("http://localhost:56789/", "application/json", buf) | ||||
| g.Expect(err).ToNot(HaveOccurred()) | ||||
| g.Expect(res.StatusCode).To(Equal(202)) // event_server responds with 202 Accepted | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot to remove the newline? |
||||
| } | ||||
|
|
||||
| testForwarded := func() { | ||||
|
|
@@ -294,4 +361,5 @@ func TestEventHandler(t *testing.T) { | |||
| req = nil | ||||
| }) | ||||
| } | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot to remove the newline? |
||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ import ( | |
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/url" | ||
| "regexp" | ||
| "strings" | ||
| "time" | ||
|
|
@@ -243,6 +244,22 @@ func (s *EventServer) handleEvent() func(w http.ResponseWriter, r *http.Request) | |
| continue | ||
| } | ||
|
|
||
| webhookUrl, err := url.Parse(webhook) | ||
| if err != nil { | ||
| s.logger.Error(nil, "Error parsing webhook url", | ||
| "reconciler kind", v1beta1.ProviderKind, | ||
| "name", providerName.Name, | ||
| "namespace", providerName.Namespace) | ||
| continue | ||
| } | ||
|
gunishmatta marked this conversation as resolved.
|
||
|
|
||
| if !s.supportHttpScheme && webhookUrl.Scheme == "http" { | ||
| s.logger.Error(nil, "http scheme is blocked", | ||
| "reconciler kind", v1beta1.ProviderKind, | ||
| "name", providerName.Name, | ||
| "namespace", providerName.Namespace) | ||
| continue | ||
| } | ||
|
gunishmatta marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having reviewed #435, I think we can implement this block in a different way such that it becomes more apparent to the user that their Provider and Alert won't work.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I propose we delay this PR until v1beta2 is released. If RFC-0004 instructs the objects to marked as stalled, then we'll probably need to add secret watches to NC and cascade stalling from Provider to all dependant Alerts.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gunishmatta unfortunately, we will have to label these changes on hold until the #435 is merged, by which point we will need to review the implementation based on @darkowlzz comments above.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No Worries, Will keep following it and would focus on contributing to other issues at Flux and understanding the code in depth. |
||
| factory := notifier.NewFactory(webhook, proxy, username, provider.Spec.Channel, token, headers, certPool, password) | ||
| sender, err := factory.Notifier(provider.Spec.Type) | ||
| if err != nil { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please initialize a new
ghere before usingg.Expect, as it's a subtest.Refer https://github.com/fluxcd/pkg/blob/c6f9759287b14231463a30d7ff8e416e53984a60/git/internal/e2e/gitlab_test.go#L110 for an example.