[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404

gunishmatta · 2022-08-15T05:32:36Z

Hi,
Please review and suggest changes.

Thanks

Fixes: #386

main.go

internal/server/event_server.go

internal/server/event_handlers.go

gunishmatta · 2022-08-15T07:46:12Z

@pjbgf I have done the suggested changes.
Please review

main.go

pjbgf · 2022-08-15T15:01:08Z

@gunishmatta please squash your changes into a single commit.

main.go

internal/server/event_handlers.go

gunishmatta · 2022-08-16T12:22:31Z

@makkes I will work on the suggested changes by you and will update the PR

Thanks for the review

stefanprodan · 2022-08-16T12:35:49Z

@pjbgf @makkes I'm not Ok with breaking Flux Grafana integration by disabling http by default. If you think this is acceptable please create an RFC.

stefanprodan · 2022-08-16T12:40:05Z

@gunishmatta please hold working on this, a decision to break Flux for all users is not something that can be taken lightly in a PR, an RFC is needed.

makkes · 2022-08-16T12:53:54Z

@pjbgf @makkes I'm not Ok with breaking Flux Grafana integration by disabling http by default. If you think this is acceptable please create an RFC.

My line of thought is that #404 (comment) recommends setting the default to true for backwards-compatibility to explicitly not break any existing integration.

gunishmatta · 2022-08-23T08:10:10Z

@makkes @stefanprodan just wanted to check if we can go ahead with making this flag disabled by default?

makkes · 2022-08-23T08:16:23Z

@makkes @stefanprodan just wanted to check if we can go ahead with making this flag disabled by default?

No, I don't think we can without further discussion. Disabling plain HTTP by default would break Grafana integration as @stefanprodan pointed out. An RFC would be needed.

gunishmatta · 2022-08-23T08:26:38Z

@makkes @stefanprodan just wanted to check if we can go ahead with making this flag disabled by default?

No, I don't think we can without further discussion. Disabling plain HTTP by default would break Grafana integration as @stefanprodan pointed out. An RFC would be needed.

Sorry, what I mean is releasing with enabling http scheme by default.
Anyways, please let me know whatever would be the update I will be doing it accordingly.

Thanks

makkes · 2022-08-23T08:54:05Z

@makkes @stefanprodan just wanted to check if we can go ahead with making this flag disabled by default?

No, I don't think we can without further discussion. Disabling plain HTTP by default would break Grafana integration as @stefanprodan pointed out. An RFC would be needed.

Sorry, what I mean is releasing with enabling http scheme by default. Anyways, please let me know whatever would be the update I will be doing it accordingly.

If all comments from my and @pjbgf's reviews are addressed, this should be fine to get merged.

gunishmatta · 2022-10-31T07:01:01Z

@pjbgf I have addressed all the changes requested above, Please review and merge if everything is okay

main.go

gunishmatta · 2022-10-31T12:40:27Z

@makkes have enabled http by default.

controllers/event_handling_test.go

gunishmatta · 2022-11-07T12:52:24Z

@pjbgf Please review, have updated the tests

controllers/event_handling_test.go

makkes

Other than the unnecessary condition this looks good to me now.

controllers/event_handling_test.go

no blocking change requests

gunishmatta · 2022-11-13T17:29:53Z

Other than the unnecessary condition this looks good to me now.

Done this optional change too, Please merge if everything is okay

controllers/event_handling_test.go

pjbgf

@gunishmatta thank you for working on this. Once the change above is made, squash your changes to a single commit please and we should be good to merge the PR.

gunishmatta · 2022-11-14T11:11:07Z

Hi @pjbgf , I have squashed all commits into one and it was an amazing experience contributing to Flux, Also since I am new to Golang, thanks for patiently reviewing all my changes in PR.
I would love to contribute to Flux in future too.

Thanks everyone @makkes @stefanprodan

Signed-off-by: Gunish Matta <gunishmatta@gmail.com> Signed-off-by: gunishmatta <gunishmatta@gmail.com>

darkowlzz · 2022-11-14T15:28:03Z

controllers/event_handling_test.go

+	}
+	for _, eventServerTest := range eventServerTests {
+		t.Run(eventServerTest.name, func(t *testing.T) {
+


Please initialize a new g here before using g.Expect, as it's a subtest.
Refer https://github.com/fluxcd/pkg/blob/c6f9759287b14231463a30d7ff8e416e53984a60/git/internal/e2e/gitlab_test.go#L110 for an example.

- Use noopstore to disable throttling behaviour. - Fake k8s client to remove need of interacting with an envtest apiserver. - Replace HTTP Status Code magic numbers, with their respective constants. Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>

Dismissing review as I made the last changes.

darkowlzz · 2022-11-15T09:46:42Z

internal/server/event_server_test.go

 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/kubectl/pkg/scheme"


We should be able to get rid of this new dependency by using k8s.io/apimachinery/pkg/runtime to create a new scheme. Refer

notification-controller/internal/server/receiver_handler_test.go

Line 272 in 610fd63

scheme := runtime.NewScheme()

darkowlzz · 2022-11-15T10:00:25Z

internal/server/event_handlers.go

+					"name", providerName.Name,
+					"namespace", providerName.Namespace)
+				continue
+			}


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.
At the beginning of this function, handleEvent(), all the alerts are listed and alerts are matched against the event. While doing so, the alerts are checked to be Ready. Not ready alerts are ignored.
In Provider reconciler, we can parse the address of the webhook and mark the object as stalled, as per the RFC https://github.com/fluxcd/flux2/tree/main/rfcs/0004-insecure-http#design-details and Ready=False. But if the address is specified in a secret ref, the address in the secret can change without updating the Provider object. So, when secretRef is present, we can just fail the reconciliation with Ready=False and allow it to retry with exponential backoff.
Because the Provider is not ready, the associated Alert would also become not ready and that intern would make the event handler to drop the event early in the above function. The failure in the configuration would be visible on the object itself, compared to the current implementation where it'll be just logged and may not be visible.

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.

@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.

No Worries, Will keep following it and would focus on contributing to other issues at Flux and understanding the code in depth.

darkowlzz · 2024-02-09T13:29:37Z

This PR should no longer be on hold. A lot has changed since the last discussion. With notification-controller v1.2 released recently, the reconcilers for the Provider has been removed, simplifying the implementation. Disabling insecure HTTP connection should be implementable in the event handler based on the given configureation.

@gunishmatta sorry for a long hold. Since it has been a long time, if you can no longer work on this, we can make this available for others to implement or carry it forward.

Converting the PR to a draft for now as it'll require a lot of changes before it's ready for review.

gunishmatta force-pushed the disable-http-scheme-flag branch 2 times, most recently from 94e3f95 to 88e5c43 Compare August 15, 2022 05:47

pjbgf requested changes Aug 15, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

internal/server/event_server.go Outdated Show resolved Hide resolved

internal/server/event_handlers.go Outdated Show resolved Hide resolved

gunishmatta force-pushed the disable-http-scheme-flag branch from 9c03a45 to 8470951 Compare August 15, 2022 07:40

gunishmatta requested a review from pjbgf August 15, 2022 14:12

pjbgf reviewed Aug 15, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

makkes requested changes Aug 16, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

internal/server/event_handlers.go Show resolved Hide resolved

internal/server/event_handlers.go Show resolved Hide resolved

pjbgf mentioned this pull request Sep 6, 2022

RFC - Consolidate use of insecure endpoints across controllers fluxcd/flux2#3076

Closed

gunishmatta force-pushed the disable-http-scheme-flag branch from 549a300 to cedf3c2 Compare October 30, 2022 19:06

makkes previously requested changes Oct 31, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

stefanprodan changed the title ~~flag added to Disable http scheme for providers~~ [RFC-0004] Allow disabling of insecure HTTP connections for alert providers Oct 31, 2022

stefanprodan added enhancement New feature or request area/alerting Alerting related issues and PRs labels Oct 31, 2022

gunishmatta requested a review from makkes October 31, 2022 12:55

pjbgf reviewed Oct 31, 2022

View reviewed changes

controllers/event_handling_test.go Outdated Show resolved Hide resolved

gunishmatta force-pushed the disable-http-scheme-flag branch from e0e3dd1 to 0c1485d Compare November 7, 2022 12:49

gunishmatta removed the request for review from makkes November 7, 2022 17:23

pjbgf reviewed Nov 10, 2022

View reviewed changes

controllers/event_handling_test.go Outdated Show resolved Hide resolved

gunishmatta force-pushed the disable-http-scheme-flag branch from 084d443 to 1207eea Compare November 11, 2022 04:24

gunishmatta requested a review from pjbgf November 11, 2022 04:38

makkes reviewed Nov 11, 2022

View reviewed changes

controllers/event_handling_test.go Outdated Show resolved Hide resolved

makkes self-requested a review November 11, 2022 07:18

pjbgf reviewed Nov 14, 2022

View reviewed changes

controllers/event_handling_test.go Outdated Show resolved Hide resolved

pjbgf requested changes Nov 14, 2022

View reviewed changes

gunishmatta force-pushed the disable-http-scheme-flag branch from b2c1262 to c5cacf2 Compare November 14, 2022 11:06

gunishmatta force-pushed the disable-http-scheme-flag branch 3 times, most recently from 57727d3 to f81a866 Compare November 14, 2022 12:00

Add support for blocking HTTP connections

8ea5d6c

Signed-off-by: Gunish Matta <gunishmatta@gmail.com> Signed-off-by: gunishmatta <gunishmatta@gmail.com>

gunishmatta force-pushed the disable-http-scheme-flag branch from f81a866 to 8ea5d6c Compare November 14, 2022 12:01

makkes approved these changes Nov 14, 2022

View reviewed changes

darkowlzz reviewed Nov 14, 2022

View reviewed changes

pjbgf requested review from makkes and darkowlzz November 14, 2022 17:09

pjbgf force-pushed the disable-http-scheme-flag branch from 5c72e84 to 5a06288 Compare November 14, 2022 17:12

pjbgf previously approved these changes Nov 14, 2022

View reviewed changes

darkowlzz reviewed Nov 15, 2022

View reviewed changes

pjbgf added the hold Issues and pull requests put on hold label Nov 15, 2022

darkowlzz removed the hold Issues and pull requests put on hold label Feb 9, 2024

darkowlzz marked this pull request as draft February 9, 2024 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404

[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404

gunishmatta commented Aug 15, 2022 •

edited by pjbgf

gunishmatta commented Aug 15, 2022

pjbgf commented Aug 15, 2022

gunishmatta commented Aug 16, 2022

stefanprodan commented Aug 16, 2022 •

edited

stefanprodan commented Aug 16, 2022

makkes commented Aug 16, 2022

gunishmatta commented Aug 23, 2022

makkes commented Aug 23, 2022

gunishmatta commented Aug 23, 2022

makkes commented Aug 23, 2022

gunishmatta commented Oct 31, 2022

gunishmatta commented Oct 31, 2022

gunishmatta commented Nov 7, 2022

makkes left a comment

gunishmatta commented Nov 13, 2022

pjbgf left a comment

gunishmatta commented Nov 14, 2022

darkowlzz Nov 14, 2022

darkowlzz Nov 15, 2022

darkowlzz Nov 15, 2022

stefanprodan Nov 15, 2022

pjbgf Nov 15, 2022

gunishmatta Nov 15, 2022

darkowlzz commented Feb 9, 2024

[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404

Are you sure you want to change the base?

[RFC-0004] Allow disabling of insecure HTTP connections for alert providers #404

Conversation

gunishmatta commented Aug 15, 2022 • edited by pjbgf

gunishmatta commented Aug 15, 2022

pjbgf commented Aug 15, 2022

gunishmatta commented Aug 16, 2022

stefanprodan commented Aug 16, 2022 • edited

stefanprodan commented Aug 16, 2022

makkes commented Aug 16, 2022

gunishmatta commented Aug 23, 2022

makkes commented Aug 23, 2022

gunishmatta commented Aug 23, 2022

makkes commented Aug 23, 2022

gunishmatta commented Oct 31, 2022

gunishmatta commented Oct 31, 2022

gunishmatta commented Nov 7, 2022

makkes left a comment

Choose a reason for hiding this comment

gunishmatta commented Nov 13, 2022

pjbgf left a comment

Choose a reason for hiding this comment

gunishmatta commented Nov 14, 2022

darkowlzz Nov 14, 2022

Choose a reason for hiding this comment

darkowlzz Nov 15, 2022

Choose a reason for hiding this comment

darkowlzz Nov 15, 2022

Choose a reason for hiding this comment

stefanprodan Nov 15, 2022

Choose a reason for hiding this comment

pjbgf Nov 15, 2022

Choose a reason for hiding this comment

gunishmatta Nov 15, 2022

Choose a reason for hiding this comment

darkowlzz commented Feb 9, 2024

gunishmatta commented Aug 15, 2022 •

edited by pjbgf

stefanprodan commented Aug 16, 2022 •

edited