Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

chore: check all error returns in main code #3184

Merged
merged 4 commits into from
May 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ run:
linters:
disable-all: true
enable:
- errcheck
- goimports
- gosimple
- golint
Expand Down
12 changes: 7 additions & 5 deletions cmd/addpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,24 +88,24 @@ func (apc *addPoolCmd) validate(cmd *cobra.Command) error {
}

if apc.resourceGroupName == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--resource-group must be specified")
}

if apc.location == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--location must be specified")
}

apc.location = helpers.NormalizeAzureRegion(apc.location)

if apc.apiModelPath == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--api-model must be specified")
}

if apc.nodePoolPath == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--nodepool must be specified")
}
return nil
Expand Down Expand Up @@ -144,7 +144,9 @@ func (apc *addPoolCmd) load() error {
}

if apc.containerService.Properties.IsCustomCloudProfile() {
writeCustomCloudProfile(apc.containerService)
if err = writeCustomCloudProfile(apc.containerService); err != nil {
return errors.Wrap(err, "error writing custom cloud profile")
}
if err = apc.containerService.Properties.SetCustomCloudSpec(api.AzureCustomCloudSpecParams{IsUpgrade: false, IsScale: true}); err != nil {
return errors.Wrap(err, "error parsing the api model")
}
Expand Down
6 changes: 4 additions & 2 deletions cmd/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (dc *deployCmd) validateArgs(cmd *cobra.Command, args []string) error {
if len(args) == 1 {
dc.apimodelPath = args[0]
} else if len(args) > 1 {
cmd.Usage()
_ = cmd.Usage()
return errors.New("too many arguments were provided to 'deploy'")
}
}
Expand Down Expand Up @@ -225,7 +225,9 @@ func (dc *deployCmd) loadAPIModel() error {
if err != nil {
return errors.Wrap(err, "error parsing the api model")
}
writeCustomCloudProfile(dc.containerService)
if err = writeCustomCloudProfile(dc.containerService); err != nil {
return errors.Wrap(err, "error writing custom cloud profile")
}

if dc.containerService.Properties.CustomCloudProfile.IdentitySystem == "" || dc.containerService.Properties.CustomCloudProfile.IdentitySystem != dc.authProvider.getAuthArgs().IdentitySystem {
if dc.authProvider != nil {
Expand Down
4 changes: 2 additions & 2 deletions cmd/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) error {
if len(args) == 1 {
gc.apimodelPath = args[0]
} else if len(args) > 1 {
cmd.Usage()
_ = cmd.Usage()
return errors.New("too many arguments were provided to 'generate'")
} else {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--api-model was not supplied, nor was one specified as a positional argument")
}
}
Expand Down
12 changes: 6 additions & 6 deletions cmd/get_logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func newGetLogsCmd() *cobra.Command {
Long: getLogsLongDescription,
RunE: func(cmd *cobra.Command, args []string) error {
if err := glc.validateArgs(); err != nil {
cmd.Usage()
_ = cmd.Usage()
return errors.Wrap(err, "validating get-logs args")
}
if err := glc.loadAPIModel(); err != nil {
Expand All @@ -77,11 +77,11 @@ func newGetLogsCmd() *cobra.Command {
command.Flags().StringVar(&glc.linuxScriptPath, "linux-script", "", "path to the log collection script to execute on the cluster's Linux nodes (required)")
command.Flags().StringVarP(&glc.outputDirectory, "output-directory", "o", "", "collected logs destination directory, derived from --api-model if missing")
command.Flags().BoolVarP(&glc.controlPlaneOnly, "control-plane-only", "", false, "get logs from control plane VMs only")
command.MarkFlagRequired("location")
command.MarkFlagRequired("api-model")
command.MarkFlagRequired("ssh-host")
command.MarkFlagRequired("linux-ssh-private-key")
command.MarkFlagRequired("linux-script") // optional once in VHD
_ = command.MarkFlagRequired("location")
_ = command.MarkFlagRequired("api-model")
_ = command.MarkFlagRequired("ssh-host")
_ = command.MarkFlagRequired("linux-ssh-private-key")
_ = command.MarkFlagRequired("linux-script") // optional once in VHD
return command
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/rotate_certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func newRotateCertsCmd() *cobra.Command {
f.StringVar(&rcc.masterFQDN, "apiserver", "", "apiserver endpoint (required)")
f.StringVarP(&rcc.outputDirectory, "output-directory", "o", "", "output directory where generated TLS artifacts will be saved (derived from DNS prefix if absent)")

f.MarkDeprecated("master-FQDN", "--apiserver is preferred")
_ = f.MarkDeprecated("master-FQDN", "--apiserver is preferred")

addAuthFlags(rcc.getAuthArgs(), f)

Expand Down
20 changes: 11 additions & 9 deletions cmd/scale.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ func newScaleCmd() *cobra.Command {
f.StringVar(&sc.masterFQDN, "master-FQDN", "", "FQDN for the master load balancer that maps to the apiserver endpoint")
f.StringVar(&sc.masterFQDN, "apiserver", "", "apiserver endpoint (required to cordon and drain nodes)")

f.MarkDeprecated("deployment-dir", "--deployment-dir is no longer required for scale or upgrade. Please use --api-model.")
f.MarkDeprecated("master-FQDN", "--apiserver is preferred")
_ = f.MarkDeprecated("deployment-dir", "--deployment-dir is no longer required for scale or upgrade. Please use --api-model.")
_ = f.MarkDeprecated("master-FQDN", "--apiserver is preferred")

addAuthFlags(&sc.authArgs, f)

Expand All @@ -103,29 +103,29 @@ func (sc *scaleCmd) validate(cmd *cobra.Command) error {
}

if sc.resourceGroupName == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--resource-group must be specified")
}

if sc.location == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--location must be specified")
}

sc.location = helpers.NormalizeAzureRegion(sc.location)

if sc.newDesiredAgentCount == 0 {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--new-node-count must be specified")
}

if sc.apiModelPath == "" && sc.deploymentDirectory == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--api-model must be specified")
}

if sc.apiModelPath != "" && sc.deploymentDirectory != "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("ambiguous, please specify only one of --api-model and --deployment-dir")
}

Expand Down Expand Up @@ -160,7 +160,9 @@ func (sc *scaleCmd) load() error {
}

if sc.containerService.Properties.IsCustomCloudProfile() {
writeCustomCloudProfile(sc.containerService)
if err = writeCustomCloudProfile(sc.containerService); err != nil {
return errors.Wrap(err, "error writing custom cloud profile")
}

if err = sc.containerService.Properties.SetCustomCloudSpec(api.AzureCustomCloudSpecParams{IsUpgrade: false, IsScale: true}); err != nil {
return errors.Wrap(err, "error parsing the api model")
Expand Down Expand Up @@ -296,7 +298,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error {
// VMAS Scale down Scenario
if currentNodeCount > sc.newDesiredAgentCount {
if sc.apiserverURL == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--apiserver is required to scale down a kubernetes cluster's agent pool")
}

Expand Down
16 changes: 9 additions & 7 deletions cmd/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func newUpgradeCmd() *cobra.Command {
f.BoolVarP(&uc.controlPlaneOnly, "control-plane-only", "", false, "upgrade control plane VMs only, do not upgrade node pools")
addAuthFlags(uc.getAuthArgs(), f)

f.MarkDeprecated("deployment-dir", "deployment-dir is no longer required for scale or upgrade. Please use --api-model.")
_ = f.MarkDeprecated("deployment-dir", "deployment-dir is no longer required for scale or upgrade. Please use --api-model.")

return upgradeCmd
}
Expand All @@ -101,12 +101,12 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command) error {
}

if uc.resourceGroupName == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--resource-group must be specified")
}

if uc.location == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--location must be specified")
}
uc.location = helpers.NormalizeAzureRegion(uc.location)
Expand All @@ -122,17 +122,17 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command) error {
}

if uc.upgradeVersion == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--upgrade-version must be specified")
}

if uc.apiModelPath == "" && uc.deploymentDirectory == "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("--api-model must be specified")
}

if uc.apiModelPath != "" && uc.deploymentDirectory != "" {
cmd.Usage()
_ = cmd.Usage()
return errors.New("ambiguous, please specify only one of --api-model and --deployment-dir")
}

Expand Down Expand Up @@ -175,7 +175,9 @@ func (uc *upgradeCmd) loadCluster() error {
}

if uc.containerService.Properties.IsCustomCloudProfile() {
writeCustomCloudProfile(uc.containerService)
if err = writeCustomCloudProfile(uc.containerService); err != nil {
return errors.Wrap(err, "error writing custom cloud profile")
}
if err = uc.containerService.Properties.SetCustomCloudSpec(api.AzureCustomCloudSpecParams{
IsUpgrade: true,
IsScale: false,
Expand Down
4 changes: 3 additions & 1 deletion pkg/api/convertertoagentpoolonlyapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,9 @@ func isAgentPoolOnlyClusterJSON(contents []byte) bool {

func propertiesAsMap(contents []byte) (map[string]interface{}, bool) {
var raw interface{}
json.Unmarshal(contents, &raw)
if err := json.Unmarshal(contents, &raw); err != nil {
return nil, false
}
jsonMap := raw.(map[string]interface{})
properties, propertiesPresent := jsonMap["properties"]
if !propertiesPresent {
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ func mapToString(valueMap map[string]string) string {

func generateEtcdEncryptionKey() string {
b := make([]byte, 32)
rand.Read(b)
_, _ = rand.Read(b)
return base64.StdEncoding.EncodeToString(b)
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -1148,11 +1148,11 @@ func (p *Properties) GetClusterID() string {
// from the master dns name
h := fnv.New64a()
if p.MasterProfile != nil {
h.Write([]byte(p.MasterProfile.DNSPrefix))
_, _ = h.Write([]byte(p.MasterProfile.DNSPrefix))
} else if p.HostedMasterProfile != nil {
h.Write([]byte(p.HostedMasterProfile.DNSPrefix))
_, _ = h.Write([]byte(p.HostedMasterProfile.DNSPrefix))
} else if len(p.AgentPoolProfiles) > 0 {
h.Write([]byte(p.AgentPoolProfiles[0].Name))
_, _ = h.Write([]byte(p.AgentPoolProfiles[0].Name))
}
r := rand.New(rand.NewSource(int64(h.Sum64())))
mutex.Lock()
Expand Down
20 changes: 15 additions & 5 deletions pkg/armhelpers/azureclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,15 +163,19 @@ func NewAzureClientWithDeviceAuth(env azure.Environment, subscriptionID string)
if err != nil {
return nil, err
}
armSpt.Refresh()
if err = armSpt.Refresh(); err != nil {
log.Error(err)
}

adRawToken := armSpt.Token()
adRawToken.Resource = env.GraphEndpoint
graphSpt, err := adal.NewServicePrincipalTokenFromManualToken(*oauthConfig, aksEngineClientID, env.GraphEndpoint, adRawToken)
if err != nil {
return nil, err
}
graphSpt.Refresh()
if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand All @@ -191,7 +195,9 @@ func NewAzureClientWithClientSecret(env azure.Environment, subscriptionID, clien
if err != nil {
return nil, err
}
graphSpt.Refresh()
if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand All @@ -211,7 +217,9 @@ func NewAzureClientWithClientSecretExternalTenant(env azure.Environment, subscri
if err != nil {
return nil, err
}
graphSpt.Refresh()
Copy link
Member Author

Choose a reason for hiding this comment

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

This was returning a consistent error (once we started checking it). Need to investigate more.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this and similar calls to log.Error(err) so as not to change existing control flow.

if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand Down Expand Up @@ -278,7 +286,9 @@ func newAzureClientWithCertificate(env azure.Environment, oauthConfig *adal.OAut
if err != nil {
return nil, err
}
graphSpt.Refresh()
if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand Down
12 changes: 9 additions & 3 deletions pkg/armhelpers/azurestack/azureclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ func NewAzureClientWithClientSecret(env azure.Environment, subscriptionID, clien
if err != nil {
return nil, err
}
graphSpt.Refresh()
if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand All @@ -109,7 +111,9 @@ func NewAzureClientWithClientSecretExternalTenant(env azure.Environment, subscri
if err != nil {
return nil, err
}
graphSpt.Refresh()
if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand Down Expand Up @@ -181,7 +185,9 @@ func newAzureClientWithCertificate(env azure.Environment, oauthConfig *adal.OAut
if err != nil {
return nil, err
}
graphSpt.Refresh()
if err = graphSpt.Refresh(); err != nil {
log.Error(err)
}

return getClient(env, subscriptionID, tenantID, autorest.NewBearerAuthorizer(armSpt), autorest.NewBearerAuthorizer(graphSpt)), nil
}
Expand Down
9 changes: 4 additions & 5 deletions pkg/armhelpers/azurestack/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@ import (

// DeepCopy dst and src should be the same type in different API version
// dst should be pointer type
func DeepCopy(dst, src interface{}) error {
defer func() error {
func DeepCopy(dst, src interface{}) (err error) {
defer func() {
if r := recover(); r != nil {
return fmt.Errorf("fail to copy object %v", r)
err = fmt.Errorf("fail to copy object %v", r)
}
return nil
}()
dstValue := reflect.ValueOf(dst)
srcValue := reflect.ValueOf(src)
Expand All @@ -26,7 +25,7 @@ func DeepCopy(dst, src interface{}) error {
return fmt.Errorf("the dst type (%q) and src type (%q) are not the same", dstValue.Type().String(), srcValue.Type().String())
}
deepCopyInternal(dstValue, srcValue, 0)
return nil
return err
}

func deepCopyInternal(dstValue, srcValue reflect.Value, depth int) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/armhelpers/azurestack/deploymentError.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func DeployTemplateSync(az armhelpers.AKSEngineClient, logger *logrus.Entry, res
// try to extract error from ARM Response
if deploymentExtended.Response.Response != nil && deploymentExtended.Body != nil {
buf := new(bytes.Buffer)
buf.ReadFrom(deploymentExtended.Body)
_, _ = buf.ReadFrom(deploymentExtended.Body)
logger.Infof("StatusCode: %d, Error: %s", deploymentExtended.Response.StatusCode, buf.String())
deploymentErr.Response = buf.Bytes()
deploymentErr.StatusCode = deploymentExtended.Response.StatusCode
Expand Down
Loading