✨ cloud/services: add bastion host service #708

cpanato · 2020-06-14T17:13:25Z

What this PR does / why we need it:
Add initial implementation for a new cloud service to be able to create Azure Bastion Hosts

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 partially #165

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

cloud/services: add bastion host service

cpanato · 2020-06-14T17:14:37Z

/cc @devigned @CecileRobertMichon
all types of feedback are welcome

nader-ziada · 2020-06-15T14:09:36Z

everything looks good, but how will this be used by controllers?

devigned

PR looks pretty solid. We do need to discuss behavior around the public IP.

devigned · 2020-06-15T18:43:04Z

cloud/services/bastionhosts/bastionhosts.go

+}
+
+// Delete deletes the bastion host with the provided scope.
+func (s *Service) Delete(ctx context.Context, spec interface{}) error {


Delete will only delete the bastion service. If a public IP address was created in the Reconcile func, then it seems like it should be cleaned up here too.

Here is a similar scenario where a public IP is provided by the user. We should decided on a way to represent ownership of the Azure resource to help us to understand how to act during provisioning and deprovisioning.

/cc @alexeldeib and @CecileRobertMichon

Yes, I was thinking if the user needs to provide the IP or CAPZ will create it and I'm not sure the path to follow, that's is why I implemented in that way, if we decide the user need to provide the IP then I can remove the creation part.
I was following a similar approach that the VMs do as well to create the IP.
I don't have strong opinions on that, i think that should follow what the team wants for this.

I agree the IP should be created if it doesn't exist in a similar manor as VMs. Where this implementation differs is that it does not delete the IP when the bastion host is deleted. That means, the IP which is created in the Create func may not get deleted, but should be deleted if it was created in the Create func.

If created in Create, the IP should be deleted in Delete (imo).

FYI #716 will impact this change... We might want to use the same Reconcile/Delete for Bastion IPs as other public IPs (see how Reconcile() is used for VM public IPs in #716) and use tags (similar to resource group and vnet tags) to mark the IP as "owned"/managed by CAPZ. When deleting IPs, it should only delete the ones that are owned by that CAPZ cluster.

#716 merged so you should be able to reuse it now.

@CecileRobertMichon updated this PR to use the new way for services

k8s-ci-robot · 2020-06-15T18:44:17Z

@devigned: GitHub didn't allow me to request PR reviews from the following users: and.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Delete will only delete the bastion service. If a public IP address was created in the Reconcile func, then it seems like it should be cleaned up here too.

Here is a similar scenario where a public IP is provided by the user. We should decided on a way to represent ownership of the Azure resource to help us to understand how to act during provisioning and deprovisioning.

/cc @alexeldeib and @CecileRobertMichon

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.

cpanato · 2020-06-17T06:29:27Z

@nader-ziada sorry for the delay to reply, we discussed this in this thread the idea was add the service first and in other followups add the controller code, to make a bit easier to review and for doing baby steps 😄

Also i can deliver small things a bit faster

cpanato · 2020-06-17T06:35:32Z

@devigned thanks for your review.
Yes, I was thinking if the user needs to provide the IP or CAPZ will create it and I'm not sure the path to follow, that's is why I implemented in that way, if we decide the user need to provide the IP then I can remove the creation part.
I was following a similar approach that the VMs do as well to create the IP.
I don't have strong opinions on that, i think that should follow what the team wants for this.

cc @CecileRobertMichon

cloud/services/bastionhosts/bastionhosts.go

cpanato · 2020-06-19T14:19:41Z

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

cpanato · 2020-07-08T09:48:13Z

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

nader-ziada · 2020-08-05T22:34:09Z

@cpanato @devigned is this PR ready to move forward? (other than rebase)

devigned

/lgtm
/approve

I think we should consider adding the initial bastion types into experimental for a couple release to allow some settling when they are introduced.

k8s-ci-robot · 2020-08-06T14:51:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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~~ [devigned]

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

nader-ziada · 2020-08-10T15:05:17Z

@cpanato I think this is good once its rebased

cpanato · 2020-08-11T09:40:43Z

@nader-ziada rebased sorry for the delay

devigned

I might not have had enough coffee yet, but looks like cloud/services/bastionhosts/bastionhosts_test.go could use a go fmt.

nader-ziada · 2020-08-11T13:04:41Z

I might not have had enough coffee yet, but looks like cloud/services/bastionhosts/bastionhosts_test.go could use a go fmt.

it might be github somehow, looked fine when I pulled down the pr

cpanato · 2020-08-11T13:20:57Z

@devigned @nader-ziada applied here and did not get any differences, but also can be i had too much coffee for today :)
/shrug

devigned

lgtm

@nader-ziada or @CecileRobertMichon any comments?

nader-ziada · 2020-08-11T13:31:56Z

lgtm but will let @devigned take another look if he wants to

nader-ziada · 2020-08-11T13:33:02Z

/lgtm

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 14, 2020

k8s-ci-robot requested review from justaugustus and nader-ziada June 14, 2020 17:13

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 Jun 14, 2020

cpanato changed the title ~~cloud/services: add bastion host service~~ ✨ cloud/services: add bastion host service Jun 14, 2020

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 14, 2020

k8s-ci-robot requested review from CecileRobertMichon and devigned June 14, 2020 17:14

devigned suggested changes Jun 15, 2020

View reviewed changes

k8s-ci-robot requested a review from alexeldeib June 15, 2020 18:44

CecileRobertMichon reviewed Jun 17, 2020

View reviewed changes

cloud/services/bastionhosts/bastionhosts.go Outdated Show resolved Hide resolved

cpanato force-pushed the add-bastion-service branch from f1c84c8 to a2094fc Compare June 19, 2020 13:35

CecileRobertMichon mentioned this pull request Jun 29, 2020

✨ enable BYO IP for control plane #652

Closed

cpanato force-pushed the add-bastion-service branch from a2094fc to 801d896 Compare July 8, 2020 09:31

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 8, 2020

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

devigned approved these changes Aug 6, 2020

View reviewed changes

k8s-ci-robot assigned devigned Aug 6, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2020

cpanato force-pushed the add-bastion-service branch from 801d896 to c139925 Compare August 11, 2020 07:53

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 11, 2020

cpanato force-pushed the add-bastion-service branch from c139925 to 05f5211 Compare August 11, 2020 07:59

cloud/services: add bastion host service

99f90b1

cpanato force-pushed the add-bastion-service branch from 05f5211 to 99f90b1 Compare August 11, 2020 08:14

devigned suggested changes Aug 11, 2020

View reviewed changes

k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Aug 11, 2020

devigned approved these changes Aug 11, 2020

View reviewed changes

k8s-ci-robot assigned nader-ziada Aug 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2020

k8s-ci-robot merged commit 9241b7e into kubernetes-sigs:master Aug 11, 2020

k8s-ci-robot added this to the v0.4.8 milestone Aug 11, 2020

cpanato deleted the add-bastion-service branch August 11, 2020 14:04

devigned mentioned this pull request Apr 1, 2021

Implement Azure bastion host #165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ cloud/services: add bastion host service #708

✨ cloud/services: add bastion host service #708

cpanato commented Jun 14, 2020 •

edited

Loading

cpanato commented Jun 14, 2020

nader-ziada commented Jun 15, 2020

devigned left a comment

devigned Jun 15, 2020

devigned Jun 22, 2020

CecileRobertMichon Jun 22, 2020 •

edited

Loading

CecileRobertMichon Jun 29, 2020

cpanato Jul 8, 2020

k8s-ci-robot commented Jun 15, 2020

cpanato commented Jun 17, 2020

cpanato commented Jun 17, 2020

cpanato commented Jun 19, 2020

cpanato commented Jul 8, 2020

nader-ziada commented Aug 5, 2020

devigned left a comment

k8s-ci-robot commented Aug 6, 2020

nader-ziada commented Aug 10, 2020

cpanato commented Aug 11, 2020

devigned left a comment

nader-ziada commented Aug 11, 2020

cpanato commented Aug 11, 2020 •

edited

Loading

devigned left a comment

nader-ziada commented Aug 11, 2020

nader-ziada commented Aug 11, 2020

✨ cloud/services: add bastion host service #708

✨ cloud/services: add bastion host service #708

Conversation

cpanato commented Jun 14, 2020 • edited Loading

cpanato commented Jun 14, 2020

nader-ziada commented Jun 15, 2020

devigned left a comment

Choose a reason for hiding this comment

devigned Jun 15, 2020

Choose a reason for hiding this comment

devigned Jun 22, 2020

Choose a reason for hiding this comment

CecileRobertMichon Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon Jun 29, 2020

Choose a reason for hiding this comment

cpanato Jul 8, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 15, 2020

cpanato commented Jun 17, 2020

cpanato commented Jun 17, 2020

cpanato commented Jun 19, 2020

cpanato commented Jul 8, 2020

nader-ziada commented Aug 5, 2020

devigned left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 6, 2020

nader-ziada commented Aug 10, 2020

cpanato commented Aug 11, 2020

devigned left a comment

Choose a reason for hiding this comment

nader-ziada commented Aug 11, 2020

cpanato commented Aug 11, 2020 • edited Loading

devigned left a comment

Choose a reason for hiding this comment

nader-ziada commented Aug 11, 2020

nader-ziada commented Aug 11, 2020

cpanato commented Jun 14, 2020 •

edited

Loading

CecileRobertMichon Jun 22, 2020 •

edited

Loading

cpanato commented Aug 11, 2020 •

edited

Loading