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

[gunishmatta]

Hi, Please review and suggest changes.

Thanks

Fixes: https://github.com/fluxcd/notification-controller/issues/386

[gunishmatta]

@pjbgf I have done the suggested changes. Please review

[pjbgf]

@gunishmatta please squash your changes into a single commit.

[gunishmatta]

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

Thanks for the review

[stefanprodan]

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

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

@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 https://github.com/fluxcd/notification-controller/pull/404#discussion_r945833752 recommends setting the default to true for backwards-compatibility to explicitly not break any existing integration.

[gunishmatta]

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

[makkes]

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

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

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

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

[gunishmatta]

@makkes have enabled http by default.

[gunishmatta]

@pjbgf Please review, have updated the tests

[gunishmatta]

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

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

[gunishmatta]

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

[darkowlzz]

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.