Skip to content

Conversation

dustymabe
Copy link
Member

In db803c3 we add support for ConfidentialType but we made it so that we always execute this code where we don't want to do that if the user didn't specify any ConfidentialType for GCP.

The way the code is now we can't run any GCP tests on non confidential instances.

failed to create instance "kola-dd144aac7413f66c85e9":
    Does not support confidential type , should be: sev, sev_snp

In db803c3 we add support for ConfidentialType but we made it so
that we always execute this code where we don't want to do that if
the user didn't specify any ConfidentialType for GCP.

The way the code is now we can't run any GCP tests on non confidential
instances.

```
failed to create instance "kola-dd144aac7413f66c85e9":
    Does not support confidential type , should be: sev, sev_snp
```
This is to appease golangci-lint:

```
Error: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative
has been available since Go 1.0: As of Go 1.20 there is no reason to call Seed
with a random value. Programs that call Seed with a known value to get a
specific sequence of results should use New(NewSource(seed)) to obtain a local
random generator.  (staticcheck)
```

According to https://pkg.go.dev/math/rand#Seed "If Seed is not called, the
generator is seeded randomly at program startup." so I think it's safe to
just drop it.
@HuijingHei
Copy link
Member

HuijingHei commented Sep 13, 2024

Overall LGTM, thanks for the fixing! Seems need to fix CI.

[2024-09-13T04:26:19.945Z] Checking gofmt...

[2024-09-13T04:26:19.945Z] diff ./platform/api/azure/api.go.orig ./platform/api/azure/api.go

[2024-09-13T04:26:19.945Z] --- ./platform/api/azure/api.go.orig

[2024-09-13T04:26:19.945Z] +++ ./platform/api/azure/api.go

[2024-09-13T04:26:19.945Z] @@ -16,8 +16,8 @@

[2024-09-13T04:26:19.945Z]  package azure

[2024-09-13T04:26:19.945Z]  

[2024-09-13T04:26:19.945Z]  import (

[2024-09-13T04:26:19.945Z] -	"fmt"

[2024-09-13T04:26:19.945Z]  	"crypto/rand"

[2024-09-13T04:26:19.945Z] +	"fmt"

[2024-09-13T04:26:19.945Z]  	"os"

[2024-09-13T04:26:19.945Z]  	"strings"

[2024-09-13T04:26:19.945Z]  	"time"

[2024-09-13T04:26:19.945Z] diff ./platform/machine/azure/cluster.go.orig ./platform/machine/azure/cluster.go

[2024-09-13T04:26:19.945Z] --- ./platform/machine/azure/cluster.go.orig

[2024-09-13T04:26:19.945Z] +++ ./platform/machine/azure/cluster.go

[2024-09-13T04:26:19.945Z] @@ -15,9 +15,9 @@

[2024-09-13T04:26:19.945Z]  package azure

[2024-09-13T04:26:19.945Z]  

[2024-09-13T04:26:19.945Z]  import (

[2024-09-13T04:26:19.945Z] +	"crypto/rand"

[2024-09-13T04:26:19.945Z]  	"errors"

[2024-09-13T04:26:19.945Z]  	"fmt"

[2024-09-13T04:26:19.945Z] -	"crypto/rand"

[2024-09-13T04:26:19.945Z]  	"os"

[2024-09-13T04:26:19.945Z]  	"path/filepath"

[2024-09-13T04:26:19.945Z]  

[2024-09-13T04:26:19.945Z] diff ./platform/machine/esx/cluster.go.orig ./platform/machine/esx/cluster.go

[2024-09-13T04:26:19.945Z] --- ./platform/machine/esx/cluster.go.orig

[2024-09-13T04:26:19.945Z] +++ ./platform/machine/esx/cluster.go

[2024-09-13T04:26:19.945Z] @@ -15,9 +15,9 @@

[2024-09-13T04:26:19.945Z]  package esx

[2024-09-13T04:26:19.945Z]  

[2024-09-13T04:26:19.945Z]  import (

[2024-09-13T04:26:19.945Z] +	"crypto/rand"

[2024-09-13T04:26:19.945Z]  	"errors"

[2024-09-13T04:26:19.945Z]  	"fmt"

[2024-09-13T04:26:19.945Z] -	"crypto/rand"

[2024-09-13T04:26:19.945Z]  	"os"

[2024-09-13T04:26:19.945Z]  	"path/filepath"

[2024-09-13T04:26:19.945Z]  

[2024-09-13T04:26:19.945Z] gofmt check failed

[2024-09-13T04:26:19.945Z] make: *** [Makefile:79: mantle-check] Error 1

HuijingHei
HuijingHei previously approved these changes Sep 13, 2024
@HuijingHei
Copy link
Member

For the ci error:

Check failure on line 119 in mantle/platform/api/azure/api.go

/ golangci-lint
Error return value of `rand.Read` is not checked (errcheck)

Can change rand.Read(b) to

	if _, err := rand.Read(b); err != nil {
		panic(err)
	}

@HuijingHei
Copy link
Member

Confirm the patch works with both normal and confidential vm.

[coreos-assembler]$ make KOLET_ARCHES=x86_64
[coreos-assembler]$ ./bin/kola run -E ./fedora-coreos-config -p=gcp --gcp-image=projects/fedora-coreos-cloud/global/images/fedora-coreos-40-20240903-20-0-gcp-x86-64 --gcp-json-key=/srv/tool/gcp.json --gcp-project=openshift-rhcos-devel basic
...
PASS, output in _kola_temp/gcp-2024-09-13-0718-10847

[coreos-assembler]$ ./bin/kola run -E ../fedora-coreos-config -p=gcp --gcp-image=projects/fedora-coreos-cloud/global/images/fedora-coreos-40-20240903-20-0-gcp-x86-64 --gcp-json-key=/srv/tool/gcp.json --gcp-project=openshift-rhcos-devel --gcp-confidential-type sev_snp --tag confidential ext.fedora-coreos-config.platforms.gcp.nvme-symlink
Setting instance type for confidential computing
Using n2d-standard-2 instance type
=== RUN   ext.fedora-coreos-config.platforms.gcp.nvme-symlink
Using confidential type for confidential computing SEV_SNP
--- PASS: ext.fedora-coreos-config.platforms.gcp.nvme-symlink (85.46s)

@dustymabe
Copy link
Member Author

hey @HuijingHei thanks fo trying to fix this. It looks like in other code where we use rand.Read() from crypto/rand we just print out an error rather than panicing. I'll just match that code for now:

if _, err := rand.Read(b); err != nil {
plog.Errorf("failed to generate a random vmname: %v", err)
}

@dustymabe dustymabe force-pushed the dusty-kola-gcp-fix branch 2 times, most recently from b1d2b59 to fe8b2f1 Compare September 13, 2024 12:40
To appease golangci-lint:

```
Error: SA1019: rand.Read has been deprecated since Go 1.20 because
it shouldn't be used: For almost all use cases, [crypto/rand.Read]
is more appropriate.
```
@dustymabe dustymabe enabled auto-merge (rebase) September 13, 2024 12:43
Copy link
Member

@HuijingHei HuijingHei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dustymabe dustymabe merged commit 51dc4ab into coreos:main Sep 13, 2024
5 checks passed
@dustymabe dustymabe deleted the dusty-kola-gcp-fix branch September 13, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants