-
Notifications
You must be signed in to change notification settings - Fork 135
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
[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404
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()) | ||||
|
||||
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)) | ||||
} | ||||
|
||||
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.
Show resolved
Hide resolved
|
||||
|
||||
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 | ||||
|
||||
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
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 | ||||
}) | ||||
} | ||||
|
||||
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
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.
Show resolved
Hide 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.
Show resolved
Hide resolved
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. 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. 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. 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
g
here 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.