diff --git a/.golangci.yml b/.golangci.yml index f7322988a68..ea42306b028 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -3,9 +3,16 @@ run: allow-parallel-runners: true linters: enable: + - dupword + - durationcheck + - errorlint + - fatcontext - gocyclo - misspell + - nilerr + - noctx - unparam + - usestdlibvars settings: gocyclo: min-complexity: 30 diff --git a/availability-prober/availability_prober.go b/availability-prober/availability_prober.go index d68d0df137e..0020bb8b852 100644 --- a/availability-prober/availability_prober.go +++ b/availability-prober/availability_prober.go @@ -103,13 +103,13 @@ func NewStartCommand() *cobra.Command { } } - check(log, url, time.Second, time.Second, opts.requiredAPIsParsed, opts.waitForInfrastructureResource, opts.waitForClusterRolebinding, opts.waitForLabeledPodsGone, discoveryClient, kubeClient) + check(cmd.Context(), log, url, time.Second, time.Second, opts.requiredAPIsParsed, opts.waitForInfrastructureResource, opts.waitForClusterRolebinding, opts.waitForLabeledPodsGone, discoveryClient, kubeClient) } return cmd } -func check(log logr.Logger, target *url.URL, requestTimeout time.Duration, sleepTime time.Duration, requiredAPIs []schema.GroupVersionKind, waitForInfrastructureResource bool, waitForClusterRolebinding, waitForLabeledPodsGone string, discoveryClient discovery.DiscoveryInterface, kubeClient crclient.Client) { +func check(ctx context.Context, log logr.Logger, target *url.URL, requestTimeout time.Duration, sleepTime time.Duration, requiredAPIs []schema.GroupVersionKind, waitForInfrastructureResource bool, waitForClusterRolebinding, waitForLabeledPodsGone string, discoveryClient discovery.DiscoveryInterface, kubeClient crclient.Client) { log = log.WithValues("sleepTime", sleepTime.String()) client := &http.Client{ Timeout: requestTimeout, @@ -118,7 +118,12 @@ func check(log logr.Logger, target *url.URL, requestTimeout time.Duration, sleep }, } for ; ; time.Sleep(sleepTime) { - response, err := client.Get(target.String()) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) + if err != nil { + log.Error(err, "Failed to create request, retrying...") + continue + } + response, err := client.Do(req) if err != nil { log.Error(err, "Request failed, retrying...") continue diff --git a/cmd/bastion/aws/create.go b/cmd/bastion/aws/create.go index 4d8cb5c244e..f5abf0965f3 100644 --- a/cmd/bastion/aws/create.go +++ b/cmd/bastion/aws/create.go @@ -146,7 +146,7 @@ func (o *CreateBastionOpts) Run(ctx context.Context, logger logr.Logger) (string var err error sshPublicKey, err = os.ReadFile(o.SSHKeyFile) if err != nil { - return "", "", fmt.Errorf("cannot read SSH public key from %s: %v", o.SSHKeyFile, err) + return "", "", fmt.Errorf("cannot read SSH public key from %s: %w", o.SSHKeyFile, err) } } diff --git a/cmd/cluster/core/create.go b/cmd/cluster/core/create.go index 4bed9956c4c..0e1fb107a2f 100644 --- a/cmd/cluster/core/create.go +++ b/cmd/cluster/core/create.go @@ -1235,7 +1235,7 @@ func validateMgmtClusterAndNodePoolCPUArchitectures(ctx context.Context, opts *R if !validMultiArchImage { mgmtClusterCPUArch, err := hyperutil.GetMgmtClusterCPUArch(kc) if err != nil { - return fmt.Errorf("failed to check mgmt cluster CPU arch: %v", err) + return fmt.Errorf("failed to check mgmt cluster CPU arch: %w", err) } if !strings.EqualFold(mgmtClusterCPUArch, opts.Arch) { @@ -1253,7 +1253,7 @@ func validateMgmtClusterAndNodePoolCPUArchitectures(ctx context.Context, opts *R func validateVersion(ctx context.Context, versionCLI string, client crclient.Client) error { _, operatorVersion, err := supportedversion.GetSupportedOCPVersions(ctx, "hypershift", client, nil) if err != nil { - return fmt.Errorf("failed to get supported OCP versions: %v", err) + return fmt.Errorf("failed to get supported OCP versions: %w", err) } if operatorVersion != versionCLI { return fmt.Errorf("version mismatch detected, CLI: %s, Operator: %s", versionCLI, operatorVersion) diff --git a/cmd/cluster/powervs/create.go b/cmd/cluster/powervs/create.go index a3e25ae0799..1c93184d3c0 100644 --- a/cmd/cluster/powervs/create.go +++ b/cmd/cluster/powervs/create.go @@ -243,7 +243,7 @@ var _ core.Platform = (*CreateOptions)(nil) func NewCreateCommand(opts *core.RawCreateOptions) *cobra.Command { cmd := &cobra.Command{ Use: "powervs", - Short: "Creates basic functional HostedCluster resources on PowerVS PowerVS", + Short: "Creates basic functional HostedCluster resources on PowerVS", SilenceUsage: true, } diff --git a/cmd/infra/aws/iam.go b/cmd/infra/aws/iam.go index 2100dfa0f4b..73928e74a65 100644 --- a/cmd/infra/aws/iam.go +++ b/cmd/infra/aws/iam.go @@ -891,7 +891,7 @@ func (o *CreateIAMOptions) CreateOIDCResources(ctx context.Context, iamClient aw // Create a single shared role with all policies sharedRoleARN, err := o.CreateSharedOIDCRole(ctx, iamClient, bindings, providerARN, providerName, logger) if err != nil { - return nil, fmt.Errorf("failed to create shared OIDC role: %v", err) + return nil, fmt.Errorf("failed to create shared OIDC role: %w", err) } // Set all role ARNs to the shared role ARN for into := range bindings { @@ -903,7 +903,7 @@ func (o *CreateIAMOptions) CreateOIDCResources(ctx context.Context, iamClient aw trustPolicy := oidcTrustPolicy(providerARN, providerName, binding.serviceAccounts...) arn, err := o.CreateOIDCRole(ctx, iamClient, binding, trustPolicy, logger) if err != nil { - return nil, fmt.Errorf("failed to create OIDC Role %q: with trust policy %s and permission policy %s: %v", binding.name, trustPolicy, binding.policy, err) + return nil, fmt.Errorf("failed to create OIDC Role %q: with trust policy %s and permission policy %s: %w", binding.name, trustPolicy, binding.policy, err) } *into = arn } @@ -966,7 +966,7 @@ func (o *CreateIAMOptions) CreateOIDCResources(ctx context.Context, iamClient aw ] }`, ingressPolicyStatement, ccmPolicyStatement)), }); err != nil { - return nil, fmt.Errorf("failed to create role policy %q: with permission policy %s: %v", ingressRoleName, ingressPolicyStatement, err) + return nil, fmt.Errorf("failed to create role policy %q: with permission policy %s: %w", ingressRoleName, ingressPolicyStatement, err) } logger.Info("Added inline shared policy to ROSA Managed Role", "role", ingressRoleName) } else { @@ -979,7 +979,7 @@ func (o *CreateIAMOptions) CreateOIDCResources(ctx context.Context, iamClient aw "Statement": [%s] }`, ingressPolicyStatement)), }); err != nil { - return nil, fmt.Errorf("failed to create role policy %q: with permission policy %s: %v", ingressRoleName, ingressPolicyStatement, err) + return nil, fmt.Errorf("failed to create role policy %q: with permission policy %s: %w", ingressRoleName, ingressPolicyStatement, err) } logger.Info("Added inline policy to ROSA Ingress Managed Role", "role", ingressRoleName) @@ -992,7 +992,7 @@ func (o *CreateIAMOptions) CreateOIDCResources(ctx context.Context, iamClient aw "Statement": [%s] }`, ccmPolicyStatement)), }); err != nil { - return nil, fmt.Errorf("failed to create role policy %q: with permission policy %s: %v", ccmRoleName, ccmPolicyStatement, err) + return nil, fmt.Errorf("failed to create role policy %q: with permission policy %s: %w", ccmRoleName, ccmPolicyStatement, err) } logger.Info("Added inline policy to ROSA Cloud Controller Manager Managed Role", "role", ccmRoleName) } diff --git a/cmd/infra/aws/iam_policies.go b/cmd/infra/aws/iam_policies.go index d99594b3ca2..6f569fdae55 100644 --- a/cmd/infra/aws/iam_policies.go +++ b/cmd/infra/aws/iam_policies.go @@ -36,7 +36,7 @@ func APIsByDelegatedServices() (ServicesByDelegate, error) { for _, binding := range bindings { p := policy{} if err := json.Unmarshal([]byte(binding.policy), &p); err != nil { - return nil, fmt.Errorf("error unmarshalling delegate policy for %q: %v", binding.name, err) + return nil, fmt.Errorf("error unmarshalling delegate policy for %q: %w", binding.name, err) } delegate := EndpointsByService{} for i, statement := range p.Statement { diff --git a/cmd/infra/aws/route53.go b/cmd/infra/aws/route53.go index bb6034aadaa..048055579ca 100644 --- a/cmd/infra/aws/route53.go +++ b/cmd/infra/aws/route53.go @@ -188,7 +188,10 @@ func (o *DestroyInfraOptions) CleanupPublicZone(ctx context.Context, client awsa name := o.BaseDomain id, err := LookupZone(ctx, client, name, false) if err != nil { - return nil + if strings.Contains(err.Error(), "not found") { + return nil + } + return fmt.Errorf("failed to lookup public hosted zone %s: %w", name, err) } recordName := fmt.Sprintf("*.apps.%s.%s", o.Name, o.BaseDomain) err = deleteRecord(ctx, client, id, recordName) @@ -239,12 +242,12 @@ func setSOAMinimum(ctx context.Context, client awsapi.ROUTE53API, id, name strin func deleteZone(ctx context.Context, id string, client awsapi.ROUTE53API, logger logr.Logger) error { err := deleteRecords(ctx, client, id, logger) if err != nil { - return fmt.Errorf("failed to delete hosted zone records: %v", err) + return fmt.Errorf("failed to delete hosted zone records: %w", err) } if _, err = client.DeleteHostedZone(ctx, &route53.DeleteHostedZoneInput{ Id: aws.String(id), }); err != nil { - return fmt.Errorf("failed to delete hosted zone: %v", err) + return fmt.Errorf("failed to delete hosted zone: %w", err) } return nil } diff --git a/cmd/infra/aws/route53_test.go b/cmd/infra/aws/route53_test.go index 7c65d850d6e..01c1913a726 100644 --- a/cmd/infra/aws/route53_test.go +++ b/cmd/infra/aws/route53_test.go @@ -317,10 +317,14 @@ func TestCreatePrivateZone(t *testing.T) { func TestCleanupPublicZone(t *testing.T) { tests := []struct { - name string - setupMock func(*awsapi.MockROUTE53API) - expectError bool - errorContains string + name string + redact bool + setupMock func(*awsapi.MockROUTE53API) + expectError bool + errorContains string + useCtx func() context.Context + wantLogRedacted bool + wantLogSubstring string }{ { name: "When zone and wildcard record exist it should delete the record and return nil", @@ -340,6 +344,27 @@ func TestCleanupPublicZone(t *testing.T) { Return(&route53.ChangeResourceRecordSetsOutput{}, nil) }, }, + { + name: "When zone and wildcard record exist with redact enabled it should delete the record and redact the domain in logs", + redact: true, + setupMock: func(m *awsapi.MockROUTE53API) { + m.EXPECT().ListHostedZones(gomock.Any(), gomock.Any(), gomock.Any()). + Return(publicZonePage("PUBZONE", testBaseDomain), nil) + m.EXPECT().ListResourceRecordSets(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []route53types.ResourceRecordSet{ + { + Name: aws.String("*.apps." + testCluster + "." + testBaseDomain + "."), + Type: route53types.RRTypeA, + }, + }, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&route53.ChangeResourceRecordSetsOutput{}, nil) + }, + wantLogRedacted: true, + wantLogSubstring: "[REDACTED]", + }, { name: "When the zone is not found it should return nil as a no-op", setupMock: func(m *awsapi.MockROUTE53API) { @@ -358,6 +383,16 @@ func TestCleanupPublicZone(t *testing.T) { }, nil) }, }, + { + name: "When LookupZone fails with a non-not-found error it should return a wrapped error", + useCtx: cancelledCtx, + setupMock: func(m *awsapi.MockROUTE53API) { + m.EXPECT().ListHostedZones(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("throttling exception")) + }, + expectError: true, + errorContains: "failed to lookup public hosted zone", + }, { name: "When ChangeResourceRecordSets fails with a non-404 error it should return the error", setupMock: func(m *awsapi.MockROUTE53API) { @@ -382,28 +417,44 @@ func TestCleanupPublicZone(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) ctrl := gomock.NewController(t) mockR53 := awsapi.NewMockROUTE53API(ctrl) tt.setupMock(mockR53) + ctx := t.Context() + if tt.useCtx != nil { + ctx = tt.useCtx() + } + + var logOutput strings.Builder + logger := logr.Discard() + if tt.wantLogRedacted { + logger = funcr.New(func(prefix, args string) { + logOutput.WriteString(args) + }, funcr.Options{}) + } + o := &DestroyInfraOptions{ - BaseDomain: testBaseDomain, - Name: testCluster, - Log: logr.Discard(), + BaseDomain: testBaseDomain, + Name: testCluster, + RedactBaseDomain: tt.redact, + Log: logger, } - err := o.CleanupPublicZone(context.Background(), mockR53) + err := o.CleanupPublicZone(ctx, mockR53) if tt.expectError { - if err == nil { - t.Fatal("expected error, got nil") - } - if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { - t.Errorf("expected error containing %q, got: %v", tt.errorContains, err) + g.Expect(err).To(HaveOccurred()) + if tt.errorContains != "" { + g.Expect(err).To(MatchError(ContainSubstring(tt.errorContains))) } } else { - if err != nil { - t.Errorf("expected no error, got: %v", err) - } + g.Expect(err).ToNot(HaveOccurred()) + } + + if tt.wantLogRedacted { + g.Expect(logOutput.String()).To(ContainSubstring(tt.wantLogSubstring)) + g.Expect(logOutput.String()).ToNot(ContainSubstring(testBaseDomain)) } }) } diff --git a/cmd/infra/aws/util/errors.go b/cmd/infra/aws/util/errors.go index 074cd062e74..e25c8926e41 100644 --- a/cmd/infra/aws/util/errors.go +++ b/cmd/infra/aws/util/errors.go @@ -9,7 +9,8 @@ import ( ) func IsErrorRetryable(err error) bool { - if aggregate, isAggregate := err.(utilerrors.Aggregate); isAggregate { + var aggregate utilerrors.Aggregate + if errors.As(err, &aggregate) { if len(aggregate.Errors()) == 1 { err = aggregate.Errors()[0] } else { diff --git a/cmd/infra/aws/util/errors_test.go b/cmd/infra/aws/util/errors_test.go new file mode 100644 index 00000000000..2e2ed1fb536 --- /dev/null +++ b/cmd/infra/aws/util/errors_test.go @@ -0,0 +1,76 @@ +package util + +import ( + "errors" + "fmt" + "testing" + + . "github.com/onsi/gomega" + + "github.com/aws/aws-sdk-go-v2/config" + + utilerrors "k8s.io/apimachinery/pkg/util/errors" +) + +func TestIsErrorRetryable(t *testing.T) { + t.Parallel() + tests := []struct { + name string + err error + expected bool + }{ + { + name: "When error is a generic error it should be retryable", + err: errors.New("some transient error"), + expected: true, + }, + { + name: "When error is a credential load error it should not be retryable", + err: config.SharedConfigLoadError{}, + expected: false, + }, + { + name: "When error is a wrapped credential load error it should not be retryable", + err: fmt.Errorf("loading config: %w", config.SharedConfigLoadError{}), + expected: false, + }, + { + name: "When aggregate has single generic error it should be retryable", + err: utilerrors.NewAggregate([]error{errors.New("transient")}), + expected: true, + }, + { + name: "When aggregate has single credential load error it should not be retryable", + err: utilerrors.NewAggregate([]error{config.SharedConfigLoadError{}}), + expected: false, + }, + { + name: "When aggregate has only credential load errors it should not be retryable", + err: utilerrors.NewAggregate([]error{ + config.SharedConfigLoadError{}, + config.SharedConfigLoadError{}, + }), + expected: false, + }, + { + name: "When aggregate has mixed errors it should be retryable", + err: utilerrors.NewAggregate([]error{ + config.SharedConfigLoadError{}, + errors.New("some other error"), + }), + expected: true, + }, + { + name: "When wrapped aggregate has single generic error it should be retryable", + err: fmt.Errorf("operation failed: %w", utilerrors.NewAggregate([]error{errors.New("transient")})), + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(IsErrorRetryable(tt.err)).To(Equal(tt.expected)) + }) + } +} diff --git a/cmd/infra/azure/create_iam.go b/cmd/infra/azure/create_iam.go index 81b8ae2342c..4864236006a 100644 --- a/cmd/infra/azure/create_iam.go +++ b/cmd/infra/azure/create_iam.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "os" "github.com/openshift/hypershift/cmd/log" @@ -198,7 +199,7 @@ func (o *CreateIAMOptions) ensureResourceGroup(ctx context.Context, l logr.Logge } var respErr *azcore.ResponseError - if !errors.As(err, &respErr) || respErr.StatusCode != 404 { + if !errors.As(err, &respErr) || respErr.StatusCode != http.StatusNotFound { return fmt.Errorf("failed to check resource group %q: %w", o.ResourceGroupName, err) } diff --git a/cmd/infra/azure/rbac.go b/cmd/infra/azure/rbac.go index 2f72d35d412..f330c4dbdc2 100644 --- a/cmd/infra/azure/rbac.go +++ b/cmd/infra/azure/rbac.go @@ -117,7 +117,7 @@ func (r *RBACManager) assignRolesForComponents(ctx context.Context, opts *Create } for component, clientID := range components { - objectID, err := r.getObjectIDFromClientID(string(clientID), token) + objectID, err := r.getObjectIDFromClientID(ctx, string(clientID), token) if err != nil { return err } @@ -150,7 +150,7 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr } // Setup Data Plane MI role assignments - objectID, err := r.getObjectIDFromClientID(dataPlaneIdentities.ImageRegistryMSIClientID, token) + objectID, err := r.getObjectIDFromClientID(ctx, dataPlaneIdentities.ImageRegistryMSIClientID, token) if err != nil { return err } @@ -159,7 +159,7 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr return err } - objectID, err = r.getObjectIDFromClientID(dataPlaneIdentities.DiskMSIClientID, token) + objectID, err = r.getObjectIDFromClientID(ctx, dataPlaneIdentities.DiskMSIClientID, token) if err != nil { return err } @@ -168,7 +168,7 @@ func (r *RBACManager) AssignDataPlaneRoles(ctx context.Context, opts *CreateInfr return err } - objectID, err = r.getObjectIDFromClientID(dataPlaneIdentities.FileMSIClientID, token) + objectID, err = r.getObjectIDFromClientID(ctx, dataPlaneIdentities.FileMSIClientID, token) if err != nil { return err } @@ -366,7 +366,7 @@ func (r *RBACManager) getAzureToken() (azcore.AccessToken, error) { return token, nil } -func (r *RBACManager) getObjectIDFromClientID(clientID string, token azcore.AccessToken) (string, error) { +func (r *RBACManager) getObjectIDFromClientID(ctx context.Context, clientID string, token azcore.AccessToken) (string, error) { // Validate clientID is a UUID to prevent OData injection uuidPattern := regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) if !uuidPattern.MatchString(clientID) { @@ -377,7 +377,7 @@ func (r *RBACManager) getObjectIDFromClientID(clientID string, token azcore.Acce url := graphAPIEndpoint + "?" + strings.ReplaceAll(filterQuery, " ", "%20") // Make the API request - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return "", fmt.Errorf("failed to create request: %w", err) } diff --git a/cmd/infra/powervs/create.go b/cmd/infra/powervs/create.go index 68a915e252a..b787bdd01ef 100644 --- a/cmd/infra/powervs/create.go +++ b/cmd/infra/powervs/create.go @@ -963,7 +963,7 @@ func (infra *Infra) createVpc(ctx context.Context, logger logr.Logger, options * }) if err != nil { - return nil, fmt.Errorf("error attaching inbound security group rule to allow %d to vpc %v", port, err) + return nil, fmt.Errorf("error attaching inbound security group rule to allow %d to vpc %w", port, err) } } diff --git a/cmd/infra/powervs/destroy.go b/cmd/infra/powervs/destroy.go index 1c15c2c5895..7ba387596f6 100644 --- a/cmd/infra/powervs/destroy.go +++ b/cmd/infra/powervs/destroy.go @@ -3,6 +3,7 @@ package powervs import ( "context" "encoding/json" + coreerrors "errors" "fmt" "net" "net/http" @@ -768,7 +769,8 @@ func deleteCOSBucket(ctx context.Context, bucketName string, cosClient *s3.S3) e if _, err := cosClient.DeleteBucketWithContext(ctx, &s3.DeleteBucketInput{ Bucket: aws.String(bucketName), }); err != nil { - if aerr, ok := err.(awserr.Error); ok { + var aerr awserr.Error + if coreerrors.As(err, &aerr) { if aerr.Code() != s3.ErrCodeNoSuchBucket { return err } @@ -886,7 +888,7 @@ func destroyTransitGateway(ctx context.Context, logger logr.Logger, options *Des TransitGatewayID: tg.ID, }) if err != nil { - return fmt.Errorf("error retrieving transit gateway connection list: %v", err) + return fmt.Errorf("error retrieving transit gateway connection list: %w", err) } for _, tgConn := range tgConnList.Connections { @@ -895,7 +897,7 @@ func destroyTransitGateway(ctx context.Context, logger logr.Logger, options *Des TransitGatewayID: tg.ID, ID: tgConn.ID, }); err != nil { - return fmt.Errorf("error deleting transit gateway connection %s, %v", *tgConn.Name, err) + return fmt.Errorf("error deleting transit gateway connection %s, %w", *tgConn.Name, err) } } @@ -904,7 +906,7 @@ func destroyTransitGateway(ctx context.Context, logger logr.Logger, options *Des TransitGatewayID: tg.ID, }) if err != nil { - return false, fmt.Errorf("error retrieving transit gateway connection list: %v", err) + return false, fmt.Errorf("error retrieving transit gateway connection list: %w", err) } if len(tgConnList.Connections) > 0 { return false, nil @@ -915,7 +917,7 @@ func destroyTransitGateway(ctx context.Context, logger logr.Logger, options *Des logger.Info("Waiting for transit gateway connections to get deleted") if err = wait.PollUntilContextTimeout(ctx, time.Minute*1, time.Minute*10, true, f); err != nil { - return fmt.Errorf("error waiting for tranist gateway connections to get deleted: %v", err) + return fmt.Errorf("error waiting for tranist gateway connections to get deleted: %w", err) } logger.Info("Deleting transit gateway", "name", *tg.Name) diff --git a/cmd/kubeconfig/create.go b/cmd/kubeconfig/create.go index 7fb8e6d89eb..2c7973c71cc 100644 --- a/cmd/kubeconfig/create.go +++ b/cmd/kubeconfig/create.go @@ -114,7 +114,7 @@ func Render(ctx context.Context, namespace string, name string, portForward bool }, } if err := c.Get(ctx, client.ObjectKeyFromObject(&kubeConfigSecret), &kubeConfigSecret); err != nil { - return fmt.Errorf("failed to get kubeconfig secret %s: %s", client.ObjectKeyFromObject(&kubeConfigSecret), err) + return fmt.Errorf("failed to get kubeconfig secret %s: %w", client.ObjectKeyFromObject(&kubeConfigSecret), err) } data, hasData := kubeConfigSecret.Data["kubeconfig"] if !hasData || len(data) == 0 { diff --git a/cmd/nodepool/core/create.go b/cmd/nodepool/core/create.go index 199c32c86fa..a623359bb7f 100644 --- a/cmd/nodepool/core/create.go +++ b/cmd/nodepool/core/create.go @@ -195,12 +195,13 @@ func validateHostedClusterPayloadSupportsNodePoolCPUArch(ctx context.Context, cl logger := ctrl.LoggerFrom(ctx) hc := &hyperv1.HostedCluster{} - err := client.Get(context.Background(), types.NamespacedName{Namespace: namespace, Name: name}, hc) + err := client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, hc) if err != nil { - // This is expected to happen when we create a cluster since there is no created HostedCluster CR to check the - // payload from. - logger.Info("WARNING: failed to get HostedCluster to check payload type") - return nil + if apierrors.IsNotFound(err) { + logger.Info("WARNING: failed to get HostedCluster to check payload type") + return nil + } + return fmt.Errorf("failed to get HostedCluster to check payload type: %w", err) } if hc.Status.PayloadArch == "" { diff --git a/cmd/nodepool/core/create_test.go b/cmd/nodepool/core/create_test.go index 75c048d7257..9dc25ea84ac 100644 --- a/cmd/nodepool/core/create_test.go +++ b/cmd/nodepool/core/create_test.go @@ -1,6 +1,8 @@ package core import ( + "context" + "fmt" "testing" . "github.com/onsi/gomega" @@ -16,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) func TestValidateHostedClusterPayloadSupportsNodePoolCPUArch(t *testing.T) { @@ -104,6 +107,23 @@ func TestValidateHostedClusterPayloadSupportsNodePoolCPUArch(t *testing.T) { } }) } + + t.Run("When client.Get fails with a non-NotFound error it should return the error", func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return fmt.Errorf("API server unavailable") + }, + }). + Build() + + err := validateHostedClusterPayloadSupportsNodePoolCPUArch(t.Context(), c, "hc", "clusters", hyperv1.ArchitectureAMD64) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(ContainSubstring("failed to get HostedCluster to check payload type"))) + }) } func TestValidMinorVersionCompatibility(t *testing.T) { diff --git a/control-plane-operator/controllers/gcpprivateserviceconnect/dns.go b/control-plane-operator/controllers/gcpprivateserviceconnect/dns.go index b3050652aff..126261156f2 100644 --- a/control-plane-operator/controllers/gcpprivateserviceconnect/dns.go +++ b/control-plane-operator/controllers/gcpprivateserviceconnect/dns.go @@ -17,6 +17,7 @@ package gcpprivateserviceconnect import ( "context" + "errors" "fmt" "os" "regexp" @@ -72,7 +73,8 @@ func ensureDNSDot(name string) string { // isNotFound checks if a GCP API error indicates a resource was not found. func isNotFound(err error) bool { - if apiErr, ok := err.(*googleapi.Error); ok { + var apiErr *googleapi.Error + if errors.As(err, &apiErr) { return apiErr.Code == 404 // HTTP 404 Not Found } // Fallback: check if error message contains "404" or "not found" patterns diff --git a/control-plane-operator/controllers/gcpprivateserviceconnect/dns_test.go b/control-plane-operator/controllers/gcpprivateserviceconnect/dns_test.go index 88b5c7b9e48..a6f2a706a02 100644 --- a/control-plane-operator/controllers/gcpprivateserviceconnect/dns_test.go +++ b/control-plane-operator/controllers/gcpprivateserviceconnect/dns_test.go @@ -2,6 +2,7 @@ package gcpprivateserviceconnect import ( "errors" + "fmt" "testing" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" @@ -103,6 +104,16 @@ func TestIsNotFound(t *testing.T) { err: errors.New("RESOURCE NOT FOUND"), expected: true, }, + { + name: "When error is a wrapped googleapi 404 it should return true", + err: fmt.Errorf("operation failed: %w", &googleapi.Error{Code: 404, Message: "not found"}), + expected: true, + }, + { + name: "When error is a wrapped googleapi 500 it should return false", + err: fmt.Errorf("operation failed: %w", &googleapi.Error{Code: 500, Message: "internal error"}), + expected: false, + }, { name: "When error is generic without 404 it should return false", err: errors.New("connection timeout"), diff --git a/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go b/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go index c1918c4a8de..f3168810ad1 100644 --- a/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go +++ b/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go @@ -899,7 +899,8 @@ func (r *GCPPrivateServiceConnectReconciler) handleGCPError(ctx context.Context, var requeueAfter time.Duration var message string - if googleErr, ok := err.(*googleapi.Error); ok { + var googleErr *googleapi.Error + if errors.As(err, &googleErr) { switch googleErr.Code { case 429: // Rate limit requeueAfter = time.Minute * 5 @@ -943,7 +944,8 @@ func (r *GCPPrivateServiceConnectReconciler) handleGCPError(ctx context.Context, // isNotFoundError checks if the error is a GCP "not found" error func isNotFoundError(err error) bool { - if googleErr, ok := err.(*googleapi.Error); ok { + var googleErr *googleapi.Error + if errors.As(err, &googleErr) { return googleErr.Code == 404 } return false diff --git a/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go b/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go index 01b58a9e359..70641acdc81 100644 --- a/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go +++ b/control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller_test.go @@ -3,6 +3,7 @@ package gcpprivateserviceconnect import ( "context" "errors" + "fmt" "testing" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" @@ -21,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" ) func TestConstructEndpointName(t *testing.T) { @@ -257,8 +259,26 @@ func TestIsNotFoundError(t *testing.T) { err: assert.AnError, expected: false, }, - // Note: We can't easily test the GCP API error case without importing the full GCP SDK - // and creating mock errors, but the logic is straightforward + { + name: "When given a GCP 404 error it should return true", + err: &googleapi.Error{Code: 404, Message: "not found"}, + expected: true, + }, + { + name: "When given a GCP 500 error it should return false", + err: &googleapi.Error{Code: 500, Message: "internal error"}, + expected: false, + }, + { + name: "When given a wrapped GCP 404 error it should return true", + err: fmt.Errorf("operation failed: %w", &googleapi.Error{Code: 404, Message: "not found"}), + expected: true, + }, + { + name: "When given a wrapped GCP 500 error it should return false", + err: fmt.Errorf("operation failed: %w", &googleapi.Error{Code: 500, Message: "internal error"}), + expected: false, + }, } for _, tt := range tests { diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index 629ab0e2b60..3aa22bd6c16 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go @@ -865,7 +865,7 @@ func (r *HostedControlPlaneReconciler) reconcileControlPlaneVersionStatus(ctx co // Persist the Partial entry before returning the error. hostedControlPlane.Status.ControlPlaneVersion = ensureControlPlaneVersionPartial(hostedControlPlane, clk, releaseImage.Version(), resolvedImage) if patchErr := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); patchErr != nil { - return fmt.Errorf("failed to patch status after component list failure: %w (list error: %v)", patchErr, listErr) + return fmt.Errorf("failed to patch status after component list failure: %w (list error: %w)", patchErr, listErr) } return fmt.Errorf("failed to list control plane components for version reconciliation: %w", listErr) } @@ -944,9 +944,9 @@ func (r *HostedControlPlaneReconciler) healthCheckKASLoadBalancers(ctx context.C // When the cluster is private, checking the load balancers will depend on whether the load balancer is // using the right subnets. To avoid uncertainty, we'll limit the check to the service endpoint. if hcp.Spec.Platform.Type == hyperv1.IBMCloudPlatform { - return healthCheckKASEndpoint(manifests.KubeAPIServerService("").Name, config.KASSVCIBMCloudPort) + return healthCheckKASEndpoint(ctx, manifests.KubeAPIServerService("").Name, config.KASSVCIBMCloudPort) } - return healthCheckKASEndpoint(manifests.KubeAPIServerService("").Name, config.KASSVCPort) + return healthCheckKASEndpoint(ctx, manifests.KubeAPIServerService("").Name, config.KASSVCPort) case serviceStrategy.Type == hyperv1.Route: if hcp.Spec.Platform.Type != hyperv1.IBMCloudPlatform { externalRoute := manifests.KubeAPIServerExternalPublicRoute(hcp.Namespace) @@ -958,7 +958,7 @@ func (r *HostedControlPlaneReconciler) healthCheckKASLoadBalancers(ctx context.C if err != nil { return err } - return healthCheckKASEndpoint(endpoint, port) + return healthCheckKASEndpoint(ctx, endpoint, port) } case serviceStrategy.Type == hyperv1.LoadBalancer: svc := manifests.KubeAPIServerService(hcp.Namespace) @@ -987,20 +987,25 @@ func (r *HostedControlPlaneReconciler) healthCheckKASLoadBalancers(ctx context.C } else if LBIngress.IP != "" { ingressPoint = LBIngress.IP } - return healthCheckKASEndpoint(ingressPoint, port) + return healthCheckKASEndpoint(ctx, ingressPoint, port) } return nil } -func healthCheckKASEndpoint(ingressPoint string, port int) error { +func healthCheckKASEndpoint(ctx context.Context, ingressPoint string, port int) error { healthEndpoint := fmt.Sprintf("https://%s:%d/healthz", ingressPoint, port) httpClient := util.InsecureHTTPClient() httpClient.Timeout = 10 * time.Second - resp, err := httpClient.Get(healthEndpoint) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthEndpoint, nil) if err != nil { return err } + resp, err := httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("APIServer endpoint %s is not healthy", ingressPoint) @@ -1454,14 +1459,14 @@ func (r *HostedControlPlaneReconciler) reconcileKonnectivityCerts(ctx context.Co if _, err := createOrUpdate(ctx, r, konnectivitySigner, func() error { return pki.ReconcileKonnectivitySignerSecret(konnectivitySigner, p.OwnerRef) }); err != nil { - return fmt.Errorf("failed to reconcile konnectivity signer secret: %v", err) + return fmt.Errorf("failed to reconcile konnectivity signer secret: %w", err) } konnectivityCACM := manifests.KonnectivityCAConfigMap(hcp.Namespace) if _, err := createOrUpdate(ctx, r, konnectivityCACM, func() error { return pki.ReconcileKonnectivityConfigMap(konnectivityCACM, p.OwnerRef, konnectivitySigner) }); err != nil { - return fmt.Errorf("failed to reconcile konnectivity CA config map: %v", err) + return fmt.Errorf("failed to reconcile konnectivity CA config map: %w", err) } konnectivityServerSecret := manifests.KonnectivityServerSecret(hcp.Namespace) @@ -2133,7 +2138,7 @@ func (r *HostedControlPlaneReconciler) reconcileCoreIgnitionConfig(ctx context.C } data, hasSSHKeyData := sshKeySecret.Data["id_rsa.pub"] if !hasSSHKeyData { - return fmt.Errorf("SSH key secret secret %s is missing the id_rsa.pub key", hcp.Spec.SSHKey.Name) + return fmt.Errorf("SSH key secret %s is missing the id_rsa.pub key", hcp.Spec.SSHKey.Name) } sshKey = string(data) } @@ -3181,11 +3186,11 @@ func (r *HostedControlPlaneReconciler) verifyResourceGroupLocationsMatch(ctx con certPath := config.ManagedAzureCertificatePath + hcp.Spec.Platform.Azure.AzureAuthenticationConfig.ManagedIdentities.ControlPlane.ControlPlaneOperator.CredentialsSecretName cloudConfig, err := hyperazureutil.GetAzureCloudConfiguration(hcp.Spec.Platform.Azure.Cloud) if err != nil { - return fmt.Errorf("failed to get Azure cloud configuration: %v", err) + return fmt.Errorf("failed to get Azure cloud configuration: %w", err) } creds, err = dataplane.NewUserAssignedIdentityCredential(ctx, certPath, dataplane.WithClientOpts(azcore.ClientOptions{Cloud: cloudConfig}), dataplane.WithLogger(&log)) if err != nil { - return fmt.Errorf("failed to create azure creds to verify resource group locations: %v", err) + return fmt.Errorf("failed to create azure creds to verify resource group locations: %w", err) } r.cpoAzureCredentialsLoaded.Store(key, creds) @@ -3201,17 +3206,17 @@ func (r *HostedControlPlaneReconciler) verifyResourceGroupLocationsMatch(ctx con // Retrieve full vnet information from the VNET ID vnet, err := hyperazureutil.GetVnetInfoFromVnetID(ctx, hcp.Spec.Platform.Azure.VnetID, hcp.Spec.Platform.Azure.SubscriptionID, creds, cloudName) if err != nil { - return fmt.Errorf("failed to get vnet info to verify its location: %v", err) + return fmt.Errorf("failed to get vnet info to verify its location: %w", err) } // Retrieve full network security group information from the network security group ID nsg, err := hyperazureutil.GetNetworkSecurityGroupInfo(ctx, hcp.Spec.Platform.Azure.SecurityGroupID, hcp.Spec.Platform.Azure.SubscriptionID, creds, cloudName) if err != nil { - return fmt.Errorf("failed to get network security group info to verify its location: %v", err) + return fmt.Errorf("failed to get network security group info to verify its location: %w", err) } // Retrieve full resource group information from the resource group name rg, err := hyperazureutil.GetResourceGroupInfo(ctx, hcp.Spec.Platform.Azure.ResourceGroupName, hcp.Spec.Platform.Azure.SubscriptionID, creds, cloudName) if err != nil { - return fmt.Errorf("failed to get resource group info to verify its location: %v", err) + return fmt.Errorf("failed to get resource group info to verify its location: %w", err) } // Verify the vnet resource group location, network security group resource group location, and the managed resource group location match if ptr.Deref(vnet.Location, "") != ptr.Deref(nsg.Location, "") || ptr.Deref(nsg.Location, "") != ptr.Deref(rg.Location, "") { diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go index f6c060f3e05..7fbcdb4032c 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go @@ -4,7 +4,11 @@ import ( "context" _ "embed" "fmt" + "net" + "net/http" + "net/http/httptest" "sort" + "strconv" "strings" "testing" "time" @@ -891,7 +895,7 @@ func TestSetKASCustomKubeconfigStatus(t *testing.T) { c := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(objs...).WithStatusSubresource(&hyperv1.HostedControlPlane{}).Build() err := setKASCustomKubeconfigStatus(ctx, hcp, c) - g.Expect(err).To(BeNil(), fmt.Errorf("error setting custom kubeconfig status failed: %v", err)) + g.Expect(err).To(BeNil(), fmt.Errorf("error setting custom kubeconfig status failed: %w", err)) g.Expect(hcp.Status.CustomKubeconfig).To(Equal(tc.expectedStatus)) }) } @@ -4426,6 +4430,78 @@ func TestReconcileDeletion(t *testing.T) { } } +func TestHealthCheckKASEndpoint(t *testing.T) { + t.Parallel() + tests := []struct { + name string + handler http.HandlerFunc + cancelCtx bool + wantErr bool + errSubstr string + }{ + { + name: "When endpoint returns 200 OK, it should succeed", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + }, + { + name: "When endpoint returns 503, it should return an unhealthy error", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + }, + wantErr: true, + errSubstr: "is not healthy", + }, + { + name: "When context is canceled, it should return an error", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + cancelCtx: true, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + server := httptest.NewTLSServer(tt.handler) + defer server.Close() + + host, portStr, err := net.SplitHostPort(server.Listener.Addr().String()) + g.Expect(err).ToNot(HaveOccurred()) + port, err := strconv.Atoi(portStr) + g.Expect(err).ToNot(HaveOccurred()) + + ctx := t.Context() + if tt.cancelCtx { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + cancel() + } + + err = healthCheckKASEndpoint(ctx, host, port) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + if tt.errSubstr != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } + + t.Run("When the ingress point contains invalid characters, it should return a request creation error", func(t *testing.T) { + g := NewWithT(t) + err := healthCheckKASEndpoint(t.Context(), "host\x7f", 443) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("invalid control character")) + }) +} + // Compile-time assertion that fakeVersionImageMetadataProvider satisfies the interface. var _ util.ImageMetadataProvider = &fakeVersionImageMetadataProvider{} diff --git a/control-plane-operator/controllers/hostedcontrolplane/kas/auth.go b/control-plane-operator/controllers/hostedcontrolplane/kas/auth.go index ac14459a7f2..157b17a3a59 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/kas/auth.go +++ b/control-plane-operator/controllers/hostedcontrolplane/kas/auth.go @@ -38,7 +38,7 @@ func GenerateAuthConfig(spec *configv1.AuthenticationSpec, ctx context.Context, for _, provider := range spec.OIDCProviders { jwt, err := generateJWTForProvider(ctx, provider, c, namespace) if err != nil { - return nil, fmt.Errorf("generating JWT authenticator for provider %q: %v", provider.Name, err) + return nil, fmt.Errorf("generating JWT authenticator for provider %q: %w", provider.Name, err) } config.JWT = append(config.JWT, jwt) } @@ -50,17 +50,17 @@ func generateJWTForProvider(ctx context.Context, provider configv1.OIDCProvider, issuer, err := generateIssuer(ctx, provider.Issuer, client, namespace) if err != nil { - return out, fmt.Errorf("generating issuer: %v", err) + return out, fmt.Errorf("generating issuer: %w", err) } claimMappings, err := generateClaimMappings(provider.ClaimMappings, issuer.URL) if err != nil { - return out, fmt.Errorf("generating claim mappings: %v", err) + return out, fmt.Errorf("generating claim mappings: %w", err) } claimValidationRules, err := generateClaimValidationRules(provider.ClaimValidationRules...) if err != nil { - return out, fmt.Errorf("generating claim validation rules: %v", err) + return out, fmt.Errorf("generating claim validation rules: %w", err) } out.Issuer = issuer @@ -83,7 +83,7 @@ func generateIssuer(ctx context.Context, issuer configv1.TokenIssuer, client crc if len(issuer.CertificateAuthority.Name) > 0 { ca, err := getCertificateAuthorityFromConfigMap(ctx, client, issuer.CertificateAuthority.Name, namespace) if err != nil { - return out, fmt.Errorf("getting certificate authority for issuer: %v", err) + return out, fmt.Errorf("getting certificate authority for issuer: %w", err) } out.CertificateAuthority = ca } @@ -110,7 +110,7 @@ func generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL username, err := generateUsernameClaimMapping(claimMappings.Username, issuerURL) if err != nil { - return out, fmt.Errorf("generating username claim mapping: %v", err) + return out, fmt.Errorf("generating username claim mapping: %w", err) } groups := generateGroupsClaimMapping(claimMappings.Groups) @@ -121,12 +121,12 @@ func generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL if featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUIDAndExtraClaimMappings) { uid, err := generateUIDClaimMapping(claimMappings.UID) if err != nil { - return out, fmt.Errorf("generating uid claim mapping: %v", err) + return out, fmt.Errorf("generating uid claim mapping: %w", err) } extras, err := generateExtraClaimMapping(claimMappings.Extra...) if err != nil { - return out, fmt.Errorf("generating extra claim mapping: %v", err) + return out, fmt.Errorf("generating extra claim mapping: %w", err) } out.UID = uid @@ -193,7 +193,7 @@ func generateUIDClaimMapping(uid *configv1.TokenClaimOrExpressionMapping) (Claim case uid.Expression != "" && uid.Claim == "": err := validateClaimMappingExpression(uid.Expression) if err != nil { - return out, fmt.Errorf("validating CEL expression: %v", err) + return out, fmt.Errorf("validating CEL expression: %w", err) } out.Expression = uid.Expression case uid.Claim != "" && uid.Expression != "": @@ -233,7 +233,7 @@ func generateExtraMapping(extra configv1.ExtraMapping) (ExtraMapping, error) { err := validateExtraMappingExpression(extra.ValueExpression) if err != nil { - return out, fmt.Errorf("validating valueExpression: %v", err) + return out, fmt.Errorf("validating valueExpression: %w", err) } out.Key = extra.Key @@ -295,13 +295,13 @@ func validateExtraMappingExpression(expression string) error { func HCPAuthConfigToAPIServerAuthConfig(authConfig *AuthenticationConfiguration) (*apiserver.AuthenticationConfiguration, error) { outBytes, err := json.Marshal(authConfig) if err != nil { - return nil, fmt.Errorf("marshaling HCP auth config to JSON: %v", err) + return nil, fmt.Errorf("marshaling HCP auth config to JSON: %w", err) } apiserverAuthConfig := &apiserver.AuthenticationConfiguration{} err = json.Unmarshal(outBytes, apiserverAuthConfig) if err != nil { - return nil, fmt.Errorf("unmarshalling HCP auth config JSON to apiserver auth config: %v", err) + return nil, fmt.Errorf("unmarshalling HCP auth config JSON to apiserver auth config: %w", err) } return apiserverAuthConfig, nil diff --git a/control-plane-operator/controllers/hostedcontrolplane/kas/auth_test.go b/control-plane-operator/controllers/hostedcontrolplane/kas/auth_test.go new file mode 100644 index 00000000000..6c05da4451a --- /dev/null +++ b/control-plane-operator/controllers/hostedcontrolplane/kas/auth_test.go @@ -0,0 +1,652 @@ +package kas + +import ( + "testing" + + . "github.com/onsi/gomega" + + configv1 "github.com/openshift/api/config/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + crclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestGenerateAuthConfig(t *testing.T) { + t.Parallel() + tests := []struct { + name string + spec *configv1.AuthenticationSpec + wantErr bool + errSubstr string + }{ + { + name: "When spec is nil, it should return empty config without error", + spec: nil, + }, + { + name: "When issuer references a missing CA ConfigMap, it should return a wrapped error", + spec: &configv1.AuthenticationSpec{ + OIDCProviders: []configv1.OIDCProvider{ + { + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://test.example.com", + Audiences: []configv1.TokenAudience{"test-audience"}, + CertificateAuthority: configv1.ConfigMapNameReference{ + Name: "nonexistent-ca-configmap", + }, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + }, + }, + }, + wantErr: true, + errSubstr: "generating JWT authenticator for provider", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder().Build() + _, err := GenerateAuthConfig(tt.spec, t.Context(), c, "test-namespace") + + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + if tt.errSubstr != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateIssuer(t *testing.T) { + t.Parallel() + tests := []struct { + name string + issuer configv1.TokenIssuer + objects []crclient.Object + wantErr bool + errSubstr string + }{ + { + name: "When issuer has no CA reference, it should return issuer without error", + issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"aud1", "aud2"}, + }, + }, + { + name: "When issuer has CA reference and ConfigMap exists, it should return issuer with CA", + issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"aud1"}, + CertificateAuthority: configv1.ConfigMapNameReference{ + Name: "test-ca", + }, + }, + objects: []crclient.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ca", Namespace: "test-ns"}, + Data: map[string]string{"ca-bundle.crt": "-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----"}, + }, + }, + }, + { + name: "When issuer has CA reference but ConfigMap is missing, it should return a wrapped error", + issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"aud1"}, + CertificateAuthority: configv1.ConfigMapNameReference{ + Name: "nonexistent", + }, + }, + wantErr: true, + errSubstr: "getting certificate authority for issuer", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + c := fake.NewClientBuilder().WithObjects(tt.objects...).Build() + _, err := generateIssuer(t.Context(), tt.issuer, c, "test-ns") + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGetCertificateAuthorityFromConfigMap(t *testing.T) { + t.Parallel() + tests := []struct { + name string + caName string + objects []crclient.Object + wantErr bool + errSubstr string + }{ + { + name: "When ConfigMap does not exist, it should return a wrapped error", + caName: "nonexistent", + wantErr: true, + errSubstr: "failed to get issuer certificate authority configmap", + }, + { + name: "When ConfigMap exists but lacks ca-bundle.crt key, it should return an error", + caName: "test-ca", + objects: []crclient.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ca", Namespace: "test-ns"}, + Data: map[string]string{"wrong-key": "data"}, + }, + }, + wantErr: true, + errSubstr: "does not contain key", + }, + { + name: "When ConfigMap exists with ca-bundle.crt key, it should return the CA data", + caName: "test-ca", + objects: []crclient.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ca", Namespace: "test-ns"}, + Data: map[string]string{"ca-bundle.crt": "-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----"}, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + c := fake.NewClientBuilder().WithObjects(tt.objects...).Build() + _, err := getCertificateAuthorityFromConfigMap(t.Context(), c, tt.caName, "test-ns") + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateJWTForProvider(t *testing.T) { + t.Parallel() + tests := []struct { + name string + provider configv1.OIDCProvider + wantErr bool + errSubstr string + }{ + { + name: "When provider has valid issuer and claim mappings, it should return JWT without error", + provider: configv1.OIDCProvider{ + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"test-audience"}, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + }, + }, + { + name: "When claim mappings are invalid, it should return a wrapped error", + provider: configv1.OIDCProvider{ + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"test-audience"}, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.Prefix, + Prefix: nil, + }, + }, + }, + wantErr: true, + errSubstr: "generating claim mappings", + }, + { + name: "When claim validation rules are invalid, it should return a wrapped error", + provider: configv1.OIDCProvider{ + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"test-audience"}, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + ClaimValidationRules: []configv1.TokenClaimValidationRule{ + {Type: "InvalidType"}, + }, + }, + wantErr: true, + errSubstr: "generating claim validation rules", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + c := fake.NewClientBuilder().Build() + _, err := generateJWTForProvider(t.Context(), tt.provider, c, "test-namespace") + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateUsernameClaimMapping(t *testing.T) { + t.Parallel() + tests := []struct { + name string + username configv1.UsernameClaimMapping + issuerURL string + wantErr bool + errSubstr string + }{ + { + name: "When prefix policy is NoPrefix, it should return empty prefix", + username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + issuerURL: "https://issuer.example.com", + }, + { + name: "When prefix policy is Prefix but prefix is nil, it should return an error", + username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.Prefix, + Prefix: nil, + }, + issuerURL: "https://issuer.example.com", + wantErr: true, + errSubstr: "no prefix is specified", + }, + { + name: "When prefix policy is Prefix with value, it should use that prefix", + username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.Prefix, + Prefix: &configv1.UsernamePrefix{PrefixString: "myprefix"}, + }, + issuerURL: "https://issuer.example.com", + }, + { + name: "When prefix policy is NoOpinion and claim is email, it should use empty prefix", + username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoOpinion, + }, + issuerURL: "https://issuer.example.com", + }, + { + name: "When prefix policy is NoOpinion and claim is not email, it should use issuer URL prefix", + username: configv1.UsernameClaimMapping{ + Claim: "sub", + PrefixPolicy: configv1.NoOpinion, + }, + issuerURL: "https://issuer.example.com", + }, + { + name: "When prefix policy is unknown, it should return an error", + username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: "InvalidPolicy", + }, + issuerURL: "https://issuer.example.com", + wantErr: true, + errSubstr: "unknown prefix policy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateUsernameClaimMapping(tt.username, tt.issuerURL) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateClaimMappings(t *testing.T) { + t.Parallel() + tests := []struct { + name string + mappings configv1.TokenClaimMappings + issuerURL string + wantErr bool + errSubstr string + }{ + { + name: "When username mapping is valid, it should return mappings without error", + mappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + issuerURL: "https://issuer.example.com", + }, + { + name: "When username mapping has invalid prefix policy, it should return a wrapped error", + mappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.Prefix, + Prefix: nil, + }, + }, + issuerURL: "https://issuer.example.com", + wantErr: true, + errSubstr: "generating username claim mapping", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateClaimMappings(tt.mappings, tt.issuerURL) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateExtraMapping(t *testing.T) { + t.Parallel() + tests := []struct { + name string + extra configv1.ExtraMapping + wantErr bool + errSubstr string + }{ + { + name: "When valueExpression is valid, it should return mapping without error", + extra: configv1.ExtraMapping{ + Key: "example.com/foo", + ValueExpression: "claims.groups", + }, + }, + { + name: "When valueExpression is invalid CEL, it should return a wrapped error", + extra: configv1.ExtraMapping{ + Key: "example.com/foo", + ValueExpression: "this is not valid CEL !!!", + }, + wantErr: true, + errSubstr: "validating valueExpression", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateExtraMapping(tt.extra) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateUIDClaimMapping(t *testing.T) { + t.Parallel() + tests := []struct { + name string + uid *configv1.TokenClaimOrExpressionMapping + wantErr bool + errSubstr string + wantClaim string + }{ + { + name: "When uid is nil, it should default claim to sub", + uid: nil, + wantClaim: "sub", + }, + { + name: "When uid has only a claim, it should use that claim", + uid: &configv1.TokenClaimOrExpressionMapping{Claim: "email"}, + wantClaim: "email", + }, + { + name: "When uid has only a valid expression, it should use that expression", + uid: &configv1.TokenClaimOrExpressionMapping{Expression: "claims.sub"}, + }, + { + name: "When uid has an invalid CEL expression, it should return a wrapped error", + uid: &configv1.TokenClaimOrExpressionMapping{Expression: "invalid CEL !!!"}, + wantErr: true, + errSubstr: "validating CEL expression", + }, + { + name: "When uid has both claim and expression, it should return an error", + uid: &configv1.TokenClaimOrExpressionMapping{Claim: "sub", Expression: "claims.sub"}, + wantErr: true, + errSubstr: "uid mapping must set either claim or expression, not both", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + result, err := generateUIDClaimMapping(tt.uid) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + if tt.wantClaim != "" { + g.Expect(result.Claim).To(Equal(tt.wantClaim)) + } + } + }) + } +} + +func TestGenerateClaimValidationRule(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rule configv1.TokenClaimValidationRule + wantErr bool + errSubstr string + }{ + { + name: "When type is RequiredClaim with valid required claim, it should return rule without error", + rule: configv1.TokenClaimValidationRule{ + Type: configv1.TokenValidationRuleTypeRequiredClaim, + RequiredClaim: &configv1.TokenRequiredClaim{ + Claim: "iss", + RequiredValue: "https://issuer.example.com", + }, + }, + }, + { + name: "When type is RequiredClaim but requiredClaim is nil, it should return an error", + rule: configv1.TokenClaimValidationRule{ + Type: configv1.TokenValidationRuleTypeRequiredClaim, + }, + wantErr: true, + errSubstr: "requiredClaim is not set", + }, + { + name: "When type is unknown, it should return an error", + rule: configv1.TokenClaimValidationRule{ + Type: "InvalidType", + }, + wantErr: true, + errSubstr: "unknown claimValidationRule type", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateClaimValidationRule(tt.rule) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateClaimValidationRules(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rules []configv1.TokenClaimValidationRule + wantErr bool + wantCount int + }{ + { + name: "When all rules are valid, it should return rules without error", + rules: []configv1.TokenClaimValidationRule{ + { + Type: configv1.TokenValidationRuleTypeRequiredClaim, + RequiredClaim: &configv1.TokenRequiredClaim{ + Claim: "iss", + RequiredValue: "https://issuer.example.com", + }, + }, + }, + wantCount: 1, + }, + { + name: "When a rule is invalid, it should return an error", + rules: []configv1.TokenClaimValidationRule{ + { + Type: "InvalidType", + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + result, err := generateClaimValidationRules(tt.rules...) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(HaveLen(tt.wantCount)) + } + }) + } +} + +func TestGenerateExtraClaimMapping(t *testing.T) { + t.Parallel() + tests := []struct { + name string + extras []configv1.ExtraMapping + wantErr bool + wantCount int + }{ + { + name: "When all mappings are valid, it should return mappings without error", + extras: []configv1.ExtraMapping{ + {Key: "example.com/foo", ValueExpression: "claims.groups"}, + }, + wantCount: 1, + }, + { + name: "When a mapping has empty key, it should return an error", + extras: []configv1.ExtraMapping{ + {Key: "", ValueExpression: "claims.groups"}, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + result, err := generateExtraClaimMapping(tt.extras...) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(HaveLen(tt.wantCount)) + } + }) + } +} + +func TestHCPAuthConfigToAPIServerAuthConfig(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + input := &AuthenticationConfiguration{ + JWT: []JWTAuthenticator{ + { + Issuer: Issuer{ + URL: "https://issuer.example.com", + Audiences: []string{"test-audience"}, + }, + ClaimMappings: ClaimMappings{ + Username: PrefixedClaimOrExpression{ + Claim: "email", + Prefix: ptr.To(""), + }, + }, + }, + }, + } + + result, err := HCPAuthConfigToAPIServerAuthConfig(input) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.JWT).To(HaveLen(1)) + g.Expect(result.JWT[0].Issuer.URL).To(Equal("https://issuer.example.com")) +} diff --git a/control-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.go b/control-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.go index a9c83f2d168..ed2f1a724d5 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.go +++ b/control-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.go @@ -30,7 +30,7 @@ func (r *HostedControlPlaneReconciler) setupKASClientSigners( } if _, err := createOrUpdate(ctx, r, s, applyFunc); err != nil { - return nil, fmt.Errorf("failed to reconcile secret '%s/%s': %v", s.Namespace, s.Name, err) + return nil, fmt.Errorf("failed to reconcile secret '%s/%s': %w", s.Namespace, s.Name, err) } return s, nil } @@ -41,7 +41,7 @@ func (r *HostedControlPlaneReconciler) setupKASClientSigners( } if _, err := createOrUpdate(ctx, r, target, applyFunc); err != nil { - return nil, fmt.Errorf("failed to reconcile secret '%s/%s': %v", target.Namespace, target.Name, err) + return nil, fmt.Errorf("failed to reconcile secret '%s/%s': %w", target.Namespace, target.Name, err) } return target, nil } diff --git a/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.go b/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.go index 5704c079812..36a848fe668 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.go +++ b/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.go @@ -104,7 +104,7 @@ func ConvertIdentityProviders(ctx context.Context, identityProviders []configv1. } data, err := convertProviderConfigToIDPData(ctx, &idp.IdentityProviderConfig, providerConfigOverride, i, volumeMountInfo, kclient, namespace, false) if err != nil { - errs = append(errs, fmt.Errorf("failed to apply IDP %s config: %v", idp.Name, err)) + errs = append(errs, fmt.Errorf("failed to apply IDP %s config: %w", idp.Name, err)) continue } converted = append(converted, @@ -329,7 +329,7 @@ func convertOpenIDIDP(ctx context.Context, providerConfig *configv1.IdentityProv // to allow challenge-issuing flow if it's available on the OIDC side challengeFlowsAllowed, err := checkOIDCPasswordGrantFlow(ctx, kclient, openIDProvider.URLs.Token, openIDConfig.ClientID, namespace, openIDConfig.CA, openIDConfig.ClientSecret, skipKonnectivityDialer) if err != nil { - return nil, fmt.Errorf("error attempting password grant flow: %v", err) + return nil, fmt.Errorf("error attempting password grant flow: %w", err) } data.challenge = challengeFlowsAllowed } @@ -478,11 +478,10 @@ func discoverOpenIDURLs(ctx context.Context, kclient crclient.Client, issuer, ke reqCtx, cancel := context.WithTimeout(ctx, externalHTTPRequestTimeout) defer cancel() - req, err := http.NewRequest(http.MethodGet, wellKnown, nil) + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, wellKnown, nil) if err != nil { return nil, err } - req = req.WithContext(reqCtx) rt, err := transportForCARef(ctx, kclient, namespace, ca.Name, key, skipKonnectivityDialer) if err != nil { @@ -501,7 +500,7 @@ func discoverOpenIDURLs(ctx context.Context, kclient crclient.Client, issuer, ke metadata := &openIDProviderJSON{} if err := json.NewDecoder(resp.Body).Decode(metadata); err != nil { - return nil, fmt.Errorf("failed to decode metadata: %v", err) + return nil, fmt.Errorf("failed to decode metadata: %w", err) } for _, arg := range []struct { @@ -552,7 +551,7 @@ func checkOIDCPasswordGrantFlow(ctx context.Context, } err := kclient.Get(ctx, crclient.ObjectKeyFromObject(secret), secret) if err != nil { - return false, fmt.Errorf("couldn't get the referenced secret: %v", err) + return false, fmt.Errorf("couldn't get the referenced secret: %w", err) } // check whether we already attempted this not to send unnecessary login @@ -569,7 +568,7 @@ func checkOIDCPasswordGrantFlow(ctx context.Context, transport, err := transportForCARef(ctx, kclient, namespace, caRererence.Name, corev1.ServiceAccountRootCAKey, skipKonnectivityDialer) if err != nil { - return false, fmt.Errorf("couldn't get a transport for the referenced CA: %v", err) + return false, fmt.Errorf("couldn't get a transport for the referenced CA: %w", err) } // prepare the grant-checking query @@ -585,11 +584,10 @@ func checkOIDCPasswordGrantFlow(ctx context.Context, reqCtx, cancel := context.WithTimeout(ctx, externalHTTPRequestTimeout) defer cancel() - req, err := http.NewRequest("POST", tokenURL, body) + req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, tokenURL, body) if err != nil { return false, err } - req = req.WithContext(reqCtx) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") // explicitly set Accept to 'application/json' as that's the expected deserializable output req.Header.Set("Accept", "application/json") diff --git a/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go b/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go index d96a6ce199e..c76051057ff 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go @@ -1122,3 +1122,22 @@ users: }) } } + +func TestConvertIdentityProviders_ErrorWrapping(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + providers := []configv1.IdentityProvider{ + { + Name: "bad-provider", + IdentityProviderConfig: configv1.IdentityProviderConfig{ + Type: "UnsupportedType", + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() + _, _, err := ConvertIdentityProviders(t.Context(), providers, nil, c, "test-ns") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to apply IDP bad-provider config")) +} diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.go b/control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.go index 140a408fd43..450706c4a6e 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.go @@ -80,7 +80,7 @@ func LoadManifestInto(componentName string, fileName string, into client.Object) obj, gvk, err := hyperapi.AllMonitoringYamlSerializer.Decode(bytes, nil, into) if err != nil { - return nil, nil, fmt.Errorf("failed to load %s manifest: %v", filePath, err) + return nil, nil, fmt.Errorf("failed to load %s manifest: %w", filePath, err) } return obj.(client.Object), gvk, err } diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.go b/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.go index 1c6531d6b3a..3e7969ba166 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.go @@ -54,7 +54,7 @@ func adaptConfig(cpContext component.WorkloadContext, cm *corev1.ConfigMap) erro data := []byte(cm.Data[CloudConfigKey]) cloudConfig := &CloudConfig{} if err := yaml.Unmarshal(data, cloudConfig); err != nil { - return fmt.Errorf("failed to unmarshal CloudConfig: %v", err) + return fmt.Errorf("failed to unmarshal CloudConfig: %w", err) } if kubevirt := cpContext.HCP.Spec.Platform.Kubevirt; kubevirt != nil && kubevirt.Credentials != nil { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.go b/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.go index 73cdcdb9fff..7f7f5488593 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.go @@ -38,7 +38,7 @@ func adaptConfig(cpContext component.WorkloadContext, cm *corev1.ConfigMap) erro configData := &bytes.Buffer{} err := template.Execute(configData, config) if err != nil { - return fmt.Errorf("error while parsing ccm config map template %v", err) + return fmt.Errorf("error while parsing ccm config map template %w", err) } cm.Data[configKey] = configData.String() diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.go index 1be0732f1d2..19710668990 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.go @@ -147,7 +147,7 @@ func TestRewriteConfigInitContainer(t *testing.T) { testScript := buildTestScript(script, kubectlLog) - cmd := exec.Command("bash", "-c", testScript) + cmd := exec.CommandContext(t.Context(), "bash", "-c", testScript) cmd.Env = []string{ "KUBERNETES_SERVICE_HOST=" + tt.host, "KUBERNETES_SERVICE_PORT=" + tt.port, diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go b/control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go index 49de8fc17d3..4d4d740303c 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go @@ -23,7 +23,7 @@ func adaptStatefulSet(cpContext component.WorkloadContext, sts *appsv1.StatefulS ipv4, err := netutil.IsIPv4CIDR(hcp.Spec.Networking.ClusterNetwork[0].CIDR.String()) if err != nil { - return fmt.Errorf("error checking the ClusterNetworkCIDR: %v", err) + return fmt.Errorf("error checking the ClusterNetworkCIDR: %w", err) } podspec.UpdateContainer(ComponentName, sts.Spec.Template.Spec.Containers, func(c *corev1.Container) { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go index 79bf570f4f7..1be308ceceb 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go @@ -45,7 +45,7 @@ func adaptServingCertSecret(cpContext component.WorkloadContext, secret *corev1. if apierrors.IsNotFound(err) { return nil } - return fmt.Errorf("failed to get ignition ca-cert secret: %v", err) + return fmt.Errorf("failed to get ignition ca-cert secret: %w", err) } serviceStrategy := netutil.ServicePublishingStrategyByTypeForHCP(cpContext.HCP, hyperv1.Ignition) @@ -67,7 +67,7 @@ func adaptServingCertSecret(cpContext component.WorkloadContext, secret *corev1. if apierrors.IsNotFound(err) { return nil } - return fmt.Errorf("failed to get ignition route: %v", err) + return fmt.Errorf("failed to get ignition route: %w", err) } // The route must be admitted and assigned a host before we can generate certs if len(ignitionServerRoute.Status.Ingress) == 0 || len(ignitionServerRoute.Status.Ingress[0].Host) == 0 { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go index 714395d4f35..91b67d9397c 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go @@ -74,7 +74,7 @@ func GenerateAuthConfig(ctx context.Context, spec *configv1.AuthenticationSpec, for _, provider := range spec.OIDCProviders { jwt, err := generateJWTForProvider(ctx, provider, c, namespace) if err != nil { - return nil, fmt.Errorf("generating JWT authenticator for provider %q: %v", provider.Name, err) + return nil, fmt.Errorf("generating JWT authenticator for provider %q: %w", provider.Name, err) } config.JWT = append(config.JWT, jwt) } @@ -86,23 +86,23 @@ func generateJWTForProvider(ctx context.Context, provider configv1.OIDCProvider, issuer, err := generateIssuer(ctx, provider.Issuer, client, namespace) if err != nil { - return out, fmt.Errorf("generating issuer: %v", err) + return out, fmt.Errorf("generating issuer: %w", err) } claimMappings, err := generateClaimMappings(provider.ClaimMappings, issuer.URL) if err != nil { - return out, fmt.Errorf("generating claim mappings: %v", err) + return out, fmt.Errorf("generating claim mappings: %w", err) } claimValidationRules, err := generateClaimValidationRules(provider.ClaimValidationRules...) if err != nil { - return out, fmt.Errorf("generating claim validation rules: %v", err) + return out, fmt.Errorf("generating claim validation rules: %w", err) } if featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUpstreamParity) { userValidationRules, err := generateUserValidationRules(provider.UserValidationRules...) if err != nil { - return out, fmt.Errorf("generating userValidationRules for provider %q: %v", provider.Name, err) + return out, fmt.Errorf("generating userValidationRules for provider %q: %w", provider.Name, err) } out.UserValidationRules = userValidationRules } @@ -128,7 +128,7 @@ func generateIssuer(ctx context.Context, issuer configv1.TokenIssuer, client crc // Validate the URL scheme u, err := url.Parse(issuer.DiscoveryURL) if err != nil { - return out, fmt.Errorf("invalid discovery URL: %v", err) + return out, fmt.Errorf("invalid discovery URL: %w", err) } if strings.TrimRight(issuer.DiscoveryURL, "/") == strings.TrimRight(issuer.URL, "/") { return out, fmt.Errorf("discovery URL must not be identical to issuer URL") @@ -155,7 +155,7 @@ func generateIssuer(ctx context.Context, issuer configv1.TokenIssuer, client crc if len(issuer.CertificateAuthority.Name) > 0 { ca, err := getCertificateAuthorityFromConfigMap(ctx, client, issuer.CertificateAuthority.Name, namespace) if err != nil { - return out, fmt.Errorf("getting certificate authority for issuer: %v", err) + return out, fmt.Errorf("getting certificate authority for issuer: %w", err) } out.CertificateAuthority = ca } @@ -182,12 +182,12 @@ func generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL username, err := generateUsernameClaimMapping(claimMappings.Username, issuerURL) if err != nil { - return out, fmt.Errorf("generating username claim mapping: %v", err) + return out, fmt.Errorf("generating username claim mapping: %w", err) } groups, err := generateGroupsClaimMapping(claimMappings.Groups) if err != nil { - return out, fmt.Errorf("generating groups claim mapping: %v", err) + return out, fmt.Errorf("generating groups claim mapping: %w", err) } out.Username = username @@ -196,12 +196,12 @@ func generateClaimMappings(claimMappings configv1.TokenClaimMappings, issuerURL if featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUIDAndExtraClaimMappings) { uid, err := generateUIDClaimMapping(claimMappings.UID) if err != nil { - return out, fmt.Errorf("generating uid claim mapping: %v", err) + return out, fmt.Errorf("generating uid claim mapping: %w", err) } extras, err := generateExtraClaimMapping(claimMappings.Extra...) if err != nil { - return out, fmt.Errorf("generating extra claim mapping: %v", err) + return out, fmt.Errorf("generating extra claim mapping: %w", err) } out.UID = uid @@ -419,7 +419,7 @@ func generateUserValidationRules(rules ...configv1.TokenUserValidationRule) ([]U for _, r := range rules { uvr, err := generateUserValidationRule(r) if err != nil { - errs = append(errs, fmt.Errorf("generating userValidationRule: %v", err)) + errs = append(errs, fmt.Errorf("generating userValidationRule: %w", err)) continue } out = append(out, uvr) @@ -469,7 +469,7 @@ func validateAuthConfig(authConfig *AuthenticationConfiguration, disallowIssuers apiServerAuthConfig, err := HCPAuthConfigToAPIServerAuthConfig(authConfig) if err != nil { - return fmt.Errorf("converting from HCP auth config type to apiserver auth config type: %v", err) + return fmt.Errorf("converting from HCP auth config type to apiserver auth config type: %w", err) } fieldErrors := validation.ValidateAuthenticationConfiguration(celCompiler, apiServerAuthConfig, disallowIssuers) @@ -483,13 +483,13 @@ func validateAuthConfig(authConfig *AuthenticationConfiguration, disallowIssuers func HCPAuthConfigToAPIServerAuthConfig(authConfig *AuthenticationConfiguration) (*apiserver.AuthenticationConfiguration, error) { outBytes, err := json.Marshal(authConfig) if err != nil { - return nil, fmt.Errorf("marshaling HCP auth config to JSON: %v", err) + return nil, fmt.Errorf("marshaling HCP auth config to JSON: %w", err) } apiserverAuthConfig := &apiserver.AuthenticationConfiguration{} err = json.Unmarshal(outBytes, apiserverAuthConfig) if err != nil { - return nil, fmt.Errorf("unmarshalling HCP auth config JSON to apiserver auth config: %v", err) + return nil, fmt.Errorf("unmarshalling HCP auth config JSON to apiserver auth config: %w", err) } return apiserverAuthConfig, nil diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go index 85177fda298..fb9e592dc1a 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go @@ -3,8 +3,11 @@ package kas import ( "context" "encoding/json" + "strings" "testing" + . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/control-plane-operator/featuregates" controlplanecomponent "github.com/openshift/hypershift/support/controlplane-component" @@ -70,6 +73,7 @@ func TestGenerateAuthConfig(t *testing.T) { namespace string expectedAuthenticationConfiguration *AuthenticationConfiguration shouldError bool + errSubstr string featureGates []featuregate.Feature } @@ -138,6 +142,33 @@ func TestGenerateAuthConfig(t *testing.T) { }, shouldError: false, }, + { + name: "When issuer references a missing CA ConfigMap, it should return a wrapped error", + spec: &configv1.AuthenticationSpec{ + OIDCProviders: []configv1.OIDCProvider{ + { + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://test.example.com", + Audiences: []configv1.TokenAudience{"test-audience"}, + CertificateAuthority: configv1.ConfigMapNameReference{ + Name: "nonexistent-ca-configmap", + }, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + }, + }, + }, + client: fake.NewClientBuilder().Build(), + namespace: "test-namespace", + shouldError: true, + errSubstr: "generating JWT authenticator for provider", + }, { name: "When OIDC provider with CEL validation is provided, it should generate configuration with validation rules", ctx: context.Background(), @@ -228,7 +259,9 @@ func TestGenerateAuthConfig(t *testing.T) { case !tc.shouldError && err != nil: t.Fatalf("unexpected error: %v", err) case tc.shouldError && err != nil: - // as expected + if tc.errSubstr != "" && !strings.Contains(err.Error(), tc.errSubstr) { + t.Fatalf("expected error to contain %q, got: %v", tc.errSubstr, err) + } return } @@ -1766,3 +1799,386 @@ func TestAdaptAuthConfig(t *testing.T) { }) } } + +func TestGenerateClaimMappings(t *testing.T) { + t.Parallel() + tests := []struct { + name string + mappings configv1.TokenClaimMappings + issuerURL string + wantErr bool + errSubstr string + }{ + { + name: "When username mapping is valid, it should return mappings without error", + mappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + issuerURL: "https://issuer.example.com", + }, + { + name: "When username mapping has Prefix policy with nil prefix, it should return a wrapped error", + mappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.Prefix, + Prefix: nil, + }, + }, + issuerURL: "https://issuer.example.com", + wantErr: true, + errSubstr: "generating username claim mapping", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateClaimMappings(tt.mappings, tt.issuerURL) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateExtraMapping(t *testing.T) { + t.Parallel() + tests := []struct { + name string + extra configv1.ExtraMapping + wantErr bool + errSubstr string + }{ + { + name: "When key and valueExpression are valid, it should return mapping without error", + extra: configv1.ExtraMapping{ + Key: "example.com/foo", + ValueExpression: "claims.groups", + }, + }, + { + name: "When key is empty, it should return an error", + extra: configv1.ExtraMapping{ + Key: "", + ValueExpression: "claims.groups", + }, + wantErr: true, + errSubstr: "must specify a key", + }, + { + name: "When valueExpression is empty, it should return an error", + extra: configv1.ExtraMapping{ + Key: "example.com/foo", + ValueExpression: "", + }, + wantErr: true, + errSubstr: "must specify a valueExpression", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateExtraMapping(tt.extra) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateClaimValidationRule(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rule configv1.TokenClaimValidationRule + wantErr bool + errSubstr string + }{ + { + name: "When type is RequiredClaim with valid required claim, it should return rule without error", + rule: configv1.TokenClaimValidationRule{ + Type: configv1.TokenValidationRuleTypeRequiredClaim, + RequiredClaim: &configv1.TokenRequiredClaim{ + Claim: "aud", + RequiredValue: "my-audience", + }, + }, + }, + { + name: "When type is RequiredClaim but requiredClaim is nil, it should return an error", + rule: configv1.TokenClaimValidationRule{ + Type: configv1.TokenValidationRuleTypeRequiredClaim, + }, + wantErr: true, + errSubstr: "requiredClaim is not set", + }, + { + name: "When type is CEL with valid expression, it should return rule without error", + rule: configv1.TokenClaimValidationRule{ + Type: configv1.TokenValidationRuleTypeCEL, + CEL: configv1.TokenClaimValidationCELRule{ + Expression: "claims.email_verified == true", + Message: "email must be verified", + }, + }, + }, + { + name: "When type is CEL but expression is empty, it should return an error", + rule: configv1.TokenClaimValidationRule{ + Type: configv1.TokenValidationRuleTypeCEL, + CEL: configv1.TokenClaimValidationCELRule{}, + }, + wantErr: true, + errSubstr: "expression is not set", + }, + { + name: "When type is unknown, it should return an error", + rule: configv1.TokenClaimValidationRule{ + Type: "UnknownType", + }, + wantErr: true, + errSubstr: "unknown claimValidationRule type", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateClaimValidationRule(tt.rule) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateClaimValidationRules(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rules []configv1.TokenClaimValidationRule + wantErr bool + }{ + { + name: "When all rules are valid, it should return rules without error", + rules: []configv1.TokenClaimValidationRule{ + { + Type: configv1.TokenValidationRuleTypeRequiredClaim, + RequiredClaim: &configv1.TokenRequiredClaim{ + Claim: "aud", + RequiredValue: "my-audience", + }, + }, + { + Type: configv1.TokenValidationRuleTypeCEL, + CEL: configv1.TokenClaimValidationCELRule{ + Expression: "claims.email_verified == true", + Message: "email must be verified", + }, + }, + }, + }, + { + name: "When a rule is invalid, it should return an error", + rules: []configv1.TokenClaimValidationRule{ + { + Type: configv1.TokenValidationRuleTypeRequiredClaim, + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateClaimValidationRules(tt.rules...) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateUserValidationRule(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rule configv1.TokenUserValidationRule + wantErr bool + errSubstr string + }{ + { + name: "When expression is valid, it should return rule without error", + rule: configv1.TokenUserValidationRule{ + Expression: "user.username != 'admin'", + Message: "admin not allowed", + }, + }, + { + name: "When expression is empty, it should return an error", + rule: configv1.TokenUserValidationRule{ + Expression: "", + Message: "should fail", + }, + wantErr: true, + errSubstr: "expression must be non-empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateUserValidationRule(tt.rule) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateUserValidationRules(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rules []configv1.TokenUserValidationRule + wantErr bool + }{ + { + name: "When all rules are valid, it should return rules without error", + rules: []configv1.TokenUserValidationRule{ + {Expression: "user.username != 'admin'", Message: "admin not allowed"}, + {Expression: "user.username != 'root'", Message: "root not allowed"}, + }, + }, + { + name: "When a rule has empty expression, it should return an error", + rules: []configv1.TokenUserValidationRule{ + {Expression: "user.username != 'admin'", Message: "ok"}, + {Expression: "", Message: "should fail"}, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := generateUserValidationRules(tt.rules...) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestGenerateJWTForProvider(t *testing.T) { + t.Parallel() + tests := []struct { + name string + provider configv1.OIDCProvider + wantErr bool + errSubstr string + }{ + { + name: "When username claim is empty, it should return a claim mappings error", + provider: configv1.OIDCProvider{ + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"aud"}, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{}, + }, + }, + wantErr: true, + errSubstr: "generating claim mappings", + }, + { + name: "When claim validation rule has unknown type, it should return a claim validation rules error", + provider: configv1.OIDCProvider{ + Name: "test-provider", + Issuer: configv1.TokenIssuer{ + URL: "https://issuer.example.com", + Audiences: []configv1.TokenAudience{"aud"}, + }, + ClaimMappings: configv1.TokenClaimMappings{ + Username: configv1.UsernameClaimMapping{ + Claim: "email", + PrefixPolicy: configv1.NoPrefix, + }, + }, + ClaimValidationRules: []configv1.TokenClaimValidationRule{ + {Type: "UnknownType"}, + }, + }, + wantErr: true, + errSubstr: "generating claim validation rules", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder().Build() + _, err := generateJWTForProvider(t.Context(), tt.provider, c, "test-namespace") + + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestHCPAuthConfigToAPIServerAuthConfig(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + input := &AuthenticationConfiguration{ + JWT: []JWTAuthenticator{ + { + Issuer: Issuer{ + URL: "https://issuer.example.com", + Audiences: []string{"test-audience"}, + }, + ClaimMappings: ClaimMappings{ + Username: PrefixedClaimOrExpression{ + Claim: "email", + Prefix: ptr.To(""), + }, + }, + }, + }, + } + + result, err := HCPAuthConfigToAPIServerAuthConfig(input) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.JWT).To(HaveLen(1)) + g.Expect(result.JWT[0].Issuer.URL).To(Equal("https://issuer.example.com")) +} diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go index cedcc84af82..a8f0aa04000 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go @@ -81,11 +81,11 @@ func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Dep bootstrapUpdateErrors := []error{} for _, bootstrapContainer := range bootstrapContainers { if err := updateBootstrapInitContainer(deployment, hcp, payloadVersion, bootstrapContainer); err != nil { - bootstrapUpdateErrors = append(bootstrapUpdateErrors, fmt.Errorf("updating bootstrap container %q: %v", bootstrapContainer, err)) + bootstrapUpdateErrors = append(bootstrapUpdateErrors, fmt.Errorf("updating bootstrap container %q: %w", bootstrapContainer, err)) } } if err := errors.Join(bootstrapUpdateErrors...); err != nil { - return fmt.Errorf("updating bootstrap containers: %v", err) + return fmt.Errorf("updating bootstrap containers: %w", err) } if hcp.Spec.Configuration.GetAuditPolicyConfig().Profile == configv1.NoneAuditProfileType { @@ -180,7 +180,7 @@ func updateMainContainer(podSpec *corev1.PodSpec, hcp *hyperv1.HostedControlPlan ) // We have to exempt the pod and service CIDR, otherwise the proxy will get respected by the transport inside - // the the egress transport and that breaks the egress selection/konnektivity usage. + // the egress transport and that breaks the egress selection/konnektivity usage. // Using a CIDR is not supported by Go's default ProxyFunc, but Kube uses a custom one by default that does support it: // https://github.com/kubernetes/kubernetes/blob/ab13c85316015cf9f115e29923ba9740bd1564fd/staging/src/k8s.io/apimachinery/pkg/util/net/http.go#L112-L114 var additionalNoProxyCIDRS []string diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go index 25dedd9fbb6..a04553b58b7 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go @@ -181,7 +181,7 @@ func adaptBootstrapKubeconfigSecret(cpContext component.WorkloadContext, secret func adaptAWSPodIdentityWebhookKubeconfigSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error { csrSigner := manifests.CSRSignerCASecret(cpContext.HCP.Namespace) if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(csrSigner), csrSigner); err != nil { - return fmt.Errorf("failed to get cluster-signer-ca secret: %v", err) + return fmt.Errorf("failed to get cluster-signer-ca secret: %w", err) } rootCA := manifests.RootCASecret(cpContext.HCP.Namespace) if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(rootCA), rootCA); err != nil { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.go index 4372e294eb6..984294240cb 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.go @@ -36,7 +36,7 @@ func adaptOauthMetadata(cpContext component.WorkloadContext, cfg *corev1.ConfigM var oauthMetadata map[string]interface{} if err := json.Unmarshal([]byte(cfg.Data[OauthMetadataConfigKey]), &oauthMetadata); err != nil { - return fmt.Errorf("failed to unmarshal oauth metadata: %v", err) + return fmt.Errorf("failed to unmarshal oauth metadata: %w", err) } oauthURL := fmt.Sprintf("https://%s:%d", cpContext.InfraStatus.OAuthHost, cpContext.InfraStatus.OAuthPort) @@ -46,7 +46,7 @@ func adaptOauthMetadata(cpContext component.WorkloadContext, cfg *corev1.ConfigM data, err := json.Marshal(oauthMetadata) if err != nil { - return fmt.Errorf("failed to marshal oauth metadata: %v", err) + return fmt.Errorf("failed to marshal oauth metadata: %w", err) } cfg.Data[OauthMetadataConfigKey] = string(data) return nil diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go new file mode 100644 index 00000000000..596c7cdb065 --- /dev/null +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go @@ -0,0 +1,55 @@ +package kas + +import ( + "testing" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + controlplanecomponent "github.com/openshift/hypershift/support/controlplane-component" + + corev1 "k8s.io/api/core/v1" + + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestAdaptOauthMetadata(t *testing.T) { + t.Parallel() + tests := []struct { + name string + cfg *corev1.ConfigMap + wantErr bool + errSubstr string + }{ + { + name: "When ConfigMap contains invalid JSON, it should return an unmarshal error", + cfg: &corev1.ConfigMap{ + Data: map[string]string{ + OauthMetadataConfigKey: "not-valid-json{{{", + }, + }, + wantErr: true, + errSubstr: "failed to unmarshal oauth metadata", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + cpContext := controlplanecomponent.WorkloadContext{ + Context: t.Context(), + HCP: &hyperv1.HostedControlPlane{}, + Client: fake.NewClientBuilder().Build(), + } + + err := adaptOauthMetadata(cpContext, tt.cfg) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.go index 61946ac37a8..5baaf8c1f7f 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.go @@ -96,7 +96,7 @@ func getKMSAPIVersion(cpContext component.WorkloadContext, secret *corev1.Secret if apierrors.IsNotFound(err) { return apiVersion, nil } - return "", fmt.Errorf("failed to get existing secret encryption config: %v", err) + return "", fmt.Errorf("failed to get existing secret encryption config: %w", err) } encryptionConfigBytes := secret.Data[secretEncryptionConfigurationKey] @@ -104,10 +104,10 @@ func getKMSAPIVersion(cpContext component.WorkloadContext, secret *corev1.Secret currentConfig := apiserverv1.EncryptionConfiguration{} gvks, _, err := api.Scheme.ObjectKinds(¤tConfig) if err != nil || len(gvks) == 0 { - return "", fmt.Errorf("cannot determine gvk of resource: %v", err) + return "", fmt.Errorf("cannot determine gvk of resource: %w", err) } if _, _, err = api.YamlSerializer.Decode(encryptionConfigBytes, &gvks[0], ¤tConfig); err != nil { - return "", fmt.Errorf("cannot decode resource: %v", err) + return "", fmt.Errorf("cannot decode resource: %w", err) } // Only look at write keys to return the APIVersion currently used. diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go index 82d2ea68d32..7f9d658ded6 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go @@ -2,9 +2,13 @@ package kas import ( "bytes" + "context" + "fmt" "testing" "time" + . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/config" @@ -16,7 +20,9 @@ import ( v1 "k8s.io/apiserver/pkg/apis/apiserver/v1" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "github.com/google/go-cmp/cmp" ) @@ -363,6 +369,78 @@ func TestReconcileKMSEncryptionConfigAzureSelfManaged(t *testing.T) { } } +func TestGetKMSAPIVersion(t *testing.T) { + t.Parallel() + tests := []struct { + name string + secret *corev1.Secret + client client.Client + wantErr bool + errSubstr string + wantResult string + }{ + { + name: "When client Get returns a non-NotFound error, it should return a wrapped error", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "encryption-config", Namespace: "test-ns"}, + }, + client: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ + Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return fmt.Errorf("connection refused") + }, + }).Build(), + wantErr: true, + errSubstr: "failed to get existing secret encryption config", + }, + { + name: "When secret contains invalid YAML, it should return a decode error", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "encryption-config", Namespace: "test-ns"}, + Data: map[string][]byte{ + secretEncryptionConfigurationKey: []byte("not-valid-yaml: {{{"), + }, + }, + wantErr: true, + errSubstr: "cannot decode resource", + }, + { + name: "When secret does not exist, it should return default v2", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "encryption-config", Namespace: "test-ns"}, + }, + wantResult: "v2", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := tt.client + if c == nil { + c = fake.NewClientBuilder().Build() + } + if tt.secret.Data != nil && tt.client == nil { + c = fake.NewClientBuilder().WithObjects(tt.secret).Build() + } + + cpContext := controlplanecomponent.WorkloadContext{ + Context: t.Context(), + Client: c, + } + + result, err := getKMSAPIVersion(cpContext, tt.secret) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(tt.wantResult)) + } + }) + } +} + func generateExpectedAzureEncryptionConfig(t testing.TB, apiVersion string) *v1.EncryptionConfiguration { t.Helper() activeKeyHash, err := util.HashStruct(hyperv1.AzureKMSKey{ diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go index 4a7dd1bc65b..e31e102fe6c 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go @@ -109,7 +109,7 @@ func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Dep kubeadminPasswordSecret := common.KubeadminPasswordSecret(deployment.Namespace) if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(kubeadminPasswordSecret), kubeadminPasswordSecret); err != nil { if !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to get kubeadmin password secret: %v", err) + return fmt.Errorf("failed to get kubeadmin password secret: %w", err) } delete(deployment.Spec.Template.ObjectMeta.Annotations, KubeadminSecretHashAnnotation) } else { diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.go b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.go index 4f5f85944c9..0668647ad8e 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.go @@ -104,7 +104,7 @@ func ConvertIdentityProviders(ctx context.Context, identityProviders []configv1. } data, err := convertProviderConfigToIDPData(ctx, &idp.IdentityProviderConfig, providerConfigOverride, i, volumeMountInfo, kclient, namespace, false) if err != nil { - errs = append(errs, fmt.Errorf("failed to apply IDP %s config: %v", idp.Name, err)) + errs = append(errs, fmt.Errorf("failed to apply IDP %s config: %w", idp.Name, err)) continue } converted = append(converted, @@ -429,7 +429,7 @@ func convertOpenIDIDP( skipKonnectivityDialer, ) if err != nil { - return nil, fmt.Errorf("error attempting password grant flow: %v", err) + return nil, fmt.Errorf("error attempting password grant flow: %w", err) } data.challenge = challengeFlowsAllowed } @@ -548,11 +548,10 @@ func discoverOpenIDURLs(ctx context.Context, kclient crclient.Reader, issuer, ke reqCtx, cancel := context.WithTimeout(ctx, externalHTTPRequestTimeout) defer cancel() - req, err := http.NewRequest(http.MethodGet, wellKnown, nil) + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, wellKnown, nil) if err != nil { return nil, err } - req = req.WithContext(reqCtx) rt, err := transportForCARef(ctx, kclient, namespace, ca.Name, key, skipKonnectivityDialer) if err != nil { @@ -571,7 +570,7 @@ func discoverOpenIDURLs(ctx context.Context, kclient crclient.Reader, issuer, ke metadata := &openIDProviderJSON{} if err := json.NewDecoder(resp.Body).Decode(metadata); err != nil { - return nil, fmt.Errorf("failed to decode metadata: %v", err) + return nil, fmt.Errorf("failed to decode metadata: %w", err) } for _, arg := range []struct { @@ -622,7 +621,7 @@ func checkOIDCPasswordGrantFlow(ctx context.Context, } err := kclient.Get(ctx, crclient.ObjectKeyFromObject(secret), secret) if err != nil { - return false, fmt.Errorf("couldn't get the referenced secret: %v", err) + return false, fmt.Errorf("couldn't get the referenced secret: %w", err) } // check whether we already attempted this not to send unnecessary login @@ -639,7 +638,7 @@ func checkOIDCPasswordGrantFlow(ctx context.Context, transport, err := transportForCARef(ctx, kclient, namespace, caRererence.Name, corev1.ServiceAccountRootCAKey, skipKonnectivityDialer) if err != nil { - return false, fmt.Errorf("couldn't get a transport for the referenced CA: %v", err) + return false, fmt.Errorf("couldn't get a transport for the referenced CA: %w", err) } // prepare the grant-checking query @@ -655,11 +654,10 @@ func checkOIDCPasswordGrantFlow(ctx context.Context, reqCtx, cancel := context.WithTimeout(ctx, externalHTTPRequestTimeout) defer cancel() - req, err := http.NewRequest("POST", tokenURL, body) + req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, tokenURL, body) if err != nil { return false, err } - req = req.WithContext(reqCtx) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") // explicitly set Accept to 'application/json' as that's the expected deserializable output req.Header.Set("Accept", "application/json") diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go index 24adb544a93..a5e6e474c9d 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go @@ -1122,3 +1122,22 @@ users: }) } } + +func TestConvertIdentityProviders_ErrorWrapping(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + providers := []configv1.IdentityProvider{ + { + Name: "bad-provider", + IdentityProviderConfig: configv1.IdentityProviderConfig{ + Type: "UnsupportedType", + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() + _, _, err := ConvertIdentityProviders(t.Context(), providers, nil, c, "test-ns") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to apply IDP bad-provider config")) +} diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.go b/control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.go index 9b49abd358b..5ac89a16c01 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.go @@ -35,7 +35,7 @@ func (c *catalogOptions) adaptCatalogDeployment(cpContext component.WorkloadCont existingDeployment := &appsv1.Deployment{} if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(deployment), existingDeployment); err != nil { if !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to get existing deployment: %v", err) + return fmt.Errorf("failed to get existing deployment: %w", err) } } else { // If deployment already exists, imagestream tag will already populate the container image @@ -96,7 +96,7 @@ func getCatalogImagesOverrides(cpContext component.WorkloadContext, capabilityIm digest, _, err := cpContext.ImageMetadataProvider.GetDigest(cpContext, imageRef.Exact(), pullSecret.Data[corev1.DockerConfigJsonKey]) if err != nil { - return nil, fmt.Errorf("failed to get manifest for image %s: %v", imageRef.Exact(), err) + return nil, fmt.Errorf("failed to get manifest for image %s: %w", imageRef.Exact(), err) } imageRef.ID = digest.String() diff --git a/control-plane-operator/endpoint-resolver/server_test.go b/control-plane-operator/endpoint-resolver/server_test.go index fa63d651f0b..4228864dffe 100644 --- a/control-plane-operator/endpoint-resolver/server_test.go +++ b/control-plane-operator/endpoint-resolver/server_test.go @@ -7,6 +7,8 @@ import ( "net/http/httptest" "testing" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1listers "k8s.io/client-go/listers/core/v1" @@ -43,11 +45,11 @@ func newFakePodLister(namespace string, pods []*corev1.Pod) corev1listers.PodNam func newResolveRequest(t *testing.T, selector map[string]string) *http.Request { t.Helper() + g := NewWithT(t) body, err := json.Marshal(ResolveRequest{Selector: selector}) - if err != nil { - t.Fatalf("failed to marshal request: %v", err) - } - req := httptest.NewRequest(http.MethodPost, resolvePath, bytes.NewReader(body)) + g.Expect(err).ToNot(HaveOccurred()) + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, resolvePath, bytes.NewReader(body)) + g.Expect(err).ToNot(HaveOccurred()) req.Header.Set("Content-Type", "application/json") return req } @@ -134,6 +136,7 @@ func TestResolverHandler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) lister := newFakePodLister("test-namespace", tt.pods) handler := newResolverHandler(lister) req := newResolveRequest(t, tt.selector) @@ -141,22 +144,15 @@ func TestResolverHandler(t *testing.T) { handler.ServeHTTP(rec, req) - if rec.Code != tt.expectedCode { - t.Errorf("expected status code %d, got %d: %s", tt.expectedCode, rec.Code, rec.Body.String()) - } + g.Expect(rec.Code).To(Equal(tt.expectedCode)) if tt.expectedPods != nil { var response ResolveResponse - if err := json.NewDecoder(rec.Body).Decode(&response); err != nil { - t.Fatalf("failed to decode response: %v", err) - } - if len(response.Pods) != len(tt.expectedPods) { - t.Fatalf("expected %d pods, got %d", len(tt.expectedPods), len(response.Pods)) - } + g.Expect(json.NewDecoder(rec.Body).Decode(&response)).To(Succeed()) + g.Expect(response.Pods).To(HaveLen(len(tt.expectedPods))) for i, expected := range tt.expectedPods { - if response.Pods[i].Name != expected.Name || response.Pods[i].IP != expected.IP { - t.Errorf("pod %d: expected %+v, got %+v", i, expected, response.Pods[i]) - } + g.Expect(response.Pods[i].Name).To(Equal(expected.Name)) + g.Expect(response.Pods[i].IP).To(Equal(expected.IP)) } } }) @@ -165,21 +161,22 @@ func TestResolverHandler(t *testing.T) { func TestResolverHandlerMethodNotAllowed(t *testing.T) { t.Run("When sending GET request it should return 405", func(t *testing.T) { + g := NewWithT(t) lister := newFakePodLister("test-namespace", nil) handler := newResolverHandler(lister) - req := httptest.NewRequest(http.MethodGet, resolvePath, nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, resolvePath, nil) + g.Expect(err).ToNot(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) - if rec.Code != http.StatusMethodNotAllowed { - t.Errorf("expected status code %d, got %d", http.StatusMethodNotAllowed, rec.Code) - } + g.Expect(rec.Code).To(Equal(http.StatusMethodNotAllowed)) }) } func TestResolverHandlerEmptySelector(t *testing.T) { t.Run("When selector is empty it should return 400", func(t *testing.T) { + g := NewWithT(t) lister := newFakePodLister("test-namespace", nil) handler := newResolverHandler(lister) req := newResolveRequest(t, map[string]string{}) @@ -187,23 +184,21 @@ func TestResolverHandlerEmptySelector(t *testing.T) { handler.ServeHTTP(rec, req) - if rec.Code != http.StatusBadRequest { - t.Errorf("expected status code %d, got %d", http.StatusBadRequest, rec.Code) - } + g.Expect(rec.Code).To(Equal(http.StatusBadRequest)) }) } func TestResolverHandlerInvalidBody(t *testing.T) { t.Run("When request body is invalid JSON it should return 400", func(t *testing.T) { + g := NewWithT(t) lister := newFakePodLister("test-namespace", nil) handler := newResolverHandler(lister) - req := httptest.NewRequest(http.MethodPost, resolvePath, bytes.NewReader([]byte("not json"))) + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, resolvePath, bytes.NewReader([]byte("not json"))) + g.Expect(err).ToNot(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) - if rec.Code != http.StatusBadRequest { - t.Errorf("expected status code %d, got %d", http.StatusBadRequest, rec.Code) - } + g.Expect(rec.Code).To(Equal(http.StatusBadRequest)) }) } diff --git a/control-plane-operator/hostedclusterconfigoperator/cmd.go b/control-plane-operator/hostedclusterconfigoperator/cmd.go index d5a34ea14fb..a201321fe7f 100644 --- a/control-plane-operator/hostedclusterconfigoperator/cmd.go +++ b/control-plane-operator/hostedclusterconfigoperator/cmd.go @@ -231,10 +231,10 @@ func (o *HostedClusterConfigOperator) Run(ctx context.Context) error { opt.Scheme = api.Scheme }) if err != nil { - return fmt.Errorf("cannot create control plane cluster: %v", err) + return fmt.Errorf("cannot create control plane cluster: %w", err) } if err := mgr.Add(cpCluster); err != nil { - return fmt.Errorf("cannot add CPCluster to manager: %v", err) + return fmt.Errorf("cannot add CPCluster to manager: %w", err) } var kubevirtInfraConfig *rest.Config if o.KubevirtInfraKubeconfig != "" { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile.go index d1b0161f188..bd06b42b091 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile.go @@ -128,7 +128,7 @@ func ReconcileDefaultIngressControllerCertSecret(certSecret *corev1.Secret, sour return fmt.Errorf("source secret %s/%s does not have a cert key", sourceSecret.Namespace, sourceSecret.Name) } if _, hasKeyKey := sourceSecret.Data[corev1.TLSPrivateKeyKey]; !hasKeyKey { - return fmt.Errorf("source secret %s/%s does not have a key key", sourceSecret.Namespace, sourceSecret.Name) + return fmt.Errorf("source secret %s/%s does not have the expected key", sourceSecret.Namespace, sourceSecret.Name) } certSecret.Data = map[string][]byte{} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile_test.go index 672f28640e5..7033877dee6 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile_test.go @@ -998,3 +998,64 @@ func TestConfigurationPriority(t *testing.T) { }) } } + +func TestReconcileDefaultIngressControllerCertSecret(t *testing.T) { + t.Parallel() + tests := []struct { + name string + sourceSecret *corev1.Secret + wantErr bool + errSubstr string + }{ + { + name: "When source secret has both cert and key, it should succeed", + sourceSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "source", Namespace: "test-ns"}, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte("cert-data"), + corev1.TLSPrivateKeyKey: []byte("key-data"), + }, + }, + }, + { + name: "When source secret is missing the cert key, it should return an error", + sourceSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "source", Namespace: "test-ns"}, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: []byte("key-data"), + }, + }, + wantErr: true, + errSubstr: "does not have a cert key", + }, + { + name: "When source secret is missing the private key, it should return an error", + sourceSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "source", Namespace: "test-ns"}, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte("cert-data"), + }, + }, + wantErr: true, + errSubstr: "does not have the expected key", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + certSecret := &corev1.Secret{} + err := ReconcileDefaultIngressControllerCertSecret(certSecret, tt.sourceSecret) + + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(certSecret.Data).To(HaveKeyWithValue(corev1.TLSCertKey, tt.sourceSecret.Data[corev1.TLSCertKey])) + g.Expect(certSecret.Data).To(HaveKeyWithValue(corev1.TLSPrivateKeyKey, tt.sourceSecret.Data[corev1.TLSPrivateKeyKey])) + } + }) + } +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.go index b7708238a77..c7b7bed7cee 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.go @@ -61,15 +61,15 @@ var ( // from being updated/deleted from the DataPlane side. func ReconcileKASValidatingAdmissionPolicies(ctx context.Context, hcp *hyperv1.HostedControlPlane, client client.Client, createOrUpdate upsert.CreateOrUpdateFN) error { if err := reconcileConfigValidatingAdmissionPolicy(ctx, hcp, client, createOrUpdate); err != nil { - return fmt.Errorf("failed to reconcile Config Validating Admission Policy: %v", err) + return fmt.Errorf("failed to reconcile Config Validating Admission Policy: %w", err) } if err := reconcileMirrorValidatingAdmissionPolicy(ctx, hcp, client, createOrUpdate); err != nil { - return fmt.Errorf("failed to reconcile Mirror Validating Admission Policies: %v", err) + return fmt.Errorf("failed to reconcile Mirror Validating Admission Policies: %w", err) } if err := reconcileInfraValidatingAdmissionPolicy(ctx, hcp, client, createOrUpdate); err != nil { - return fmt.Errorf("failed to reconcile Infrastructure Validating Admission Policy: %v", err) + return fmt.Errorf("failed to reconcile Infrastructure Validating Admission Policy: %w", err) } if err := reconcileConfigMapsValidatingAdmissionPolicy(ctx, client, createOrUpdate); err != nil { @@ -109,7 +109,7 @@ func reconcileConfigValidatingAdmissionPolicy(ctx context.Context, hcp *hyperv1. configAdmissionPolicy.Validations = []k8sadmissionv1.Validation{HCCOUserValidation} configAdmissionPolicy.MatchConstraints = constructPolicyMatchConstraints(configResources, configAPIVersion, configAPIGroup, []k8sadmissionv1.OperationType{"UPDATE", "DELETE"}) if err := configAdmissionPolicy.reconcileAdmissionPolicy(ctx, client, createOrUpdate); err != nil { - return fmt.Errorf("error reconciling Config Validating Admission Policy: %v", err) + return fmt.Errorf("error reconciling Config Validating Admission Policy: %w", err) } return nil @@ -128,7 +128,7 @@ func reconcileInfraValidatingAdmissionPolicy(ctx context.Context, _ *hyperv1.Hos infraAdmissionPolicy.Validations = []k8sadmissionv1.Validation{HCCOUserValidation} infraAdmissionPolicy.MatchConstraints = constructPolicyMatchConstraints(infraResources, infraAPIVersion, infraAPIGroup, []k8sadmissionv1.OperationType{"UPDATE", "DELETE"}) if err := infraAdmissionPolicy.reconcileAdmissionPolicy(ctx, client, createOrUpdate); err != nil { - return fmt.Errorf("error reconciling Infrastructure Validating Admission Policy: %v", err) + return fmt.Errorf("error reconciling Infrastructure Validating Admission Policy: %w", err) } return nil @@ -152,7 +152,7 @@ func reconcileMirrorValidatingAdmissionPolicy(ctx context.Context, hcp *hyperv1. mirrorAdmissionPolicy.Validations = []k8sadmissionv1.Validation{HCCOUserValidation} mirrorAdmissionPolicy.MatchConstraints = constructPolicyMatchConstraints(mirrorResources, mirrorAPIVersion, mirrorAPIGroup, allAdmissionPoliciesOperations) if err := mirrorAdmissionPolicy.reconcileAdmissionPolicy(ctx, client, createOrUpdate); err != nil { - return fmt.Errorf("error reconciling Mirror Validating Admission Policy: %v", err) + return fmt.Errorf("error reconciling Mirror Validating Admission Policy: %w", err) } // ICSP lives in other API, this is why we need to create another vap and vap-binding @@ -164,7 +164,7 @@ func reconcileMirrorValidatingAdmissionPolicy(ctx context.Context, hcp *hyperv1. icspAdmissionPolicy.Validations = []k8sadmissionv1.Validation{HCCOUserValidation} icspAdmissionPolicy.MatchConstraints = constructPolicyMatchConstraints(icspResources, icspAPIVersion, icspAPIGroup, allAdmissionPoliciesOperations) if err := icspAdmissionPolicy.reconcileAdmissionPolicy(ctx, client, createOrUpdate); err != nil { - return fmt.Errorf("error reconciling ICSP Validating Admission Policy: %v", err) + return fmt.Errorf("error reconciling ICSP Validating Admission Policy: %w", err) } return nil @@ -182,7 +182,7 @@ func reconcileConfigMapsValidatingAdmissionPolicy(ctx context.Context, client cl // we want to block changes only for configmaps with "hypershift.openshift.io/mirrored-config" label mirroredConfigsAdmissionPolicy.MatchConstraints.ObjectSelector = &metav1.LabelSelector{MatchLabels: map[string]string{nodepool.NTOMirroredConfigLabel: "true"}} if err := mirroredConfigsAdmissionPolicy.reconcileAdmissionPolicy(ctx, client, createOrUpdate); err != nil { - return fmt.Errorf("error reconciling mirrored ConfigMaps Validating Admission Policy: %v", err) + return fmt.Errorf("error reconciling mirrored ConfigMaps Validating Admission Policy: %w", err) } return nil } @@ -200,7 +200,7 @@ func (ap *AdmissionPolicy) reconcileAdmissionPolicy(ctx context.Context, client return nil }); err != nil { - return fmt.Errorf("failed to create/update Validating Admission Policy with name %s: %v", ap.Name, err) + return fmt.Errorf("failed to create/update Validating Admission Policy with name %s: %w", ap.Name, err) } policyBinding := manifests.ValidatingAdmissionPolicyBinding(fmt.Sprintf("%s-binding", ap.Name)) @@ -209,7 +209,7 @@ func (ap *AdmissionPolicy) reconcileAdmissionPolicy(ctx context.Context, client policyBinding.Spec.ValidationActions = []k8sadmissionv1.ValidationAction{k8sadmissionv1.Deny} return nil }); err != nil { - return fmt.Errorf("failed to create/update Validating Admission Policy Binding with name %s: %v", ap.Name, err) + return fmt.Errorf("failed to create/update Validating Admission Policy Binding with name %s: %w", ap.Name, err) } return nil diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies_test.go index 83f7a84b3b8..991eb8b6627 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies_test.go @@ -1,10 +1,22 @@ package kas import ( + "context" + "fmt" "testing" . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/upsert" + + k8sadmissionv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/google/cel-go/cel" ) @@ -127,3 +139,150 @@ func TestGenerateCelExpression(t *testing.T) { }) } } + +func failOnNthCreateOrUpdate(n int) upsert.CreateOrUpdateFN { + call := 0 + return func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + call++ + if call == n { + return controllerutil.OperationResultNone, fmt.Errorf("injected failure on call %d", n) + } + return upsert.New(false).CreateOrUpdate(ctx, c, obj, f) + } +} + +func TestReconcileAdmissionPolicy(t *testing.T) { + tests := []struct { + name string + createOrUpdate upsert.CreateOrUpdateFN + wantErr bool + errSubstr string + }{ + { + name: "When createOrUpdate succeeds, it should create VAP and binding without error", + createOrUpdate: upsert.New(false).CreateOrUpdate, + }, + { + name: "When VAP createOrUpdate fails, it should return a wrapped error", + createOrUpdate: func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + if _, ok := obj.(*k8sadmissionv1.ValidatingAdmissionPolicy); ok { + return controllerutil.OperationResultNone, fmt.Errorf("API conflict") + } + return upsert.New(false).CreateOrUpdate(ctx, c, obj, f) + }, + wantErr: true, + errSubstr: "failed to create/update Validating Admission Policy with name", + }, + { + name: "When binding createOrUpdate fails, it should return a wrapped error", + createOrUpdate: func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + if _, ok := obj.(*k8sadmissionv1.ValidatingAdmissionPolicyBinding); ok { + return controllerutil.OperationResultNone, fmt.Errorf("API conflict") + } + return upsert.New(false).CreateOrUpdate(ctx, c, obj, f) + }, + wantErr: true, + errSubstr: "failed to create/update Validating Admission Policy Binding with name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + scheme := fake.NewClientBuilder().Build().Scheme() + g.Expect(k8sadmissionv1.AddToScheme(scheme)).To(Succeed()) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + ap := &AdmissionPolicy{ + Name: "test-policy", + Validations: []k8sadmissionv1.Validation{{Expression: "true", Message: "allowed"}}, + MatchConstraints: &k8sadmissionv1.MatchResources{ + ResourceRules: []k8sadmissionv1.NamedRuleWithOperations{ + { + RuleWithOperations: k8sadmissionv1.RuleWithOperations{ + Operations: []k8sadmissionv1.OperationType{"CREATE"}, + }, + }, + }, + }, + } + + err := ap.reconcileAdmissionPolicy(t.Context(), c, tt.createOrUpdate) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestReconcileKASValidatingAdmissionPolicies(t *testing.T) { + tests := []struct { + name string + createOrUpdate upsert.CreateOrUpdateFN + wantErr bool + errSubstr string + }{ + { + name: "When all sub-reconcilers succeed, it should return no error", + createOrUpdate: upsert.New(false).CreateOrUpdate, + }, + { + name: "When Config VAP reconcile fails, it should return a wrapped error", + createOrUpdate: func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + return controllerutil.OperationResultNone, fmt.Errorf("injected failure") + }, + wantErr: true, + errSubstr: "failed to reconcile Config Validating Admission Policy", + }, + { + name: "When Mirror VAP reconcile fails, it should return a wrapped error", + createOrUpdate: failOnNthCreateOrUpdate(3), + wantErr: true, + errSubstr: "failed to reconcile Mirror Validating Admission Policies", + }, + { + name: "When ICSP VAP reconcile fails, it should return a wrapped ICSP error", + createOrUpdate: failOnNthCreateOrUpdate(5), + wantErr: true, + errSubstr: "error reconciling ICSP Validating Admission Policy", + }, + { + name: "When Infra VAP reconcile fails, it should return a wrapped error", + createOrUpdate: failOnNthCreateOrUpdate(7), + wantErr: true, + errSubstr: "failed to reconcile Infrastructure Validating Admission Policy", + }, + { + name: "When ConfigMaps VAP reconcile fails, it should return a wrapped ConfigMaps error", + createOrUpdate: failOnNthCreateOrUpdate(9), + wantErr: true, + errSubstr: "error reconciling mirrored ConfigMaps Validating Admission Policy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + scheme := fake.NewClientBuilder().Build().Scheme() + g.Expect(k8sadmissionv1.AddToScheme(scheme)).To(Succeed()) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-hcp", Namespace: "test-ns"}, + } + + err := ReconcileKASValidatingAdmissionPolicies(t.Context(), hcp, c, tt.createOrUpdate) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.go index 55bc7cc5d3d..d11d1d5c373 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.go @@ -39,7 +39,7 @@ var ( func ReconcileRegistryConfigValidatingAdmissionPolicies(ctx context.Context, hcp *hyperv1.HostedControlPlane, client client.Client, createOrUpdate upsert.CreateOrUpdateFN) error { if err := reconcileRegistryConfigManagementStateValidatingAdmissionPolicy(ctx, hcp, client, createOrUpdate); err != nil { - return fmt.Errorf("failed to reconcile ManagementState Validating Admission Policy: %v", err) + return fmt.Errorf("failed to reconcile ManagementState Validating Admission Policy: %w", err) } return nil @@ -66,7 +66,7 @@ func reconcileRegistryConfigManagementStateValidatingAdmissionPolicy(ctx context registryConfigManagementStateAdmissionPolicy.Validations = []k8sadmissionv1.Validation{denyRemovedManagementStateValidation} registryConfigManagementStateAdmissionPolicy.MatchConstraints = constructPolicyMatchConstraints(registryConfigManagementStateResources, registryConfigManagementStateAPIVersion, registryConfigManagementStateAPIGroup, []k8sadmissionv1.OperationType{"CREATE", "UPDATE"}) if err := registryConfigManagementStateAdmissionPolicy.reconcileAdmissionPolicy(ctx, client, createOrUpdate); err != nil { - return fmt.Errorf("error reconciling management State Validating Admission Policy: %v", err) + return fmt.Errorf("error reconciling management State Validating Admission Policy: %w", err) } return nil @@ -85,7 +85,7 @@ func (ap *AdmissionPolicy) reconcileAdmissionPolicy(ctx context.Context, client return nil }); err != nil { - return fmt.Errorf("failed to create/update Validating Admission Policy with name %s: %v", ap.Name, err) + return fmt.Errorf("failed to create/update Validating Admission Policy with name %s: %w", ap.Name, err) } policyBinding := manifests.ValidatingAdmissionPolicyBinding(fmt.Sprintf("%s-binding", ap.Name)) @@ -94,7 +94,7 @@ func (ap *AdmissionPolicy) reconcileAdmissionPolicy(ctx context.Context, client policyBinding.Spec.ValidationActions = []k8sadmissionv1.ValidationAction{k8sadmissionv1.Deny} return nil }); err != nil { - return fmt.Errorf("failed to create/update Validating Admission Policy Binding with name %s: %v", ap.Name, err) + return fmt.Errorf("failed to create/update Validating Admission Policy Binding with name %s: %w", ap.Name, err) } return nil diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies_test.go index 013dc5896f1..1d509f15275 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies_test.go @@ -2,6 +2,7 @@ package registry import ( "context" + "fmt" "testing" "time" @@ -150,9 +151,11 @@ func TestReconcileRegistryConfigManagementStateValidatingAdmissionPolicy(t *test func TestReconcileRegistryConfigValidatingAdmissionPolicies(t *testing.T) { tests := []struct { - name string - hcp *hyperv1.HostedControlPlane - expectError bool + name string + hcp *hyperv1.HostedControlPlane + createOrUpdate func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) + expectError bool + errSubstr string }{ { name: "When reconciliation succeeds it should return no error", @@ -175,33 +178,74 @@ func TestReconcileRegistryConfigValidatingAdmissionPolicies(t *testing.T) { }, expectError: false, }, + { + name: "When createOrUpdate fails for VAP, it should return a wrapped error", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-namespace", + }, + }, + createOrUpdate: func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + if _, ok := obj.(*k8sadmissionv1.ValidatingAdmissionPolicy); ok { + return controllerutil.OperationResultNone, fmt.Errorf("API conflict") + } + if err := f(); err != nil { + return controllerutil.OperationResultNone, err + } + return controllerutil.OperationResultCreated, nil + }, + expectError: true, + errSubstr: "failed to reconcile ManagementState Validating Admission Policy", + }, + { + name: "When createOrUpdate fails for binding, it should return a wrapped error", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-namespace", + }, + }, + createOrUpdate: func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + if _, ok := obj.(*k8sadmissionv1.ValidatingAdmissionPolicyBinding); ok { + return controllerutil.OperationResultNone, fmt.Errorf("API conflict") + } + if err := f(); err != nil { + return controllerutil.OperationResultNone, err + } + return controllerutil.OperationResultCreated, nil + }, + expectError: true, + errSubstr: "failed to create/update Validating Admission Policy Binding", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewGomegaWithT(t) - // Create a fake client scheme := hyperapi.Scheme fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() - // Create a mock createOrUpdate function - mockCreateOrUpdate := func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { - if err := f(); err != nil { - return controllerutil.OperationResultNone, err + createOrUpdate := tt.createOrUpdate + if createOrUpdate == nil { + createOrUpdate = func(ctx context.Context, c client.Client, obj client.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + if err := f(); err != nil { + return controllerutil.OperationResultNone, err + } + return controllerutil.OperationResultCreated, nil } - return controllerutil.OperationResultCreated, nil } - // Create a context with a logger ctx := ctrl.LoggerInto(context.Background(), ctrl.Log) - // Call the function - err := ReconcileRegistryConfigValidatingAdmissionPolicies(ctx, tt.hcp, fakeClient, mockCreateOrUpdate) + err := ReconcileRegistryConfigValidatingAdmissionPolicies(ctx, tt.hcp, fakeClient, createOrUpdate) - // Verify error expectations if tt.expectError { g.Expect(err).To(HaveOccurred()) + if tt.errSubstr != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } } else { g.Expect(err).NotTo(HaveOccurred()) } diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 1ee6a3a7180..38ff5a8ec35 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -511,12 +511,12 @@ func (r *reconciler) reconcileDeletion(ctx context.Context, log logr.Logger, hcp binding := manifests.ValidatingAdmissionPolicyBinding(fmt.Sprintf("%s-binding", registryConfigManagementStateAdmissionPolicy.Name)) if _, err := k8sutil.DeleteIfNeeded(ctx, r.client, binding); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to delete ValidatingAdmissionPolicyBinding %s: %v", binding.Name, err) + return ctrl.Result{}, fmt.Errorf("failed to delete ValidatingAdmissionPolicyBinding %s: %w", binding.Name, err) } vap := manifests.ValidatingAdmissionPolicy(registryConfigManagementStateAdmissionPolicy.Name) if _, err := k8sutil.DeleteIfNeeded(ctx, r.client, vap); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to delete ValidatingAdmissionPolicy %s: %v", vap.Name, err) + return ctrl.Result{}, fmt.Errorf("failed to delete ValidatingAdmissionPolicy %s: %w", vap.Name, err) } } @@ -1996,7 +1996,7 @@ func (r *reconciler) reconcileKubeadminPasswordHashSecret(ctx context.Context, h kubeadminPasswordSecret.Annotations[cpoauth.KubeadminSecretHashAnnotation] = string(kubeadminPasswordHashSecret.Data["kubeadmin"]) return nil }); err != nil { - return fmt.Errorf("failed to annotate kubeadmin-password secret in hcp namespace: %v", err) + return fmt.Errorf("failed to annotate kubeadmin-password secret in hcp namespace: %w", err) } return nil diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index b4d08dbb5c7..b461373d5e6 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -49,6 +49,7 @@ import ( controllerruntime "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/go-logr/logr" @@ -3108,10 +3109,12 @@ func TestReconcileDeletion(t *testing.T) { name string hcp *hyperv1.HostedControlPlane existingObjects []client.Object + interceptorFuncs *interceptor.Funcs expectVAPDeleted bool expectVAPBDeleted bool expectCloudCleanup bool expectError bool + errSubstr string }{ { name: "When platform is Azure, it should delete the registry management state VAP and binding", @@ -3237,13 +3240,44 @@ func TestReconcileDeletion(t *testing.T) { }, expectCloudCleanup: false, }, + { + name: "When Delete fails for the VAP binding on Azure, it should return a wrapped error", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "test-ns", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AzurePlatform, + }, + }, + }, + existingObjects: []client.Object{ + manifests.ValidatingAdmissionPolicyBinding(fmt.Sprintf("%s-binding", registry.AdmissionPolicyNameManagementState)), + }, + interceptorFuncs: &interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if obj.GetName() == fmt.Sprintf("%s-binding", registry.AdmissionPolicyNameManagementState) { + return fmt.Errorf("API server unavailable") + } + return c.Delete(ctx, obj, opts...) + }, + }, + expectError: true, + errSubstr: "failed to delete ValidatingAdmissionPolicyBinding", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - guestClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tt.existingObjects...).Build() + guestClientBuilder := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tt.existingObjects...) + if tt.interceptorFuncs != nil { + guestClientBuilder = guestClientBuilder.WithInterceptorFuncs(*tt.interceptorFuncs) + } + guestClient := guestClientBuilder.Build() cpClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tt.hcp).WithStatusSubresource(&hyperv1.HostedControlPlane{}).Build() r := &reconciler{ @@ -3269,6 +3303,9 @@ func TestReconcileDeletion(t *testing.T) { if tt.expectError { g.Expect(err).To(HaveOccurred()) + if tt.errSubstr != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } return } g.Expect(err).ToNot(HaveOccurred()) diff --git a/control-plane-operator/hostedclusterconfigoperator/operator/config.go b/control-plane-operator/hostedclusterconfigoperator/operator/config.go index e59c3b97463..ba50b2f61dd 100644 --- a/control-plane-operator/hostedclusterconfigoperator/operator/config.go +++ b/control-plane-operator/hostedclusterconfigoperator/operator/config.go @@ -223,7 +223,7 @@ func (c *HostedClusterConfigOperatorConfig) Start(ctx context.Context) error { for controllerName, setupFunc := range c.ControllerFuncs { c.Logger.Info("setting up controller", "controller", controllerName) if err := setupFunc(ctx, c); err != nil { - return fmt.Errorf("cannot setup controller %s: %v", controllerName, err) + return fmt.Errorf("cannot setup controller %s: %w", controllerName, err) } } return c.Manager.Start(ctx) diff --git a/control-plane-operator/metrics-proxy/proxy_test.go b/control-plane-operator/metrics-proxy/proxy_test.go index 1b50986984f..f571f36b84a 100644 --- a/control-plane-operator/metrics-proxy/proxy_test.go +++ b/control-plane-operator/metrics-proxy/proxy_test.go @@ -140,7 +140,8 @@ func TestProxyHandler_ServeHTTP(t *testing.T) { handler := newTestHandler(tt.components, tt.discoverer) - req := httptest.NewRequest(http.MethodGet, tt.path, nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, tt.path, nil) + g.Expect(err).NotTo(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) @@ -183,7 +184,8 @@ func TestProxyHandler_ServeHTTP(t *testing.T) { }, ) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd", nil) + g.Expect(err).NotTo(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) @@ -217,7 +219,8 @@ func TestProxyHandler_ServeHTTP(t *testing.T) { }, ) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd", nil) + g.Expect(err).NotTo(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) @@ -258,7 +261,8 @@ func TestProxyHandler_ServeHTTP(t *testing.T) { }, ) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd", nil) + g.Expect(err).NotTo(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) @@ -282,7 +286,8 @@ func TestProxyHandler_ServeHTTP(t *testing.T) { &fakeTargetDiscoverer{targets: []ScrapeTarget{}}, ) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd/", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd/", nil) + g.Expect(err).NotTo(HaveOccurred()) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) @@ -338,7 +343,8 @@ func TestRequireClientCert(t *testing.T) { t.Parallel() g := NewWithT(t) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd", nil) + g.Expect(err).NotTo(HaveOccurred()) req.TLS = nil rec := httptest.NewRecorder() @@ -352,7 +358,8 @@ func TestRequireClientCert(t *testing.T) { t.Parallel() g := NewWithT(t) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd", nil) + g.Expect(err).NotTo(HaveOccurred()) req.TLS = &tls.ConnectionState{VerifiedChains: nil} rec := httptest.NewRecorder() @@ -366,7 +373,8 @@ func TestRequireClientCert(t *testing.T) { t.Parallel() g := NewWithT(t) - req := httptest.NewRequest(http.MethodGet, "/metrics/etcd", nil) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/metrics/etcd", nil) + g.Expect(err).NotTo(HaveOccurred()) req.TLS = &tls.ConnectionState{ VerifiedChains: [][]*x509.Certificate{{}}, } diff --git a/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.go b/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.go index e449d5b9368..44766766e49 100644 --- a/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.go +++ b/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.go @@ -126,7 +126,7 @@ func (c *CertificateSigningController) processCertificateSigningRequest(ctx cont x509cr, err := certificates.ParseCSR(csr.Spec.Request) if err != nil { - return nil, false, nil, fmt.Errorf("unable to parse csr %q: %v", csr.Name, err) + return nil, false, nil, fmt.Errorf("unable to parse csr %q: %w", csr.Name, err) } if validationErr := c.validator(csr, x509cr); validationErr != nil { cfg := certificatesv1applyconfigurations.CertificateSigningRequest(name) @@ -158,7 +158,7 @@ func (c *CertificateSigningController) processCertificateSigningRequest(ctx cont func sign(ca *librarygocrypto.CA, x509cr *x509.CertificateRequest, usages []certificatesv1.KeyUsage, certTTL time.Duration, expirationSeconds *int32, now func() time.Time) ([]byte, error) { if err := x509cr.CheckSignature(); err != nil { - return nil, fmt.Errorf("unable to verify certificate request signature: %v", err) + return nil, fmt.Errorf("unable to verify certificate request signature: %w", err) } notBefore, notAfter, err := boundaries( diff --git a/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller_test.go b/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller_test.go index 6019248702e..92f8a7e22f0 100644 --- a/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller_test.go +++ b/control-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + . "github.com/onsi/gomega" + hypershiftv1beta1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/control-plane-pki-operator/certificates" @@ -537,6 +539,26 @@ func TestSign(t *testing.T) { } } +func TestSign_WhenCSRSignatureIsCorrupted_ItShouldReturnAnError(t *testing.T) { + t.Parallel() + g := NewWithT(t) + ca := certificateAuthority(t) + pk := privateKey(t) + csrb, err := x509.CreateCertificateRequest(insecureRand, &x509.CertificateRequest{ + Subject: pkix.Name{CommonName: "test-cn"}, + }, pk) + g.Expect(err).ToNot(HaveOccurred()) + + x509cr, err := certificates.ParseCSR(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrb})) + g.Expect(err).ToNot(HaveOccurred()) + + x509cr.Signature[0] ^= 0xFF + + _, err = sign(ca, x509cr, []certificatesv1.KeyUsage{certificatesv1.UsageClientAuth}, time.Hour, nil, time.Now) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(HavePrefix("unable to verify certificate request signature")) +} + func TestDuration(t *testing.T) { t.Parallel() diff --git a/control-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.go b/control-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.go index d633ab437b9..2d104879b0a 100644 --- a/control-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.go +++ b/control-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.go @@ -71,7 +71,7 @@ func createTargetConfig(ctx context.Context, c TargetConfigController, recorder _, _, err := ManageClientCABundle(ctx, c.configMapLister, c.kubeClient.CoreV1(), recorder, c.hostedControlPlane) if err != nil { - errors = append(errors, fmt.Errorf("%q: %v", "configmap/"+pkimanifests.TotalKASClientCABundle("placeholder").Name, err)) + errors = append(errors, fmt.Errorf("%q: %w", "configmap/"+pkimanifests.TotalKASClientCABundle("placeholder").Name, err)) } if len(errors) > 0 { diff --git a/control-plane-pki-operator/topology/detector.go b/control-plane-pki-operator/topology/detector.go index 3cdd85dcd6c..fa1b6a7d179 100644 --- a/control-plane-pki-operator/topology/detector.go +++ b/control-plane-pki-operator/topology/detector.go @@ -37,7 +37,7 @@ func (d Detector) DetectTopology(ctx context.Context, restClient *rest.Config) ( hcp, err := client.HostedControlPlanes(namespace).Get(ctx, name, metav1.GetOptions{}) if err != nil { - return "", fmt.Errorf("failed to get hosted control plane %s/%s: %q", namespace, name, err) + return "", fmt.Errorf("failed to get hosted control plane %s/%s: %w", namespace, name, err) } if hcp == nil { diff --git a/etcd-backup/etcdbackup.go b/etcd-backup/etcdbackup.go index 579269e04fb..88f100743ae 100644 --- a/etcd-backup/etcdbackup.go +++ b/etcd-backup/etcdbackup.go @@ -120,7 +120,7 @@ func uploadToS3(ctx context.Context, opts options) error { f, err := os.Open(opts.snapshotFilePath) if err != nil { - return fmt.Errorf("failed to open file %q, %v", opts.snapshotFilePath, err) + return fmt.Errorf("failed to open file %q, %w", opts.snapshotFilePath, err) } defer f.Close() diff --git a/etcd-recovery/etcdrecovery.go b/etcd-recovery/etcdrecovery.go index 59063e9bbfa..4c08d934f33 100644 --- a/etcd-recovery/etcdrecovery.go +++ b/etcd-recovery/etcdrecovery.go @@ -476,7 +476,7 @@ func isEndpointHealthy(ctx context.Context, opts options, name, endpointURL stri healthCtx, healthCtxCancel := context.WithTimeout(ctx, defaultEtcdClientTimeout) defer healthCtxCancel() _, err = cli.Get(healthCtx, "health") - if err != nil && err != rpctypes.ErrPermissionDenied { + if err != nil && !errors.Is(err, rpctypes.ErrPermissionDenied) { log.Error(err, "cannot access etcd endpoint, returning healthy=false") return false } diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go index bec35502b68..a2d95704066 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go @@ -1419,7 +1419,7 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques Message: err.Error(), }) if statusErr := r.Client.Status().Update(ctx, hcluster); statusErr != nil { - return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %s, failed to update status: %w", err, statusErr) + return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %w, failed to update status: %w", err, statusErr) } return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %w", err) } @@ -1432,7 +1432,7 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques Message: "Required platform credentials are found", }) if statusErr := r.Client.Status().Update(ctx, hcluster); statusErr != nil { - return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %s, failed to update status: %w", err, statusErr) + return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %w, failed to update status: %w", err, statusErr) } } } @@ -1801,7 +1801,7 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques hcp, shouldCheckForStaleCerts(hcluster, defaultToControlPlaneV2), r.kasServingCertHashFromSecret(ctx, hcp), - r.kasServingCertHashFromEndpoint(kasHostAndPortFromHCP(hcp)))) + r.kasServingCertHashFromEndpoint(ctx, kasHostAndPortFromHCP(hcp)))) }) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile hostedcontrolplane: %w", err) @@ -2242,16 +2242,20 @@ func (r *HostedClusterReconciler) kasServingCertHashFromSecret(ctx context.Conte } } -func (r *HostedClusterReconciler) kasServingCertHashFromEndpoint(kasHostAndPort string) func() (string, error) { +func (r *HostedClusterReconciler) kasServingCertHashFromEndpoint(ctx context.Context, kasHostAndPort string) func() (string, error) { return func() (string, error) { - conn, err := tls.Dial("tcp", kasHostAndPort, &tls.Config{ + netConn, err := (&tls.Dialer{Config: &tls.Config{ InsecureSkipVerify: true, ServerName: "kubernetes", - }) + }}).DialContext(ctx, "tcp", kasHostAndPort) if err != nil { return "", fmt.Errorf("failed to dial %s: %w", kasHostAndPort, err) } - defer conn.Close() + defer netConn.Close() + conn, ok := netConn.(*tls.Conn) + if !ok { + return "", fmt.Errorf("connection to %s is not a TLS connection", kasHostAndPort) + } kasCerts := conn.ConnectionState().PeerCertificates if len(kasCerts) == 0 { return "", fmt.Errorf("no certificate found on KAS endpoint %s", kasHostAndPort) @@ -2334,7 +2338,7 @@ func reconcileHostedControlPlaneAnnotations(hcp *hyperv1.HostedControlPlane, hcl hyperv1.KubeAPIServerGoAwayChance, hyperv1.KubeAPIServerServiceAccountTokenMaxExpiration, hyperv1.HostedClusterRestoredFromBackupAnnotation, - // TODO: Remove this once the the input is in the HostedCluster AWS API. + // TODO: Remove this once the input is in the HostedCluster AWS API. "hypershift.openshift.io/aws-termination-handler-queue-url", hyperv1.SwiftPodNetworkInstanceAnnotation, hyperv1.EnableMetricsForwarding, @@ -2505,7 +2509,7 @@ func reconcileHostedControlPlane(hcp *hyperv1.HostedControlPlane, hcluster *hype return nil } -// reconcileCAPIManager orchestrates orchestrates of all CAPI manager components. +// reconcileCAPIManager orchestrates all CAPI manager components. func (r *HostedClusterReconciler) reconcileCAPIManager(cpContext controlplanecomponent.ControlPlaneContext, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, releaseVersion semver.Version) error { controlPlaneNamespace := manifests.HostedControlPlaneNamespaceObject(hcluster.Namespace, hcluster.Name) err := r.Client.Get(cpContext, client.ObjectKeyFromObject(controlPlaneNamespace), controlPlaneNamespace) @@ -2825,7 +2829,7 @@ func (r *HostedClusterReconciler) reconcileCLISecrets(ctx context.Context, creat util.AutoInfraLabelName: hcluster.Spec.InfraID, }) if err != nil { - return fmt.Errorf("failed to retrieve cli created secrets: %v", err) + return fmt.Errorf("failed to retrieve cli created secrets: %w", err) } ownerRef := config.OwnerRefFrom(hcluster) @@ -2835,7 +2839,7 @@ func (r *HostedClusterReconciler) reconcileCLISecrets(ctx context.Context, creat return nil }) if err != nil { - return fmt.Errorf("failed to set '%s' secret's owner reference: %v", secret.Name, err) + return fmt.Errorf("failed to set '%s' secret's owner reference: %w", secret.Name, err) } if res == controllerutil.OperationResultUpdated { log.Info("added owner reference of the Hosted cluster, to the secret", "secret", secret.Name) @@ -3224,7 +3228,7 @@ func computeAWSEndpointServiceCondition(awsEndpointServiceList hyperv1.AWSEndpoi func listNodePools(ctx context.Context, c client.Client, clusterNamespace, clusterName string) ([]hyperv1.NodePool, error) { nodePoolList := &hyperv1.NodePoolList{} if err := c.List(ctx, nodePoolList); err != nil { - return nil, fmt.Errorf("failed getting nodePool list: %v", err) + return nil, fmt.Errorf("failed getting nodePool list: %w", err) } // TODO: do a label association or something filtered := []hyperv1.NodePool{} @@ -4380,7 +4384,7 @@ func (r *HostedClusterReconciler) reconcileOIDCDocumentsWithStatus(ctx context.C Message: err.Error(), }) if statusErr := r.Client.Status().Update(ctx, hcluster); statusErr != nil { - return fmt.Errorf("failed to reconcile OIDC documents: %s, failed to update status: %w", err, statusErr) + return fmt.Errorf("failed to reconcile OIDC documents: %w, failed to update status: %w", err, statusErr) } return fmt.Errorf("failed to reconcile OIDC documents: %w", err) } @@ -4466,7 +4470,8 @@ func (r *HostedClusterReconciler) reconcileAWSOIDCDocuments(ctx context.Context, // return the code. If other specific error types can be handled, add // new switch cases and try to provide more actionable info to the // user. - wrapped = fmt.Errorf("%w: aws returned an error: %v", wrapped, err) + log.Error(err, "failed to upload OIDC document to S3", "path", path, "bucket", r.OIDCStorageProviderS3BucketName) + wrapped = fmt.Errorf("%w: aws returned an error", wrapped) } return wrapped } diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go index f289f3dd122..8b24facac98 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go @@ -8,6 +8,8 @@ import ( "encoding/pem" "errors" "fmt" + "net/http" + "net/http/httptest" "os" "reflect" "strings" @@ -66,6 +68,8 @@ import ( ctrl "sigs.k8s.io/controller-runtime" crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -6734,3 +6738,209 @@ func TestComputeEndpointServiceCondition(t *testing.T) { }) } } + +func TestListNodePools(t *testing.T) { + t.Parallel() + tests := []struct { + name string + objects []crclient.Object + interceptorFuncs interceptor.Funcs + clusterNamespace string + clusterName string + wantErr bool + errSubstr string + expectedCount int + }{ + { + name: "When client List succeeds with matching NodePools, it should return filtered results", + objects: []crclient.Object{ + &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: "np1", Namespace: "clusters"}, + Spec: hyperv1.NodePoolSpec{ClusterName: "my-cluster"}, + }, + &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: "np2", Namespace: "clusters"}, + Spec: hyperv1.NodePoolSpec{ClusterName: "other-cluster"}, + }, + }, + clusterNamespace: "clusters", + clusterName: "my-cluster", + expectedCount: 1, + }, + { + name: "When client List fails, it should return a wrapped error", + objects: []crclient.Object{}, + interceptorFuncs: interceptor.Funcs{ + List: func(ctx context.Context, c crclient.WithWatch, list crclient.ObjectList, opts ...crclient.ListOption) error { + return fmt.Errorf("API server unavailable") + }, + }, + clusterNamespace: "clusters", + clusterName: "my-cluster", + wantErr: true, + errSubstr: "failed getting nodePool list", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + builder := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tt.objects...) + if tt.interceptorFuncs.List != nil { + builder = builder.WithInterceptorFuncs(tt.interceptorFuncs) + } + c := builder.Build() + + result, err := listNodePools(t.Context(), c, tt.clusterNamespace, tt.clusterName) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(HaveLen(tt.expectedCount)) + for _, np := range result { + g.Expect(np.Namespace).To(Equal(tt.clusterNamespace)) + g.Expect(np.Spec.ClusterName).To(Equal(tt.clusterName)) + } + } + }) + } +} + +func TestReconcileCLISecretsErrors(t *testing.T) { + t.Parallel() + tests := []struct { + name string + interceptorFuncs interceptor.Funcs + existingSecrets []crclient.Object + createOrUpdate upsert.CreateOrUpdateFN + wantErrSubstr string + }{ + { + name: "When client List fails, it should return a wrapped error", + interceptorFuncs: interceptor.Funcs{ + List: func(ctx context.Context, c crclient.WithWatch, list crclient.ObjectList, opts ...crclient.ListOption) error { + return fmt.Errorf("connection refused") + }, + }, + wantErrSubstr: "failed to retrieve cli created secrets", + }, + { + name: "When createOrUpdate fails for a secret, it should return a wrapped error", + existingSecrets: []crclient.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cli-secret", + Namespace: "clusters", + Labels: map[string]string{ + util.DeleteWithClusterLabelName: "true", + util.AutoInfraLabelName: "test-infra", + }, + }, + }, + }, + createOrUpdate: func(ctx context.Context, c crclient.Client, obj crclient.Object, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { + return controllerutil.OperationResultNone, fmt.Errorf("API conflict") + }, + wantErrSubstr: "failed to set 'cli-secret' secret's owner reference", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + builder := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithInterceptorFuncs(tt.interceptorFuncs) + if len(tt.existingSecrets) > 0 { + builder = builder.WithObjects(tt.existingSecrets...) + } + cli := builder.Build() + + r := &HostedClusterReconciler{Client: cli} + hc := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "clusters", + }, + Spec: hyperv1.HostedClusterSpec{ + InfraID: "test-infra", + }, + } + + createOrUpdate := tt.createOrUpdate + if createOrUpdate == nil { + createOrUpdate = upsert.New(false).CreateOrUpdate + } + + err := r.reconcileCLISecrets(t.Context(), createOrUpdate, hc) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErrSubstr)) + }) + } +} + +func TestKasServingCertHashFromEndpoint(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setupTLS bool + cancelCtx bool + wantErr bool + errSubstr string + }{ + { + name: "When the TLS endpoint is healthy, it should return a non-empty certificate hash", + setupTLS: true, + }, + { + name: "When the endpoint is unreachable, it should return a dial error", + wantErr: true, + errSubstr: "failed to dial", + }, + { + name: "When the context is canceled, it should return an error", + setupTLS: true, + cancelCtx: true, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + addr := "127.0.0.1:1" + if tt.setupTLS { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + addr = server.Listener.Addr().String() + } + + ctx := t.Context() + if tt.cancelCtx { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + cancel() + } + + r := &HostedClusterReconciler{} + hashFn := r.kasServingCertHashFromEndpoint(ctx, addr) + hash, err := hashFn() + + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + if tt.errSubstr != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + } + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(hash).ToNot(BeEmpty()) + } + }) + } +} diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go index e9791dc4e82..6a7e8b6e3af 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go @@ -104,7 +104,7 @@ func (defaulter *nodePoolDefaulter) Default(ctx context.Context, obj runtime.Obj err := defaulter.client.Get(ctx, client.ObjectKeyFromObject(hc), hc) if err != nil { - return fmt.Errorf("error retrieving HostedCluster named [%s], %v", np.Spec.ClusterName, err) + return fmt.Errorf("error retrieving HostedCluster named [%s], %w", np.Spec.ClusterName, err) } np.Spec.Release.Image = hc.Spec.Release.Image } diff --git a/hypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.go b/hypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.go index a16845df131..5acd9f922ed 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.go +++ b/hypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.go @@ -79,7 +79,7 @@ func TestReconcileCAPIInfraCR(t *testing.T) { t.Fatalf("Expected the provided function to be called once") } if tc.expectedErr != nil { - if err != tc.expectedErr { + if !errors.Is(err, tc.expectedErr) { t.Fatalf("ReconcileCAPIInfraCR: Expected to fail. gotErr: %v, expectedErr: %v", err, tc.expectedErr) } } else if err != nil { diff --git a/hypershift-operator/controllers/hostedcluster/internal/proxy/validation.go b/hypershift-operator/controllers/hostedcluster/internal/proxy/validation.go index 3e480e340c5..b4ed435c2b2 100644 --- a/hypershift-operator/controllers/hostedcluster/internal/proxy/validation.go +++ b/hypershift-operator/controllers/hostedcluster/internal/proxy/validation.go @@ -31,7 +31,7 @@ func LoadCABundle(configMap corev1.ConfigMap) ([]*x509.Certificate, error) { } certBundle, err := crypto.CertsFromPEM(trustBundleData) if err != nil { - return nil, fmt.Errorf("failed parsing certificate data from ConfigMap %q: %v", configMap.Name, err) + return nil, fmt.Errorf("failed parsing certificate data from ConfigMap %q: %w", configMap.Name, err) } return certBundle, nil } diff --git a/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go b/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go index d868002c3dc..df8ba33615e 100644 --- a/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go +++ b/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go @@ -139,7 +139,7 @@ func (r *HAProxy) reconcileHAProxyIgnitionConfig(ctx context.Context, hcluster * if hcluster.Spec.Configuration != nil && hcluster.Spec.Configuration.Proxy != nil && hcluster.Spec.Configuration.Proxy.HTTPSProxy != "" && netutil.ConnectsThroughInternetToControlplane(hcluster.Spec.Platform) { apiserverProxy, err = joinDefaultPortIfMissing(hcluster.Spec.Configuration.Proxy.HTTPSProxy) if err != nil { - return "", fmt.Errorf("failed to parse .Spec.Configuration.Proxy.HTTPSProxy: %v", err) + return "", fmt.Errorf("failed to parse .Spec.Configuration.Proxy.HTTPSProxy: %w", err) } noProxy = hcluster.Spec.Configuration.Proxy.NoProxy } diff --git a/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy_test.go b/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy_test.go index 117e1e49a21..44b32b29ba3 100644 --- a/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy_test.go +++ b/hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy_test.go @@ -5,6 +5,8 @@ import ( "strings" "testing" + . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/api/util/ipnet" "github.com/openshift/hypershift/hypershift-operator/controllers/sharedingress" @@ -516,3 +518,47 @@ kind: Config` }) } } + +func TestJoinDefaultPortIfMissing(t *testing.T) { + t.Parallel() + tests := []struct { + name string + addr string + expected string + wantErr bool + }{ + { + name: "When HTTPS URL has no port it should add port 443", + addr: "https://proxy.example.com", + expected: "https://proxy.example.com:443", + }, + { + name: "When HTTP URL has no port it should add port 80", + addr: "http://proxy.example.com", + expected: "http://proxy.example.com:80", + }, + { + name: "When HTTPS URL already has a port it should keep existing port", + addr: "https://proxy.example.com:8443", + expected: "https://proxy.example.com:8443", + }, + { + name: "When URL has no scheme it should return an error", + addr: "proxy.example.com", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + result, err := joinDefaultPortIfMissing(tt.addr) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + return + } + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(tt.expected)) + }) + } +} diff --git a/hypershift-operator/controllers/nodepool/aws_test.go b/hypershift-operator/controllers/nodepool/aws_test.go index 00763009a15..608f3badc3c 100644 --- a/hypershift-operator/controllers/nodepool/aws_test.go +++ b/hypershift-operator/controllers/nodepool/aws_test.go @@ -2,6 +2,7 @@ package nodepool import ( "encoding/json" + "errors" "strings" "testing" @@ -148,8 +149,8 @@ func TestAWSMachineTemplateSpec(t *testing.T) { name: "NotReady error is returned if no sg specified and no cluster sg is available", clusterStatus: &hyperv1.HostedClusterStatus{Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{DefaultWorkerSecurityGroupID: ""}}}, checkError: func(t *testing.T, err error) { - _, isNotReady := err.(*NotReadyError) - if err == nil || !isNotReady { + var notReadyErr *NotReadyError + if err == nil || !errors.As(err, ¬ReadyErr) { t.Errorf("did not get expected NotReady error") } }, @@ -1479,8 +1480,8 @@ func TestBuildAWSSecurityGroups(t *testing.T) { if tc.expectError { g.Expect(err).To(HaveOccurred()) if tc.expectNotReady { - _, isNotReady := err.(*NotReadyError) - g.Expect(isNotReady).To(BeTrue()) + var notReadyErr *NotReadyError + g.Expect(errors.As(err, ¬ReadyErr)).To(BeTrue()) } } else { g.Expect(err).ToNot(HaveOccurred()) diff --git a/hypershift-operator/controllers/nodepool/conditions.go b/hypershift-operator/controllers/nodepool/conditions.go index 735a21deb9c..81eb714474a 100644 --- a/hypershift-operator/controllers/nodepool/conditions.go +++ b/hypershift-operator/controllers/nodepool/conditions.go @@ -1002,7 +1002,7 @@ func (r *NodePoolReconciler) supportedVersionSkewCondition(ctx context.Context, Message: err.Error(), ObservedGeneration: nodePool.Generation, }) - return nil, nil + return nil, nil //nolint:nilerr // validation error is surfaced via status condition, not returned } SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolSupportedVersionSkewConditionType, diff --git a/hypershift-operator/controllers/nodepool/config.go b/hypershift-operator/controllers/nodepool/config.go index 5787a6cdf19..8771f45ea83 100644 --- a/hypershift-operator/controllers/nodepool/config.go +++ b/hypershift-operator/controllers/nodepool/config.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "context" + coreerrors "errors" "fmt" "io" "sort" @@ -238,7 +239,7 @@ func (cg *ConfigGenerator) parse(configs []corev1.ConfigMap) (string, error) { yamlReader := yaml.NewYAMLReader(bufio.NewReader(strings.NewReader(cmPayload))) for { manifestRaw, err := yamlReader.Read() - if err != nil && err != io.EOF { + if err != nil && !coreerrors.Is(err, io.EOF) { errors = append(errors, fmt.Errorf("configmap %q contains invalid yaml: %w", config.Name, err)) continue } @@ -250,7 +251,7 @@ func (cg *ConfigGenerator) parse(configs []corev1.ConfigMap) (string, error) { } allConfigPlainText = append(allConfigPlainText, string(manifest)) } - if err == io.EOF { + if coreerrors.Is(err, io.EOF) { break } } diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index 9cd2cc3c484..4700d89e1a3 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -2,6 +2,7 @@ package nodepool import ( "context" + coreerrors "errors" "fmt" "os" "regexp" @@ -414,7 +415,8 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho } if err := capi.Reconcile(ctx); err != nil { - if _, isNotReady := err.(*NotReadyError); isNotReady { + var notReadyErr *NotReadyError + if coreerrors.As(err, ¬ReadyErr) { log.Info("Waiting to create machine template", "message", err.Error()) return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } diff --git a/hypershift-operator/controllers/nodepool/nto.go b/hypershift-operator/controllers/nodepool/nto.go index bc98b4e6dde..2fc5e65a4d0 100644 --- a/hypershift-operator/controllers/nodepool/nto.go +++ b/hypershift-operator/controllers/nodepool/nto.go @@ -412,7 +412,7 @@ func BuildMirrorConfigs(ctx context.Context, cg *ConfigGenerator) ([]*MirrorConf yamlReader := yaml.NewYAMLReader(bufio.NewReader(strings.NewReader(cmPayload))) for { manifestRaw, err := yamlReader.Read() - if err != nil && err != io.EOF { + if err != nil && !coreerrors.Is(err, io.EOF) { errors = append(errors, fmt.Errorf("configmap %q contains invalid yaml: %w", config.Name, err)) continue } @@ -427,7 +427,7 @@ func BuildMirrorConfigs(ctx context.Context, cg *ConfigGenerator) ([]*MirrorConf mirrorConfigs = append(mirrorConfigs, mirrorConfig) } } - if err == io.EOF { + if coreerrors.Is(err, io.EOF) { break } } diff --git a/hypershift-operator/controllers/platform/aws/controller.go b/hypershift-operator/controllers/platform/aws/controller.go index e7779ba5a57..80e82a6576f 100644 --- a/hypershift-operator/controllers/platform/aws/controller.go +++ b/hypershift-operator/controllers/platform/aws/controller.go @@ -892,7 +892,7 @@ func (r *AWSEndpointServiceReconciler) controlPlaneOperatorRoleARNWithoutPath(hc } arn, err := arn.Parse(hc.Spec.Platform.AWS.RolesRef.ControlPlaneOperatorARN) if err != nil { - return "", fmt.Errorf("failed to parse %s into an ARN: %v", hc.Spec.Platform.AWS.RolesRef.ControlPlaneOperatorARN, err) + return "", fmt.Errorf("failed to parse %s into an ARN: %w", hc.Spec.Platform.AWS.RolesRef.ControlPlaneOperatorARN, err) } // IAM names cannot have a "/" while path names are the only way to get "/" into the name diff --git a/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go b/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go index da9321e9d17..127c6caf781 100644 --- a/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go +++ b/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go @@ -2,6 +2,7 @@ package gcp import ( "context" + "errors" "fmt" "os" "strings" @@ -470,7 +471,8 @@ func (r *GCPPrivateServiceConnectReconciler) handleGCPError(ctx context.Context, var requeueAfter time.Duration var message string - if googleErr, ok := err.(*googleapi.Error); ok { + var googleErr *googleapi.Error + if errors.As(err, &googleErr) { switch googleErr.Code { case 429: // Rate limit requeueAfter = time.Minute * 5 @@ -513,7 +515,8 @@ func (r *GCPPrivateServiceConnectReconciler) handleGCPError(ctx context.Context, // isNotFoundError checks if an error is a GCP 404 Not Found error func isNotFoundError(err error) bool { - if googleErr, ok := err.(*googleapi.Error); ok { + var googleErr *googleapi.Error + if errors.As(err, &googleErr) { return googleErr.Code == 404 } return false diff --git a/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go b/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go index 37bf945facc..916374b11eb 100644 --- a/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go +++ b/hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go @@ -53,6 +53,16 @@ func TestIsNotFoundError(t *testing.T) { err: nil, expected: false, }, + { + name: "When given a wrapped GCP 404 error it should return true", + err: fmt.Errorf("operation failed: %w", &googleapi.Error{Code: 404}), + expected: true, + }, + { + name: "When given a wrapped GCP 500 error it should return false", + err: fmt.Errorf("operation failed: %w", &googleapi.Error{Code: 500}), + expected: false, + }, } for _, test := range tests { diff --git a/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go b/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go index e47c2b18e2d..53e68f6d552 100644 --- a/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go +++ b/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go @@ -103,8 +103,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu telemeterClientSecret := monitoring.TelemeterClientSecret() if err := r.Get(ctx, client.ObjectKeyFromObject(telemeterClientSecret), telemeterClientSecret); err != nil { - log.Info("user-workload-monitoring (UWM) telemetry remote writer is disabled because the 'telemeter-client' secret does not exist.") - return ctrl.Result{}, nil + if apierrors.IsNotFound(err) { + log.Info("user-workload-monitoring (UWM) telemetry remote writer is disabled because the 'telemeter-client' secret does not exist.") + return ctrl.Result{}, nil //nolint:nilerr // missing secret means UWM telemetry is disabled + } + return ctrl.Result{}, r.errorHandler(operatorDeployment, fmt.Errorf("failed to get telemeter-client secret: %w", err)) } if err := r.reconcileTelemetryRemoteWrite(ctx, string(clusterVersion.Spec.ClusterID)); err != nil { diff --git a/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go b/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go index 829be27d6b6..0d4be563b77 100644 --- a/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go +++ b/hypershift-operator/controllers/uwmtelemetry/uwm_telemetry_test.go @@ -1,8 +1,10 @@ package uwmtelemetry import ( + "context" "encoding/base64" "encoding/json" + "fmt" "testing" . "github.com/onsi/gomega" @@ -14,12 +16,14 @@ import ( configv1 "github.com/openshift/api/config/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/yaml" ) @@ -424,4 +428,133 @@ func TestReconcile(t *testing.T) { test.validate(g, c) }) } + + t.Run("When telemeter-client secret Get fails with a non-NotFound error it should return the error", func(t *testing.T) { + g := NewWithT(t) + ns := "hypershift" + deployment := manifests.OperatorDeployment(ns) + cv := monitoring.ClusterVersion() + cv.Spec.ClusterID = "fake-cluster-id" + + c := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects( + deployment, + monitoring.MonitoringNamespace(), + monitoring.UWMNamespace(), + cv, + ). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok && key.Name == "telemeter-client" { + return fmt.Errorf("API server unavailable") + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + reconciler := &Reconciler{ + Client: c, + CreateOrUpdateProvider: upsert.New(true), + errorHandler: func(obj client.Object, err error) error { return err }, + Namespace: ns, + } + req := ctrl.Request{NamespacedName: client.ObjectKey{Name: "operator", Namespace: ns}} + _, err := reconciler.Reconcile(t.Context(), req) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(ContainSubstring("failed to get telemeter-client secret"))) + }) + + t.Run("When operator deployment Get fails it should return the error", func(t *testing.T) { + g := NewWithT(t) + ns := "hypershift" + + c := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*appsv1.Deployment); ok && key.Name == "operator" { + return fmt.Errorf("connection refused") + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + reconciler := &Reconciler{ + Client: c, + CreateOrUpdateProvider: upsert.New(true), + errorHandler: func(obj client.Object, err error) error { return err }, + Namespace: ns, + } + req := ctrl.Request{NamespacedName: client.ObjectKey{Name: "operator", Namespace: ns}} + _, err := reconciler.Reconcile(t.Context(), req) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(ContainSubstring("cannot get operator deployment"))) + }) + + t.Run("When monitoring namespace Get fails with a non-NotFound error it should return the error", func(t *testing.T) { + g := NewWithT(t) + ns := "hypershift" + deployment := manifests.OperatorDeployment(ns) + + c := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(deployment). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Namespace); ok && key.Name == "openshift-monitoring" { + return fmt.Errorf("forbidden") + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + reconciler := &Reconciler{ + Client: c, + CreateOrUpdateProvider: upsert.New(true), + errorHandler: func(obj client.Object, err error) error { return err }, + Namespace: ns, + } + req := ctrl.Request{NamespacedName: client.ObjectKey{Name: "operator", Namespace: ns}} + _, err := reconciler.Reconcile(t.Context(), req) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(ContainSubstring("failed to get monitoring namespace"))) + }) + + t.Run("When clusterversion Get fails it should return the error", func(t *testing.T) { + g := NewWithT(t) + ns := "hypershift" + deployment := manifests.OperatorDeployment(ns) + + c := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects( + deployment, + monitoring.MonitoringNamespace(), + monitoring.UWMNamespace(), + ). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*configv1.ClusterVersion); ok { + return fmt.Errorf("API server unavailable") + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + reconciler := &Reconciler{ + Client: c, + CreateOrUpdateProvider: upsert.New(true), + errorHandler: func(obj client.Object, err error) error { return err }, + Namespace: ns, + } + req := ctrl.Request{NamespacedName: client.ObjectKey{Name: "operator", Namespace: ns}} + _, err := reconciler.Reconcile(t.Context(), req) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(ContainSubstring("failed to get clusterversion resource"))) + }) } diff --git a/ignition-server/cmd/start.go b/ignition-server/cmd/start.go index b816f58ca9c..3ee07a28ca9 100644 --- a/ignition-server/cmd/start.go +++ b/ignition-server/cmd/start.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "encoding/base64" + "errors" "fmt" "log" "net/http" @@ -316,7 +317,7 @@ func run(ctx context.Context, opts Options) error { }() log.Printf("Listening on %s", opts.Addr) - if err := server.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed { + if err := server.ListenAndServeTLS("", ""); err != nil && !errors.Is(err, http.ErrServerClosed) { return err } return nil diff --git a/ignition-server/controllers/cache.go b/ignition-server/controllers/cache.go index 91e4f1ec6f5..ba3ef0567b9 100644 --- a/ignition-server/controllers/cache.go +++ b/ignition-server/controllers/cache.go @@ -59,7 +59,7 @@ func (c *ExpiringCache) Set(key string, value CacheValue) { c.Lock() defer c.Unlock() - // Renew expiring time every time time we Set. + // Renew expiring time every time we Set. c.cache[key] = &entry{ value: value, expiry: time.Now().Add(c.ttl), diff --git a/ignition-server/controllers/local_ignitionprovider.go b/ignition-server/controllers/local_ignitionprovider.go index 7a17217734d..3b3e349254a 100644 --- a/ignition-server/controllers/local_ignitionprovider.go +++ b/ignition-server/controllers/local_ignitionprovider.go @@ -571,7 +571,7 @@ func (p *LocalIgnitionProvider) runMCSAndFetchPayload(ctx context.Context, dirs var payload []byte // Try connecting to the server until we get a response or the context is closed err = wait.PollUntilContextCancel(ctx, 1*time.Second, true, func(ctx context.Context) (bool, error) { - req, err := http.NewRequestWithContext(ctx, "GET", "http://localhost:22626/config/master", nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://localhost:22626/config/master", nil) if err != nil { return false, fmt.Errorf("error building http request: %w", err) } @@ -652,7 +652,7 @@ func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, cu return imageprovider.New(img), nil }() if err != nil { - return nil, fmt.Errorf("failed to get component images: %v", err) + return nil, fmt.Errorf("failed to get component images: %w", err) } mcoImage, err := p.resolveMCOImage(ctx, imageProvider, pullSecret) diff --git a/ignition-server/controllers/tokensecret_controller.go b/ignition-server/controllers/tokensecret_controller.go index 18332ff88c0..080421ef330 100644 --- a/ignition-server/controllers/tokensecret_controller.go +++ b/ignition-server/controllers/tokensecret_controller.go @@ -275,7 +275,7 @@ func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) start := time.Now() payload, err := r.IgnitionProvider.GetPayload(ctx, releaseImage, config.String(), pullSecretHash, additionalTrustBundleHash, hcConfigurationHash) if err != nil { - return nil, fmt.Errorf("error getting ignition payload: %v", err) + return nil, fmt.Errorf("error getting ignition payload: %w", err) } duration := time.Since(start).Round(time.Second).Seconds() log.Info("got ignition payload", "duration", duration) diff --git a/karpenter-operator/controllers/karpenter/machine_approver.go b/karpenter-operator/controllers/karpenter/machine_approver.go index 3dc045e0ecc..01f137f3b69 100644 --- a/karpenter-operator/controllers/karpenter/machine_approver.go +++ b/karpenter-operator/controllers/karpenter/machine_approver.go @@ -85,7 +85,7 @@ func (r *MachineApproverController) SetupWithManager(mgr ctrl.Manager) error { &handler.TypedEnqueueRequestForObject[*certificatesv1.CertificateSigningRequest]{}, predicate.NewTypedPredicateFuncs(csrFilterFn), )); err != nil { - return fmt.Errorf("failed to watch CertificateSigningRequest: %v", err) + return fmt.Errorf("failed to watch CertificateSigningRequest: %w", err) } return nil @@ -100,7 +100,7 @@ func (r *MachineApproverController) Reconcile(ctx context.Context, req ctrl.Requ if apierrors.IsNotFound(err) { return ctrl.Result{}, nil } - return ctrl.Result{}, fmt.Errorf("failed to get csr %s: %v", req.NamespacedName, err) + return ctrl.Result{}, fmt.Errorf("failed to get csr %s: %w", req.NamespacedName, err) } // Return early if deleted @@ -129,7 +129,7 @@ func (r *MachineApproverController) Reconcile(ctx context.Context, req ctrl.Requ if authorized { log.Info("Attempting to approve CSR", "csr", csr.Name) if err := r.approve(ctx, csr); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to approve csr %s: %v", csr.Name, err) + return ctrl.Result{}, fmt.Errorf("failed to approve csr %s: %w", csr.Name, err) } } @@ -243,7 +243,7 @@ func (r *MachineApproverController) approve(ctx context.Context, csr *certificat _, err := r.certClient.CertificateSigningRequests().UpdateApproval(ctx, csr.Name, csr, metav1.UpdateOptions{}) if err != nil { - return fmt.Errorf("error updating approval for csr: %v", err) + return fmt.Errorf("error updating approval for csr: %w", err) } return nil diff --git a/karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go b/karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go index e7153ada5ba..8d4d761ad29 100644 --- a/karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go +++ b/karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go @@ -420,7 +420,7 @@ func (r *EC2NodeClassReconciler) reconcileStatus(ctx context.Context, ec2NodeCla if !reflect.DeepEqual(originalObj.Status, openshiftNodeClass.Status) { if err := r.guestClient.Status().Patch(ctx, openshiftNodeClass, client.MergeFrom(originalObj)); err != nil { - return fmt.Errorf("failed to update status: %v", err) + return fmt.Errorf("failed to update status: %w", err) } } diff --git a/karpenter-operator/main.go b/karpenter-operator/main.go index a122fe8b7e6..fa45345d39d 100644 --- a/karpenter-operator/main.go +++ b/karpenter-operator/main.go @@ -116,7 +116,7 @@ func run(ctx context.Context) error { } if err := mgr.Add(managementCluster); err != nil { - return fmt.Errorf("failed to add managementCluster to controller runtime manager: %v", err) + return fmt.Errorf("failed to add managementCluster to controller runtime manager: %w", err) } hypershiftClient, err := hypershiftclient.NewForConfig(managementKubeconfig) diff --git a/konnectivity-socks5-proxy/http_proxy.go b/konnectivity-socks5-proxy/http_proxy.go index 7d39129aa4a..5239f320e4c 100644 --- a/konnectivity-socks5-proxy/http_proxy.go +++ b/konnectivity-socks5-proxy/http_proxy.go @@ -71,7 +71,7 @@ func (hpd *httpProxyDialer) Dial(network string, addr string) (net.Conn, error) return nil, err } - if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { conn.Close() f := strings.SplitN(resp.Status, " ", 2) return nil, errors.New(f[1]) diff --git a/kubernetes-default-proxy/kubernetes_default_proxy.go b/kubernetes-default-proxy/kubernetes_default_proxy.go index 8efe2e4d8fc..79c40cd3e12 100644 --- a/kubernetes-default-proxy/kubernetes_default_proxy.go +++ b/kubernetes-default-proxy/kubernetes_default_proxy.go @@ -67,7 +67,7 @@ func (s *server) validate() error { } func (s *server) run(ctx context.Context) error { - listener, err := net.Listen("tcp", s.listenAddr) + listener, err := (&net.ListenConfig{}).Listen(ctx, "tcp", s.listenAddr) if err != nil { return fmt.Errorf("failed to listen on tcp:%s: %w", s.listenAddr, err) } @@ -87,7 +87,7 @@ func (s *server) run(ctx context.Context) error { go func() { defer conn.Close() - backendConn, err := net.Dial("tcp", s.proxyAddr) + backendConn, err := (&net.Dialer{}).DialContext(ctx, "tcp", s.proxyAddr) if err != nil { s.log.Error(err, "failed diaing backend", "proxyAddr", s.proxyAddr) return @@ -95,7 +95,7 @@ func (s *server) run(ctx context.Context) error { defer backendConn.Close() req := &http.Request{ - Method: "CONNECT", + Method: http.MethodConnect, URL: &url.URL{Host: s.apiServerAddr}, Proto: "HTTP/1.1", ProtoMajor: 1, @@ -111,7 +111,7 @@ func (s *server) run(ctx context.Context) error { s.log.Error(err, "failed to read response to connect request") return } - if response.StatusCode != 200 { + if response.StatusCode != http.StatusOK { s.log.Error(fmt.Errorf("got unexpected statuscode %d to CONNECT request", response.StatusCode), "failed to establish a connection through http connect") return } diff --git a/kubevirtexternalinfra/externalinfra.go b/kubevirtexternalinfra/externalinfra.go index 5e838b5e53f..543d68a4d1f 100644 --- a/kubevirtexternalinfra/externalinfra.go +++ b/kubevirtexternalinfra/externalinfra.go @@ -222,8 +222,8 @@ func (k *kubevirtInfraClientImp) GetInfraKubevirtVersion(ctx context.Context) (* result = restClient.Get().AbsPath(uri).Do(ctx) if data, err := result.Raw(); err != nil { - connErr, isConnectionErr := err.(*url.Error) - if isConnectionErr { + var connErr *url.Error + if errors.As(err, &connErr) { err = connErr.Err } diff --git a/pkg/etcdcli/etcdcli.go b/pkg/etcdcli/etcdcli.go index ba68cbdb444..cf1dd2f2783 100644 --- a/pkg/etcdcli/etcdcli.go +++ b/pkg/etcdcli/etcdcli.go @@ -307,7 +307,7 @@ func (g *etcdClientGetter) UnhealthyMembers(ctx context.Context) ([]*etcdserverp defer cancel() etcdCluster, err := cli.MemberList(ctx) if err != nil { - return nil, fmt.Errorf("could not get member list %v", err) + return nil, fmt.Errorf("could not get member list: %w", err) } memberHealth := getMemberHealth(ctx, etcdCluster.Members) diff --git a/sharedingress-config-generator/controller.go b/sharedingress-config-generator/controller.go index 4d3f8ea61e4..53ff34394cf 100644 --- a/sharedingress-config-generator/controller.go +++ b/sharedingress-config-generator/controller.go @@ -169,7 +169,7 @@ func (r *SharedIngressConfigReconciler) Reconcile(ctx context.Context, _ ctrl.Re } logger.Info("Reloading HAProxy configuration") - if err := sendHAProxyReloadCommand(r.haProxyClient, r.haProxyRuntimeSocketPath); err != nil { + if err := sendHAProxyReloadCommand(ctx, r.haProxyClient, r.haProxyRuntimeSocketPath); err != nil { return ctrl.Result{}, fmt.Errorf("failed to reload HAProxy: %w", err) } @@ -203,8 +203,8 @@ func (r *SharedIngressConfigReconciler) currentConfigHash() ([]byte, error) { // sendHAProxyReloadCommand connects to the specified Unix socket and sends a reload command. // It inspects the returned response and return an appropriate error if the reload operation failed. -func sendHAProxyReloadCommand(client haProxyClient, socketPath string) error { - response, err := client.sendCommand(socketPath, "reload") +func sendHAProxyReloadCommand(ctx context.Context, client haProxyClient, socketPath string) error { + response, err := client.sendCommand(ctx, socketPath, "reload") if err != nil { return err } diff --git a/sharedingress-config-generator/controller_test.go b/sharedingress-config-generator/controller_test.go index 6a885624937..72d7e89b8db 100644 --- a/sharedingress-config-generator/controller_test.go +++ b/sharedingress-config-generator/controller_test.go @@ -2,6 +2,7 @@ package sharedingressconfiggenerator import ( "bytes" + "context" "fmt" "os" "path/filepath" @@ -29,7 +30,7 @@ type mockHAProxyClient struct { lastCommand string } -func (m *mockHAProxyClient) sendCommand(socketPath, command string) (string, error) { +func (m *mockHAProxyClient) sendCommand(_ context.Context, socketPath, command string) (string, error) { m.sendCommandCalled = true m.sendCommandCount++ m.lastCommand = command @@ -122,7 +123,7 @@ func TestReconcile(t *testing.T) { } // Make the temporary directory read-only to simulate a permissions error - g.Expect(os.Chmod(tempDir, 0555)).To(Succeed()) // r-x r-x r-x + g.Expect(os.Chmod(tempDir, 0555)).To(Succeed()) //nolint:dupword // r-x r-x r-x describes permission bits // Ensure we restore permissions so the deferred os.RemoveAll can work defer func() { g.Expect(os.Chmod(tempDir, 0755)).To(Succeed()) diff --git a/sharedingress-config-generator/haproxy_client.go b/sharedingress-config-generator/haproxy_client.go index a043e40b38f..95524053ba6 100644 --- a/sharedingress-config-generator/haproxy_client.go +++ b/sharedingress-config-generator/haproxy_client.go @@ -1,6 +1,7 @@ package sharedingressconfiggenerator import ( + "context" "fmt" "io" "net" @@ -11,14 +12,14 @@ import ( type haProxyClient interface { // sendHAProxyCommand connects to the specified Unix socket, sends a command, // and returns the response from HAProxy. - sendCommand(socketPath, command string) (string, error) + sendCommand(ctx context.Context, socketPath, command string) (string, error) } type defaultHAproxyClient struct { } -func (c *defaultHAproxyClient) sendCommand(socketPath, command string) (string, error) { - conn, err := net.Dial("unix", socketPath) +func (c *defaultHAproxyClient) sendCommand(ctx context.Context, socketPath, command string) (string, error) { + conn, err := (&net.Dialer{}).DialContext(ctx, "unix", socketPath) if err != nil { return "", fmt.Errorf("failed to connect to socket %s: %w", socketPath, err) } diff --git a/support/azureutil/azureutil.go b/support/azureutil/azureutil.go index 07836f93d3a..56dbbcac1b3 100644 --- a/support/azureutil/azureutil.go +++ b/support/azureutil/azureutil.go @@ -75,7 +75,7 @@ func GetAzureCloudConfiguration(cloudName string) (cloud.Configuration, error) { func GetSubnetNameFromSubnetID(subnetID string) (string, error) { subnet, err := arm.ParseResourceID(subnetID) if err != nil { - return "", fmt.Errorf("failed to parse subnet ID %q: %v", subnetID, err) + return "", fmt.Errorf("failed to parse subnet ID %q: %w", subnetID, err) } if !strings.EqualFold(subnet.ResourceType.Type, "virtualnetworks/subnets") { @@ -94,7 +94,7 @@ func GetSubnetNameFromSubnetID(subnetID string) (string, error) { func GetNameAndResourceGroupFromNetworkSecurityGroupID(nsgID string) (string, string, error) { nsg, err := arm.ParseResourceID(nsgID) if err != nil { - return "", "", fmt.Errorf("failed to parse network security group ID %q: %v", nsgID, err) + return "", "", fmt.Errorf("failed to parse network security group ID %q: %w", nsgID, err) } if !strings.EqualFold(nsg.ResourceType.Type, "networkSecurityGroups") { @@ -117,7 +117,7 @@ func GetNameAndResourceGroupFromNetworkSecurityGroupID(nsgID string) (string, st func GetVnetNameAndResourceGroupFromVnetID(vnetID string) (string, string, error) { vnet, err := arm.ParseResourceID(vnetID) if err != nil { - return "", "", fmt.Errorf("failed to parse vnet ID %q: %v", vnetID, err) + return "", "", fmt.Errorf("failed to parse vnet ID %q: %w", vnetID, err) } if !strings.EqualFold(vnet.ResourceType.Type, "virtualNetworks") { @@ -141,7 +141,7 @@ func GetVnetNameAndResourceGroupFromVnetID(vnetID string) (string, string, error func GetVnetInfoFromVnetID(ctx context.Context, vnetID string, subscriptionID string, azureCreds azcore.TokenCredential, cloudName string) (armnetwork.VirtualNetworksClientGetResponse, error) { partialVnetInfo, err := arm.ParseResourceID(vnetID) if err != nil { - return armnetwork.VirtualNetworksClientGetResponse{}, fmt.Errorf("failed to parse vnet information from vnet ID %q: %v", vnetID, err) + return armnetwork.VirtualNetworksClientGetResponse{}, fmt.Errorf("failed to parse vnet information from vnet ID %q: %w", vnetID, err) } if !strings.EqualFold(partialVnetInfo.ResourceType.Type, "virtualNetworks") { @@ -210,7 +210,7 @@ func getFullVnetInfo(ctx context.Context, subscriptionID string, vnetResourceGro func GetNetworkSecurityGroupInfo(ctx context.Context, nsgID string, subscriptionID string, azureCreds azcore.TokenCredential, cloudName string) (armnetwork.SecurityGroupsClientGetResponse, error) { partialNSGInfo, err := arm.ParseResourceID(nsgID) if err != nil { - return armnetwork.SecurityGroupsClientGetResponse{}, fmt.Errorf("failed to parse network security group id %q: %v", nsgID, err) + return armnetwork.SecurityGroupsClientGetResponse{}, fmt.Errorf("failed to parse network security group id %q: %w", nsgID, err) } cloudConfig, err := GetAzureCloudConfiguration(cloudName) diff --git a/support/controlplane-component/controlplane-component.go b/support/controlplane-component/controlplane-component.go index 442e5aa7fc6..0757f7c97ab 100644 --- a/support/controlplane-component/controlplane-component.go +++ b/support/controlplane-component/controlplane-component.go @@ -177,7 +177,7 @@ func (c *controlPlaneWorkload[T]) Reconcile(cpContext ControlPlaneContext) error unavailableDependencies, err := c.checkDependencies(cpContext) if err != nil { - return fmt.Errorf("failed checking for dependencies availability: %v", err) + return fmt.Errorf("failed checking for dependencies availability: %w", err) } var reconcilationError error if len(unavailableDependencies) == 0 { @@ -311,7 +311,7 @@ func (c *controlPlaneWorkload[T]) update(cpContext ControlPlaneContext) error { func (c *controlPlaneWorkload[T]) reconcileWorkload(cpContext ControlPlaneContext) error { workloadObj, err := c.workloadProvider.LoadManifest(c.Name()) if err != nil { - return fmt.Errorf("failed loading workload manifest: %v", err) + return fmt.Errorf("failed loading workload manifest: %w", err) } // make sure that the Deployment/Statefulset name matches the component name. workloadObj.SetName(c.Name()) @@ -320,7 +320,7 @@ func (c *controlPlaneWorkload[T]) reconcileWorkload(cpContext ControlPlaneContex oldWorkloadObj := c.workloadProvider.NewObject() if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(workloadObj), oldWorkloadObj); err != nil { if !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to get old workload object: %v", err) + return fmt.Errorf("failed to get old workload object: %w", err) } } diff --git a/support/controlplane-component/controlplane-component_test.go b/support/controlplane-component/controlplane-component_test.go index b6cc458af2f..fd05adfb750 100644 --- a/support/controlplane-component/controlplane-component_test.go +++ b/support/controlplane-component/controlplane-component_test.go @@ -372,7 +372,7 @@ func componentsFakeObjects() ([]client.Object, error) { caCfg := certs.CertCfg{IsCA: true, Subject: pkix.Name{CommonName: "root-ca", OrganizationalUnit: []string{"ou"}}} key, cert, err := certs.GenerateSelfSignedCertificate(&caCfg) if err != nil { - return nil, fmt.Errorf("failed to generate self signed CA: %v", err) + return nil, fmt.Errorf("failed to generate self signed CA: %w", err) } csrSigner := manifests.CSRSignerCASecret(testComponentNamespace) csrSigner.Data = map[string][]byte{ diff --git a/support/controlplane-component/kubeconfig.go b/support/controlplane-component/kubeconfig.go index 5e1801844ab..07be8e325bf 100644 --- a/support/controlplane-component/kubeconfig.go +++ b/support/controlplane-component/kubeconfig.go @@ -21,7 +21,7 @@ const ( func (c *controlPlaneWorkload[T]) adaptServiceAccountKubeconfigSecret(cpContext WorkloadContext, secret *corev1.Secret) error { csrSigner := manifests.CSRSignerCASecret(cpContext.HCP.Namespace) if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(csrSigner), csrSigner); err != nil { - return fmt.Errorf("failed to get cluster-signer-ca secret: %v", err) + return fmt.Errorf("failed to get cluster-signer-ca secret: %w", err) } rootCA := manifests.RootCASecret(cpContext.HCP.Namespace) if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(rootCA), rootCA); err != nil { diff --git a/support/controlplane-component/token-minter-container.go b/support/controlplane-component/token-minter-container.go index 7a1d2aa6bd2..937e8da8dfd 100644 --- a/support/controlplane-component/token-minter-container.go +++ b/support/controlplane-component/token-minter-container.go @@ -41,7 +41,7 @@ type TokenMinterContainerOptions struct { // defaults to 'kubeconfig' KubeconfingVolumeName string - // KubeconfigSecretName is the name of the the kubeconfig secret used to mint the token in the target cluster. + // KubeconfigSecretName is the name of the kubeconfig secret used to mint the token in the target cluster. KubeconfigSecretName string // OneShot, if true, will cause the token-minter container to exit after minting the token. diff --git a/support/gcpapi/gcs_client.go b/support/gcpapi/gcs_client.go index adc91305920..4c98fe5af01 100644 --- a/support/gcpapi/gcs_client.go +++ b/support/gcpapi/gcs_client.go @@ -2,6 +2,7 @@ package gcpapi import ( "context" + "errors" "fmt" "io" "net/http" @@ -42,7 +43,8 @@ func (g *GCSClient) UploadObject(ctx context.Context, bucket, objectName string, func (g *GCSClient) DeleteObject(ctx context.Context, bucket, objectName string) error { err := g.objects.Delete(bucket, objectName).Context(ctx).Do() if err != nil { - if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == http.StatusNotFound { + var gerr *googleapi.Error + if errors.As(err, &gerr) && gerr.Code == http.StatusNotFound { return nil } return fmt.Errorf("failed to delete gs://%s/%s: %w", bucket, objectName, err) diff --git a/support/globalconfig/proxy.go b/support/globalconfig/proxy.go index e51f515c9c6..76275936ddb 100644 --- a/support/globalconfig/proxy.go +++ b/support/globalconfig/proxy.go @@ -20,7 +20,7 @@ func ProxyConfig() *configv1.Proxy { } } -// TODO (relyt0925): this is is utilized by the machine config server and feeds into the user data setup to download +// TODO (relyt0925): this is utilized by the machine config server and feeds into the user data setup to download // ignition configuration. It also factors into the machine config served to the machine. These usages need to be // further examined before it directly takes what the user configures for the cluster-wide proxy at the SDN level. // In current state of the world: the in cluster proxy configuration set by the user is never picked up (only the one diff --git a/support/konnectivityproxy/dialer.go b/support/konnectivityproxy/dialer.go index cfe3a307d85..d579dba6c1d 100644 --- a/support/konnectivityproxy/dialer.go +++ b/support/konnectivityproxy/dialer.go @@ -288,7 +288,7 @@ func (p *konnectivityProxy) DialContext(ctx context.Context, network string, req } konnectivityConnection, err := tlsDialer.DialContext(ctx, "tcp", konnectivityServerAddress) if err != nil { - return nil, fmt.Errorf("dialing proxy %q failed: %v", konnectivityServerAddress, err) + return nil, fmt.Errorf("dialing proxy %q failed: %w", konnectivityServerAddress, err) } // Bound CONNECT handshake I/O to avoid indefinite stalls and clear on success. @@ -323,10 +323,10 @@ func (p *konnectivityProxy) DialContext(ctx context.Context, network string, req res, err := http.ReadResponse(br, nil) if err != nil { _ = konnectivityConnection.Close() - return nil, fmt.Errorf("reading HTTP response from CONNECT to %s via proxy %s failed: %v", + return nil, fmt.Errorf("reading HTTP response from CONNECT to %s via proxy %s failed: %w", requestAddress, konnectivityServerAddress, err) } - if res.StatusCode != 200 { + if res.StatusCode != http.StatusOK { log.Info("Status code was not 200", "statusCode", res.StatusCode) _ = konnectivityConnection.Close() return nil, fmt.Errorf("proxy error from %s while dialing %s: %v", konnectivityServerAddress, requestAddress, res.Status) diff --git a/support/konnectivityproxy/dialer_test.go b/support/konnectivityproxy/dialer_test.go index 15299d84444..ca327cad709 100644 --- a/support/konnectivityproxy/dialer_test.go +++ b/support/konnectivityproxy/dialer_test.go @@ -284,7 +284,7 @@ func TestKonnectivityHealthEndRetryPreventsStubbornFlag(t *testing.T) { // All accepted connections inherit a deadline so io.Copy never blocks indefinitely. func startTCPEchoServer(t *testing.T) net.Listener { t.Helper() - ln, err := net.Listen("tcp", "127.0.0.1:0") + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") if err != nil { t.Fatalf("failed to start echo server: %v", err) } @@ -315,7 +315,7 @@ func startTCPEchoServer(t *testing.T) net.Listener { // deadlines so they cannot outlive the test. func startConnectProxy(t *testing.T, connectCount *atomic.Int32) net.Listener { t.Helper() - ln, err := net.Listen("tcp", "127.0.0.1:0") + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") if err != nil { t.Fatalf("failed to start proxy server: %v", err) } @@ -329,7 +329,7 @@ func startConnectProxy(t *testing.T, connectCount *atomic.Int32) net.Listener { } connectCount.Add(1) - target, err := net.DialTimeout("tcp", r.Host, 5*time.Second) + target, err := (&net.Dialer{Timeout: 5 * time.Second}).DialContext(r.Context(), "tcp", r.Host) if err != nil { http.Error(w, err.Error(), http.StatusBadGateway) return diff --git a/support/konnectivityproxy/proxy_dialer.go b/support/konnectivityproxy/proxy_dialer.go index dcfec2ac578..c4550704a6d 100644 --- a/support/konnectivityproxy/proxy_dialer.go +++ b/support/konnectivityproxy/proxy_dialer.go @@ -60,7 +60,7 @@ func (hpd *httpProxyDialer) Dial(network string, addr string) (net.Conn, error) return nil, err } - if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { conn.Close() f := strings.SplitN(resp.Status, " ", 2) return nil, errors.New(f[1]) diff --git a/support/releaseinfo/registryclient/client.go b/support/releaseinfo/registryclient/client.go index dd6db378208..51160236b77 100644 --- a/support/releaseinfo/registryclient/client.go +++ b/support/releaseinfo/registryclient/client.go @@ -67,7 +67,7 @@ func ExtractImageFiles(ctx context.Context, imageRef string, pullSecret []byte, err := func() error { r, err := fromBlobs.Open(ctx, layer.Digest) if err != nil { - return fmt.Errorf("unable to access the source layer %s: %v", layer.Digest, err) + return fmt.Errorf("unable to access the source layer %s: %w", layer.Digest, err) } defer r.Close() rc, err := dockerarchive.DecompressStream(r) @@ -137,7 +137,7 @@ func ExtractImageFile(ctx context.Context, imageRef string, pullSecret []byte, f err := func() error { r, err := fromBlobs.Open(ctx, layer.Digest) if err != nil { - return fmt.Errorf("unable to access the source layer %s: %v", layer.Digest, err) + return fmt.Errorf("unable to access the source layer %s: %w", layer.Digest, err) } defer r.Close() rc, err := dockerarchive.DecompressStream(r) @@ -195,7 +195,7 @@ func ExtractImageFilesToDir(ctx context.Context, imageRef string, pullSecret []b err := func() error { r, err := fromBlobs.Open(ctx, layer.Digest) if err != nil { - return fmt.Errorf("unable to access the source layer %s: %v", layer.Digest, err) + return fmt.Errorf("unable to access the source layer %s: %w", layer.Digest, err) } defer r.Close() rc, err := dockerarchive.DecompressStream(r) diff --git a/support/releaseinfo/releaseinfo.go b/support/releaseinfo/releaseinfo.go index e6026a5e965..5e21316fe4f 100644 --- a/support/releaseinfo/releaseinfo.go +++ b/support/releaseinfo/releaseinfo.go @@ -206,7 +206,7 @@ func readComponentVersions(is *imageapi.ImageStream) (ComponentVersions, []error } all, err := parseComponentVersionsLabel(versions, tag.Annotations[annotationBuildVersionsDisplayNames]) if err != nil { - errs = append(errs, fmt.Errorf("the referenced image %s had an invalid version annotation: %v", tag.Name, err)) + errs = append(errs, fmt.Errorf("the referenced image %s had an invalid version annotation: %w", tag.Name, err)) } for k, v := range all { if k == "kubectl" { @@ -326,7 +326,7 @@ func parseComponentVersionsLabel(label, displayNames string) (ComponentVersions, } v, err := semver.Parse(parts[1]) if err != nil { - return nil, fmt.Errorf("the version pair %q must have a valid semantic version: %v", pair, err) + return nil, fmt.Errorf("the version pair %q must have a valid semantic version: %w", pair, err) } v.Build = nil labels[parts[0]] = ComponentVersion{ diff --git a/support/releaseinfo/releaseinfo_test.go b/support/releaseinfo/releaseinfo_test.go index 90522a0b51e..199d944b4bb 100644 --- a/support/releaseinfo/releaseinfo_test.go +++ b/support/releaseinfo/releaseinfo_test.go @@ -46,6 +46,11 @@ func TestParseComponentVersionsLabel(t *testing.T) { displayNames: "mycomponent=Invalid ", expectError: true, }, + { + name: "When version is not valid semver it should return an error", + label: "mycomponent=not-a-version", + expectError: true, + }, } for _, tt := range tests { @@ -98,6 +103,25 @@ func TestReadComponentVersions(t *testing.T) { }, expectKey: "machine-os", }, + { + name: "When version annotation has invalid semver, it should return an error", + tags: []imageapi.TagReference{ + { + Name: "bad-image", + Annotations: map[string]string{ + annotationBuildVersions: "component=not-a-semver", + }, + }, + { + Name: "good-image", + Annotations: map[string]string{ + annotationBuildVersions: "component=1.0.0", + }, + }, + }, + expectError: true, + expectKey: "component", + }, { name: "When multiple non-machine-os versions exist it should return an error", tags: []imageapi.TagReference{ diff --git a/support/supportedversion/version.go b/support/supportedversion/version.go index f2a51177a26..76649a9ae05 100644 --- a/support/supportedversion/version.go +++ b/support/supportedversion/version.go @@ -220,7 +220,7 @@ func LookupDefaultOCPVersion(ctx context.Context, releaseStream string, client c version, err := retrieveSupportedOCPVersion(ctx, releaseURL, client) if err != nil { - return ocpVersion{}, fmt.Errorf("failed to get OCP version from release URL %s: %v", releaseURL, err) + return ocpVersion{}, fmt.Errorf("failed to get OCP version from release URL %s: %w", releaseURL, err) } return version, nil @@ -267,7 +267,7 @@ func LookupLatestSupportedRelease(ctx context.Context, hc *hyperv1.HostedCluster var version ocpVersion - req, err := http.NewRequestWithContext(ctx, "GET", releaseURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, releaseURL, nil) if err != nil { return "", err } @@ -336,7 +336,7 @@ func GetSupportedOCPVersions(ctx context.Context, namespace string, client crcli // Fetch the supported versions ConfigMap from the specified namespace supportedVersions = manifests.ConfigMap(namespace) if err := client.Get(ctx, crclient.ObjectKeyFromObject(supportedVersions), supportedVersions); err != nil { - return SupportedVersions{}, "", fmt.Errorf("failed to find supported versions on the server: %v", err) + return SupportedVersions{}, "", fmt.Errorf("failed to find supported versions on the server: %w", err) } } @@ -350,7 +350,7 @@ func GetSupportedOCPVersions(ctx context.Context, namespace string, client crcli // Check if the ConfigMap contains the supported versions key if supportedVersionData, present := supportedVersions.Data[config.ConfigMapVersionsKey]; present { if err := json.Unmarshal([]byte(supportedVersionData), &versions); err != nil { - return SupportedVersions{}, "", fmt.Errorf("failed to parse supported versions on the server: %v", err) + return SupportedVersions{}, "", fmt.Errorf("failed to parse supported versions on the server: %w", err) } return versions, serverVersion, nil @@ -483,7 +483,7 @@ func retrieveSupportedOCPVersion(ctx context.Context, releaseURL string, client configMapList := &corev1.ConfigMapList{} err := client.List(ctx, configMapList, crclient.MatchingLabels{"hypershift.openshift.io/supported-versions": "true"}) if err != nil { - return ocpVersion{}, fmt.Errorf("failed to list ConfigMaps to find supported versions: %v", err) + return ocpVersion{}, fmt.Errorf("failed to list ConfigMaps to find supported versions: %w", err) } for _, configMap := range configMapList.Items { if configMap.Name == "supported-versions" { @@ -500,14 +500,18 @@ func retrieveSupportedOCPVersion(ctx context.Context, releaseURL string, client // Get the latest supported OCP version from the supported versions ConfigMap supportedOCPVersions, _, err := GetSupportedOCPVersions(ctx, namespace, client, supportedVersions) if err != nil { - return ocpVersion{}, fmt.Errorf("failed to get supported OCP versions: %v", err) + return ocpVersion{}, fmt.Errorf("failed to get supported OCP versions: %w", err) } if len(supportedOCPVersions.Versions) == 0 { return ocpVersion{}, fmt.Errorf("no supported OCP versions found in the ConfigMap") } // Fetch the release information from the URL - resp, err := http.Get(releaseURL) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, releaseURL, nil) + if err != nil { + return ocpVersion{}, err + } + resp, err := http.DefaultClient.Do(req) if err != nil { return ocpVersion{}, err } diff --git a/support/supportedversion/version_test.go b/support/supportedversion/version_test.go index 87d816f68a1..8907229209a 100644 --- a/support/supportedversion/version_test.go +++ b/support/supportedversion/version_test.go @@ -1,6 +1,7 @@ package supportedversion import ( + "context" "encoding/json" "fmt" "net/http" @@ -19,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "github.com/blang/semver" ) @@ -926,6 +928,13 @@ func TestRetrieveSupportedOCPVersion(t *testing.T) { expectErr: true, expectedErrMsg: "failed to get supported OCP versions", }, + { + name: "When the release URL is invalid, expect a request creation error", + cm: supportedVersionsCM, + releaseURL: "://invalid-url", + expectErr: true, + expectedErrMsg: "parse", + }, { name: "When the ConfigMap supports older versions, expect the latest older version to be returned", cm: olderSupportedVersionsCM, @@ -963,6 +972,44 @@ func TestRetrieveSupportedOCPVersion(t *testing.T) { } } +func TestRetrieveSupportedOCPVersion_ListFailure(t *testing.T) { + g := NewWithT(t) + + scheme := api.Scheme + g.Expect(corev1.AddToScheme(scheme)).To(Succeed()) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + return fmt.Errorf("connection refused") + }, + }). + Build() + + _, err := retrieveSupportedOCPVersion(t.Context(), "https://example.com/tags", fakeClient) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to list ConfigMaps to find supported versions")) +} + +func TestLookupDefaultOCPVersion_ListFailure(t *testing.T) { + g := NewWithT(t) + + scheme := api.Scheme + g.Expect(corev1.AddToScheme(scheme)).To(Succeed()) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + return fmt.Errorf("connection refused") + }, + }). + Build() + + _, err := LookupDefaultOCPVersion(t.Context(), "", fakeClient) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to get OCP version from release URL")) +} + func TestGetArchFromStream(t *testing.T) { testCases := []struct { name string diff --git a/support/thirdparty/docker/pkg/archive/archive.go b/support/thirdparty/docker/pkg/archive/archive.go index abcfeffbbe1..d7fd646b76f 100644 --- a/support/thirdparty/docker/pkg/archive/archive.go +++ b/support/thirdparty/docker/pkg/archive/archive.go @@ -140,7 +140,7 @@ func cmdStream(cmd *exec.Cmd, input io.Reader) (io.ReadCloser, error) { // Copy stdout to the returned pipe go func() { if err := cmd.Wait(); err != nil { - pipeW.CloseWithError(fmt.Errorf("%s: %s", err, errBuf.String())) + pipeW.CloseWithError(fmt.Errorf("%w: %s", err, errBuf.String())) } else { pipeW.Close() } diff --git a/support/thirdparty/library-go/pkg/image/dockerv1client/types.go b/support/thirdparty/library-go/pkg/image/dockerv1client/types.go index 3b85b81e0be..d5724da82c2 100644 --- a/support/thirdparty/library-go/pkg/image/dockerv1client/types.go +++ b/support/thirdparty/library-go/pkg/image/dockerv1client/types.go @@ -73,7 +73,7 @@ type Descriptor struct { Size int64 `json:"size,omitempty"` // Digest uniquely identifies the content. A byte stream can be verified - // against against this digest. + // against this digest. Digest string `json:"digest,omitempty"` } diff --git a/support/thirdparty/library-go/pkg/image/registryclient/client.go b/support/thirdparty/library-go/pkg/image/registryclient/client.go index 9220e2087bc..cf1ab87d860 100644 --- a/support/thirdparty/library-go/pkg/image/registryclient/client.go +++ b/support/thirdparty/library-go/pkg/image/registryclient/client.go @@ -2,6 +2,7 @@ package registryclient import ( "context" + "errors" "fmt" "hash" "io" @@ -181,7 +182,7 @@ func (c *Context) Ping(ctx context.Context, registry *url.URL, insecure bool) (h } // follow redirects - redirect, err := c.ping(src, insecure, t) + redirect, err := c.ping(ctx, src, insecure, t) c.lock.Lock() defer c.lock.Unlock() @@ -223,14 +224,14 @@ func (c *Context) Repository(ctx context.Context, registry *url.URL, repoName st return NewLimitedRetryRepository(repo, c.Retries, limiter), nil } -func (c *Context) ping(registry url.URL, insecure bool, transport http.RoundTripper) (*url.URL, error) { +func (c *Context) ping(ctx context.Context, registry url.URL, insecure bool, transport http.RoundTripper) (*url.URL, error) { pingClient := &http.Client{ Transport: transport, Timeout: 15 * time.Second, } target := registry target.Path = path.Join(target.Path, "v2") + "/" - req, err := http.NewRequest("GET", target.String(), nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) if err != nil { return nil, err } @@ -238,7 +239,7 @@ func (c *Context) ping(registry url.URL, insecure bool, transport http.RoundTrip if err != nil { if insecure && registry.Scheme == "https" { registry.Scheme = "http" - _, nErr := c.ping(registry, true, transport) + _, nErr := c.ping(ctx, registry, true, transport) if nErr != nil { return nil, nErr } @@ -377,20 +378,24 @@ func isTemporaryHTTPError(err error) (time.Duration, bool) { if err == nil { return 0, false } - switch t := err.(type) { - case net.Error: - return time.Second, t.Timeout() - case errcode.ErrorCoder: + var netErr net.Error + if errors.As(err, &netErr) { + return time.Second, netErr.Timeout() + } + var errorCoder errcode.ErrorCoder + if errors.As(err, &errorCoder) { // note: we explicitly do not check errcode.ErrorCodeUnknown because that is used in // a wide range of scenarios to convey "generic error", not "retryable error" - switch t.ErrorCode() { + switch errorCoder.ErrorCode() { case errcode.ErrorCodeUnavailable: return 5 * time.Second, true case errcode.ErrorCodeTooManyRequests: return 2 * time.Second, true } - case *registryclient.UnexpectedHTTPResponseError: - switch t.StatusCode { + } + var unexpectedErr *registryclient.UnexpectedHTTPResponseError + if errors.As(err, &unexpectedErr) { + switch unexpectedErr.StatusCode { case http.StatusInternalServerError, http.StatusGatewayTimeout, http.StatusServiceUnavailable, http.StatusBadGateway: return 5 * time.Second, true case http.StatusTooManyRequests: @@ -692,7 +697,7 @@ func (r *readSeekCloserVerifier) Read(p []byte) (n int, err error) { if n > 0 { r.hash.Write(p[:n]) } - if err == io.EOF { + if errors.Is(err, io.EOF) { actual := digest.NewDigest(r.expect.Algorithm(), r.hash) if actual != r.expect { return n, fmt.Errorf("content integrity error: the blob streamed from digest %s does not match the digest calculated from the content %s", r.expect, actual) diff --git a/support/thirdparty/oc/pkg/cli/image/manifest/manifest.go b/support/thirdparty/oc/pkg/cli/image/manifest/manifest.go index 616cc9cf626..5975074987a 100644 --- a/support/thirdparty/oc/pkg/cli/image/manifest/manifest.go +++ b/support/thirdparty/oc/pkg/cli/image/manifest/manifest.go @@ -146,12 +146,12 @@ func ManifestToImageConfig(ctx context.Context, srcManifest distribution.Manifes } configJSON, err := blobs.Get(ctx, t.Config.Digest) if err != nil { - return nil, nil, fmt.Errorf("cannot retrieve image configuration for %s: %v", location, err) + return nil, nil, fmt.Errorf("cannot retrieve image configuration for %s: %w", location, err) } klog.V(4).Infof("Raw image config json:\n%s", string(configJSON)) config := &dockerv1client.DockerImageConfig{} if err := json.Unmarshal(configJSON, &config); err != nil { - return nil, nil, fmt.Errorf("unable to parse image configuration: %v", err) + return nil, nil, fmt.Errorf("unable to parse image configuration: %w", err) } base := config @@ -169,12 +169,12 @@ func ManifestToImageConfig(ctx context.Context, srcManifest distribution.Manifes } configJSON, err := blobs.Get(ctx, t.Config.Digest) if err != nil { - return nil, nil, fmt.Errorf("cannot retrieve image configuration for %s: %v", location, err) + return nil, nil, fmt.Errorf("cannot retrieve image configuration for %s: %w", location, err) } klog.V(4).Infof("Raw image config json:\n%s", string(configJSON)) config := &dockerv1client.DockerImageConfig{} if err := json.Unmarshal(configJSON, &config); err != nil { - return nil, nil, fmt.Errorf("unable to parse image configuration: %v", err) + return nil, nil, fmt.Errorf("unable to parse image configuration: %w", err) } base := config @@ -219,11 +219,11 @@ func ProcessManifestList(ctx context.Context, srcDigest digest.Digest, srcManife var err error t, err = manifestlist.FromDescriptors(filtered) if err != nil { - return nil, nil, "", fmt.Errorf("unable to filter source image %s manifest list: %v", ref, err) + return nil, nil, "", fmt.Errorf("unable to filter source image %s manifest list: %w", ref, err) } _, body, err := t.Payload() if err != nil { - return nil, nil, "", fmt.Errorf("unable to filter source image %s manifest list (bad payload): %v", ref, err) + return nil, nil, "", fmt.Errorf("unable to filter source image %s manifest list (bad payload): %w", ref, err) } manifestList = t manifestDigest, err = registryclient.ContentDigestForManifest(t, srcDigest.Algorithm()) @@ -236,7 +236,7 @@ func ProcessManifestList(ctx context.Context, srcDigest digest.Digest, srcManife for i, manifest := range filtered { childManifest, err := manifests.Get(ctx, manifest.Digest, PreferManifestList) if err != nil { - return nil, nil, "", fmt.Errorf("unable to retrieve source image %s manifest #%d from manifest list: %v", ref, i+1, err) + return nil, nil, "", fmt.Errorf("unable to retrieve source image %s manifest #%d from manifest list: %w", ref, i+1, err) } childManifests = append(childManifests, childManifest) } diff --git a/support/util/util.go b/support/util/util.go index 20ba8b2a010..4b335251c02 100644 --- a/support/util/util.go +++ b/support/util/util.go @@ -440,7 +440,7 @@ func SanitizeIgnitionPayload(payload []byte) error { var jsonPayload ignitionapi.Config if err := json.Unmarshal(payload, &jsonPayload); err != nil { - return fmt.Errorf("error unmarshalling Ignition payload: %v", err) + return fmt.Errorf("error unmarshalling Ignition payload: %w", err) } return nil diff --git a/support/validations/authentication.go b/support/validations/authentication.go index 12a98f0326a..6c1212b03dc 100644 --- a/support/validations/authentication.go +++ b/support/validations/authentication.go @@ -68,7 +68,7 @@ func ValidateAuthenticationSpecForTypeOIDC(ctx context.Context, client crclient. apiServerAuthConfig, err := kas.HCPAuthConfigToAPIServerAuthConfig(authConfig) if err != nil { - return fmt.Errorf("converting from HCP auth config type to apiserver auth config type: %v", err) + return fmt.Errorf("converting from HCP auth config type to apiserver auth config type: %w", err) } fieldErrors := validation.ValidateAuthenticationConfiguration(celCompiler, apiServerAuthConfig, disallowIssuers) diff --git a/sync-fg-configmap/update.go b/sync-fg-configmap/update.go index a4df75ab3e8..b82e8aaba6e 100644 --- a/sync-fg-configmap/update.go +++ b/sync-fg-configmap/update.go @@ -40,7 +40,7 @@ func NewRunCommand() *cobra.Command { Name: "feature-gate", PayloadVersion: os.Getenv("PAYLOAD_VERSION"), } - cmd.Flags().StringVar(&opts.File, "file", opts.File, "The path path to the file that contains the feature gate YAML to apply.") + cmd.Flags().StringVar(&opts.File, "file", opts.File, "The path to the file that contains the feature gate YAML to apply.") cmd.Flags().StringVar(&opts.Namespace, "namespace", opts.Namespace, "The control plane namespace for the feature gate configmap.") cmd.Flags().StringVar(&opts.Name, "name", opts.Name, "The name of the feature gate configmap.") cmd.Flags().StringVar(&opts.PayloadVersion, "payload-version", opts.PayloadVersion, "The payload version of the control plane.") diff --git a/test/e2e/util/aws.go b/test/e2e/util/aws.go index dcf9b475495..fef417b8016 100644 --- a/test/e2e/util/aws.go +++ b/test/e2e/util/aws.go @@ -375,7 +375,7 @@ func CreateCapacityReservation(ctx context.Context, awsCreds, awsRegion, instanc CapacityReservationIds: []string{crID}, }) if err != nil { - return false, nil // transient error, retry + return false, nil //nolint:nilerr // transient error, retry } if len(desc.CapacityReservations) == 0 { return false, nil diff --git a/test/e2e/util/dump/dump.go b/test/e2e/util/dump/dump.go index 38e48946703..02b7e4cdea6 100644 --- a/test/e2e/util/dump/dump.go +++ b/test/e2e/util/dump/dump.go @@ -68,7 +68,7 @@ func DumpMachineConsoleLogs(ctx context.Context, hc *hyperv1.HostedCluster, awsC } err := consoleLogs.Run(ctx) if err != nil { - return fmt.Errorf("failed to get machine console logs: %v", err) + return fmt.Errorf("failed to get machine console logs: %w", err) } return nil } diff --git a/test/e2e/util/dump/journals.go b/test/e2e/util/dump/journals.go index 20a9b4fe0b9..b0ad68fc64c 100644 --- a/test/e2e/util/dump/journals.go +++ b/test/e2e/util/dump/journals.go @@ -78,7 +78,7 @@ func DumpJournals(t *testing.T, ctx context.Context, hc *hyperv1.HostedCluster, return nil } - return runJournalDumpScript(t, hc, artifactDir, copyJournalFile, privateKeyFile, bastionIP, machineIPs, machineInstances) + return runJournalDumpScript(ctx, t, hc, artifactDir, copyJournalFile, privateKeyFile, bastionIP, machineIPs, machineInstances) } func setupSSHKey(ctx context.Context, hc *hyperv1.HostedCluster) (string, error) { @@ -133,7 +133,7 @@ func setupBastionLoggers(artifactDir string) (*zap.Logger, *zap.Logger, error) { createLogFile := filepath.Join(artifactDir, "create-bastion.log") createLog, err := os.Create(createLogFile) if err != nil { - return nil, nil, fmt.Errorf("failed to create create log: %w", err) + return nil, nil, fmt.Errorf("failed to create log: %w", err) } createLogger := zap.New(zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.Lock(createLog), zap.DebugLevel)) @@ -269,7 +269,7 @@ func authorizeSSHAccess(ctx context.Context, hc *hyperv1.HostedCluster, awsCreds return nil } -func runJournalDumpScript(t *testing.T, hc *hyperv1.HostedCluster, artifactDir, copyJournalFile, privateKeyFile, bastionIP string, machineIPs []string, machineInstances []ec2types.Instance) error { +func runJournalDumpScript(ctx context.Context, t *testing.T, hc *hyperv1.HostedCluster, artifactDir, copyJournalFile, privateKeyFile, bastionIP string, machineIPs []string, machineInstances []ec2types.Instance) error { dumpJournalsLogFile := filepath.Join(artifactDir, "dump-machine-journals.log") dumpJournalsLog, err := os.Create(dumpJournalsLogFile) if err != nil { @@ -277,7 +277,7 @@ func runJournalDumpScript(t *testing.T, hc *hyperv1.HostedCluster, artifactDir, } outputDir := filepath.Join(artifactDir, "machine-journals") - scriptCmd := exec.Command(copyJournalFile, outputDir) + scriptCmd := exec.CommandContext(ctx, copyJournalFile, outputDir) env := os.Environ() env = append(env, fmt.Sprintf("BASTION_IP=%s", bastionIP)) env = append(env, fmt.Sprintf("INSTANCE_IPS=%s", strings.Join(machineIPs, " "))) diff --git a/test/e2e/util/external_oidc.go b/test/e2e/util/external_oidc.go index b4dd731e8f8..adaa8d63c0e 100644 --- a/test/e2e/util/external_oidc.go +++ b/test/e2e/util/external_oidc.go @@ -15,6 +15,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "testing" "time" @@ -264,7 +265,10 @@ func ChangeUserForKeycloakExtOIDC(t *testing.T, ctx context.Context, clientCfg * "username": []string{username}, } - response, err := httpClient.PostForm(requestURL, formData) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, requestURL, strings.NewReader(formData.Encode())) + g.Expect(err).NotTo(HaveOccurred()) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + response, err := httpClient.Do(req) g.Expect(err).NotTo(HaveOccurred()) defer response.Body.Close() g.Expect(response.StatusCode).To(Equal(http.StatusOK)) diff --git a/test/e2e/util/fixture.go b/test/e2e/util/fixture.go index f8c2100117b..e12aae01d1f 100644 --- a/test/e2e/util/fixture.go +++ b/test/e2e/util/fixture.go @@ -186,7 +186,7 @@ func createCluster(ctx context.Context, hc *hyperv1.HostedCluster, opts *Platfor func renderCreate(ctx context.Context, opts *core.RawCreateOptions, platformOpts core.PlatformValidator, outputFile string, renderLogFile string, createLogFile string) error { renderLog, err := os.Create(renderLogFile) if err != nil { - return fmt.Errorf("failed to render render log: %w", err) + return fmt.Errorf("failed to render log: %w", err) } renderLogger := zap.New(zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.Lock(renderLog), zap.DebugLevel)) defer func() { @@ -204,7 +204,7 @@ func renderCreate(ctx context.Context, opts *core.RawCreateOptions, platformOpts createLog, err := os.Create(createLogFile) if err != nil { - return fmt.Errorf("failed to create create log: %w", err) + return fmt.Errorf("failed to create log: %w", err) } createLogger := zap.New(zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.Lock(createLog), zap.DebugLevel)) defer func() { @@ -225,7 +225,7 @@ func destroyCluster(ctx context.Context, t *testing.T, hc *hyperv1.HostedCluster destroyLogFile := filepath.Join(outputDir, "destroy.log") destroyLog, err := os.Create(destroyLogFile) if err != nil { - return fmt.Errorf("failed to destroy destroy log: %w", err) + return fmt.Errorf("failed to destroy log: %w", err) } destroyLogger := zap.New(zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.Lock(destroyLog), zap.DebugLevel)) defer func() { diff --git a/test/e2e/util/generate.go b/test/e2e/util/generate.go index 3ca2004539c..99b0d60bb04 100644 --- a/test/e2e/util/generate.go +++ b/test/e2e/util/generate.go @@ -26,7 +26,7 @@ import ( // available to guide selection of new names and this interface hides those details. type NameGenerator interface { // GenerateName generates a valid name from the base name, adding a random suffix to the - // the base. If base is valid, the returned name must also be valid. The generator is + // base. If base is valid, the returned name must also be valid. The generator is // responsible for knowing the maximum valid name length. GenerateName(base string) string } diff --git a/test/e2e/util/node.go b/test/e2e/util/node.go index ca288bfb829..374e130c5cd 100644 --- a/test/e2e/util/node.go +++ b/test/e2e/util/node.go @@ -33,12 +33,12 @@ func EnsureNodeCommunication(t *testing.T, ctx context.Context, client crclient. err = wait.PollUntilContextTimeout(ctx, 10*time.Second, 5*time.Minute, true, func(ctx context.Context) (done bool, err error) { podList, err := guestClient.CoreV1().Pods("kube-system").List(ctx, metav1.ListOptions{LabelSelector: "app=konnectivity-agent"}) if err != nil || len(podList.Items) == 0 { - return false, nil + return false, nil //nolint:nilerr // retry until konnectivity-agent pod is available } _, err = guestClient.CoreV1().Pods("kube-system").GetLogs(podList.Items[0].Name, &corev1.PodLogOptions{Container: "konnectivity-agent"}).DoRaw(ctx) if err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until logs are available } return true, nil diff --git a/test/e2e/util/oauth.go b/test/e2e/util/oauth.go index 9e3497c959a..d4a116b7c7a 100644 --- a/test/e2e/util/oauth.go +++ b/test/e2e/util/oauth.go @@ -116,7 +116,7 @@ func WaitForOAuthTokenByHost(t testing.TB, ctx context.Context, oauthHost string oauthClient := configmanifests.OAuthServerChallengingClient().Name tokenReqUrl := fmt.Sprintf("https://%s/oauth/authorize?response_type=token&client_id=%s", oauthHost, oauthClient) - request, err := http.NewRequest(http.MethodGet, tokenReqUrl, nil) + request, err := http.NewRequestWithContext(ctx, http.MethodGet, tokenReqUrl, nil) g.Expect(err).ToNot(HaveOccurred()) request.Header.Set("Authorization", getBasicHeader(username, password)) @@ -173,14 +173,14 @@ func WaitForOAuthRouteReady(t *testing.T, ctx context.Context, client crclient.C err := wait.PollUntilContextTimeout(ctx, time.Second, time.Minute, true, func(ctx context.Context) (done bool, err error) { err = client.Get(context.Background(), crclient.ObjectKeyFromObject(route), route) if err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until route exists } return true, nil }) g.Expect(err).ToNot(HaveOccurred(), "failed retrieving oauth route") t.Logf("Found OAuth route %s", route.Spec.Host) - request, err := http.NewRequest(http.MethodHead, fmt.Sprintf("https://%s/healthz", route.Spec.Host), nil) + request, err := http.NewRequestWithContext(ctx, http.MethodHead, fmt.Sprintf("https://%s/healthz", route.Spec.Host), nil) g.Expect(err).ToNot(HaveOccurred()) transport, err := restclient.TransportFor(restclient.AnonymousClientConfig(restConfig)) @@ -253,7 +253,7 @@ func WaitForOauthConfig(t testing.TB, ctx context.Context, client crclient.Clien err := wait.PollUntilContextTimeout(ctx, time.Second, 10*time.Minute, true, func(ctx context.Context) (done bool, err error) { err = client.Get(context.Background(), crclient.ObjectKeyFromObject(oauthConfigCM), oauthConfigCM) if err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until configmap exists } data, ok := oauthConfigCM.Data[OAuthServerConfigKey] if !ok || data == "" { @@ -262,7 +262,7 @@ func WaitForOauthConfig(t testing.TB, ctx context.Context, client crclient.Clien ouathConfig := &osinv1.OsinServerConfig{} if _, _, err := api.YamlSerializer.Decode([]byte(data), nil, ouathConfig); err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until config is parseable } if len(ouathConfig.OAuthConfig.IdentityProviders) == 0 { return false, nil @@ -328,7 +328,7 @@ func WaitForOAuthLoadBalancerReady(t testing.TB, ctx context.Context, client crc svc := hcpmanifests.OauthServerService(hcpNamespace) err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 10*time.Minute, true, func(ctx context.Context) (done bool, err error) { if err := client.Get(ctx, crclient.ObjectKeyFromObject(svc), svc); err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until service exists } if svc.Spec.Type != corev1.ServiceTypeLoadBalancer { t.Logf("Waiting for oauth-openshift Service type to be LoadBalancer, got %s", svc.Spec.Type) @@ -349,7 +349,7 @@ func WaitForOAuthLoadBalancerReady(t testing.TB, ctx context.Context, client crc // Wait for the OAuth hostname to be resolvable via DNS (ExternalDNS creates the record) // and for the /healthz endpoint to return HTTP 200 - request, err := http.NewRequest(http.MethodHead, fmt.Sprintf("https://%s/healthz", oauthHost), nil) + request, err := http.NewRequestWithContext(ctx, http.MethodHead, fmt.Sprintf("https://%s/healthz", oauthHost), nil) g.Expect(err).ToNot(HaveOccurred()) transport, err := restclient.TransportFor(restclient.AnonymousClientConfig(restConfig)) diff --git a/test/e2e/util/reqserving/verifycp.go b/test/e2e/util/reqserving/verifycp.go index 6199246dadd..34210ab66b8 100644 --- a/test/e2e/util/reqserving/verifycp.go +++ b/test/e2e/util/reqserving/verifycp.go @@ -93,7 +93,7 @@ func verifyKASGoMemLimit(ctx context.Context, client crclient.Client, cpNamespac err := wait.PollUntilContextCancel(pollCtx, DefaultPollingInterval, true, func(pctx context.Context) (bool, error) { kasPods := &corev1.PodList{} if err := client.List(pctx, kasPods, crclient.MatchingLabels{"app": "kube-apiserver"}, crclient.InNamespace(cpNamespace)); err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until pods are listable } if len(kasPods.Items) == 0 { return false, nil @@ -132,7 +132,7 @@ func verifyInflightConfig(ctx context.Context, client crclient.Client, cpNamespa err := wait.PollUntilContextCancel(pollCtx, DefaultPollingInterval, true, func(pctx context.Context) (bool, error) { kasConfigMap := &corev1.ConfigMap{} if err := client.Get(pctx, types.NamespacedName{Name: "kas-config", Namespace: cpNamespace}, kasConfigMap); err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until configmap exists } data, ok := kasConfigMap.Data["config.json"] if !ok || data == "" { @@ -140,7 +140,7 @@ func verifyInflightConfig(ctx context.Context, client crclient.Client, cpNamespa } kasConfig := &kcpv1.KubeAPIServerConfig{} if err := json.Unmarshal([]byte(data), &kasConfig); err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until config is parseable } if !inflightArgMatches(kasConfig, "max-requests-inflight", effects.MaximumRequestsInflight) { return false, nil @@ -185,7 +185,7 @@ func verifyResourceRequests(ctx context.Context, client crclient.Client, cpNames for _, effect := range effects.ResourceRequests { containers, err := getContainersForEffect(pctx, client, cpNamespace, effect) if err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until containers are available } if !containerResourcesMatch(containers, effect) { return false, nil diff --git a/test/e2e/util/reqserving/verifypods.go b/test/e2e/util/reqserving/verifypods.go index 98cfd4d61ae..f128b01fc48 100644 --- a/test/e2e/util/reqserving/verifypods.go +++ b/test/e2e/util/reqserving/verifypods.go @@ -103,7 +103,7 @@ func VerifyRequestServingPodDistribution(ctx context.Context, hc *hyperv1.Hosted for depName, expectedReplicas := range expectedDeployments { dep := &appsv1.Deployment{} if err := client.Get(pctx, crclient.ObjectKey{Namespace: cpNamespace, Name: depName}, dep); err != nil { - errs = append(errs, fmt.Errorf("failed to get deployment %s: %v", depName, err)) + errs = append(errs, fmt.Errorf("failed to get deployment %s: %w", depName, err)) continue } if dep.Spec.Replicas == nil || *dep.Spec.Replicas != int32(expectedReplicas) { diff --git a/test/e2e/util/reqserving/vpa.go b/test/e2e/util/reqserving/vpa.go index c2b41483c7d..8e09ea9fd16 100644 --- a/test/e2e/util/reqserving/vpa.go +++ b/test/e2e/util/reqserving/vpa.go @@ -177,8 +177,7 @@ func waitForVPAResource(ctx context.Context) error { // Use targeted discovery instead of full server discovery for better performance _, err := disc.ServerResourcesForGroupVersion(vpaautoscalingv1.SchemeGroupVersion.String()) if err != nil { - // VPA API group not available yet - return false, nil + return false, nil //nolint:nilerr // VPA API group not available yet, retry } return true, nil }) diff --git a/test/e2e/util/reqserving/waitfor.go b/test/e2e/util/reqserving/waitfor.go index 354773f877b..e49f3d72d89 100644 --- a/test/e2e/util/reqserving/waitfor.go +++ b/test/e2e/util/reqserving/waitfor.go @@ -76,7 +76,7 @@ func WaitForControlPlaneWorkloadsReady(ctx context.Context, hc *hyperv1.HostedCl defer cancel() err = wait.PollUntilContextCancel(statefulSetCtx, DefaultPollingInterval, true, func(ctx context.Context) (bool, error) { if err := client.List(ctx, statefulSets, crclient.InNamespace(cpNamespace)); err != nil { - return false, nil + return false, nil //nolint:nilerr // retry until statefulsets are listable } if len(statefulSets.Items) == 0 { return false, nil diff --git a/test/e2e/util/sharedoidc.go b/test/e2e/util/sharedoidc.go index db91d01261b..845a724002b 100644 --- a/test/e2e/util/sharedoidc.go +++ b/test/e2e/util/sharedoidc.go @@ -106,7 +106,7 @@ func SetupSharedOIDCProvider(opts *Options, artifactDir string) error { createLogFile := filepath.Join(artifactDir, "create-oidc-provider.log") createLog, err := os.Create(createLogFile) if err != nil { - return fmt.Errorf("failed to create create log: %w", err) + return fmt.Errorf("failed to create log: %w", err) } createLogger := zap.New(zapcore.NewCore(zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), zapcore.Lock(createLog), zap.DebugLevel)) defer func() { diff --git a/test/e2e/util/util.go b/test/e2e/util/util.go index 47e9b812fb1..418f922f55a 100644 --- a/test/e2e/util/util.go +++ b/test/e2e/util/util.go @@ -7,6 +7,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/json" + "errors" "fmt" "io" "net" @@ -233,7 +234,7 @@ func DeleteNamespace(t *testing.T, ctx context.Context, client crclient.Client, return false, nil }) if err != nil { - return fmt.Errorf("namespace still exists after deletion timeout: %v", err) + return fmt.Errorf("namespace still exists after deletion timeout: %w", err) } if os.Getenv("EVENTUALLY_VERBOSE") != "false" { t.Logf("Deleted namespace %s", namespace) @@ -397,7 +398,7 @@ func GetGuestKubeconfigHost(t *testing.T, ctx context.Context, client crclient.C guestKubeConfigSecretData := WaitForGuestKubeConfig(t, ctx, client, hostedCluster) guestConfig, err := clientcmd.RESTConfigFromKubeConfig(guestKubeConfigSecretData) if err != nil { - return "", fmt.Errorf("couldn't load guest kubeconfig: %v", err) + return "", fmt.Errorf("couldn't load guest kubeconfig: %w", err) } host := guestConfig.Host @@ -419,7 +420,7 @@ func WaitForGuestKubeconfigHostUpdate(t *testing.T, ctx context.Context, client newHost, getHostError = GetGuestKubeconfigHost(t, ctx, client, hostedCluster) if getHostError != nil { t.Logf("failed to get guest kubeconfig host: %v", getHostError) - return false, nil + return false, nil //nolint:nilerr // retry until kubeconfig host is available } if newHost == oldHost { t.Logf("guest kubeconfig host is not yet updated, keep polling") @@ -441,12 +442,16 @@ func WaitForGuestKubeconfigHostResolutionUpdate(t *testing.T, ctx context.Contex err := wait.PollUntilContextTimeout(ctx, 15*time.Second, 30*time.Minute, true, func(ctx context.Context) (done bool, err error) { host := strings.TrimPrefix(uri, "https://") host = strings.Split(host, ":")[0] - ips, err := net.LookupIP(host) + ipAddrs, err := (&net.Resolver{}).LookupIPAddr(ctx, host) if err != nil { t.Logf("failed to resolve guest kubeconfig host: %v", err) return false, nil } - ip := ips[0].String() + if len(ipAddrs) == 0 { + t.Logf("guest kubeconfig host resolved with no IPs yet") + return false, nil + } + ip := ipAddrs[0].IP.String() if endpointAccess == hyperv1.Private { if strings.HasPrefix(ip, "10.") { t.Logf("kubeconfig host now resolves to private address") @@ -1608,7 +1613,7 @@ func GetMetricsFromPod(ctx context.Context, c crclient.Client, componentName, co command := []string{"curl", "-s", fmt.Sprintf("http://127.0.0.1:%s/metrics", port)} cmdOutput, err := RunCommandInPod(ctx, c, componentName, namespaceName, command, containerName, 5*time.Minute) if err != nil { - return nil, fmt.Errorf("couldn't obtain any metrics: %v", err) + return nil, fmt.Errorf("couldn't obtain any metrics: %w", err) } if len(cmdOutput) == 0 { return nil, fmt.Errorf("no metrics found") @@ -2247,7 +2252,7 @@ func createAdditionalPullSecret(ctx context.Context, guestClient crclient.Client } if err := guestClient.Create(ctx, secret); err != nil && !apierrors.IsAlreadyExists(err) { - return fmt.Errorf("failed to create secret: %v", err) + return fmt.Errorf("failed to create secret: %w", err) } return nil @@ -2392,9 +2397,9 @@ func EnsureKubeAPIDNSNameCustomCert(t *testing.T, ctx context.Context, mgmtClien start := time.Now() g.Eventually(func() error { t.Logf("[%s] Waiting until the URL is resolvable: %s", time.Now().Format(time.RFC3339), customApiServerHost) - _, err := net.LookupIP(customApiServerHost) + _, err := (&net.Resolver{}).LookupIPAddr(ctx, customApiServerHost) if err != nil { - return fmt.Errorf("failed to resolve the custom DNS name: %v", err) + return fmt.Errorf("failed to resolve the custom DNS name: %w", err) } t.Logf("resolved the custom DNS name after %s\n", time.Since(start)) return nil @@ -2567,7 +2572,7 @@ func EnsureKubeAPIDNSNameCustomCert(t *testing.T, ctx context.Context, mgmtClien err = retry.RetryOnConflict(retry.DefaultRetry, func() error { latestHC := &hyperv1.HostedCluster{} if err := mgmtClient.Get(ctx, crclient.ObjectKeyFromObject(hc), latestHC); err != nil { - return fmt.Errorf("failed to get latest HostedCluster: %v", err) + return fmt.Errorf("failed to get latest HostedCluster: %w", err) } latestHC.Spec.Configuration.APIServer.ServingCerts.NamedCertificates = []configv1.APIServerNamedServingCert{} return mgmtClient.Update(ctx, latestHC) @@ -2874,7 +2879,7 @@ func getIngressRouterDefaultIP(t *testing.T, ctx context.Context, client crclien } return getErr == nil, err }); err != nil { - return "", fmt.Errorf("router-default service did't become available: %v", err) + return "", fmt.Errorf("router-default service did't become available: %w", err) } routerDefaultIP := defaultIngressRouterService.Status.LoadBalancer.Ingress[0].IP @@ -4514,7 +4519,7 @@ func ApplyYAMLBytes(ctx context.Context, c crclient.Client, yamlContent []byte, for { obj := &unstructured.Unstructured{} if err := decoder.Decode(obj); err != nil { - if err == io.EOF { + if errors.Is(err, io.EOF) { return nil } return fmt.Errorf("failed to decode YAML: %w", err) @@ -4638,7 +4643,7 @@ func EnsureNodeTuningOperatorMetricsEndpoint(t *testing.T, ctx context.Context, } cmdOutput, err := RunCommandInPod(ctx, mgmtClient, "cluster-node-tuning-operator", hcpNamespace, httpsCommand, "cluster-node-tuning-operator", 30*time.Second) if err != nil { - return fmt.Errorf("failed to get metrics via ServiceMonitor HTTPS at %s: %v", httpsServiceURL, err) + return fmt.Errorf("failed to get metrics via ServiceMonitor HTTPS at %s: %w", httpsServiceURL, err) } if len(cmdOutput) == 0 { return fmt.Errorf("no metrics returned via ServiceMonitor HTTPS at %s", httpsServiceURL) diff --git a/test/e2e/util/version.go b/test/e2e/util/version.go index 7a5d677b0af..03846613990 100644 --- a/test/e2e/util/version.go +++ b/test/e2e/util/version.go @@ -50,16 +50,16 @@ func init() { func SetReleaseImageVersion(ctx context.Context, latestReleaseImage string, pullSecretFile string) error { data, err := os.ReadFile(pullSecretFile) if err != nil { - return fmt.Errorf("error reading file: %v", err) + return fmt.Errorf("error reading file: %w", err) } releaseInfoProvider := releaseinfo.RegistryClientProvider{} releaseImage, err := releaseInfoProvider.Lookup(ctx, latestReleaseImage, data) if err != nil { - return fmt.Errorf("error looking up latest release image: %v", err) + return fmt.Errorf("error looking up latest release image: %w", err) } releaseVersion, err = semver.Parse(releaseImage.Version()) if err != nil { - return fmt.Errorf("error parsing version: %v", err) + return fmt.Errorf("error parsing version: %w", err) } releaseVersion.Patch = 0 releaseVersion.Pre = nil @@ -76,7 +76,7 @@ func SetReleaseVersionFromHostedCluster(ctx context.Context, hostedCluster *hype var err error releaseVersion, err = semver.Parse(hcVersion) if err != nil { - return fmt.Errorf("error parsing version: %v", err) + return fmt.Errorf("error parsing version: %w", err) } releaseVersion.Patch = 0 releaseVersion.Pre = nil diff --git a/test/integration/framework/hosted-cluster.go b/test/integration/framework/hosted-cluster.go index d6b50d153eb..f9da27a5ec5 100644 --- a/test/integration/framework/hosted-cluster.go +++ b/test/integration/framework/hosted-cluster.go @@ -106,7 +106,12 @@ func InstallHostedCluster(ctx context.Context, logger logr.Logger, opts *Options pullSpec := opts.ReleaseImage if pullSpec == "" { - resp, err := http.Get(fmt.Sprintf(`https://multi.ocp.releases.ci.openshift.org/api/v1/releasestream/%s-0.nightly-multi/latest`, supportedversion.LatestSupportedVersion.String())) + releaseURL := fmt.Sprintf(`https://multi.ocp.releases.ci.openshift.org/api/v1/releasestream/%s-0.nightly-multi/latest`, supportedversion.LatestSupportedVersion.String()) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, releaseURL, nil) + if err != nil { + return CleanupSentinel, fmt.Errorf("couldn't create release image request: %w", err) + } + resp, err := http.DefaultClient.Do(req) if err != nil { return CleanupSentinel, fmt.Errorf("couldn't fetch latest release image: %w", err) } @@ -175,7 +180,7 @@ func InstallHostedCluster(ctx context.Context, logger logr.Logger, opts *Options if SkippedCleanupSteps().HasAny("all", "hosted-clusters") { return nil } - logger.Info("dumping hosted hosted cluster assets") + logger.Info("dumping hosted cluster assets") dumpLogPath := filepath.Join("install", "assets.dump.yaml") dumpCmd := exec.CommandContext(ctx, opts.OCPath, "get", "--ignore-not-found", "--show-managed-fields", "-f", filepath.Join(opts.ArtifactDir, yamlPath), "--kubeconfig", opts.Kubeconfig,