🐛 ipam: fix gateway being required for IPAddress

[schrej]

What this PR does / why we need it:
The validating webhook for IPAddress ensures that .spec.gateway is a valid IP address. This check fails if the field is empty, making it a required field. Since it is defined as omitempty it should be optional.

This PR wraps it with an is-empty check to allow the field to be empty.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
related to kubernetes-sigs/cluster-api-ipam-provider-in-cluster#70
Fixes #8536

[killianmuldoon]

/lgtm

I don't have any expertise in the IPAM provider, but this change seems to make sense.

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b1c1c5010355868c45260ded50512732577c365f

[schrej]

there is an alternative implementation here, that makes gateway required for ipv4, and allows it to be optional with ipv6: #8525
We should decide which one we want to use.

/hold

[sbueringer]

@schrej can you maybe open an issue for discussion? Then we can close/open PRs according to consensus

[sbueringer]

@schrej let's add "Fixes #8536" to the PR description

[schrej]

/unhold

[sbueringer]

/lgtm

Maybe you want to do a quick manual test somewhere to see if it works as expected (as we don't have e2e test coverage with IPAM)

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 4118c42282f5fa0f41d6c98d36851a0e5a1d7f55

[killianmuldoon]

Maybe you want to do a quick manual test somewhere to see if it works as expected (as we don't have e2e test coverage with IPAM)

Maybe a stupid question - really don't know much about the IPAM implementations - how difficult would it be to set up an e2e test for this API? Could we create a simplified provider or use an existing one?

[schrej]

Maybe you want to do a quick manual test somewhere to see if it works as expected (as we don't have e2e test coverage with IPAM)

Maybe a stupid question - really don't know much about the IPAM implementations - how difficult would it be to set up an e2e test for this API? Could we create a simplified provider or use an existing one?

Depends on what's considered end-to-end for this API at this level. Imo we need to ensure that the two IPAM resources work as expected/defined, so IPAM and infra providers can rely on it.

If we want to include the apiserver in the scope, we could use envtest - but I don't know if and how webhooks can be tested with that. For now we have unit tests for what those webhooks do.
I don't see benefit in having a simplified provider (or using an existing one) since CAPI doesn't implement any behaviour for those resources. It just specifies how the resources look like, and we describe how they are supposed to be used. But it's on ipam providers to implement and test that.

The ipam providers should of course have e2e tests with these resources (we currently do not, since envtest doesn't run webhooks). Same goes for infra providers that want to support the contract.

[vincepri]

/approve

Based on the discussion in the related issue

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[christianang]

I'd be interested in this being backported to release 1.4 if we could.

[vincepri]

/cherry-pick release-1.4

[k8s-infra-cherrypick-robot]

@vincepri: cannot checkout release-0.14: error checking out release-0.14: exit status 1. output: error: pathspec 'release-0.14' did not match any file(s) known to git

In response to this:

/cherry-pick release-0.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-infra-cherrypick-robot]

@vincepri: new pull request created: #8574

In response to this:

/cherry-pick release-1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[johannesfrey]

/area ipam