Skip to content
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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gunishmatta
Copy link

@gunishmatta gunishmatta commented Aug 15, 2022

Hi,
Please review and suggest changes.

Thanks

Fixes: #386

@gunishmatta gunishmatta force-pushed the disable-http-scheme-flag branch 2 times, most recently from 94e3f95 to 88e5c43 Compare August 15, 2022 05:47
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
Copy link
Author

@pjbgf I have done the suggested changes.
Please review

@gunishmatta gunishmatta requested a review from pjbgf August 15, 2022 14:12
main.go Outdated Show resolved Hide resolved
@pjbgf
Copy link
Member

pjbgf commented Aug 15, 2022

@gunishmatta please squash your changes into a single commit.

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
@gunishmatta
Copy link
Author

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

Thanks for the review

@stefanprodan
Copy link
Member

stefanprodan commented Aug 16, 2022

@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
Copy link
Member

@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
Copy link
Member

makkes commented Aug 16, 2022

@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
Copy link
Author

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

@makkes
Copy link
Member

makkes commented Aug 23, 2022

@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
Copy link
Author

@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
Copy link
Member

makkes commented Aug 23, 2022

@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
Copy link
Author

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

makkes
makkes previously requested changes Oct 31, 2022
main.go Outdated Show resolved Hide resolved
@stefanprodan 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 stefanprodan added enhancement New feature or request area/alerting Alerting related issues and PRs labels Oct 31, 2022
@gunishmatta
Copy link
Author

@makkes have enabled http by default.

@gunishmatta
Copy link
Author

@pjbgf Please review, have updated the tests

@gunishmatta gunishmatta removed the request for review from makkes November 7, 2022 17:23
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

controllers/event_handling_test.go Outdated Show resolved Hide resolved
@makkes makkes self-requested a review November 11, 2022 07:18
@makkes makkes dismissed their stale review November 11, 2022 07:19

no blocking change requests

@gunishmatta
Copy link
Author

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

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

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Author

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

@gunishmatta gunishmatta force-pushed the disable-http-scheme-flag branch 3 times, most recently from 57727d3 to f81a866 Compare November 14, 2022 12:00
Signed-off-by: Gunish Matta <[email protected]>

Signed-off-by: gunishmatta <[email protected]>
}
for _, eventServerTest := range eventServerTests {
t.Run(eventServerTest.name, func(t *testing.T) {

Copy link
Contributor

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 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 <[email protected]>
pjbgf
pjbgf previously approved these changes Nov 14, 2022
@pjbgf pjbgf dismissed their stale review November 14, 2022 17:54

Dismissing review as I made the last changes.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"name", providerName.Name,
"namespace", providerName.Namespace)
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@pjbgf pjbgf added the hold Issues and pull requests put on hold label Nov 15, 2022
@darkowlzz
Copy link
Contributor

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.

@darkowlzz darkowlzz removed the hold Issues and pull requests put on hold label Feb 9, 2024
@darkowlzz darkowlzz marked this pull request as draft February 9, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a new flag to block use of HTTP endpoints for provider
5 participants