fix: handle nil chain type in FromChainToRulesArray

[riccardotornesello]

Description

When applying a FirewallConfiguration without the type field in the chain, this error occurs:

kubectl apply -f mybrokenrule.yaml
Error from server: error when creating "mybrokenrule.yaml": admission webhook "firewallconfiguration.mutate.liqo.io" denied the request: panic: runtime error: invalid memory address or nil pointer dereference [recovered]

This happens because the mutation webhook is executed before the resource schema is validated, so the functions expect all required fields to have values, even though this is not always the case.

The error is due to the FromChainToRulesArray function, which performs a switch on *chain.Type, which can sometimes be nil.

To resolve the issue, the FromChainToRulesArray function was modified so that it returns an error if chain or chain.Type are nil. It was also necessary to modify the functions that called it in order to propagate the error.

With this update, you will see this message instead:

Error from server: error when creating "examples/riccardo/mybrokenrule.yaml": admission webhook "firewallconfiguration.mutate.liqo.io" denied the request: chain type is required

Error stack trace

This is the error message that appears in the webhook pod logs:

E1002 10:18:38.355118       1 signal_unix.go:925] "Observed a panic" logger="admission" object="test-namespace/test-configuration" namespace="test-namespace" name="test-configuration" resource={"group":"networking.liqo.io","version":"v1beta1","resource":"firewallconfigurations"} user="kubernetes-admin" requestID="d0842d67-7556-45d3-a818-a721c70aabd1" panic="runtime error: invalid memory address or nil pointer dereference" panicGoValue="\"invalid memory address or nil pointer dereference\"" stacktrace=<
        goroutine 273 [running]:
        k8s.io/apimachinery/pkg/util/runtime.logPanic({0x210d4b0, 0xc0007f9710}, {0x1b58ec0, 0x310f080})
                /go/pkg/mod/k8s.io/[email protected]/pkg/util/runtime/runtime.go:107 +0xbc
        sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).Handle.func1()
                /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/webhook/admission/webhook.go:163 +0xf8
        panic({0x1b58ec0?, 0x310f080?})
                /usr/local/go/src/runtime/panic.go:792 +0x132
        github.com/liqotech/liqo/pkg/firewall.FromChainToRulesArray(0xc0000f52d0)
                /tmp/builder/pkg/firewall/chain.go:218 +0x26
        github.com/liqotech/liqo/pkg/webhooks/firewallconfiguration.generateRuleNames({0xc0000f52d0, 0x1, 0x5d6?})
                /tmp/builder/pkg/webhooks/firewallconfiguration/rule.go:65 +0x4a
        github.com/liqotech/liqo/pkg/webhooks/firewallconfiguration.(*webhookMutate).Handle(_, {_, _}, {{{0xc0008d4c30, 0x24}, {{0xc000c90f48, 0x12}, {0xc000b44728, 0x7}, {0xc000c90f60, ...}}, ...}})
                /tmp/builder/pkg/webhooks/firewallconfiguration/firewallconfiguration.go:93 +0x2de
        sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).Handle(_, {_, _}, {{{0xc0008d4c30, 0x24}, {{0xc000c90f48, 0x12}, {0xc000b44728, 0x7}, {0xc000c90f60, ...}}, ...}})
                /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/webhook/admission/webhook.go:181 +0x224
        sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).ServeHTTP(0xc00020fc70, {0x73105191d8b8, 0xc0000fe1e0}, 0xc000aee780)
                /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/webhook/admission/http.go:119 +0xaf0
        sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics.InstrumentedHook.InstrumentHandlerInFlight.func1({0x73105191d8b8, 0xc0000fe1e0}, 0xc000aee780)
                /go/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:60 +0xb6
        net/http.HandlerFunc.ServeHTTP(0x3117af0?, {0x73105191d8b8?, 0xc0000fe1e0?}, 0x67d046?)
                /usr/local/go/src/net/http/server.go:2294 +0x29
        github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1({0x210aed0?, 0xc0005fb420?}, 0xc000aee780)
                /go/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:147 +0xc3
        net/http.HandlerFunc.ServeHTTP(0x1ce3ae0?, {0x210aed0?, 0xc0005fb420?}, 0xc0009efa20?)
                /usr/local/go/src/net/http/server.go:2294 +0x29
        github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerDuration.func2({0x210aed0, 0xc0005fb420}, 0xc000aee780)
                /go/pkg/mod/github.com/prometheus/[email protected]/prometheus/promhttp/instrument_server.go:109 +0xc5
        net/http.HandlerFunc.ServeHTTP(0xc0000ec900?, {0x210aed0?, 0xc0005fb420?}, 0xc0009efb60?)
                /usr/local/go/src/net/http/server.go:2294 +0x29
        net/http.(*ServeMux).ServeHTTP(0x419dc5?, {0x210aed0, 0xc0005fb420}, 0xc000aee780)
                /usr/local/go/src/net/http/server.go:2822 +0x1c4
        net/http.serverHandler.ServeHTTP({0x20f8840?}, {0x210aed0?, 0xc0005fb420?}, 0x1?)
                /usr/local/go/src/net/http/server.go:3301 +0x8e
        net/http.(*conn).serve(0xc0005e02d0, {0x210d4b0, 0xc0003ea510})
                /usr/local/go/src/net/http/server.go:2102 +0x625
        created by net/http.(*Server).Serve in goroutine 102
                /usr/local/go/src/net/http/server.go:3454 +0x485
 >

Test resource

This is the resource used to generate the outputs shown above:

apiVersion: networking.liqo.io/v1beta1
kind: FirewallConfiguration
metadata:
  name: test-configuration
  namespace: test-namespace
  labels:
    liqo.io/managed: "true"
    networking.liqo.io/firewall-category: gateway
    networking.liqo.io/firewall-subcategory: fabric
spec:
  table:
    family: IPV4
    name: test-table
    chains:
      - hook: forward
        name: test-forward-chain
        policy: accept
        priority: 0
        # type: filter

[adamjensenbot]

Hi @riccardotornesello. Thanks for your PR!

I am @adamjensenbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

• /rebase: Rebase this PR onto the master branch (You can add the option test=true to launch the tests
  when the rebase operation is completed)
• /merge: Merge this PR into the master branch
• /build Build Liqo components
• /test Launch the E2E and Unit tests
• /hold, /unhold Add/remove the hold label to prevent merging with /merge

Make sure this PR appears in the liqo changelog, adding one of the following labels:

• feat: 🚀 New Feature
• fix: 🐛 Bug Fix
• refactor: 🧹 Code Refactoring
• docs: 📝 Documentation
• style: 💄 Code Style
• perf: 🐎 Performance Improvement
• test: ✅ Tests
• chore: 🚚 Dependencies Management
• build: 📦 Builds Management
• ci: 👷 CI/CD
• revert: ⏪ Reverts Previous Changes

[riccardotornesello]

/test

[fra98]

Thanks for spotting this @riccardotornesello!

Your PR should definitely fix the issue.
However, I was thinking that a cleaner approach would be to drop the pointer in Type field so that enum validation actually admits only the accepted values. I am not sure if we actually need that field to be unset for some use cases. @cheina97 what do you think?

[riccardotornesello]

Hi @fra98, I have already (partially) tried that solution, but it would not work correctly in my opinion.
By removing the pointer, the JSON parsing function would insert an empty string into that field without returning an error, just as it does now with nil.
A validation error is only returned in the schema validation step, which is performed by Kubernetes after the problematic code is executed: the mutating webhook.
However, in the case of an empty string in the type field, the FromChainToRulesArray function returns the error "unknown chain type" which is not exactly the error for a missing chain type.

[fra98]

Hi @fra98, I have already (partially) tried that solution, but it would not work correctly in my opinion. By removing the pointer, the JSON parsing function would insert an empty string into that field without returning an error, just as it does now with nil. A validation error is only returned in the schema validation step, which is performed by Kubernetes after the problematic code is executed: the mutating webhook. However, in the case of an empty string in the type field, the FromChainToRulesArray function returns the error "unknown chain type" which is not exactly the error for a missing chain type.

You are right that schema validation happens after the mutating webhook run.
What I am saying is that by dropping the pointer, the mutating webhook:

• would not panic (most important thing)
• should not raise errors if we land in "default" case: the mutating webhook simply will not handle it and leave []rules and Type untouched (aka it mutates only what it knows how to handle).

Then the schema validation would correctly deny the resource since the type passed ("") was not among the enums.
IMO Handling incorrect inputs is outside the mutating webhook scope (that is more SchemaValidation and ValidatingWebhook scope).
We can actually drop the entire default case since we would not need to log a warning, since the resource is denied right after, during schema validation.

Of course, all of this holds only if we don't have a valid use case for the user to leave Type unset (nil/"").

What do you think?

[cheina97]

I agree with @fra98. I would simply change that type from a pointer to a string. If the string is empty, we can return a void array and allow the validation webhook to return an error. @riccardotornesello, could you please check if this works?

[riccardotornesello]

Sure @cheina97, I'll take care of that.
If it works, which approach do you prefer? Should I push the changes by overwriting the old commit with a foce push?

[cheina97]

Sure @cheina97, I'll take care of that.
If it works, which approach do you prefer? Should I push the changes by overwriting the old commit with a foce push?

Yep, we usually squash all PR commits in a single commit.

[riccardotornesello]

@fra98 I modified the struct to follow the strategy you suggested.
Now, when no type is passed, the following error is displayed in the output: The FirewallConfiguration "test-resource" is invalid: spec.table.chains[0].type: Unsupported value: "": supported values: "filter", "route", "nat".
The output is exactly that of the validation schema. A small note: compared to the previous strategy, this one required modifying other files as well to replace pass by reference with pass by value.

[fra98]

@riccardotornesello PR looks good (just left a minor comment). Thank you for addressing this!

Unfortunately our e2e testing pipeline is broken at the moment, thus I cannot run tests. Will launch test and hopefully merge when pipeline is fixed