✨ implement CAPI IPAM contract support

[schrej]

What this PR does / why we need it:
Adds support for the CAPI IPAM contract.

To do so, the network related specs of Metal3DataTemplate were adapted to allow referencing other types of Pools than just the one provided by metal3-io/ip-address-manager. Depending on the referenced Pool type, the corresponding controller will create the correct IP claim resource.
For the metadata section, the reference was simply extended with new fields that allow to specify the api group and resource type.
For the network section a new field FromPoolRef was added, which can be used as an alternative to "IPAddressFromPool". The new field also supports references to the ip-address-manager Pools.

Since both allocation mechanisms are very similar, the implementation is fairly close as well.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Resolves #671

[metal3-io-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[schrej]

/test all

[schrej]

/assign @furkatgofurov7

[furkatgofurov7]

/assign @furkatgofurov7

👍🏼 I'll take a closer look tomorrow

[furkatgofurov7]

Thanks for this work @schrej, mostly nits for the first iteration.
I can understand why this PR keeps the allocation logic of Metal3 IPAM as "default" and provides CAPI way as an alternative since it paves the way for smoother migration, but since our current CI is still not adapted to the new way of IPAM support, I wonder if have you already tested it and could share your thoughts on that regard?

[schrej]

I did some manual testing with the in-cluster provider and it works well.
I didn't test backwards compatibility yet, but existing e2e tests should be easy enough to adapt to that. I'll also try to work on some e2e tests for the CAPI contract. For the CAPV provider they're still waiting for a new clusterctl version that supports provisioning ipam providers (the in-cluster provider is compatible already).

[furkatgofurov7]

I did some manual testing with the in-cluster provider and it works well.

Great!

I didn't test backwards compatibility yet, but existing e2e tests should be easy enough to adapt to that. I'll also try to work on some e2e tests for the CAPI contract.

That would be nice to have to make sure we have a CI coverage for the new support. Let me know if I can help anyway with that.

For the CAPV provider they're still waiting for a new clusterctl version that supports provisioning ipam providers (the in-cluster provider is compatible already).

Which version of clusterctl specifically? do you mean v1.3,x?

[schrej]

Which version of clusterctl specifically? do you mean v1.3,x?

Yes! Release is planned for Thursday (1.12.), so not really a blocker.

[lentzi90]

Nice! I like the unit tests!

One thing I'm wondering though is if this would require an API version change? It should be backwards compatible, but since we are adding new fields I'm not sure if we can continue with v1beta1.

[furkatgofurov7]

@schrej do you still have time to take a look and fix the build job on this PR?

[schrej]

Hey @furkatgofurov7, sorry for the late response, I was on PTO the last few days. I'll update this PR as soon as I find time. I was struggling with e2e tests, it would be great if someone could assist with that! Maybe we can merge this and work on e2e tests in a separate PR.

[furkatgofurov7]

Hey @furkatgofurov7, sorry for the late response, I was on PTO the last few days. I'll update this PR as soon as I find time.

Hi @schrej , thanks for working on this and no worries, just wanted to check in if this is on track and if there is anything we can help!

I was struggling with e2e tests, it would be great if someone could assist with that! Maybe we can merge this and work on e2e tests in a separate PR.

Anything specific we can help with e2e tests? I am fine to have a follow-up to deal with e2e tests as well.

[schrej]

Tests should be good now.

Anything specific we can help with e2e tests? I am fine to have a follow-up to deal with e2e tests as well.

I think I was mainly struggling with undestanding how they work in general, and then finding a reasonable test to extend with CAPI IPAM. There doesn't seem to be any basic "deploy a cluster and make sure it works" test, or I'm just not seeing it...

[lentzi90]

I think I was mainly struggling with undestanding how they work in general, and then finding a reasonable test to extend with CAPI IPAM. There doesn't seem to be any basic "deploy a cluster and make sure it works" test, or I'm just not seeing it...

This is a good point. It is weird and surprising that the "least" test we do is a clusterctl move. I'll add an issue for an explicit "create cluster and run smoke test". Now that I think about it, the integration test that we have should probably be this. We anyway test clusterctl move in most of the other e2e tests so why would it need to be in the basic integration test?

[furkatgofurov7]

There doesn't seem to be any basic "deploy a cluster and make sure it works" test, or I'm just not seeing it...

Right, most if not all of the e2e tests we have feature specific and we are missing the simple base testing e2e tests.

[furkatgofurov7]

We anyway test clusterctl move in most of the other e2e tests so why would it need to be in the basic integration test?

indeed, but since e2e tests are quite new and when pivoting was introduced we did not have them at all, so we decided to add it as part of integration tests? /cc @kashifest

[lentzi90]

indeed, but since e2e tests are quite new and when pivoting was introduced we did not have them at all, so we decided to add it as part of integration tests? /cc @kashifest

Let's take this discussion in the issue instead #897

[schrej]

Unfortunately these references need to be pointers, otherwise the API server complains when the reference is empty.

[mboukhalfa]

In addition it might be good to add conditions regarding IP Address allocation on m3d, and a condition on m3m (if that's not there) indicating that it is waiting on data, so it's clearer why provisioning is stalling. Might be worth to analyse a bit more broadly and come up with more conditions on all of the resources as well.

We have recently added Metal3DataReadyCondition to m3m object in #836 not sure if still needs similar on m3d would be great if described in an issue.

[mboukhalfa]

/assign

[mboukhalfa]

Thanks I am approving the PR just small nit on the way

/hold you can unhold once you think it is ready to go in

[kashifest]

@schrej whats the status of CAPI IPAM btw?

[schrej]

One provider is available, which is very similar to metal3-io/ip-address-management and provides in-cluster IPAM: telekom/cluster-api-ipam-provider-in-cluster
We (= VMware) are currently working on moving the repository to kubernetes-sigs.

I've also started to work on a Infoblox provider. I'm currently trying to figure out how to handle hostnames, since Infoblox can provide DNS as well, which allows to have automatic DNS records for all machines. I'll probably open an issue on CAPI level at some point.

Upstream the resources are still experimental and receive occasional changes. Right now we're discussing whether Gateway should be optional: kubernetes-sigs/cluster-api#8536

[schrej]

/unhold

[lentzi90]

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

[lentzi90]

Thanks for this, it looks solid to me!
/lgtm

[schrej]

I think that one is still flaking
/test-centos-e2e-integration-main

[mboukhalfa]

/approve

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboukhalfa

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 [mboukhalfa]

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

[mboukhalfa]

One provider is available, which is very similar to metal3-io/ip-address-management and provides in-cluster IPAM: telekom/cluster-api-ipam-provider-in-cluster We (= VMware) are currently working on moving the repository to kubernetes-sigs.

I've also started to work on a Infoblox provider. I'm currently trying to figure out how to handle hostnames, since Infoblox can provide DNS as well, which allows to have automatic DNS records for all machines. I'll probably open an issue on CAPI level at some point.

Upstream the resources are still experimental and receive occasional changes. Right now we're discussing whether Gateway should be optional: kubernetes-sigs/cluster-api#8536

Would be nice to follow this on some issue on capm3 and set some todo plan for integration tests with capi ipam once it is upstream now we only tested with m3ipam right ?

[schrej]

Would be nice to follow this on some issue on capm3 and set some todo plan for integration tests with capi ipam once it is upstream now we only tested with m3ipam right ?

I've opened an issue to track it, as promised in some comment here.

The IPAM contract is upstream, it's just in the exp package still, marking it as experimental. It basically makes it easier to change the API. If we change anything relevant, I'll either open a PR or an issue here to let you know.