Support to create and delete ssh-keys in the PowerVS environment. #1087

kishen-v · 2023-02-08T18:37:35Z

What this PR does / why we need it:
The PR contains the changes to support creation and deletion of ssh-keys in the powervs environment using the capibmadm tool.

The supporting documentation has also been updated.

kishenv@Kishens-MacBook-Pro capibmadm % ./capibmadm powervs key create --name capiadm-sshkey --key "ssh-rsa AAAAB3NzaC...(redacted)"  --zone osa21 --service-instance-id 82cac694-b9d7-45de-bab4-XXXXXXXXXXXX
Creating ssh key...
Successfully created the ssh key. name="capiadm-sshkey"

kishenv@Kishens-MacBook-Pro capibmadm % ./capibmadm powervs key delete --name capiadm-sshkey --zone osa21 --service-instance-id 82cac694-b9d7-45de-bab4-XXXXXXXXXXXX
Deleting ssh key...
Successfully deleted the ssh key. name="capiadm-sshkey"

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 #

Special notes for your reviewer:

/area provider/ibmcloud

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

Release note:

netlify · 2023-02-08T18:37:40Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`3819a3a`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/63eb62bc50be7e00085b2233
😎 Deploy Preview	https://deploy-preview-1087--kubernetes-sigs-cluster-api-ibmcloud.netlify.app/print
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2023-02-08T18:37:44Z

Welcome @kishen-v!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-ibmcloud 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-ibmcloud has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-02-08T18:37:45Z

Hi @kishen-v. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Karthik-K-N · 2023-02-09T03:52:00Z

/ok-to-test

Karthik-K-N

Few suggestions otherwise looks good to me.

Karthik-K-N · 2023-02-09T05:03:54Z

cmd/capibmadm/cmd/powervs/key/create.go

+	logger := log.Log
+	logger.Info("Creating ssh key...")
+	auth := iam.GetIAMAuth()
+	accountID, _ := utils.GetAccountID(ctx, auth)


Lets handle the error here.

Karthik-K-N · 2023-02-09T05:04:37Z

cmd/capibmadm/cmd/powervs/key/create.go

+	if err != nil {
+		return err
+	}
+	c := instance.NewIBMPIKeyClient(ctx, sess, options.GlobalOptions.ServiceInstanceID)


instead of c lets use something meaningful name like keyClient.

Karthik-K-N · 2023-02-09T05:40:51Z

cmd/capibmadm/cmd/powervs/key/create.go

+		Example: `
+# Create an ssh key.
+export IBMCLOUD_API_KEY=<api-key>
+capibmadm powervs key create --name <key-name> --key "<ssh key>" --service-instance-id <service-instance-id> --zone <zone>`,


also should we pass name via flag or as an argument? @Prajyot-Parab @dharaneeshvrd

IMO passing via flag would be more readable.

ok, then I think we can stick to how it is now then.

Karthik-K-N · 2023-02-09T05:52:49Z

LGTM, Please take a look @dharaneeshvrd @Prajyot-Parab later will seek manju's help.

Prajyot-Parab · 2023-02-09T06:47:43Z

LGTM, Please take a look @dharaneeshvrd @Prajyot-Parab later will seek manju's help.

Testing it on my local, will update.

dharaneeshvrd

In general, new line between each block, makes code more readable
otherwise lgtm.

dharaneeshvrd · 2023-02-09T07:30:51Z

cmd/capibmadm/cmd/powervs/key/create.go

+	"sigs.k8s.io/cluster-api-provider-ibmcloud/cmd/capibmadm/utils"
+)
+
+type keyCreateParams struct {


Suggested change

type keyCreateParams struct {

type keyCreateOptions struct {

Options is used more commonly in this repo, better to follow same name conventions here too.

Changes made as suggested.
Added new-lines to improve readability.

Prajyot-Parab

Nit: Power VS --> PowerVS everywhere

kishen-v · 2023-02-09T15:40:54Z

Nit: Power VS --> PowerVS everywhere

Updated all references of Power VS to PowerVS introduced through this PR.

Prajyot-Parab · 2023-02-09T15:49:54Z

cmd/capibmadm/cmd/powervs/key/create.go

+/*
+Copyright 2023 The Kubernetes Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/


Suggested change

/*

Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");

you may not use this file except in compliance with the License.

You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software

distributed under the License is distributed on an "AS IS" BASIS,

WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and

limitations under the License.

*/

/*

Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");

you may not use this file except in compliance with the License.

You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software

distributed under the License is distributed on an "AS IS" BASIS,

WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and

limitations under the License.

*/

Fix license header in all files.

Updated license across all files.

Prajyot-Parab · 2023-02-10T05:29:37Z

/test all

Karthik-K-N · 2023-02-10T07:50:33Z

/lgtm
/assign @mkumatag

Prajyot-Parab · 2023-02-10T07:51:41Z

cmd/capibmadm/cmd/powervs/key/create.go

+		Example: `
+# Create an ssh key.
+export IBMCLOUD_API_KEY=<api-key>
+capibmadm powervs key create --name <key-name> --key "<ssh key>" --service-instance-id <service-instance-id> --zone <zone>`,


Minor Nit:

Suggested change

capibmadm powervs key create --name <key-name> --key "<ssh key>" --service-instance-id <service-instance-id> --zone <zone>`,

capibmadm powervs key create --name <key-name> --key "<ssh-key>" --service-instance-id <service-instance-id> --zone <zone>`,

Prajyot-Parab · 2023-02-10T07:52:49Z

docs/book/src/topics/capibmadm/powervs/key.md

+#### Example:
+```shell
+export IBMCLOUD_API_KEY=<api-key>
+capibmadm powervs key create --name <key-name> --key "<ssh key>" --service-instance-id <service-instance-id> --zone <zone>


Suggested change

capibmadm powervs key create --name <key-name> --key "<ssh key>" --service-instance-id <service-instance-id> --zone <zone>

capibmadm powervs key create --name <key-name> --key "<ssh-key>" --service-instance-id <service-instance-id> --zone <zone>

Addressed, made changes as suggested.

Prajyot-Parab · 2023-02-10T08:17:53Z

/lgtm
@Karthik-K-N @mkumatag PTAL

mkumatag · 2023-02-10T09:22:25Z

cmd/capibmadm/cmd/powervs/key/create.go

+	}
+
+	var keyCreateOption keyCreateOptions
+	cmd.Flags().StringVar(&keyCreateOption.key, "key", "", "SSH RSA key string within a double quotation marks. For example, \"ssh-rsa AAA... \".")


Most of the folks usually prefer file over a plain text of a key for better UX, wondering if can enhance the code to do so. One more flag can be added like key-file which reads from the file and does some validation as well.

@Prajyot-Parab @Karthik-K-N wdys?

Note: if agree - we can have a follow up PR

sure we can introduce a flag , Once this get merged, may be @kishen-v do you want to give a try for that.

@Karthik-K-N, anything works. If all the PRs for capibmdm needs to go together, I'll work on an enhancement PR to support reading of SSH key from file.

+1 for --key-file flag

Changes are made support the --key-path argument.

kishenv@Kishens-MacBook-Pro capibmadm % ./capibmadm powervs key create --name capiadm-sshkey2 --key-path /Users/kishenv/.ssh/id_rsa.pub --zone osa21 --service-instance-id <redacted> Creating SSH key... Successfully created the SSH key. name="capiadm-sshkey2"

mkumatag · 2023-02-10T09:23:32Z

cmd/capibmadm/cmd/powervs/key/create.go

+	}
+	keyClient := instance.NewIBMPIKeyClient(ctx, sess, options.GlobalOptions.ServiceInstanceID)
+
+	sshBody := models.SSHKey{Name: &keyCreateOption.keyName, SSHKey: &keyCreateOption.key}


do we need any validation for the validness of the key or Create call will take of that?!

@mkumatag, I tried to pass an invalid SSH-key to --key, there is supporting logic on the server's end to validate the key. The following error was returned :

failed to Create PI Key with error [POST /pcloud/v1/tenants/{tenant_id}/sshkeys][400] pcloudTenantsSshkeysPostBadRequest &{Code:0 Description:capiadm-sshkey sshkey is invalid; please enter a valid ssh RSA key: ssh: no key found Error:bad request Message:}

I'll add supporting changes to validate the key before the request is made.

Do we really need? or we just rely on server to take care of validation?!

We can rely on server too. :)
But I was only thinking if it would be better to validate the key locally, if not this overhead will be borne by the server.

Edit: I have added a check to validate the SSH-key in the code.

Prajyot-Parab · 2023-02-14T09:44:17Z

@kishen-v need to fix conflicts and push again.

mkumatag

/lgtm

k8s-ci-robot · 2023-02-14T13:08:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kishen-v, mkumatag

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

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

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Feb 8, 2023

k8s-ci-robot requested review from jichenjc and relyt0925 February 8, 2023 18:37

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 8, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 8, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 9, 2023

kishen-v force-pushed the capibm-delete-keys branch from f4c478e to cd77c22 Compare February 9, 2023 04:50

Karthik-K-N reviewed Feb 9, 2023

View reviewed changes

kishen-v force-pushed the capibm-delete-keys branch from ff445ea to 685bbde Compare February 9, 2023 05:16

Karthik-K-N reviewed Feb 9, 2023

View reviewed changes

kishen-v force-pushed the capibm-delete-keys branch from af0cef1 to 3cbeabf Compare February 9, 2023 05:45

Karthik-K-N mentioned this pull request Feb 9, 2023

VPC key create and delete operations added #1085

Closed

dharaneeshvrd reviewed Feb 9, 2023

View reviewed changes

kishen-v force-pushed the capibm-delete-keys branch 2 times, most recently from a59ce08 to 1f10bc1 Compare February 9, 2023 13:37

Prajyot-Parab suggested changes Feb 9, 2023

View reviewed changes

kishen-v force-pushed the capibm-delete-keys branch from 04305f6 to c4ae5c2 Compare February 9, 2023 15:37

Prajyot-Parab reviewed Feb 9, 2023

View reviewed changes

kishen-v force-pushed the capibm-delete-keys branch 2 times, most recently from 1511011 to 92e6896 Compare February 9, 2023 16:21

k8s-ci-robot requested a review from Karthik-K-N February 10, 2023 07:47

k8s-ci-robot assigned Prajyot-Parab Feb 10, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2023

k8s-ci-robot assigned mkumatag and Karthik-K-N Feb 10, 2023

Prajyot-Parab suggested changes Feb 10, 2023

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2023

kishen-v force-pushed the capibm-delete-keys branch from ada69ba to d3842a4 Compare February 10, 2023 08:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2023

mkumatag requested changes Feb 10, 2023

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2023

kishen-v force-pushed the capibm-delete-keys branch 2 times, most recently from 705d90b to fa964b7 Compare February 13, 2023 11:59

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

kishen-v force-pushed the capibm-delete-keys branch from 520e250 to bd94198 Compare February 14, 2023 09:47

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

kishen-v force-pushed the capibm-delete-keys branch from a35bbac to 5b4e469 Compare February 14, 2023 10:25

Support to create and delete ssh-keys in the PowerVS environment.

3819a3a

kishen-v force-pushed the capibm-delete-keys branch from 5b4e469 to 3819a3a Compare February 14, 2023 10:30

mkumatag approved these changes Feb 14, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2023

k8s-ci-robot merged commit 66eddb3 into kubernetes-sigs:main Feb 14, 2023

Karthik-K-N mentioned this pull request Mar 10, 2023

CLI tool for Cluster API Provider IBM Cloud #1052

Closed

16 tasks

dharaneeshvrd mentioned this pull request Mar 22, 2023

REQUEST: New membership for dharaneeshvrd kubernetes/org#4107

Closed

9 tasks

kishen-v deleted the capibm-delete-keys branch September 9, 2023 09:14

	capibmadm powervs key create --name <key-name> --key "<ssh key>" --service-instance-id <service-instance-id> --zone <zone>`,
	capibmadm powervs key create --name <key-name> --key "<ssh-key>" --service-instance-id <service-instance-id> --zone <zone>`,

Support to create and delete ssh-keys in the PowerVS environment. #1087

Support to create and delete ssh-keys in the PowerVS environment. #1087

Conversation

kishen-v commented Feb 8, 2023

netlify bot commented Feb 8, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

k8s-ci-robot commented Feb 8, 2023

k8s-ci-robot commented Feb 8, 2023

Karthik-K-N commented Feb 9, 2023

Karthik-K-N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karthik-K-N commented Feb 9, 2023

Prajyot-Parab commented Feb 9, 2023

dharaneeshvrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishen-v Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Prajyot-Parab left a comment

Choose a reason for hiding this comment

kishen-v commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prajyot-Parab commented Feb 10, 2023

Karthik-K-N commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prajyot-Parab commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishen-v Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishen-v Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Prajyot-Parab commented Feb 14, 2023

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 14, 2023

netlify bot commented Feb 8, 2023 •

edited

Loading

kishen-v Feb 9, 2023 •

edited

Loading

kishen-v Feb 11, 2023 •

edited

Loading

kishen-v Feb 10, 2023 •

edited

Loading