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

Recreate firewall on unhealthy condition #63

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Honigeintopf
Copy link
Collaborator

@Honigeintopf Honigeintopf commented Nov 4, 2024

Description

Closes #62.

This pr introduces the functionality for deleting firewalls if they exceed the firewallHealthTimeout which for now is set to 20 minutes.
Integration tests where added to make sure everything works as intended.

CA were updated, otherwise it is not possible to deploy to mini-lab.

@Honigeintopf Honigeintopf requested a review from a team as a code owner November 4, 2024 14:31
@Honigeintopf Honigeintopf linked an issue Nov 4, 2024 that may be closed by this pull request
@Honigeintopf Honigeintopf changed the title Firewall health check Firewall delete on unhealthy condition Nov 4, 2024
@Honigeintopf Honigeintopf requested a review from Gerrit91 November 4, 2024 14:33
@Gerrit91 Gerrit91 changed the title Firewall delete on unhealthy condition Recreate firewall on unhealthy condition Nov 4, 2024
Copy link
Contributor

@Gerrit91 Gerrit91 left a comment

Choose a reason for hiding this comment

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

Thanks for coming up with a PR for this.

controllers/set/delete.go Show resolved Hide resolved
controllers/set/status.go Outdated Show resolved Hide resolved
var result []*v2.Firewall

for _, fw := range fws {
fw := fw
if c.isFirewallUnhealthy(fw) {
Copy link
Contributor

@Gerrit91 Gerrit91 Nov 6, 2024

Choose a reason for hiding this comment

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

With some small changes on your newly introduced struct I think something like this would be clearer and really point out that this is about timeouts:

status := evaluateFirewallConditions(fw)

switch {
case status.CreateTimeout || status.HealthTimeout:
    r.Log.Info("firewall health or creation timeout exceeded, deleting from set", "firewall-name", fw.Name)

    err := c.deleteFirewalls(r, fw)
    if err != nil {
        return nil, err
    }

    result = append(result, fw)
}

The isProgressing that's used in setStatus would also not be required anymore as it can be derived in case all other cases do not match.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it, what do you think now?

controllers/set/status.go Outdated Show resolved Hide resolved
controllers/set/status.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firewall health check
3 participants