Amend existing IPAM contract to include a summary condition on IPAddressClaim CRs

[srm09]

What would you like to be added (User Story)?

As a consumer for the IPAddressClaim CRs, the infrastructure providers would want to query the state of the CR and make some programmatic decisions based on the current state. This issue proposes that we establish a contract for the IPAM providers to expose a summary condition on the IPAddressClaim CRs which can then be consumed by the owner infra machines.

Detailed Description

To outline how the infrastructure providers consume the CRs, here is an example:

1. An infra provider creates a IPAddressClaim objects for each address required by the infra machine object.
2. The infra provider waits for all the IPAddressClaim objects to be realized by the IPAM provider. Realizing an IPAddressClaim means that a concrete IPAddress has been assigned to the claim.
3. Once, all the IP address claims have been realized, the infra provider moves ahead with the machine provisioning logic.

There might be some lag between creating the IPAddressClaim objects and them being realized by the IPAM provider. For cases, when the IP pool has been exhausted, the claims would not be realized until an existing IP address (tied to another IPAddressClaim object) is removed or the user changes the pool definition to include a larger set of IP addresses.

These scenarios make it necessary for the IPAM providers to expose the current state of the IPAddressClaim objects. This state would then be used by the infra provider machine to expose a meaningful condition showcasing the current state of affairs for the infra machine object.

Anything else you would like to add?

A request is to be able to backport these contract changes to the v1.4.0 release as well, since the IPAM API types are experimental and the IPAM providers are being actively enhanced for ease of use.

Label(s) to be applied

/kind feature
/area networking

[srm09]

/retitle Amend existing IPAM contract to include a summary condition on IPAddressClaim CRs

[killianmuldoon]

Question - is there a current definition of the "Contract" for the IPAddressClaim CRs? If not is this more of a new contract idea?

[srm09]

There is no contract definition documented in CAPI for the IPAM CRs. The idea is to create an initial draft based on the merged IPAM proposal and then add on a thing or two about making the summary condition mandatory on these CRs, which is not a huge departure from the original proposal itself.

[fabriziopandini]

/triage accepted
+1 to document the contract.

WRT to the proposed change, this will not require changes in CAPI, so the only blocker to make it happen is getting an agreement between folks managing the first implementations of this provider
cc @schrej

(please loop in other folks working in this area)

[schrej]

We've discussed that a bit already. My proposal would be to to merely standardise the name of that condition (Ready would be the most idiomatic one) and that it only needs to be set as false during error cases.
Setting the Ready condition to true should be optional imo, since that status is also reflected by the IPAddress resource existing, and the reference in the status being set.

We should also define specific reasons for an invalid status, like PoolExhausted or AllocationFailed.

While we're at it, we could also think about standardised conditions of IP pools to have a similar UX with different providers.

[tylerschultz]

IPAddressClaim should also consider making Status.AddressRef optional so that Status Conditions can be set when an IPAddress allocation fails.

[sbueringer]

So we're basically talking about introducing new constants to the IPAM api package + a contract describing how they are used?

I'm in favor of opening a PR to get consensus and then cherry picking. It seems reasonable to me to add new constants to an alpha package and it feels unreasonable to me to block a contact for an alpha feature until we bump our "CAPI global" contract to v1beta2.

[schrej]

/area ipam

[fabriziopandini]

/priority important-longterm

[fabriziopandini]

@schrej is this something still useful? what are the next steps?

[k8s-ci-robot]

This issue is currently awaiting triage.

CAPI contributors will take a look as soon as possible, apply one of the triage/* labels and provide further guidance.

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-sigs/prow repository.

[schrej]

I think it's still useful. I agree with Stefan that we should just get consensus using a PR, I'll open one.

[schrej]

/close
since the last thing we've kept this open for is now merged 🎉

[k8s-ci-robot]

@schrej: Closing this issue.

In response to this:

/close
since the last thing we've kept this open for is now merged 🎉

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-sigs/prow repository.