instance: add a protection flag to exo compute #608

elkezza · 2024-05-28T08:50:13Z

Description

added add/remove a protection flag to exo compute, however it did not work as expected

Checklist

(For exoscale contributors)

Changelog updated (under Unreleased block)
Testing

Testing

I tested the cli after the changes locally but the protection flag does not work.

❯ ./exo compute instance update --help
This command updates an Instance .

Supported output template annotations: .ID, .Name, .CreationDate, .InstanceType, .Template, .Zone, .AntiAffinityGroups, .DeployTarget, .SecurityGroups, .PrivateInstance, .PrivateNetworks, .ElasticIPs, .IPAddress, .IPv6Address, .SSHKey, .DiskSize, .State, .Labels, .ReverseDNS

Usage:
  exo compute instance update NAME|ID [flags]

Flags:
  -c, --cloud-init string      instance cloud-init user data configuration file path
      --cloud-init-compress    compress instance cloud-init user data
  -h, --help                   help for update
      --label stringToString   instance label (format: key=value) (default [])
  -n, --name string            instance name
      --protection             enable delete protection
      --reverse-dns string     Reverse DNS Domain
  -z, --zone string            instance zone

Global Flags:
  -C, --config string            Specify an alternate config file [env EXOSCALE_CONFIG]
  -O, --output-format string     Output format (table|json|text), see "exo output --help" for more information
      --output-template string   Template to use if output format is "text"
  -Q, --quiet                    Quiet mode (disable non-essential command output)
  -A, --use-account string       Account to use in config file [env EXOSCALE_ACCOUNT]
❯ ./exo compute instance update 4ed629e1-9a63-4d74-b8c9-165d83292f9d --protection
┼──────────────────────┼──────────────────────────────────────┼
│   COMPUTE INSTANCE   │                                      │
┼──────────────────────┼──────────────────────────────────────┼
│ ID                   │ 4ed629e1-9a63-4d74-b8c9-165d83292f9d │
│ Name                 │ test                                 │
│ Creation Date        │ 2024-05-28 08:31:56 +0000 UTC        │
│ Instance Type        │ standard.medium                      │
│ Template             │ Linux Ubuntu 22.04 LTS 64-bit        │
│ Zone                 │ at-vie-1                             │
│ Anti-Affinity Groups │ n/a                                  │
│ Deploy Target        │ -                                    │
│ Security Groups      │ default                              │
│ Private Instance     │ No                                   │
│ Private Networks     │ n/a                                  │
│ Elastic IPs          │ n/a                                  │
│ IP Address           │ 194.182.184.154                      │
│ IPv6 Address         │ -                                    │
│ SSH Key              │ -                                    │
│ Disk Size            │ 50 GiB                               │
│ State                │ running                              │
│ Labels               │ n/a                                  │
│ Reverse DNS          │                                      │
┼──────────────────────┼──────────────────────────────────────┼
❯ ./exo compute instance delete 4ed629e1-9a63-4d74-b8c9-165d83292f9d
[+] Are you sure you want to delete instance "4ed629e1-9a63-4d74-b8c9-165d83292f9d"? [yN]: y
 ✔ Deleting instance "4ed629e1-9a63-4d74-b8c9-165d83292f9d"... 6s

sauterp · 2024-05-29T18:00:26Z

cmd/instance_update.go

+			if err != nil {
+				return
+			}
+			op, err = globalstate.EgoscaleV3Client.Wait(ctx, op, egoscale3.OperationStateSuccess)


Suggested change

op, err = globalstate.EgoscaleV3Client.Wait(ctx, op, egoscale3.OperationStateSuccess)

_, err = globalstate.EgoscaleV3Client.Wait(ctx, op, egoscale3.OperationStateSuccess)

Go doesn't allow you to create unused variables. You have to assign to _ if you would like to discard a returned value.

sauterp · 2024-05-29T18:07:08Z

cmd/instance_create.go

+			if err != nil {
+				return
+			}
+			op, err = globalstate.EgoscaleV3Client.Wait(ctx, op, egoscale3.OperationStateSuccess)


same issue here as below :)

sauterp · 2024-05-29T18:16:50Z

cmd/instance_update.go

@@ -25,6 +26,7 @@ type instanceUpdateCmd struct {
 	CloudInitCompress bool              `cli-flag:"cloud-init-compress" cli-usage:"compress instance cloud-init user data"`
 	Labels            map[string]string `cli-flag:"label" cli-usage:"instance label (format: key=value)"`
 	Name              string            `cli-short:"n" cli-usage:"instance name"`
+	Protection        bool              `cli-flag:"protection" cli-usage:"enable delete protection"`


Unfortunately one flag is not enough here, you'll need to create two flags --add-protection and --remove-protection.
The problem with your current implementation, is that it always removes the instance protection whenever you run an update command without the --protection flag. That's not ideal, users should have to set --remove-protection explicitly to do that. When the user doesn't set a protection flag, nothing related to delete protection should happen.

There is a bug there for sure, but we don't need a second flag.
We can use --protection=false for removing delete protection (same how we manage DBaaS --termination-protection).
There is function to check if flag was specified at all (see here).

elkezza · 2024-08-12T06:58:47Z

here is the output of the testing:

 ./exo compute instance update 3e8d4838-48a7-40f5-ae5d-c01d39567d22 --protection=false
┼──────────────────────┼──────────────────────────────────────┼
│   COMPUTE INSTANCE   │                                      │
┼──────────────────────┼──────────────────────────────────────┼
│ ID                   │ 3e8d4838-48a7-40f5-ae5d-c01d39567d22 │
│ Name                 │ test-instance                        │
│ Creation Date        │ 2024-08-09 15:27:50 +0000 UTC        │
│ Instance Type        │ standard.medium                      │
│ Template             │ Linux Ubuntu 22.04 LTS 64-bit        │
│ Zone                 │ at-vie-1                             │
│ Anti-Affinity Groups │ n/a                                  │
│ Deploy Target        │ -                                    │
│ Security Groups      │ default                              │
│ Private Instance     │ No                                   │
│ Private Networks     │ n/a                                  │
│ Elastic IPs          │ n/a                                  │
│ IP Address           │ 185.150.10.107                       │
│ IPv6 Address         │ -                                    │
│ SSH Key              │ -                                    │
│ Disk Size            │ 50 GiB                               │
│ State                │ running                              │
│ Labels               │ n/a                                  │
│ Reverse DNS          │                                      │
┼──────────────────────┼──────────────────────────────────────┼
❯ ./exo compute instance delete 3e8d4838-48a7-40f5-ae5d-c01d39567d22
[+] Are you sure you want to delete instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"? [yN]: y
 ✔ Deleting instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"... 0s
error: Delete "https://api-at-vie-1.exoscale.com/v2/instance/3e8d4838-48a7-40f5-ae5d-c01d39567d22": invalid request: Operation delete-instance on resource 3e8d4838-48a7-40f5-ae5d-c01d39567d22 is forbidden - reason: manual instance protection
❯ go run . compute instance delete 3e8d4838-48a7-40f5-ae5d-c01d39567d22
[+] Are you sure you want to delete instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"? [yN]: y
 ✔ Deleting instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"... 0s
error: Delete "https://api-at-vie-1.exoscale.com/v2/instance/3e8d4838-48a7-40f5-ae5d-c01d39567d22": invalid request: Operation delete-instance on resource 3e8d4838-48a7-40f5-ae5d-c01d39567d22 is forbidden - reason: manual instance protection
exit status 1


❯ go run . compute instance update 3e8d4838-48a7-40f5-ae5d-c01d39567d22 --protection=false

 ✔ Updating instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"... 0s
┼──────────────────────┼──────────────────────────────────────┼
│   COMPUTE INSTANCE   │                                      │
┼──────────────────────┼──────────────────────────────────────┼
│ ID                   │ 3e8d4838-48a7-40f5-ae5d-c01d39567d22 │
│ Name                 │ test-instance                        │
│ Creation Date        │ 2024-08-09 15:27:50 +0000 UTC        │
│ Instance Type        │ standard.medium                      │
│ Template             │ Linux Ubuntu 22.04 LTS 64-bit        │
│ Zone                 │ at-vie-1                             │
│ Anti-Affinity Groups │ n/a                                  │
│ Deploy Target        │ -                                    │
│ Security Groups      │ default                              │
│ Private Instance     │ No                                   │
│ Private Networks     │ n/a                                  │
│ Elastic IPs          │ n/a                                  │
│ IP Address           │ 185.150.10.107                       │
│ IPv6 Address         │ -                                    │
│ SSH Key              │ -                                    │
│ Disk Size            │ 50 GiB                               │
│ State                │ running                              │
│ Labels               │ n/a                                  │
│ Reverse DNS          │                                      │
┼──────────────────────┼──────────────────────────────────────┼
❯ go run . compute instance delete 3e8d4838-48a7-40f5-ae5d-c01d39567d22
[+] Are you sure you want to delete instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"? [yN]: y
 ✔ Deleting instance "3e8d4838-48a7-40f5-ae5d-c01d39567d22"... 6s

sauterp · 2024-08-13T07:45:11Z

@elkezza I renamed this PR, please try to use a similar format when naming your PRs.

sauterp · 2024-08-13T07:57:10Z

LTGM, I tested it on my side as well and it works. Please rebase your branch to resolve the conflicts, then I'll approve. Thanks!

CHANGELOG.md

sauterp

you need to rebase again to fix the changelog conflict. It's not your mistake, the problem will disappear when we generate the changelog automatically.

…hich is once the protection flag is set.

elkezza · 2024-08-22T09:06:07Z

you need to rebase again to fix the changelog conflict. It's not your mistake, the problem will disappear when we generate the changelog automatically.

ack, thanks for the feedback!

elkezza requested a review from sauterp May 28, 2024 08:50

pierre-emmanuelJ requested a review from a team May 28, 2024 13:12

sauterp reviewed May 29, 2024

View reviewed changes

elkezza requested review from kobajagi, sauterp and a team August 9, 2024 16:03

elkezza marked this pull request as ready for review August 12, 2024 06:55

sauterp changed the title ~~Salehelkaza/sc 89768/cli add a protection flag to exo compute~~ instance: add a protection flag to exo compute Aug 13, 2024

elkezza force-pushed the salehelkaza/sc-89768/cli-add-a-protection-flag-to-exo-compute branch from 86c610b to 8794f48 Compare August 16, 2024 14:10

kobajagi approved these changes Aug 16, 2024

View reviewed changes

elkezza force-pushed the salehelkaza/sc-89768/cli-add-a-protection-flag-to-exo-compute branch from 6a472a0 to 97dceda Compare August 20, 2024 11:55

sauterp approved these changes Aug 20, 2024

View reviewed changes

sauterp requested changes Aug 21, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

elkezza requested a review from sauterp August 22, 2024 07:50

sauterp approved these changes Aug 22, 2024

View reviewed changes

elkezza added 10 commits August 22, 2024 10:57

Resolved merge conflicts

2a467e1

Update .gitignore to exclude .idea/ directory

7643a7b

new update in the instance_update.go

3e92391

adding the condition in order the add protection block get executed w…

b6ca8be

…hich is once the protection flag is set.

deleting some debug prints.

5b77720

applied ''goimports -w cmd/instance_create.go''

2f53ea0

running goimports, to resolve gitHub action issues.

c46128b

the instanceId was wrongly used.

18f833e

removing duplicated import.

242fe82

adding line into the CHANGELOG.md reg. the new added Feature.

21f2507

elkezza force-pushed the salehelkaza/sc-89768/cli-add-a-protection-flag-to-exo-compute branch from 541c382 to 21f2507 Compare August 22, 2024 09:05

elkezza merged commit 3dbd9b5 into master Aug 22, 2024
2 checks passed

elkezza deleted the salehelkaza/sc-89768/cli-add-a-protection-flag-to-exo-compute branch August 22, 2024 14:02

pierre-emmanuelJ mentioned this pull request Sep 10, 2024

[Feature Request]: Add instance destruction protection option to exo compute instance create/update #599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

instance: add a protection flag to exo compute #608

instance: add a protection flag to exo compute #608

elkezza commented May 28, 2024

sauterp May 29, 2024

sauterp May 29, 2024

sauterp May 29, 2024

kobajagi May 30, 2024

elkezza commented Aug 12, 2024 •

edited

Loading

sauterp commented Aug 13, 2024

sauterp commented Aug 13, 2024

sauterp left a comment

elkezza commented Aug 22, 2024 •

edited

Loading

	op, err = globalstate.EgoscaleV3Client.Wait(ctx, op, egoscale3.OperationStateSuccess)
	_, err = globalstate.EgoscaleV3Client.Wait(ctx, op, egoscale3.OperationStateSuccess)

instance: add a protection flag to exo compute #608

instance: add a protection flag to exo compute #608

Conversation

elkezza commented May 28, 2024

Description

Checklist

Testing

sauterp May 29, 2024

Choose a reason for hiding this comment

sauterp May 29, 2024

Choose a reason for hiding this comment

sauterp May 29, 2024

Choose a reason for hiding this comment

kobajagi May 30, 2024

Choose a reason for hiding this comment

elkezza commented Aug 12, 2024 • edited Loading

sauterp commented Aug 13, 2024

sauterp commented Aug 13, 2024

sauterp left a comment

Choose a reason for hiding this comment

elkezza commented Aug 22, 2024 • edited Loading

elkezza commented Aug 12, 2024 •

edited

Loading

elkezza commented Aug 22, 2024 •

edited

Loading