✨ enable BYO IP for control plane #652

alexeldeib · 2020-05-28T02:09:06Z

What this PR does / why we need it:
Enables users to bring their own IP for the control plane. This allows using a CAPI control plane with self-managed nodes by setting advertise address on KCP at create-time.

Defaults to previous behavior if unset. Updating this field is invalid.

Special notes for your reviewer:
Demo for Azure edge scenarios

Release note:

Enable users to pre-provision the control plane IP so it may be used e.g. in advertise address.

k8s-ci-robot · 2020-05-28T02:09:07Z

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

alexeldeib · 2020-05-28T02:09:17Z

/test all

alexeldeib · 2020-05-28T02:21:36Z

/test all

alexeldeib · 2020-05-28T03:51:13Z

/test pull-cluster-api-provider-azure-e2e

devigned · 2020-05-28T16:13:16Z

I think BYOR (bring your own resource) is common across almost all resources. What do folks think about allowing the ProviderID to be set by a user, and the controller would use the ProviderID being set on an unowned resources as the indication the resource should be a BYO?

MirzaSikander · 2020-06-08T22:25:42Z

Can we get this pull request to completion? Need this functionality to move forward with our deployment.

alexeldeib · 2020-06-08T23:08:35Z

I think at the very least, this PR needs to add a concept of ownership so that CAPZ doesn't delete IPs brought by the user and does not fail to cleanup IPs it creates.

We've discussed this as a general issue a bit, @devigned @CecileRobertMichon any thoughts on how you think it should look for this particular use case?

@MirzaSikander feel free to take over and drive this PR if you need the functionality sooner as mentioned offline

MirzaSikander · 2020-06-08T23:20:27Z

For us, deleting the IP address is actually preferred so its not an issue for us. We can always extend it later to optionally delete it.

devigned · 2020-06-09T15:37:44Z

Could we tag managed pubIPs with "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": "owned" (similar ownership indicator), and just check for that upon delete? That seems like this would deal with the ownership problem for this instance.

In the longer term, I don't know that we want to use only resource name as the way we look up resources. Using resource ID would be more precise. Moreover, I'm worried that if we pull this in quickly, then decide on a generic way of accepting "bring your own" resources, then this could be a painful migration in the next api version.

CecileRobertMichon · 2020-06-09T17:18:54Z

/cc @vincepri @detiber is this a use case that has come up for other CAPI providers?

MirzaSikander · 2020-06-10T09:58:47Z

Just wanted to provide our context and use case for this feature.

We are team in Azure that is responsible for managing bare-metal infra in locations external to Azure DC aka the edge. We are in the process of layering k8s on top of our bare-metal infra to allow containerized apps to be deployed and managed. Our current topology involves the control plane deployed in Azure managing worker nodes on a private network in the edge. The master has a public IP address through which the workers are able to join the cluster. We wanted to transition over to CAPZ to avoid manually managing these clusters (Only the control plane as of now).

For things to work properly we need to specify the advertise-address on the API-server

kubeadmConfigSpec:
    clusterConfiguration:
      apiServer:
        timeoutForControlPlane: 20m
        extraArgs:
          advertise-address: ${AZURE_PUBLIC_IP_ADDRESS}

Currently there is no way CAPI/KCP to dynamically do this. (If there is maybe we can try that approach)

With this pull request, we can sort of get around that by pre-provisioning a static IP address and populating the cluster configuration.

devigned · 2020-06-16T16:59:12Z

Ok... This has been hanging around for a little while. It need to come to a conclusion.

I don't love having name required on IPAddress. What if we leave the IPAddress type as it was, and just check with ARM if the IPAddress exists if name is set? If it does exist, we use it, else create it and tag it with CAPZ managed tag. On delete, we would not delete the IPAddress if it is tagged with the CAPZ managed tag.

Thoughts?

alexeldeib · 2020-06-16T17:05:17Z

Yep, I think that approach makes sense. This is my next task up, I got a little distracted by a flurry of work on disks again -_- Thankfully all finished with that

CecileRobertMichon · 2020-06-29T22:25:30Z

cloud/services/publicips/publicips.go

 	if err != nil {
 		return errors.Wrapf(err, "failed to delete public ip %s in resource group %s", publicIPSpec.Name, s.Scope.ResourceGroup())
 	}

 	klog.V(2).Infof("deleted public ip %s", publicIPSpec.Name)
 	return err
 }
+
+func (s *Service) isManaged(ctx context.Context, name string) (bool, error) {


cc @cpanato

This is exactly the change you need for #708

will review and follow the approach! thanks!

CecileRobertMichon · 2020-06-29T22:27:52Z

@alexeldeib this will need a pretty big rebase since #716 merged... sorry about that
Let me know if you want help, I'm happy to pair on applying this change to the new code organization.

alexeldeib · 2020-07-15T06:36:49Z

@CecileRobertMichon if it's fine with you i'll leave the validation as is. I updated the PR title and description since this implementation is no longer breaking. lmk if you have additional comments?

k8s-ci-robot · 2020-07-15T06:39:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cecilerobertmichon
You can assign the PR to them by writing /assign @cecilerobertmichon in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

alexeldeib · 2020-07-15T07:22:19Z

/test pull-cluster-api-provider-azure-test

alexeldeib · 2020-07-15T17:45:42Z

/test pull-cluster-api-provider-azure-test

api/v1alpha3/types.go

CecileRobertMichon · 2020-07-15T17:52:38Z

cloud/services/publicips/publicips.go

-		err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), ip.Name)
-		if err != nil && azure.ResourceNotFound(err) {
-			// already deleted
-			return nil


good catch 😬

cloud/scope/cluster.go

CecileRobertMichon

one last comment about the FQDN, otherwise lgtm

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

alexeldeib · 2020-07-15T18:25:14Z

@MirzaSikander I know this ended up taking quite a bit longer than anticipated, would appreciate if you have a chance to take a quick look at this for your scenario.

@CecileRobertMichon force pushed but added DNS name and pass it through directly

alexeldeib · 2020-07-15T19:25:48Z

/test pull-cluster-api-provider-azure-e2e

CecileRobertMichon · 2020-07-22T16:18:52Z

/assign @MirzaSikander

k8s-ci-robot · 2020-07-22T16:18:59Z

@alexeldeib: PR needs rebase.

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.

CecileRobertMichon · 2020-09-29T23:46:24Z

kubernetes-sigs/cluster-api#3715

k8s-ci-robot · 2020-10-06T19:25:51Z

@alexeldeib: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-e2e	`fd0b16e`	link	`/test pull-cluster-api-provider-azure-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

CecileRobertMichon · 2020-10-13T23:46:14Z

/close

closing in favor of #974

k8s-ci-robot · 2020-10-13T23:46:21Z

@CecileRobertMichon: Closed this PR.

In response to this:

/close

closing in favor of #974

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-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 28, 2020

k8s-ci-robot requested review from devigned and vincepri May 28, 2020 02:09

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels May 28, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 28, 2020

alexeldeib force-pushed the ace/ip branch from 0fa0148 to 097662b Compare May 28, 2020 02:21

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2020

alexeldeib force-pushed the ace/ip branch from 097662b to 1342d1b Compare June 28, 2020 08:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2020

alexeldeib marked this pull request as ready for review June 28, 2020 10:01

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2020

CecileRobertMichon reviewed Jun 29, 2020

View reviewed changes

k8s-ci-robot removed the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jul 15, 2020

alexeldeib changed the title ~~⚠ enable BYO IP for control plane~~ ✨ enable BYO IP for control plane Jul 15, 2020

alexeldeib force-pushed the ace/ip branch from 2080d67 to 3f3313a Compare July 15, 2020 06:39

alexeldeib force-pushed the ace/ip branch from 3f3313a to 426c516 Compare July 15, 2020 17:26

CecileRobertMichon reviewed Jul 15, 2020

View reviewed changes

api/v1alpha3/types.go Outdated Show resolved Hide resolved

alexeldeib force-pushed the ace/ip branch 2 times, most recently from 9c1982a to 3213055 Compare July 15, 2020 17:52

CecileRobertMichon reviewed Jul 15, 2020

View reviewed changes

alexeldeib force-pushed the ace/ip branch from 3213055 to 71410da Compare July 15, 2020 17:55

CecileRobertMichon reviewed Jul 15, 2020

View reviewed changes

cloud/scope/cluster.go Outdated Show resolved Hide resolved

CecileRobertMichon approved these changes Jul 15, 2020

View reviewed changes

✨ enable BYO IP address for control plane

fd0b16e

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

alexeldeib force-pushed the ace/ip branch from 71410da to fd0b16e Compare July 15, 2020 18:24

k8s-ci-robot assigned MirzaSikander Jul 22, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2020

CecileRobertMichon mentioned this pull request Oct 7, 2020

Enable configurable API Server Load Balancer #974

Merged

7 tasks

k8s-ci-robot closed this Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ enable BYO IP for control plane #652

✨ enable BYO IP for control plane #652

alexeldeib commented May 28, 2020 •

edited

Loading

k8s-ci-robot commented May 28, 2020

alexeldeib commented May 28, 2020

alexeldeib commented May 28, 2020

alexeldeib commented May 28, 2020

devigned commented May 28, 2020

MirzaSikander commented Jun 8, 2020

alexeldeib commented Jun 8, 2020

MirzaSikander commented Jun 8, 2020 •

edited

Loading

devigned commented Jun 9, 2020 •

edited

Loading

CecileRobertMichon commented Jun 9, 2020

MirzaSikander commented Jun 10, 2020

devigned commented Jun 16, 2020

alexeldeib commented Jun 16, 2020

CecileRobertMichon Jun 29, 2020

cpanato Jun 30, 2020

CecileRobertMichon commented Jun 29, 2020

alexeldeib commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

alexeldeib commented Jul 15, 2020

alexeldeib commented Jul 15, 2020

CecileRobertMichon Jul 15, 2020

CecileRobertMichon left a comment

alexeldeib commented Jul 15, 2020

alexeldeib commented Jul 15, 2020

CecileRobertMichon commented Jul 22, 2020

k8s-ci-robot commented Jul 22, 2020

CecileRobertMichon commented Sep 29, 2020

k8s-ci-robot commented Oct 6, 2020

CecileRobertMichon commented Oct 13, 2020

k8s-ci-robot commented Oct 13, 2020

✨ enable BYO IP for control plane #652

✨ enable BYO IP for control plane #652

Conversation

alexeldeib commented May 28, 2020 • edited Loading

k8s-ci-robot commented May 28, 2020

alexeldeib commented May 28, 2020

alexeldeib commented May 28, 2020

alexeldeib commented May 28, 2020

devigned commented May 28, 2020

MirzaSikander commented Jun 8, 2020

alexeldeib commented Jun 8, 2020

MirzaSikander commented Jun 8, 2020 • edited Loading

devigned commented Jun 9, 2020 • edited Loading

CecileRobertMichon commented Jun 9, 2020

MirzaSikander commented Jun 10, 2020

devigned commented Jun 16, 2020

alexeldeib commented Jun 16, 2020

CecileRobertMichon Jun 29, 2020

Choose a reason for hiding this comment

cpanato Jun 30, 2020

Choose a reason for hiding this comment

CecileRobertMichon commented Jun 29, 2020

alexeldeib commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

alexeldeib commented Jul 15, 2020

alexeldeib commented Jul 15, 2020

CecileRobertMichon Jul 15, 2020

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

alexeldeib commented Jul 15, 2020

alexeldeib commented Jul 15, 2020

CecileRobertMichon commented Jul 22, 2020

k8s-ci-robot commented Jul 22, 2020

CecileRobertMichon commented Sep 29, 2020

k8s-ci-robot commented Oct 6, 2020

CecileRobertMichon commented Oct 13, 2020

k8s-ci-robot commented Oct 13, 2020

alexeldeib commented May 28, 2020 •

edited

Loading

MirzaSikander commented Jun 8, 2020 •

edited

Loading

devigned commented Jun 9, 2020 •

edited

Loading