📖 CAEP: Add support for infrastructure cluster resources to be managed externally

[enxebre]

What this PR does / why we need it:

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

[caruccio]

Great!!! Looking forward to putting my Pentium back again in the game.

btw: s/introudce/introduce/

[randomvariable]

Thanks for this. Maybe a little more detail required. Just make sure if there's any new terms, refer to the glossary, and use personas already listed there as best suits.

[randomvariable]

I think we need to have a section that deals with single controller multitenancy, and in addition modify the multitenancy contract doc in #4074 within this PR.

[enxebre]

Not sure how this would affect single controller multitenancy? In such a model privileges are given for each infraResource so this would be transparent right? can you elaborate a bit?

[randomvariable]

Suggested change

	- The provider infraCluster controller will skip any infrastructure reconciliation.
	- The provider infraCluster controller will skip any infrastructure reconciliation with the exception of read-only queries to ensure the operation of the related cloud provider integration, e.g. required tags for working of the AWS Cloud Provider..

[enxebre]

@randomvariable I know we discuss about this but would you mind refreshing my mind where specifically you see the need to read?

[JoelSpeed]

IIRC there are some requirements for the cloud-controller-manager that the AWSCluster controller verifies at the moment. I think checking that tags have been set on the VPC correctly was one of them?

I think it's reasonable that if a InfraCluster controller normally would perform validation of the environment it has created as a requirement for parts of core K8s to function, then we should also do this validation on an unmanaged cluster.

Though, we should make sure the expectations of the environment are documented well so users can ensure their infrastructure provisioning can set the right tags etc.

[enxebre]

cloud-controller-manager that the AWSCluster controller verifies at the moment.

@JoelSpeed @randomvariable I see, we have a list of this somewhere atm?

[randomvariable]

Another thing I think we should go into more details is why the unmanaged field is useful based on our discussions - that we need information from the infraCluster object in order to inform the infraMachine controller what to do without a lot of repetition. Given our discussions, maybe walk through the example with the CAPA controller. A flow diagram may also be useful.

[enxebre]

Another thing I think we should go into more details is why the unmanaged field is useful based on our discussions - that we need information from the infraCluster object in order to inform the infraMachine controller what to do without a lot of repetition. Given our discussions, maybe walk through the example with the CAPA controller. A flow diagram may also be useful.

@randomvariable You mean the info inferred from the infraCluster when managed as a reason to reuse the CR to support unmanaged?

[enxebre]

Open questions:
@randomvariable can you confirm which tags would you expect to be validated #4135 (comment)

@vincepri #4135 (comment) isn't this what the existing unmanaged VPC approach does https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/0987c8d9f16a4f891d05a622aa92b4592ad5c154/api/v1alpha3/types.go#L244 but we'd need to teach it to not reconcile sg and LBs, hence we'd use the externalManagedField for this, i.e the contract for an externalManaged AWSCluster is the vpc ID.

Could you please provide specific details of what are you missing from the proposal in its current form?

[vincepri]

@enxebre I've outlined most of the thinking in #4135 (comment) — Yes there is going to be contract changes for the InfraCluster controllers, although those can be made generic based on a signal (annotation) that's common across all infrastructure providers.

[vincepri]

Hi folks, a few of us met today to discuss some details of this proposal, and wanted to provide a summary and outline next step.

The proposal is focused only on getting the InfrastructureCluster bits to be externally managed, while keeping the infrastructure machine controller intact, at least for now.

A few considerations / things to decide:

• Identify that an InfraCluster is externally managed.

  • Use an annotation
  • Use a spec field
  • Use a different CRD / Kind resource that included all fields from the current one.

• Readiness and contracts.

  • Have external managers / controllers to respect the Cluster API contracts.
  • Have an additional annotation, or spec field that marks the resource as ready.
    • Open question: How do we deal with new contracts as they come in? (example, failure domains)

[enxebre]

@vincepri @randomvariable @CecileRobertMichon @detiber @fabriziopandini do you have any additional feedback after latest changes pushed?

Also can anyone clarify why couldn't we just reuse the existing unmanagedVPC and teach it to not reconcile the lb when the field is not set and SGs as suggested above?

[vincepri]

Also can anyone clarify why couldn't we just reuse the existing unmanagedVPC and teach it to not reconcile the lb when the field is not set and SGs as suggested above?

This point should probably be tackled in the load balancer proposal later, cc @randomvariable @detiber

[fabriziopandini]

What about adding this as a requirement for the external management system up in the document?

[JoelSpeed]

My only concern with making this a requirement of the external management is that we will have to duplicate that across each external system right? If this is a core requirement of this contract and the check just needs to be, if this annotation is present, don't let it be changed, that feels like it could be part of the core validation?

I guess we would need to implement this for each provider anyway so maybe not much is lost 🤔

[fabriziopandini]

TBH I would prefer to lust this as a responsibility of the External providers, because I don't see how this can be centralised given that we are talking about InfraCluster objects and one of the of the goal of this KEP is to avoid changes to the existing infrastructure providers.
If instead you can come out with a solution that centralises this responsibility, +1 to that

[JoelSpeed]

I was wondering about a generic validating webhook that could be implemented to look at the metadata only (therefore could accept any type), this code could be provided centrally and generic, but providers or external infrastructure providers would need to set it up to look at the correct type

Ie all they would need to do is configure the ValidatingWebhookConfiguration correctly. Do you think something like this would work/be useful?

[fabriziopandini]

Assuming that this requires all the infrastructure providers to reconcile the externally managed infra clusters, I'm +1 to remove this option from the list

[JoelSpeed]

I think there's value in keeping it so we know it's been discussed, but maybe we can expand on this later to explain why we don't want to go down this route?

[fabriziopandini]

ok to keep to as a record, but please add a note reporting the outcome of the discussion.

[CecileRobertMichon]

Has this been done?

[sbueringer]

I would also prefer to not implement this option as it's basically additional complexity in ClusterAPI because of a (current) limitation of kubectl which will probably be resolved mid-term: [WIP] kubectl: support --subresource flag #99556

[vincepri]

/kind proposal

[CecileRobertMichon]

@enxebre are we waiting for any reviewer's lgtm?

[CecileRobertMichon]

(checked from the list of reviewers in the PR)

/assign @yastij @vincepri

[vincepri]

Action item: Add some text on the decision about how do we manage or integrate with load balancer proposal

[JoelSpeed]

I've re-reviewed the load balancer proposal today and 'm not sure there is anything worth noting in this proposal with regards to it.

My evaluation from #4135 (comment) still stands.

• Load balancers are optional, and would take precedence if they are defined.
• Load balancer provider reads from InfraClusters but doesn't write to them (as per the proposal today), as such, given we state the external infra must fill all the required fields, nothing to note here
• The LB proposal doesn't mention making any changes to InfraClusters at present, nothing for us to account for here

The only thing I found, was that the Load Balancer proposal should (I don't think it does yet) specify some mechanism for signalling to InfraCluster providers that they should not be creating a control plane LB as this is handled separately. However that manifests (either by some annotation or changing the InfraCluster contract to check the Cluster resource or whatever), it will mean updating the InfraCluster contract documentation, which this proposal already notes that external infra providers must fulfil, so I don't think it is worth calling out explicitly.

@detiber I would appreciate if you could have a double check of my evaluation in case I've misinterpreted the LB proposal or missed some of the details

@vincepri Was there some specific interaction you were concerned about that I might not have had in mind while reviewing the two proposals? I can double check or provide further context if there are specific concerns, but at the moment I don't see real overlap here given the ordering and precedence set out in the LB proposal

[randomvariable]

Broadly agree with the approach.

I think @JoelSpeed 's comments are correct here.

+1 on proceeding with this.

[vincepri]

@vincepri Was there some specific interaction you were concerned about that I might not have had in mind while reviewing the two proposals? I can double check or provide further context if there are specific concerns, but at the moment I don't see real overlap here given the ordering and precedence set out in the LB proposal

I think we're good to proceed, the explanation above seems in line with my assumptions, although it's good to have a sounding board.

[vincepri]

/lgtm
/approve

/hold
1 week lazy-consensus

[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

[yastij]

/lgtm

[enxebre]

Let's merge this and proceed with the implementation since lazy consensus week is over today and does not seem to be any objection.
Thanks a lot everyone for the reviews.
/hold cancel