Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Cloudstack multiple endpoint for preflight checks #2559

Merged
merged 25 commits into from
Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
bda21a0
Support Cloudstack multiple endpoint for preflight checks
wongni Jun 27, 2022
c464ce9
Update for review comments
wongni Jun 28, 2022
e8e931f
Merge branch 'main' into support-multiple-acs-for-cmk-2
wongni Jun 28, 2022
3efce41
Fix unit test failures
wongni Jun 28, 2022
31679e2
Remove debug print
wongni Jun 28, 2022
461a804
Remove unnecessary file
wongni Jun 28, 2022
dbdac2b
Remove multierror module
wongni Jun 30, 2022
0a87c51
Update to use single cmk object instead of list or map of multiple cmks
wongni Jun 30, 2022
cbd45c9
Remove ValidateCloudStackConnection when cleaning up VMs
wongni Jun 30, 2022
369b53d
Remove NewProviderCustomNet because it's the same as NewProvider
wongni Jun 30, 2022
260ec6e
Avoid using 'failed to' in errors
wongni Jun 30, 2022
5382943
Remove localAvailabilityZones from Validator
wongni Jun 30, 2022
dca6a9e
Remove multipleZone from ValidateNetworkPresent
wongni Jun 30, 2022
074a62c
Return list of errors CleanUpCloudstackTestResources
wongni Jul 1, 2022
bdf278e
Increase unit test coverage
wongni Jul 1, 2022
40c9ddb
Increase unit test coverage #2
wongni Jul 1, 2022
bdbd178
Change error format
wongni Jul 5, 2022
9813e46
Change function names to be more descriptive
wongni Jul 5, 2022
2ddc742
Add default verifySslValue as a constant
wongni Jul 5, 2022
187c194
Fix a bug where empty Zone IDs are used in validateMachineConfig
wongni Jul 5, 2022
0fe30a1
Merge branch 'aws:main' into support-multiple-acs-for-cmk-2
wongni Jul 5, 2022
2ed1d16
Add error message when validating management api endpoint fails
wongni Jul 5, 2022
f6259dd
Make MarkPass message more readable
wongni Jul 5, 2022
3683c61
Merge branch 'aws:main' into support-multiple-acs-for-cmk-2
wongni Jul 6, 2022
222889c
Fix presubmit job failure
wongni Jul 6, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/dependencies/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ func (f *Factory) WithProvider(clusterConfigFile string, clusterConfig *v1alpha1
return fmt.Errorf("unable to get machine config from file %s: %v", clusterConfigFile, err)
}

// map[string]*executables.Cmk and map[string]ProviderCmkClient are not compatible so we convert the map manually
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is no longer relevant right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

cmkClientMap := cloudstack.CmkClientMap{}
for name, cmk := range f.dependencies.Cmks {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment explaining this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cmkClientMap[name] = cmk
Expand Down Expand Up @@ -419,7 +420,7 @@ func (f *Factory) WithCmk() *Factory {
f.WithExecutableBuilder().WithWriter()

f.buildSteps = append(f.buildSteps, func(ctx context.Context) error {
if f.dependencies.Cmks != nil {
if f.dependencies.Cmks != nil && len(f.dependencies.Cmks) > 0 {
return nil
}
f.dependencies.Cmks = map[string]*executables.Cmk{}
Expand Down
8 changes: 2 additions & 6 deletions pkg/executables/cmk.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,6 @@ func (c *Cmk) ValidateDomainPresent(ctx context.Context, domain string) (v1alpha

func (c *Cmk) ValidateNetworkPresent(ctx context.Context, domainId string, network v1alpha1.CloudStackResourceIdentifier, zoneId string, account string, multipleZone bool) error {
command := newCmkCommand("list networks")
if len(network.Id) > 0 {
applyCmkArgs(&command, withCloudStackId(network.Id))
}
if multipleZone {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this parameter? I think we could get rid of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like multipleZone is still here

applyCmkArgs(&command, withCloudStackNetworkType(Shared))
}
Expand Down Expand Up @@ -432,14 +429,13 @@ func (c *Cmk) exec(ctx context.Context, args ...string) (stdout bytes.Buffer, er
func (c *Cmk) buildCmkConfigFile() (configFile string, err error) {
t := templater.New(c.writer)

cloudstackPreflightTimeout := defaultCloudStackPreflightTimeout
c.config.Timeout = defaultCloudStackPreflightTimeout
if timeout, isSet := os.LookupEnv("CLOUDSTACK_PREFLIGHT_TIMEOUT"); isSet {
if _, err := strconv.ParseUint(timeout, 10, 16); err != nil {
return "", fmt.Errorf("CLOUDSTACK_PREFLIGHT_TIMEOUT must be a number: %v", err)
}
cloudstackPreflightTimeout = timeout
c.config.Timeout = timeout
}
c.config.Timeout = cloudstackPreflightTimeout
writtenFileName, err := t.WriteToFile(cmkConfigTemplate, c.config, fmt.Sprintf(cmkConfigFileNameTemplate, c.config.Name))
if err != nil {
return "", fmt.Errorf("creating file for cmk config: %v", err)
Expand Down
15 changes: 0 additions & 15 deletions pkg/executables/cmk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,21 +518,6 @@ func TestCmkListOperations(t *testing.T) {
shouldSecondCallOccur: false,
wantResultCount: 1,
},
{
testName: "listnetworks success on id filter",
jsonResponseFile: "testdata/cmk_list_network_singular.json",
argumentsExecCall: []string{
"-c", configFilePath,
"list", "networks", fmt.Sprintf("id=\"%s\"", resourceId.Id), fmt.Sprintf("domainid=\"%s\"", domainId), fmt.Sprintf("account=\"%s\"", accountName), fmt.Sprintf("zoneid=\"%s\"", "TEST_RESOURCE"),
},
cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
return cmk.ValidateNetworkPresent(ctx, domainId, zones[3].Network, zones[3].Id, accountName, false)
},
cmkResponseError: nil,
wantErr: false,
shouldSecondCallOccur: true,
wantResultCount: 1,
},
{
testName: "listnetworks no results",
jsonResponseFile: "testdata/cmk_list_empty_response.json",
Expand Down
14 changes: 7 additions & 7 deletions pkg/providers/cloudstack/cloudstack.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,15 +395,15 @@ func (p *cloudstackProvider) validateClusterSpec(ctx context.Context, clusterSpe

func (p *cloudstackProvider) SetupAndValidateCreateCluster(ctx context.Context, clusterSpec *cluster.Spec) error {
if err := p.validateEnv(ctx); err != nil {
return fmt.Errorf("failed setup and validations: %v", err)
return fmt.Errorf("validating environment variables: %v", err)
}

if err := p.validateClusterSpec(ctx, clusterSpec); err != nil {
return fmt.Errorf("failed cluster spec validation: %v", err)
return fmt.Errorf("validating cluster spec: %v", err)
}

if err := p.setupSSHAuthKeysForCreate(); err != nil {
return fmt.Errorf("failed setup and validations: %v", err)
return fmt.Errorf("setting up SSH keys: %v", err)
}

if clusterSpec.Cluster.IsManaged() {
Expand Down Expand Up @@ -434,15 +434,15 @@ func (p *cloudstackProvider) SetupAndValidateCreateCluster(ctx context.Context,

func (p *cloudstackProvider) SetupAndValidateUpgradeCluster(ctx context.Context, cluster *types.Cluster, clusterSpec *cluster.Spec) error {
if err := p.validateEnv(ctx); err != nil {
return fmt.Errorf("failed setup and validations: %v", err)
return fmt.Errorf("validating environment variables: %v", err)
}

if err := p.validateClusterSpec(ctx, clusterSpec); err != nil {
return fmt.Errorf("failed cluster spec validation: %v", err)
return fmt.Errorf("validating cluster spec: %v", err)
}

if err := p.setupSSHAuthKeysForUpgrade(); err != nil {
return fmt.Errorf("failed setup and validations: %v", err)
return fmt.Errorf("setting up SSH keys: %v", err)
}

if err := p.validateMachineConfigsNameUniqueness(ctx, cluster, clusterSpec); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be part of validateClusterSpec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a code clean up change.
from

err = func1()
if err != nil {}

to

if err := func1(); err != nil {}

If you think validateMachineConfigsNameUniqueness is a part of validateClusterSpec then I should be handled in a separate PR because this pattern shows up in vsphere provider as well.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think vsphere provider code is a good reference. But yea separate pr sounds good to me.

Expand All @@ -454,7 +454,7 @@ func (p *cloudstackProvider) SetupAndValidateUpgradeCluster(ctx context.Context,
func (p *cloudstackProvider) SetupAndValidateDeleteCluster(ctx context.Context, _ *types.Cluster) error {
err := p.validateEnv(ctx)
if err != nil {
return fmt.Errorf("failed setup and validations: %v", err)
return fmt.Errorf("validating environment variables: %v", err)
}
return nil
}
Expand Down
78 changes: 37 additions & 41 deletions pkg/providers/cloudstack/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import (
)

type Validator struct {
cmks CmkClientMap
availabilityZones []localAvailabilityZone
cmks CmkClientMap
localAvailabilityZones []localAvailabilityZone
}

// Taken from https://github.com/shapeblue/cloudstack/blob/08bb4ad9fea7e422c3d3ac6d52f4670b1e89eed7/api/src/main/java/com/cloud/vm/VmDetailConstants.java
Expand All @@ -32,8 +32,8 @@ var restrictedUserCustomDetails = [...]string{

func NewValidator(cmks CmkClientMap) *Validator {
return &Validator{
cmks: cmks,
availabilityZones: []localAvailabilityZone{},
cmks: cmks,
localAvailabilityZones: []localAvailabilityZone{},
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you should have localAvailabilityZones in the validator struct if you are generating it from one of the validation method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

}
}

Expand All @@ -59,25 +59,25 @@ type ProviderCmkClient interface {
type CmkClientMap map[string]ProviderCmkClient

func (v *Validator) validateCloudStackAccess(ctx context.Context, datacenterConfig *anywherev1.CloudStackDatacenterConfig) error {
azNamesToCheck := []string{}
refNamesToCheck := []string{}
if len(datacenterConfig.Spec.Domain) > 0 {
azNamesToCheck = append(azNamesToCheck, decoder.CloudStackGlobalAZ)
refNamesToCheck = append(refNamesToCheck, decoder.CloudStackGlobalAZ)
}
for _, az := range datacenterConfig.Spec.AvailabilityZones {
azNamesToCheck = append(azNamesToCheck, az.CredentialsRef)
refNamesToCheck = append(refNamesToCheck, az.CredentialsRef)
}

for _, azName := range azNamesToCheck {
cmk, ok := v.cmks[azName]
for _, refName := range refNamesToCheck {
cmk, ok := v.cmks[refName]
if !ok {
return fmt.Errorf("cannot find CloudStack profile for availability zone %s", azName)
return fmt.Errorf("cannot find CloudStack profile for credentialsRef %s", refName)
}
if err := cmk.ValidateCloudStackConnection(ctx); err != nil {
return fmt.Errorf("failed validating connection to cloudstack %s: %v", azName, err)
return fmt.Errorf("failed validating connection to cloudstack %s: %v", refName, err)
}
}

logger.MarkPass("Connected to", "servers", azNamesToCheck)
logger.MarkPass("Connected to", "servers", refNamesToCheck)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does the formatting look like here? I don't see any %s substitution so I'm a bit confused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MarkPass takes keysAndValues... so refNamesToCheck will be correctly formatted.

Copy link
Member

Choose a reason for hiding this comment

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

Could you paste an example here for reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Connected to {"servers": ["Global", "acs-172"]}

Copy link
Member

Choose a reason for hiding this comment

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

If we are doing a mark pass here, can we make it more readable like Connected to servers: Global, acs-172?

return nil
}

Expand All @@ -86,22 +86,33 @@ func (v *Validator) ValidateCloudStackDatacenterConfig(ctx context.Context, data
return err
}

for _, az := range v.availabilityZones {
for _, az := range v.localAvailabilityZones {
fmt.Printf("az: %+v\n", az.CloudStackAvailabilityZone)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf("az: %+v\n", az.CloudStackAvailabilityZone)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Removed.

_, err := getHostnameFromUrl(az.ManagementApiEndpoint)
if err != nil {
return fmt.Errorf("checking management api endpoint: %v", err)
}

cmk, ok := v.cmks[az.CredentialsRef]
if !ok {
return fmt.Errorf("cannot find CloudStack profile for availability zone %s", az.CredentialsRef)
return fmt.Errorf("cannot find CloudStack profile named %s for availability zone %s", az.CredentialsRef, az.Name)
}
endpoint := cmk.GetManagementApiEndpoint()
if endpoint != az.ManagementApiEndpoint {
return fmt.Errorf("cloudstack secret management url (%s) differs from cluster spec management url (%s)",
endpoint, az.ManagementApiEndpoint)
}

domain, err := cmk.ValidateDomainPresent(ctx, az.Domain)
if err != nil {
return err
}
az.DomainId = domain.Id

if err := cmk.ValidateAccountPresent(ctx, az.Account, az.DomainId); err != nil {
return err
}

zoneId, err := cmk.ValidateZonePresent(ctx, az.CloudStackAvailabilityZone.Zone)
if err != nil {
return err
Expand All @@ -125,52 +136,37 @@ func (v *Validator) generateLocalAvailabilityZones(ctx context.Context, datacent
}

if len(datacenterConfig.Spec.Domain) > 0 {
cmk, ok := v.cmks[decoder.CloudStackGlobalAZ]
_, ok := v.cmks[decoder.CloudStackGlobalAZ]
if !ok {
return fmt.Errorf("cannot find CloudStack profile for availability zone %s", decoder.CloudStackGlobalAZ)
}
domain, err := cmk.ValidateDomainPresent(ctx, datacenterConfig.Spec.Domain)
if err != nil {
return err
}
if err := cmk.ValidateAccountPresent(ctx, datacenterConfig.Spec.Account, domain.Id); err != nil {
return err
return fmt.Errorf("cannot find CloudStack profile named %s for default availability zone", decoder.CloudStackGlobalAZ)
}
for _, zone := range datacenterConfig.Spec.Zones {
for index, zone := range datacenterConfig.Spec.Zones {
availabilityZone := localAvailabilityZone{
CloudStackAvailabilityZone: &anywherev1.CloudStackAvailabilityZone{
Name: fmt.Sprintf("availability-zone-%d", index),
CredentialsRef: decoder.CloudStackGlobalAZ,
Domain: datacenterConfig.Spec.Domain,
Account: datacenterConfig.Spec.Account,
ManagementApiEndpoint: datacenterConfig.Spec.ManagementApiEndpoint,
Zone: zone,
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be missing a name, doesn't it? I think we need the name too for some of the error messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding Name as SetDefaults does

},
DomainId: domain.Id,
}
v.availabilityZones = append(v.availabilityZones, availabilityZone)
v.localAvailabilityZones = append(v.localAvailabilityZones, availabilityZone)
}
}
for _, az := range datacenterConfig.Spec.AvailabilityZones {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a check for nil before iterating over the items in an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need, I think. AvailablityZones is []CloudStackAvailabilityZone and it will be initialized as an empty list.

cmk, ok := v.cmks[az.CredentialsRef]
_, ok := v.cmks[az.CredentialsRef]
if !ok {
return fmt.Errorf("cannot find CloudStack profile for availability zone %s", az.CredentialsRef)
}
domain, err := cmk.ValidateDomainPresent(ctx, az.Domain)
if err != nil {
return err
}
if err := cmk.ValidateAccountPresent(ctx, az.Account, domain.Id); err != nil {
return err
return fmt.Errorf("cannot find CloudStack profile named %s for availability zone %s", az.CredentialsRef, az.Name)
}
availabilityZone := localAvailabilityZone{
CloudStackAvailabilityZone: &az,
DomainId: domain.Id,
}
v.availabilityZones = append(v.availabilityZones, availabilityZone)
v.localAvailabilityZones = append(v.localAvailabilityZones, availabilityZone)
}

if len(v.availabilityZones) <= 0 {
return fmt.Errorf("CloudStackDatacenterConfig domain or availabilityZones is not set or is empty")
if len(v.localAvailabilityZones) <= 0 {
return fmt.Errorf("CloudStackDatacenterConfig domain or localAvailabilityZones is not set or is empty")
}
return nil
}
Expand Down Expand Up @@ -286,10 +282,10 @@ func (v *Validator) validateMachineConfig(ctx context.Context, datacenterConfig
}
}

for _, az := range v.availabilityZones {
for _, az := range v.localAvailabilityZones {
cmk, ok := v.cmks[az.CredentialsRef]
if !ok {
return fmt.Errorf("cannot find CloudStack profile for availability zone %s", az.CredentialsRef)
return fmt.Errorf("cannot find CloudStack profile named %s for availability zone %s", az.CredentialsRef, az.Name)
}
if err := cmk.ValidateTemplatePresent(ctx, az.DomainId, az.CloudStackAvailabilityZone.Zone.Id, az.Account, machineConfig.Spec.Template); err != nil {
return fmt.Errorf("validating template: %v", err)
Expand Down