From 7d4d6dbac79ddda10fb71c02a7444b52ce0bff32 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Fri, 8 May 2026 15:26:50 -0400 Subject: [PATCH 01/14] fix(e2e): fix AllowedCIDRs test for Route-based KAS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ValidateKubeAPIServerAllowedCIDRs test fails on v2 Azure self-managed clusters because KAS uses Route publishing strategy (via external-dns-domain), not LoadBalancer. Two fixes: 1. Wait for the downstream LB service (router or KAS LB) to have its LoadBalancerSourceRanges updated by the CPO before asserting KAS reachability. The target service is determined by the HC's APIServer publishing strategy. 2. Create a fresh kubeclient per poll iteration to prevent HTTP/2 connection reuse. Go's HTTP/2 transport multiplexes all requests over a single persistent TCP connection — if a prior request succeeded before Azure NSG rules took effect, subsequent requests bypass the restriction on the same connection. Co-Authored-By: Claude Opus 4.6 --- test/e2e/create_cluster_test.go | 5 +- test/e2e/util/azure.go | 56 +++++++-------- test/e2e/util/util.go | 85 +++++++++++++++++++--- test/e2e/util/util_test.go | 120 ++++++++++++++++++++++++++++++++ 4 files changed, 228 insertions(+), 38 deletions(-) diff --git a/test/e2e/create_cluster_test.go b/test/e2e/create_cluster_test.go index 81b1124d1e21..b74804b0afd5 100644 --- a/test/e2e/create_cluster_test.go +++ b/test/e2e/create_cluster_test.go @@ -116,8 +116,11 @@ func TestCreateCluster(t *testing.T) { e2eutil.EnsureDefaultSecurityGroupTags(t, ctx, mgtClient, hostedCluster, clusterOpts) if globalOpts.Platform == hyperv1.AzurePlatform { - e2eutil.EnsureKubeAPIServerAllowedCIDRs(t, ctx, mgtClient, guestConfig, hostedCluster) + // WI webhook must run before AllowedCIDRs. AllowedCIDRs blocks and restores + // all KAS traffic; the webhook sidecar (FailurePolicy: Ignore) may not be + // ready during recovery, silently skipping mutation on pods created in that window. e2eutil.EnsureAzureWorkloadIdentityWebhookMutation(t, ctx, guestClient) + e2eutil.EnsureKubeAPIServerAllowedCIDRs(t, ctx, mgtClient, guestConfig, hostedCluster) } e2eutil.EnsureGlobalPullSecret(t, ctx, mgtClient, hostedCluster, globalOpts.AdditionalPullSecretFile) diff --git a/test/e2e/util/azure.go b/test/e2e/util/azure.go index bcd55516bf7e..8397691fc50e 100644 --- a/test/e2e/util/azure.go +++ b/test/e2e/util/azure.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -38,7 +39,7 @@ func ValidateAzureWorkloadIdentityWebhookMutation(t testing.TB, ctx context.Cont } g.Expect(guestClient.Create(ctx, serviceAccount)).To(Succeed(), "failed to create test service account") - pod := &corev1.Pod{ + podTemplate := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "azure-wi-webhook-test-pod", Namespace: nsName, @@ -70,34 +71,33 @@ func ValidateAzureWorkloadIdentityWebhookMutation(t testing.TB, ctx context.Cont RestartPolicy: corev1.RestartPolicyNever, }, } - g.Expect(guestClient.Create(ctx, pod)).To(Succeed(), "failed to create pod for webhook mutation test") - EventuallyObject( - t, - ctx, - "Azure workload identity webhook to mutate test pod", - func(ctx context.Context) (*corev1.Pod, error) { - mutatedPod := &corev1.Pod{} - err := guestClient.Get(ctx, types.NamespacedName{Name: pod.Name, Namespace: pod.Namespace}, mutatedPod) - return mutatedPod, err - }, - []Predicate[*corev1.Pod]{ - func(mutatedPod *corev1.Pod) (bool, string, error) { - if hasProjectedTokenVolume(mutatedPod.Spec.Volumes) { - return true, "", nil - } - return false, "expected projected service account token volume to be injected", nil - }, - func(mutatedPod *corev1.Pod) (bool, string, error) { - if hasAzureFederatedTokenEnv(mutatedPod.Spec.Containers) { - return true, "", nil - } - return false, "expected AZURE_FEDERATED_TOKEN_FILE env var in pod containers", nil - }, - }, - WithTimeout(3*time.Minute), - WithInterval(5*time.Second), - ) + // The WI webhook is a MutatingAdmissionWebhook with FailurePolicy: Ignore + // that only fires on pod CREATE. If the webhook sidecar isn't ready when + // the pod is created, the pod is admitted without mutation and no amount + // of GET polling can recover. Delete and recreate each iteration to + // re-trigger admission until the webhook is ready. + g.Eventually(func(g Gomega) { + existing := &corev1.Pod{} + err := guestClient.Get(ctx, types.NamespacedName{Name: podTemplate.Name, Namespace: podTemplate.Namespace}, existing) + if err == nil { + g.Expect(guestClient.Delete(ctx, existing)).To(Succeed(), "failed to delete pod for retry") + g.Eventually(func() bool { + err := guestClient.Get(ctx, types.NamespacedName{Name: podTemplate.Name, Namespace: podTemplate.Namespace}, &corev1.Pod{}) + return apierrors.IsNotFound(err) + }).WithContext(ctx).WithTimeout(30*time.Second).WithPolling(time.Second).Should(BeTrue(), "pod should be deleted before retry") + } else { + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "unexpected error getting existing pod: %v", err) + } + + fresh := podTemplate.DeepCopy() + g.Expect(guestClient.Create(ctx, fresh)).To(Succeed(), "failed to create pod for webhook mutation test") + + mutated := &corev1.Pod{} + g.Expect(guestClient.Get(ctx, types.NamespacedName{Name: fresh.Name, Namespace: fresh.Namespace}, mutated)).To(Succeed()) + g.Expect(hasProjectedTokenVolume(mutated.Spec.Volumes)).To(BeTrue(), "expected projected service account token volume to be injected") + g.Expect(hasAzureFederatedTokenEnv(mutated.Spec.Containers)).To(BeTrue(), "expected AZURE_FEDERATED_TOKEN_FILE env var in pod containers") + }).WithContext(ctx).WithTimeout(3 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) } func EnsureAzureWorkloadIdentityWebhookMutation(t *testing.T, ctx context.Context, guestClient crclient.Client) { diff --git a/test/e2e/util/util.go b/test/e2e/util/util.go index dcbf6ed7e55b..5085c92ceb00 100644 --- a/test/e2e/util/util.go +++ b/test/e2e/util/util.go @@ -3602,17 +3602,23 @@ func ValidateKubeAPIServerAllowedCIDRs(t testing.TB, ctx context.Context, mgmtCl } }) g.Expect(err).NotTo(HaveOccurred(), "failed to restore HostedCluster API server CIDRs") - }() - kubeClient, err := kubeclient.NewForConfig(guestConfig) - g.Expect(err).NotTo(HaveOccurred()) + // Verify KAS is reachable on the original transport before returning. The + // AllowedCIDRs test uses cfg.Dial to create isolated transports, but subsequent + // tests share the original guestConfig's transport. Without this wait, the next + // test may start before Azure LB propagation completes the CIDR restoration. + g.Eventually(func(g Gomega) { + client, err := kubeclient.NewForConfig(guestConfig) + g.Expect(err).ToNot(HaveOccurred(), "failed to create kubeclient for transport recovery") + _, err = client.ServerVersion() + g.Expect(err).ToNot(HaveOccurred(), "KAS should be reachable on original transport after CIDR cleanup") + }).WithContext(ctx).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) + }() // ensure that kube-apiserver is not reachable from anywhere - ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, kubeClient, hc, []string{"0.0.0.0/32"}, false) + ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, guestConfig, hc, []string{"0.0.0.0/32"}, false) // ensure kube-apiserver is reachable when allowed CIDRs allow access from everywhere - // This is useful for testing purposes, as it allows us to access the kube-apiserver from any IP - // In a production environment, this should be restricted to specific CIDRs - ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, kubeClient, hc, append([]string{"0.0.0.0/0"}, generateTestCIDRs250()...), true) + ensureAPIServerAllowedCIDRs(ctx, t, g, mgmtClient, guestConfig, hc, append([]string{"0.0.0.0/0"}, generateTestCIDRs250()...), true) } func EnsureKubeAPIServerAllowedCIDRs(t *testing.T, ctx context.Context, mgmtClient crclient.Client, guestConfig *rest.Config, hc *hyperv1.HostedCluster) { @@ -3621,7 +3627,7 @@ func EnsureKubeAPIServerAllowedCIDRs(t *testing.T, ctx context.Context, mgmtClie }) } -func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mgmtClient crclient.Client, guestClient *kubeclient.Clientset, hc *hyperv1.HostedCluster, allowedCIDRs []string, shouldBeReachable bool) { +func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mgmtClient crclient.Client, guestConfig *rest.Config, hc *hyperv1.HostedCluster, allowedCIDRs []string, shouldBeReachable bool) { expectedCIDRs := make([]hyperv1.CIDRBlock, len(allowedCIDRs)) for i, cidr := range allowedCIDRs { expectedCIDRs[i] = hyperv1.CIDRBlock(cidr) @@ -3660,8 +3666,40 @@ func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mg "HCP AllowedCIDRBlocks should match the HostedCluster spec") }).WithContext(ctx).WithTimeout(time.Minute * 3).WithPolling(time.Second * 5).Should(Succeed()) + // Wait for the CPO to reconcile the downstream service with the expected LoadBalancerSourceRanges. + // The target service depends on the APIServer publishing strategy: + // - Route: the "router" LB service carries the CIDRs + // - LoadBalancer: the KAS LB service itself carries the CIDRs + targetSvc := allowedCIDRsTargetService(hc, hcpNamespace) + if targetSvc != nil { + expectedSourceRanges := slices.Clone(allowedCIDRs) + slices.Sort(expectedSourceRanges) + t.Logf("Waiting for service %s/%s LoadBalancerSourceRanges to match %d CIDRs", targetSvc.Namespace, targetSvc.Name, len(expectedSourceRanges)) + g.Eventually(func(g Gomega) { + svc := &corev1.Service{} + err := mgmtClient.Get(ctx, crclient.ObjectKeyFromObject(targetSvc), svc) + g.Expect(err).ToNot(HaveOccurred(), "failed to get service %s/%s", targetSvc.Namespace, targetSvc.Name) + actualSourceRanges := slices.Clone(svc.Spec.LoadBalancerSourceRanges) + slices.Sort(actualSourceRanges) + g.Expect(actualSourceRanges).To(Equal(expectedSourceRanges), + "service %s/%s LoadBalancerSourceRanges should match expected CIDRs", targetSvc.Namespace, targetSvc.Name) + }).WithContext(ctx).WithTimeout(time.Minute * 3).WithPolling(time.Second * 5).Should(Succeed()) + } else { + t.Log("No downstream LB service identified for this cluster configuration; skipping LoadBalancerSourceRanges wait") + } + + // A fresh kubeclient is created on every poll iteration because Go's HTTP/2 transport + // keeps a single TCP connection open and multiplexes all requests over it. If a poll + // succeeds before the LB source-range block takes effect, every later poll reuses that + // same connection and never sees the block. Setting cfg.Dial to a new net.Dialer gives + // each config a unique pointer in client-go's TLS transport cache, which forces a brand + // new TCP connection per iteration so each poll independently tests reachability. g.Eventually(func(g Gomega) { - _, err = guestClient.ServerVersion() + cfg := rest.CopyConfig(guestConfig) + cfg.Dial = (&net.Dialer{Timeout: 30 * time.Second, KeepAlive: 30 * time.Second}).DialContext + freshClient, err := kubeclient.NewForConfig(cfg) + g.Expect(err).ToNot(HaveOccurred(), "failed to create kubeclient") + _, err = freshClient.ServerVersion() if shouldBeReachable { g.Expect(err).ToNot(HaveOccurred(), "kube-apiserver should be reachable") } else { @@ -3670,6 +3708,35 @@ func ensureAPIServerAllowedCIDRs(ctx context.Context, t testing.TB, g Gomega, mg }).WithContext(ctx).WithTimeout(time.Minute * 3).WithPolling(time.Second * 5).Should(Succeed()) } +// allowedCIDRsTargetService returns the LoadBalancer service that enforces AllowedCIDRBlocks +// based on the HostedCluster's APIServer publishing strategy. Returns nil when no LB service +// carries source ranges (private clusters, NodePort, ARO HCP). +// Mirrors CPO's API server and router service reconciliation logic. +func allowedCIDRsTargetService(hc *hyperv1.HostedCluster, hcpNamespace string) *corev1.Service { + if !netutil.IsPublicHC(hc) { + return nil + } + strategy := netutil.ServicePublishingStrategyByTypeByHC(hc, hyperv1.APIServer) + if strategy == nil { + return nil + } + switch strategy.Type { + case hyperv1.Route: + if azureutil.IsAroHCP() { + return nil + } + return cpomanifests.RouterPublicService(hcpNamespace) + case hyperv1.LoadBalancer: + if hc.Spec.Platform.Type == hyperv1.AzurePlatform || + (hc.Annotations != nil && hc.Annotations[hyperv1.ManagementPlatformAnnotation] == string(hyperv1.AzurePlatform)) { + return cpomanifests.KubeAPIServerServiceAzureLB(hcpNamespace) + } + return cpomanifests.KubeAPIServerService(hcpNamespace) + default: + return nil + } +} + // generateTestCIDRs250 is a helper to generate 250 /32 CIDRs starting at 250.250.250.1 func generateTestCIDRs250() []string { cidrs := make([]string, 0, 250) diff --git a/test/e2e/util/util_test.go b/test/e2e/util/util_test.go index 1c5761ae3ec6..53ad960232f4 100644 --- a/test/e2e/util/util_test.go +++ b/test/e2e/util/util_test.go @@ -7,9 +7,129 @@ import ( . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/azureutil" "github.com/openshift/hypershift/support/certs" + + "k8s.io/utils/ptr" ) +func TestAllowedCIDRsTargetService(t *testing.T) { + const ns = "test-hcp" + + publicHC := func(platform hyperv1.PlatformType, svcType hyperv1.PublishingStrategyType) *hyperv1.HostedCluster { + hc := &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{Type: platform}, + Services: []hyperv1.ServicePublishingStrategyMapping{{ + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: svcType}, + }}, + }, + } + switch platform { + case hyperv1.AWSPlatform: + hc.Spec.Platform.AWS = ptr.To(hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Public}) + case hyperv1.AzurePlatform: + hc.Spec.Platform.Azure = ptr.To(hyperv1.AzurePlatformSpec{Topology: hyperv1.AzureTopologyPublic}) + } + return hc + } + + tests := []struct { + name string + hc *hyperv1.HostedCluster + aroHCP bool + wantName string + wantNil bool + }{ + { + name: "When Route strategy on AWS it should return the router service", + hc: publicHC(hyperv1.AWSPlatform, hyperv1.Route), + wantName: "router", + }, + { + name: "When Route strategy on Azure self-managed it should return the router service", + hc: publicHC(hyperv1.AzurePlatform, hyperv1.Route), + wantName: "router", + }, + { + name: "When Route strategy on ARO HCP it should return nil", + hc: publicHC(hyperv1.AzurePlatform, hyperv1.Route), + aroHCP: true, + wantNil: true, + }, + { + name: "When LoadBalancer strategy on Azure it should return the Azure LB service", + hc: publicHC(hyperv1.AzurePlatform, hyperv1.LoadBalancer), + wantName: "kube-apiserverlb", + }, + { + name: "When LoadBalancer strategy with Azure management annotation it should return the Azure LB service", + hc: func() *hyperv1.HostedCluster { + hc := publicHC(hyperv1.NonePlatform, hyperv1.LoadBalancer) + hc.Annotations = map[string]string{ + hyperv1.ManagementPlatformAnnotation: string(hyperv1.AzurePlatform), + } + return hc + }(), + wantName: "kube-apiserverlb", + }, + { + name: "When LoadBalancer strategy on AWS it should return the KAS service", + hc: publicHC(hyperv1.AWSPlatform, hyperv1.LoadBalancer), + wantName: "kube-apiserver", + }, + { + name: "When private Azure cluster it should return nil", + hc: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AzurePlatform, + Azure: ptr.To(hyperv1.AzurePlatformSpec{Topology: hyperv1.AzureTopologyPrivate}), + }, + Services: []hyperv1.ServicePublishingStrategyMapping{{ + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.Route}, + }}, + }, + }, + wantNil: true, + }, + { + name: "When NodePort strategy it should return nil", + hc: publicHC(hyperv1.AWSPlatform, hyperv1.NodePort), + wantNil: true, + }, + { + name: "When no APIServer strategy it should return nil", + hc: func() *hyperv1.HostedCluster { + hc := publicHC(hyperv1.AWSPlatform, hyperv1.Route) + hc.Spec.Services = nil + return hc + }(), + wantNil: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + if tc.aroHCP { + azureutil.SetAsAroHCPTest(t) + } + svc := allowedCIDRsTargetService(tc.hc, ns) + if tc.wantNil { + g.Expect(svc).To(BeNil()) + } else { + g.Expect(svc).ToNot(BeNil()) + g.Expect(svc.Name).To(Equal(tc.wantName)) + g.Expect(svc.Namespace).To(Equal(ns)) + } + }) + } +} + // TestGenerateCustomCertificate verifies that our certificate generation works correctly func TestGenerateCustomCertificate(t *testing.T) { testsCases := []struct { From bf121ad0345e23b1d456fe64f75cbbc38723105b Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 13 May 2026 11:12:59 -0400 Subject: [PATCH 02/14] fix(cli): validate externalDNSDomain does not shadow cluster apps domain Add validateExternalDNSDomain() that detects when the external DNS domain would shadow *.apps.. resolution, causing TLS certificate mismatches via the Two Routers Problem. The check runs during `hypershift create cluster azure` validation. Signed-off-by: Bryan Cox Commit-Message-Assisted-by: Claude (via Claude Code) --- cmd/cluster/azure/create.go | 38 +++++++++++++- cmd/cluster/azure/create_test.go | 85 +++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 3 deletions(-) diff --git a/cmd/cluster/azure/create.go b/cmd/cluster/azure/create.go index 75fb377606f9..6f06ee738383 100644 --- a/cmd/cluster/azure/create.go +++ b/cmd/cluster/azure/create.go @@ -161,7 +161,7 @@ func BindProductCoreFlags(opts *core.RawCreateOptions, flags *pflag.FlagSet) { } // Validate validates the Azure create cluster command options -func (o *RawCreateOptions) Validate(ctx context.Context, _ *core.CreateOptions) (core.PlatformCompleter, error) { +func (o *RawCreateOptions) Validate(ctx context.Context, opts *core.CreateOptions) (core.PlatformCompleter, error) { var err error // Check if the network security group is set and the resource group is not @@ -222,6 +222,12 @@ func (o *RawCreateOptions) Validate(ctx context.Context, _ *core.CreateOptions) } } + if opts != nil { + if err := validateExternalDNSDomain(opts.ExternalDNSDomain, opts.Name, opts.BaseDomain); err != nil { + return nil, err + } + } + validOpts := &ValidatedCreateOptions{ validatedCreateOptions: &validatedCreateOptions{ RawCreateOptions: o, @@ -252,6 +258,36 @@ func (o *RawCreateOptions) Validate(ctx context.Context, _ *core.CreateOptions) return validOpts, nil } +// validateExternalDNSDomain checks that the external DNS domain does not conflict with the cluster +// domain. When a private Azure HostedCluster uses an externalDNSDomain that matches or is a parent +// of the cluster domain (clusterName.baseDomain), the PLS controller creates an Azure Private DNS +// zone that shadows *.apps DNS resolution. +func validateExternalDNSDomain(externalDNSDomain, clusterName, baseDomain string) error { + if externalDNSDomain == "" { + return nil + } + + if clusterName == "" || baseDomain == "" { + return nil + } + + clusterDomain := clusterName + "." + baseDomain + + extLower := strings.ToLower(strings.TrimSuffix(externalDNSDomain, ".")) + clusterLower := strings.ToLower(strings.TrimSuffix(clusterDomain, ".")) + + // Check if the externalDNSDomain matches or is a parent of the cluster domain. + // An exact match means the Private DNS zone would directly shadow *.apps. + // A suffix match (with dot boundary) means the zone is a parent that would also shadow. + if extLower == clusterLower || strings.HasSuffix(clusterLower, "."+extLower) { + return fmt.Errorf("external DNS domain %q conflicts with cluster domain %q: "+ + "this would create an Azure Private DNS zone that shadows *.apps DNS resolution. "+ + "Use a different --external-dns-domain value", externalDNSDomain, clusterDomain) + } + + return nil +} + // Complete completes the Azure create cluster command options func (o *ValidatedCreateOptions) Complete(ctx context.Context, opts *core.CreateOptions) (core.Platform, error) { output := &CreateOptions{ diff --git a/cmd/cluster/azure/create_test.go b/cmd/cluster/azure/create_test.go index 4f73b8d7646b..bd62093d3192 100644 --- a/cmd/cluster/azure/create_test.go +++ b/cmd/cluster/azure/create_test.go @@ -65,7 +65,7 @@ func TestValidateEndpointAccess(t *testing.T) { opts.EndpointAccessPrivateNATSubnetID = test.endpointAccessPrivateNATSubnetID opts.EndpointAccessPrivateAdditionalAllowedSubscriptions = test.endpointAccessPrivateAdditionalAllowedSubscriptions - _, err := opts.Validate(context.Background(), &core.CreateOptions{}) + _, err := opts.Validate(context.Background(), nil) if test.expectError { if err == nil { t.Fatalf("expected error but got nil") @@ -335,6 +335,87 @@ func TestCreateCluster(t *testing.T) { } } +func TestValidateExternalDNSDomain(t *testing.T) { + t.Parallel() + tests := map[string]struct { + externalDNSDomain string + name string + baseDomain string + expectError bool + }{ + "When externalDNSDomain matches cluster domain, it should return an error": { + externalDNSDomain: "test-cluster.example.com", + name: "test-cluster", + baseDomain: "example.com", + expectError: true, + }, + "When externalDNSDomain is parent of cluster domain, it should return an error": { + externalDNSDomain: "example.com", + name: "test-cluster", + baseDomain: "example.com", + expectError: true, + }, + "When externalDNSDomain differs from cluster domain, it should return nil": { + externalDNSDomain: "external.different.com", + name: "test-cluster", + baseDomain: "example.com", + expectError: false, + }, + "When externalDNSDomain is empty, it should return nil": { + externalDNSDomain: "", + name: "test-cluster", + baseDomain: "example.com", + expectError: false, + }, + "When externalDNSDomain matches cluster domain case-insensitively, it should return an error": { + externalDNSDomain: "Test-Cluster.Example.COM", + name: "test-cluster", + baseDomain: "example.com", + expectError: true, + }, + "When cluster name is empty, it should return nil": { + externalDNSDomain: "test-cluster.example.com", + name: "", + baseDomain: "example.com", + expectError: false, + }, + "When base domain is empty, it should return nil": { + externalDNSDomain: "test-cluster.example.com", + name: "test-cluster", + baseDomain: "", + expectError: false, + }, + "When externalDNSDomain shares a suffix but not on dot boundary, it should return nil": { + externalDNSDomain: "ample.com", + name: "test-cluster", + baseDomain: "example.com", + expectError: false, + }, + "When externalDNSDomain has trailing dot, it should still detect shadowing": { + externalDNSDomain: "test-cluster.example.com.", + name: "test-cluster", + baseDomain: "example.com", + expectError: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + + err := validateExternalDNSDomain(test.externalDNSDomain, test.name, test.baseDomain) + if test.expectError { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("conflicts with cluster domain")) + g.Expect(err.Error()).To(ContainSubstring("shadows *.apps DNS resolution")) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + func TestValidateOAuthPublishingStrategy(t *testing.T) { tests := map[string]struct { oauthPublishingStrategy string @@ -375,7 +456,7 @@ func TestValidateOAuthPublishingStrategy(t *testing.T) { opts.ManagedIdentitiesFile = test.managedIdentitiesFile opts.DataPlaneIdentitiesFile = test.dataPlaneIdentitiesFile - _, err := opts.Validate(context.Background(), &core.CreateOptions{}) + _, err := opts.Validate(context.Background(), nil) if test.expectError { g.Expect(err).To(HaveOccurred()) g.Expect(err).To(MatchError(test.expectedErrorMsg)) From be26321400bb176f293add695f187ff202d6dca0 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 13 May 2026 11:13:06 -0400 Subject: [PATCH 03/14] fix(hypershift-operator): reject Azure HostedClusters with shadowing service hostnames Add webhook validation that rejects HostedCluster creation when any service hostname would shadow the cluster apps domain (*.apps.. ). This prevents the Two Routers Problem where the PE IP routes to private-router (HAProxy) instead of router-default, causing TLS cert mismatches for apps traffic. Signed-off-by: Bryan Cox Commit-Message-Assisted-by: Claude (via Claude Code) --- api/hypershift/v1beta1/hostedcluster_types.go | 1 + .../AAA_ungated.yaml | 11 + .../ClusterUpdateAcceptRisks.yaml | 11 + .../ClusterVersionOperatorConfiguration.yaml | 11 + .../ExternalOIDC.yaml | 11 + ...ernalOIDCWithUIDAndExtraClaimMappings.yaml | 11 + .../ExternalOIDCWithUpstreamParity.yaml | 11 + .../GCPPlatform.yaml | 11 + .../HCPEtcdBackup.yaml | 11 + ...perShiftOnlyDynamicResourceAllocation.yaml | 11 + .../ImageStreamImportMode.yaml | 11 + .../KMSEncryptionProvider.yaml | 11 + .../OpenStack.yaml | 11 + .../TLSAdherence.yaml | 11 + ...stable.hostedclusters.azure.testsuite.yaml | 268 ++++++++++++++++++ ...usters-Hypershift-CustomNoUpgrade.crd.yaml | 11 + ...hostedclusters-Hypershift-Default.crd.yaml | 11 + ...s-Hypershift-TechPreviewNoUpgrade.crd.yaml | 11 + .../hypershift/v1beta1/hostedcluster_types.go | 1 + 19 files changed, 446 insertions(+) diff --git a/api/hypershift/v1beta1/hostedcluster_types.go b/api/hypershift/v1beta1/hostedcluster_types.go index c2d634988708..dac886efb32e 100644 --- a/api/hypershift/v1beta1/hostedcluster_types.go +++ b/api/hypershift/v1beta1/hostedcluster_types.go @@ -527,6 +527,7 @@ type Capabilities struct { // +kubebuilder:validation:XValidation:rule=`!self.services.exists(s, s.service == 'APIServer' && has(s.servicePublishingStrategy.loadBalancer) && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration) && has(self.configuration.apiServer) && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))`, message="APIServer loadBalancer hostname cannot be in ClusterConfiguration.apiserver.servingCerts.namedCertificates[]" // +kubebuilder:validation:XValidation:rule="!has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.disableMultiNetwork) || !self.operatorConfiguration.clusterNetworkOperator.disableMultiNetwork || self.networking.networkType == 'Other'",message="disableMultiNetwork can only be set to true when networkType is 'Other'" // +kubebuilder:validation:XValidation:rule="self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)", message="ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes" +// +kubebuilder:validation:XValidation:rule=`self.platform.type != "Azure" || self.dns.baseDomain == "" || !self.services.exists(s, (has(s.servicePublishingStrategy.route) && has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + 1))))`,message="Azure service hostname domain must not overlap with the cluster base domain. An Azure Private DNS zone matching or containing the base domain would shadow *.apps DNS resolution." type HostedClusterSpec struct { // release specifies the desired OCP release payload for all the hosted cluster components. // This includes those components running management side like the Kube API Server and the CVO but also the operands which land in the hosted cluster data plane like the ingress controller, ovn agents, etc. diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml index 7838ef9d8f66..db7d833cb031 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml @@ -6501,6 +6501,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml index 29186fae1769..2ea68d47b75d 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml @@ -6484,6 +6484,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml index 75a647659faa..8bb84af490ab 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml @@ -6504,6 +6504,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml index 9de23f8d67e4..f19028c01688 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml @@ -6816,6 +6816,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml index c62d6ad473fe..66b446d9abbf 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml @@ -6956,6 +6956,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml index 3e972abdcf76..dd14b56cbcfd 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml @@ -6947,6 +6947,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml index dae5c617ab62..0bb77ebd9f9c 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml @@ -6930,6 +6930,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml index d0327faa3dec..7bc6353290fa 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml @@ -6549,6 +6549,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml index 14c27c8efc19..35712f5dd8a9 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml @@ -6506,6 +6506,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml index 5dccbaf9408d..48c54bdf5359 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml @@ -6502,6 +6502,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml index 27d0b7170ef2..da1864b3e2b8 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml @@ -6560,6 +6560,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml index 99173632616b..5534e0aabb71 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml @@ -7035,6 +7035,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml index cb7aaf7b0b2d..da6a93d2134a 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml @@ -6524,6 +6524,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yaml b/cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yaml index ed9896396e96..0b35dae88ed4 100644 --- a/cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yaml +++ b/cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yaml @@ -813,3 +813,271 @@ tests: type: Route route: {} expectedError: "workloadIdentities.controlPlaneOperator is required when Private Link is configured with WorkloadIdentities authentication" + + # --- Azure DNS shadowing validation --- + - name: When Azure route hostname domain overlaps with baseDomain it should fail + initial: | + apiVersion: hypershift.openshift.io/v1beta1 + kind: HostedCluster + spec: + dns: + baseDomain: example.com + platform: + type: Azure + azure: + location: eastus + resourceGroupName: test-rg + vnetID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet" + subnetID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet" + subscriptionID: "12345678-1234-5678-9012-123456789012" + securityGroupID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/networkSecurityGroups/test-nsg" + tenantID: "87654321-4321-8765-2109-876543210987" + azureAuthenticationConfig: + azureAuthenticationConfigType: ManagedIdentities + managedIdentities: + controlPlane: + managedIdentitiesKeyVault: + name: test-kv + tenantID: "87654321-4321-8765-2109-876543210987" + cloudProvider: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: cp-secret + nodePoolManagement: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: npm-secret + controlPlaneOperator: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: cpo-secret + imageRegistry: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: ir-secret + ingress: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: ingress-secret + network: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: network-secret + disk: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: disk-secret + file: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: file-secret + dataPlane: + imageRegistryMSIClientID: "12345678-1234-5678-9012-123456789012" + diskMSIClientID: "12345678-1234-5678-9012-123456789012" + fileMSIClientID: "12345678-1234-5678-9012-123456789012" + pullSecret: + name: secret + release: + image: quay.io/openshift-release-dev/ocp-release:4.15.11-x86_64 + secretEncryption: + aescbc: + activeKey: + name: key + type: aescbc + services: + - service: APIServer + servicePublishingStrategy: + type: Route + route: + hostname: api-mycluster.example.com + - service: OAuthServer + servicePublishingStrategy: + type: Route + route: {} + - service: Konnectivity + servicePublishingStrategy: + type: Route + route: {} + - service: Ignition + servicePublishingStrategy: + type: Route + route: {} + expectedError: "Azure service hostname domain must not overlap with the cluster base domain" + + - name: When Azure loadBalancer hostname domain overlaps with baseDomain it should fail + initial: | + apiVersion: hypershift.openshift.io/v1beta1 + kind: HostedCluster + spec: + dns: + baseDomain: example.com + platform: + type: Azure + azure: + location: eastus + resourceGroupName: test-rg + vnetID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet" + subnetID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet" + subscriptionID: "12345678-1234-5678-9012-123456789012" + securityGroupID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/networkSecurityGroups/test-nsg" + tenantID: "87654321-4321-8765-2109-876543210987" + azureAuthenticationConfig: + azureAuthenticationConfigType: ManagedIdentities + managedIdentities: + controlPlane: + managedIdentitiesKeyVault: + name: test-kv + tenantID: "87654321-4321-8765-2109-876543210987" + cloudProvider: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: cp-secret + nodePoolManagement: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: npm-secret + controlPlaneOperator: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: cpo-secret + imageRegistry: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: ir-secret + ingress: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: ingress-secret + network: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: network-secret + disk: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: disk-secret + file: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: file-secret + dataPlane: + imageRegistryMSIClientID: "12345678-1234-5678-9012-123456789012" + diskMSIClientID: "12345678-1234-5678-9012-123456789012" + fileMSIClientID: "12345678-1234-5678-9012-123456789012" + pullSecret: + name: secret + release: + image: quay.io/openshift-release-dev/ocp-release:4.15.11-x86_64 + secretEncryption: + aescbc: + activeKey: + name: key + type: aescbc + services: + - service: APIServer + servicePublishingStrategy: + type: LoadBalancer + loadBalancer: + hostname: api-mycluster.example.com + - service: OAuthServer + servicePublishingStrategy: + type: Route + route: {} + - service: Konnectivity + servicePublishingStrategy: + type: Route + route: {} + - service: Ignition + servicePublishingStrategy: + type: Route + route: {} + expectedError: "Azure service hostname domain must not overlap with the cluster base domain" + + - name: When Azure route hostname domain differs from baseDomain it should pass + initial: | + apiVersion: hypershift.openshift.io/v1beta1 + kind: HostedCluster + spec: + dns: + baseDomain: example.com + platform: + type: Azure + azure: + location: eastus + resourceGroupName: test-rg + vnetID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet" + subnetID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet" + subscriptionID: "12345678-1234-5678-9012-123456789012" + securityGroupID: "/subscriptions/12345678-1234-5678-9012-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/networkSecurityGroups/test-nsg" + tenantID: "87654321-4321-8765-2109-876543210987" + azureAuthenticationConfig: + azureAuthenticationConfigType: ManagedIdentities + managedIdentities: + controlPlane: + managedIdentitiesKeyVault: + name: test-kv + tenantID: "87654321-4321-8765-2109-876543210987" + cloudProvider: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: cp-secret + nodePoolManagement: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: npm-secret + controlPlaneOperator: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: cpo-secret + imageRegistry: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: ir-secret + ingress: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: ingress-secret + network: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: network-secret + disk: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: disk-secret + file: + clientID: "12345678-1234-5678-9012-123456789012" + objectEncoding: utf-8 + credentialsSecretName: file-secret + dataPlane: + imageRegistryMSIClientID: "12345678-1234-5678-9012-123456789012" + diskMSIClientID: "12345678-1234-5678-9012-123456789012" + fileMSIClientID: "12345678-1234-5678-9012-123456789012" + pullSecret: + name: secret + release: + image: quay.io/openshift-release-dev/ocp-release:4.15.11-x86_64 + secretEncryption: + aescbc: + activeKey: + name: key + type: aescbc + services: + - service: APIServer + servicePublishingStrategy: + type: Route + route: + hostname: api-mycluster.external.different.com + - service: OAuthServer + servicePublishingStrategy: + type: Route + route: {} + - service: Konnectivity + servicePublishingStrategy: + type: Route + route: {} + - service: Ignition + servicePublishingStrategy: + type: Route + route: {} + diff --git a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml index 6de80f8f0a78..dd695487563c 100644 --- a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml +++ b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml @@ -8322,6 +8322,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml index 8226337d326d..796025a74a3c 100644 --- a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml +++ b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml @@ -6993,6 +6993,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml index 919e3b55bbfd..b7b1a1036109 100644 --- a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml +++ b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml @@ -8193,6 +8193,17 @@ spec: - message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig) + - message: Azure service hostname domain must not overlap with the cluster + base domain. An Azure Private DNS zone matching or containing the + base domain would shadow *.apps DNS resolution. + rule: self.platform.type != "Azure" || self.dns.baseDomain == "" || + !self.services.exists(s, (has(s.servicePublishingStrategy.route) && + has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) + && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') + && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + + 1)))) status: description: status is the latest observed status of the HostedCluster. properties: diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go index c2d634988708..dac886efb32e 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go @@ -527,6 +527,7 @@ type Capabilities struct { // +kubebuilder:validation:XValidation:rule=`!self.services.exists(s, s.service == 'APIServer' && has(s.servicePublishingStrategy.loadBalancer) && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration) && has(self.configuration.apiServer) && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))`, message="APIServer loadBalancer hostname cannot be in ClusterConfiguration.apiserver.servingCerts.namedCertificates[]" // +kubebuilder:validation:XValidation:rule="!has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.disableMultiNetwork) || !self.operatorConfiguration.clusterNetworkOperator.disableMultiNetwork || self.networking.networkType == 'Other'",message="disableMultiNetwork can only be set to true when networkType is 'Other'" // +kubebuilder:validation:XValidation:rule="self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)", message="ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes" +// +kubebuilder:validation:XValidation:rule=`self.platform.type != "Azure" || self.dns.baseDomain == "" || !self.services.exists(s, (has(s.servicePublishingStrategy.route) && has(s.servicePublishingStrategy.route.hostname) && s.servicePublishingStrategy.route.hostname.contains('.') && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.route.hostname.substring(s.servicePublishingStrategy.route.hostname.indexOf('.') + 1))) || (has(s.servicePublishingStrategy.loadBalancer) && has(s.servicePublishingStrategy.loadBalancer.hostname) && s.servicePublishingStrategy.loadBalancer.hostname.contains('.') && ('.' + self.dns.baseDomain).endsWith('.' + s.servicePublishingStrategy.loadBalancer.hostname.substring(s.servicePublishingStrategy.loadBalancer.hostname.indexOf('.') + 1))))`,message="Azure service hostname domain must not overlap with the cluster base domain. An Azure Private DNS zone matching or containing the base domain would shadow *.apps DNS resolution." type HostedClusterSpec struct { // release specifies the desired OCP release payload for all the hosted cluster components. // This includes those components running management side like the Kube API Server and the CVO but also the operands which land in the hosted cluster data plane like the ingress controller, ovn agents, etc. From a419304d38570e51ad5c4dac4a5f7f0cf4fde256 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 13 May 2026 11:13:15 -0400 Subject: [PATCH 04/14] fix(control-plane-operator): skip DNS zone creation when base domain shadows cluster domain Add shadowing detection guard in reconcileBaseDomainDNS() that sets a BaseDomainShadowsClusterDomain degraded condition and skips *.apps wildcard creation when the base domain zone would shadow cluster DNS. This is defense-in-depth for pre-existing misconfigured clusters that bypass the webhook. Signed-off-by: Bryan Cox Commit-Message-Assisted-by: Claude (via Claude Code) --- .../azureprivatelinkservice/controller.go | 54 +++- .../controller_test.go | 272 ++++++++++++++++-- 2 files changed, 304 insertions(+), 22 deletions(-) diff --git a/control-plane-operator/controllers/azureprivatelinkservice/controller.go b/control-plane-operator/controllers/azureprivatelinkservice/controller.go index a43b09b57be4..e20d5da786e4 100644 --- a/control-plane-operator/controllers/azureprivatelinkservice/controller.go +++ b/control-plane-operator/controllers/azureprivatelinkservice/controller.go @@ -334,7 +334,7 @@ func (r *AzurePrivateLinkServiceReconciler) Reconcile(ctx context.Context, req c // Workers need to resolve api-. and oauth-. // to the PE IP so that the console, OAuth, and other services work on private clusters. if azPLS.Spec.BaseDomain != "" { - if result, err := r.reconcileBaseDomainDNS(ctx, azPLS, hcp.Name, log); err != nil || !result.IsZero() { + if result, err := r.reconcileBaseDomainDNS(ctx, azPLS, hcp.Name, hcp.Spec.DNS.BaseDomain, log); err != nil || !result.IsZero() { return result, err } } @@ -744,6 +744,19 @@ func (r *AzurePrivateLinkServiceReconciler) reconcileDNS(ctx context.Context, az }, log) } +func baseDomainShadowsClusterDomain(baseDomain, clusterName, hcpBaseDomain string) bool { + if baseDomain == "" || clusterName == "" || hcpBaseDomain == "" { + return false + } + clusterDomain := strings.ToLower(clusterName + "." + hcpBaseDomain) + // A Private DNS zone named baseDomain is authoritative for all queries + // under *.baseDomain. Prepending a dot to both sides ensures we match at + // domain label boundaries only (e.g. "ample.com" does NOT match + // "example.com") while also catching baseDomain == hcpBaseDomain + // (e.g. baseDomain "example.com" shadows "my-cluster.example.com"). + return strings.HasSuffix("."+clusterDomain, "."+strings.ToLower(baseDomain)) +} + // reconcileBaseDomainDNS creates a Private DNS Zone for the cluster's base domain, // links it to the guest VNet, and creates A records for the API and/or OAuth hostnames. // This enables worker VMs to resolve api-. and oauth-. @@ -756,7 +769,44 @@ func (r *AzurePrivateLinkServiceReconciler) reconcileDNS(ctx context.Context, az // (backward compatibility for clusters without a separate OAuth PLS). // - Any other CR (e.g., oauth-openshift): Creates only oauth- record, pointing // to this CR's own PE IP. -func (r *AzurePrivateLinkServiceReconciler) reconcileBaseDomainDNS(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, clusterName string, log logr.Logger) (ctrl.Result, error) { +// +// When the base domain would shadow the cluster's apps domain (baseDomain == +// clusterName.hcpBaseDomain or is a parent domain of it), a degraded condition +// is set on the CR and zone creation is skipped entirely. Creating a Private DNS +// zone for the base domain in this case would make it authoritative for *.apps +// queries, but the Private Endpoint routes to the management-plane private-router +// (HAProxy) whose SNI ACLs only match *.hypershift.local hostnames. Base domain +// *.apps hostnames would fall through to KAS, which presents a TLS cert that does +// not cover them, breaking ingress. The correct fix is to recreate the cluster +// with a different --external-dns-domain value. +func (r *AzurePrivateLinkServiceReconciler) reconcileBaseDomainDNS(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, clusterName, hcpBaseDomain string, log logr.Logger) (ctrl.Result, error) { + // Skip zone creation when shadowing is detected. We cannot add *.apps → PE IP + // because the PE routes to private-router (HAProxy) which only serves + // .hypershift.local hostnames — apps traffic gets KAS certs. The data-plane + // router-default IP is not discoverable from this controller. + if baseDomainShadowsClusterDomain(azPLS.Spec.BaseDomain, clusterName, hcpBaseDomain) { + log.Info("Base domain zone shadows cluster apps domain, DNS resolution for *.apps will be affected", + "baseDomain", azPLS.Spec.BaseDomain, + "clusterDomain", clusterName+"."+hcpBaseDomain) + + patch := client.MergeFrom(azPLS.DeepCopy()) + meta.SetStatusCondition(&azPLS.Status.Conditions, metav1.Condition{ + Type: string(hyperv1.AzurePrivateDNSAvailable), + Status: metav1.ConditionFalse, + Reason: "BaseDomainShadowsClusterDomain", + Message: fmt.Sprintf("Base domain %q shadows the cluster domain %q. "+ + "The Private DNS zone will intercept *.apps queries, breaking ingress. "+ + "Recreate the cluster with a different --external-dns-domain value.", + azPLS.Spec.BaseDomain, clusterName+"."+hcpBaseDomain), + ObservedGeneration: azPLS.Generation, + }) + if err := r.Status().Patch(ctx, azPLS, patch); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update degraded condition for base domain shadowing: %w", err) + } + + return ctrl.Result{RequeueAfter: azureutil.DriftDetectionRequeueInterval}, nil + } + recordNames, err := r.recordNamesForCR(ctx, azPLS, clusterName, log) if err != nil { return ctrl.Result{}, err diff --git a/control-plane-operator/controllers/azureprivatelinkservice/controller_test.go b/control-plane-operator/controllers/azureprivatelinkservice/controller_test.go index bf377fa6c9df..e4ccdb289f16 100644 --- a/control-plane-operator/controllers/azureprivatelinkservice/controller_test.go +++ b/control-plane-operator/controllers/azureprivatelinkservice/controller_test.go @@ -182,24 +182,29 @@ func (m *mockVirtualNetworkLinks) BeginDelete(_ context.Context, _ string, _ str } type mockRecordSets struct { - createErr error - deleteErr error - deleteErrZone string // if set, deleteErr only applies to this zone - createCalled bool - deleteCalled bool - createCallCount int - deleteCallCount int - createdRecordNames []string - deletedRecordNames []string - lastRecordSetName string - lastRecordType armprivatedns.RecordType - lastRecordParams armprivatedns.RecordSet -} - -func (m *mockRecordSets) CreateOrUpdate(_ context.Context, _ string, _ string, recordType armprivatedns.RecordType, relativeRecordSetName string, parameters armprivatedns.RecordSet, _ *armprivatedns.RecordSetsClientCreateOrUpdateOptions) (armprivatedns.RecordSetsClientCreateOrUpdateResponse, error) { + createErr error + deleteErr error + deleteErrZone string // if set, deleteErr only applies to this zone + createCalled bool + deleteCalled bool + createCallCount int + deleteCallCount int + createdRecordNames []string + createdRecordsByZone map[string][]string + deletedRecordNames []string + lastRecordSetName string + lastRecordType armprivatedns.RecordType + lastRecordParams armprivatedns.RecordSet +} + +func (m *mockRecordSets) CreateOrUpdate(_ context.Context, _ string, privateDnsZoneName string, recordType armprivatedns.RecordType, relativeRecordSetName string, parameters armprivatedns.RecordSet, _ *armprivatedns.RecordSetsClientCreateOrUpdateOptions) (armprivatedns.RecordSetsClientCreateOrUpdateResponse, error) { m.createCalled = true m.createCallCount++ m.createdRecordNames = append(m.createdRecordNames, relativeRecordSetName) + if m.createdRecordsByZone == nil { + m.createdRecordsByZone = make(map[string][]string) + } + m.createdRecordsByZone[privateDnsZoneName] = append(m.createdRecordsByZone[privateDnsZoneName], relativeRecordSetName) m.lastRecordSetName = relativeRecordSetName m.lastRecordType = recordType m.lastRecordParams = parameters @@ -1526,7 +1531,7 @@ func TestReconcileBaseDomainDNS_WhenPrivateRouterWithNoSibling_ItShouldCreateBot RecordSets: mockRecords, } - result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", testr.New(t)) + result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", "", testr.New(t)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(result.IsZero()).To(BeTrue()) @@ -1565,7 +1570,7 @@ func TestReconcileBaseDomainDNS_WhenPrivateRouterWithSiblingOAuth_ItShouldOnlyCr RecordSets: mockRecords, } - result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", testr.New(t)) + result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", "", testr.New(t)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(result.IsZero()).To(BeTrue()) @@ -1600,7 +1605,7 @@ func TestReconcileBaseDomainDNS_WhenOAuthCR_ItShouldOnlyCreateOAuthRecord(t *tes RecordSets: mockRecords, } - result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", testr.New(t)) + result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", "", testr.New(t)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(result.IsZero()).To(BeTrue()) @@ -1649,7 +1654,7 @@ func TestReconcileDelete_WhenSiblingCRsExist_ItShouldNotDeleteBaseDomainZone(t * // A records should only include the api record (sibling OAuth owns the oauth record) g.Expect(mockRecords.deleteCalled).To(BeTrue(), "should delete A records") - // The hypershift.local records (api, *.apps) + only api-test-hcp from base domain = 3 + // The hypershift.local records (api, *.apps) + api-test-hcp from base domain = 3 g.Expect(mockRecords.deletedRecordNames).To(ConsistOf("api", "*.apps", "api-test-hcp"), "should delete hypershift.local records and only api base domain record (sibling owns oauth)") @@ -2073,7 +2078,7 @@ func TestReconcileBaseDomainDNS_WhenDNSZoneCreateFails_ItShouldRequeueAfterError RecordSets: &mockRecordSets{}, } - result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", testr.New(t)) + result, err := r.reconcileBaseDomainDNS(t.Context(), azPLS, "test-hcp", "", testr.New(t)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(result.RequeueAfter).ToNot(BeZero()) } @@ -3535,6 +3540,233 @@ func TestReconcile_WhenNonPrivateRouterDNSZoneNamePatchFails_ItShouldReturnError g.Expect(err).To(MatchError(ContainSubstring("failed to persist DNS zone name in status"))) } +func TestReconcile_WhenBaseDomainShadowsClusterDomain_ItShouldSetDegradedConditionAndSkipZoneCreation(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + scheme := newTestScheme(t, g) + + azPLS := newTestAzurePLS(t, "private-router", "test-ns") + azPLS.Finalizers = []string{azurePrivateLinkServiceFinalizer} + azPLS.Status.PrivateLinkServiceAlias = "test-alias" + azPLS.Status.PrivateEndpointIP = "10.0.1.5" + azPLS.Status.PrivateEndpointID = "/pe/id" + // This is the shadowing condition: baseDomain == hcpName.hcpDNSBaseDomain + azPLS.Spec.BaseDomain = "test-hcp.example.com" + + hcp := newTestHCP(t, "test-hcp", "test-ns", "api.test.example.com") + hcp.Spec.DNS.BaseDomain = "example.com" + hcp.Finalizers = []string{hcpAzurePLSFinalizerName} + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(azPLS, hcp). + WithStatusSubresource(azPLS). + Build() + + mockPE := &mockPrivateEndpoints{ + getResponse: armnetwork.PrivateEndpointsClientGetResponse{ + PrivateEndpoint: armnetwork.PrivateEndpoint{ + ID: ptr.To("/pe/id"), + Properties: &armnetwork.PrivateEndpointProperties{ + CustomDNSConfigs: []*armnetwork.CustomDNSConfigPropertiesFormat{ + {IPAddresses: []*string{ptr.To("10.0.1.5")}}, + }, + }, + }, + }, + } + mockDNS := &mockPrivateDNSZones{} + mockRecords := &mockRecordSets{} + + r := &AzurePrivateLinkServiceReconciler{ + Client: fakeClient, + PrivateEndpoints: mockPE, + PrivateDNSZones: mockDNS, + VirtualNetworkLinks: &mockVirtualNetworkLinks{}, + RecordSets: mockRecords, + } + + result, err := r.Reconcile(log.IntoContext(t.Context(), testr.New(t)), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: "private-router", Namespace: "test-ns"}, + }) + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).ToNot(BeZero(), "should requeue for drift detection") + + // No base domain zone should be created when shadowing is detected. + // The last zone created should be the hypershift.local zone, not the base domain zone. + g.Expect(mockDNS.lastZoneName).To(Equal("test-hcp.hypershift.local"), + "only the hypershift.local zone should be created, not the base domain zone") + + // Verify no records were created for the base domain zone + baseDomainRecords := mockRecords.createdRecordsByZone["test-hcp.example.com"] + g.Expect(baseDomainRecords).To(BeEmpty(), + "no records should be created in the base domain zone when shadowing is detected") + + // Verify the DNS condition is set to False with the degraded reason + updated := &hyperv1.AzurePrivateLinkService{} + err = fakeClient.Get(t.Context(), types.NamespacedName{Name: "private-router", Namespace: "test-ns"}, updated) + g.Expect(err).ToNot(HaveOccurred()) + + dnsCondition := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AzurePrivateDNSAvailable)) + g.Expect(dnsCondition).ToNot(BeNil(), "DNS condition should be set") + g.Expect(dnsCondition.Status).To(Equal(metav1.ConditionFalse), + "DNS condition should be False when shadowing is detected") + g.Expect(dnsCondition.Reason).To(Equal("BaseDomainShadowsClusterDomain"), + "DNS condition reason should indicate base domain shadowing") + g.Expect(dnsCondition.Message).To(ContainSubstring("shadows the cluster domain"), + "DNS condition message should explain the shadowing issue") + g.Expect(dnsCondition.Message).To(ContainSubstring("--external-dns-domain"), + "DNS condition message should suggest recreating with a different external-dns-domain") +} + +func TestBaseDomainShadowsClusterDomain(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + baseDomain string + clusterName string + hcpBaseDomain string + expected bool + }{ + "When baseDomain equals clusterName.hcpBaseDomain, it should detect shadowing": { + baseDomain: "my-cluster.example.com", + clusterName: "my-cluster", + hcpBaseDomain: "example.com", + expected: true, + }, + "When baseDomain matches with different casing, it should detect shadowing": { + baseDomain: "My-Cluster.Example.COM", + clusterName: "my-cluster", + hcpBaseDomain: "example.com", + expected: true, + }, + "When baseDomain differs from clusterName.hcpBaseDomain, it should not detect shadowing": { + baseDomain: "other-prefix.example.com", + clusterName: "my-cluster", + hcpBaseDomain: "example.com", + expected: false, + }, + "When baseDomain is empty, it should not detect shadowing": { + baseDomain: "", + clusterName: "my-cluster", + hcpBaseDomain: "example.com", + expected: false, + }, + "When hcpBaseDomain is empty, it should not detect shadowing": { + baseDomain: "my-cluster.example.com", + clusterName: "my-cluster", + hcpBaseDomain: "", + expected: false, + }, + "When clusterName is empty, it should return false": { + baseDomain: "my-cluster.example.com", + clusterName: "", + hcpBaseDomain: "example.com", + expected: false, + }, + "When baseDomain equals hcpBaseDomain, it should detect shadowing": { + baseDomain: "example.com", + clusterName: "my-cluster", + hcpBaseDomain: "example.com", + expected: true, + }, + "When baseDomain is a non-domain-boundary suffix, it should not detect shadowing": { + baseDomain: "ample.com", + clusterName: "my-cluster", + hcpBaseDomain: "example.com", + expected: false, + }, + "When baseDomain is a multi-level parent domain, it should detect shadowing": { + baseDomain: "devcluster.openshift.com", + clusterName: "hcp-two", + hcpBaseDomain: "acm-dev04.devcluster.openshift.com", + expected: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(baseDomainShadowsClusterDomain(tt.baseDomain, tt.clusterName, tt.hcpBaseDomain)).To(Equal(tt.expected)) + }) + } +} + +func TestReconcile_WhenBaseDomainDiffersFromClusterDomain_ItShouldCreateZoneNormally(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + scheme := newTestScheme(t, g) + + azPLS := newTestAzurePLS(t, "private-router", "test-ns") + azPLS.Finalizers = []string{azurePrivateLinkServiceFinalizer} + azPLS.Status.PrivateLinkServiceAlias = "test-alias" + azPLS.Status.PrivateEndpointIP = "10.0.1.5" + azPLS.Status.PrivateEndpointID = "/pe/id" + // No shadowing: base domain is unrelated to hcpName.hcpDNSBaseDomain + azPLS.Spec.BaseDomain = "custom-dns.example.com" + + hcp := newTestHCP(t, "test-hcp", "test-ns", "api.test.example.com") + hcp.Spec.DNS.BaseDomain = "example.com" + hcp.Finalizers = []string{hcpAzurePLSFinalizerName} + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(azPLS, hcp). + WithStatusSubresource(azPLS). + Build() + + mockPE := &mockPrivateEndpoints{ + getResponse: armnetwork.PrivateEndpointsClientGetResponse{ + PrivateEndpoint: armnetwork.PrivateEndpoint{ + ID: ptr.To("/pe/id"), + Properties: &armnetwork.PrivateEndpointProperties{ + CustomDNSConfigs: []*armnetwork.CustomDNSConfigPropertiesFormat{ + {IPAddresses: []*string{ptr.To("10.0.1.5")}}, + }, + }, + }, + }, + } + mockDNS := &mockPrivateDNSZones{} + mockRecords := &mockRecordSets{} + + r := &AzurePrivateLinkServiceReconciler{ + Client: fakeClient, + PrivateEndpoints: mockPE, + PrivateDNSZones: mockDNS, + VirtualNetworkLinks: &mockVirtualNetworkLinks{}, + RecordSets: mockRecords, + } + + result, err := r.Reconcile(log.IntoContext(t.Context(), testr.New(t)), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: "private-router", Namespace: "test-ns"}, + }) + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).ToNot(BeZero(), "should requeue for drift detection") + + // The base domain zone should be created since there's no shadowing + g.Expect(mockDNS.lastZoneName).To(Equal("custom-dns.example.com"), + "last zone should be the base domain zone") + + // *.apps should only appear in the hypershift.local zone, NOT the base domain zone + baseDomainRecords := mockRecords.createdRecordsByZone["custom-dns.example.com"] + g.Expect(baseDomainRecords).ToNot(ContainElement("*.apps"), + "*.apps wildcard should NOT be added to the base domain zone when there is no shadowing") + + // Verify the DNS condition is set to True (success) + updated := &hyperv1.AzurePrivateLinkService{} + err = fakeClient.Get(t.Context(), types.NamespacedName{Name: "private-router", Namespace: "test-ns"}, updated) + g.Expect(err).ToNot(HaveOccurred()) + + dnsCondition := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AzurePrivateDNSAvailable)) + g.Expect(dnsCondition).ToNot(BeNil(), "DNS condition should be set") + g.Expect(dnsCondition.Status).To(Equal(metav1.ConditionTrue)) +} + func TestReconcile_WhenAvailableConditionPatchFails_ItShouldReturnError(t *testing.T) { t.Parallel() g := NewGomegaWithT(t) From 241621ff12f4a12164d3c180885da4ab947a18c9 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 13 May 2026 11:13:20 -0400 Subject: [PATCH 05/14] docs: document externalDNSDomain shadowing and the Two Routers Problem Expand Azure private cluster docs to explain why externalDNSDomain must not overlap with the cluster apps domain. Document the Two Routers Problem (PE IP routes to HAProxy, not router-default) and why the controller cannot self-heal (data-plane IP not discoverable). Signed-off-by: Bryan Cox Commit-Message-Assisted-by: Claude (via Claude Code) --- .../create-self-managed-azure-cluster.md | 9 +++ .../azure/deploy-azure-private-clusters.md | 70 +++++++++++++++- docs/content/reference/aggregated-docs.md | 79 ++++++++++++++++++- 3 files changed, 156 insertions(+), 2 deletions(-) diff --git a/docs/content/how-to/azure/create-self-managed-azure-cluster.md b/docs/content/how-to/azure/create-self-managed-azure-cluster.md index 6d213260b891..801e3b410bf6 100644 --- a/docs/content/how-to/azure/create-self-managed-azure-cluster.md +++ b/docs/content/how-to/azure/create-self-managed-azure-cluster.md @@ -168,6 +168,15 @@ ${HYPERSHIFT_BINARY_PATH}/hypershift create cluster azure \ cluster's VNet, `--endpoint-access Private` flag, and HyperShift operator installation with `--private-platform Azure`. +!!! warning "Private Clusters: Avoid DNS Zone Shadowing" + + If creating a **private** Azure HostedCluster, ensure `--external-dns-domain` does + not match `{clusterName}.{baseDomain}` or its parent domain. A matching value + causes an Azure Private DNS zone to shadow `*.apps` resolution, breaking console + and all ingress. This cannot be fixed after creation. See + [External DNS Domain Must Not Match Cluster Domain](deploy-azure-private-clusters.md#external-dns-domain-must-not-match-cluster-domain) + for details. + ### Configuring Azure Marketplace Images HyperShift supports multiple approaches for configuring Azure Marketplace images for your cluster nodes. The recommended approach varies based on your OpenShift version and requirements. diff --git a/docs/content/how-to/azure/deploy-azure-private-clusters.md b/docs/content/how-to/azure/deploy-azure-private-clusters.md index 1184500fcdf2..99931b13d047 100644 --- a/docs/content/how-to/azure/deploy-azure-private-clusters.md +++ b/docs/content/how-to/azure/deploy-azure-private-clusters.md @@ -468,6 +468,74 @@ The deletion process automatically cleans up Private Link resources in the corre 1. `.hypershift.local` — synthetic internal zone with `api` and `*.apps` records 2. `` — base domain zone with `api-` and `oauth-` records +### External DNS Domain Must Not Match Cluster Domain + +!!! warning "Azure Private DNS Zone Shadowing" + + On private Azure HostedClusters, do **not** set `--external-dns-domain` to a value + that matches or is a parent domain of `{clusterName}.{baseDomain}`. For example, + if your cluster is named `my-cluster` with base domain `example.com`, do not use + `--external-dns-domain my-cluster.example.com` or `--external-dns-domain example.com`. + + This misconfiguration **cannot be corrected after cluster creation** because the + relevant fields (`spec.services`, `spec.dns.baseDomain`, and `metadata.name`) are + all immutable. The cluster must be destroyed and recreated with a different + `--external-dns-domain` value. + + **Safe example**: If your cluster is `my-cluster` with base domain `example.com`, + use a separate subdomain such as `--external-dns-domain custom-dns.example.com` + that does not overlap with `my-cluster.example.com`. + +#### What Goes Wrong + +Private Azure clusters use two separate routing paths: + +1. **Management-plane router** (`private-router`): An HAProxy pod in the hosted + control plane namespace, fronted by an internal load balancer and exposed to the + guest VNet through Azure Private Link. Worker nodes reach this router via the + Private Endpoint IP. HAProxy uses SNI-based routing and only has ACLs for + `.hypershift.local` hostnames (KAS, ignition, konnectivity, OAuth). Any hostname + that does not match an ACL falls through to the `default_backend kube_api`, which + returns KAS certificates. + +2. **Data-plane router** (`router-default`): The OpenShift ingress controller running + on worker nodes, serving `*.apps.{clusterName}.{baseDomain}` hostnames with the + correct wildcard ingress certificate. + +When `--external-dns-domain` matches the cluster domain, the PLS controller creates a +Private DNS zone named `{clusterName}.{baseDomain}`. This zone becomes authoritative +for **all** queries under that name within the guest VNet, including +`*.apps.{clusterName}.{baseDomain}`. Since the zone only has `api` and `oauth` A +records pointing to the Private Endpoint IP, apps queries either: + +- Return **NXDOMAIN** (if no `*.apps` record exists in the zone), or +- Resolve to the **Private Endpoint IP**, which routes to `private-router` (HAProxy). + Because `*.apps` hostnames do not match any HAProxy SNI ACL, traffic falls through + to `kube_api` and the client receives a **TLS certificate mismatch** (KAS cert + instead of the ingress wildcard cert). + +Neither outcome is usable. The console, OAuth login, and all application routes are +unreachable. + +#### Why the Controller Cannot Self-Heal + +The controller cannot fix this by adding a `*.apps` wildcard record to the shadowing +zone because: + +- The Private Endpoint IP routes to the management-plane `private-router`, not the + data-plane `router-default`. Adding `*.apps → PE IP` would route apps traffic to + HAProxy, which does not serve those hostnames. +- The correct target (the data-plane ingress IP on worker nodes) is not available to + the PLS controller. The controller runs in the control plane and has no client to + the guest cluster. There is no HCP status field that reports the guest ingress IP, + and the HostedCluster Controller Operator (HCCO) does not propagate it back. + +When the controller detects shadowing, it sets `AzurePrivateDNSAvailable=False` with +reason `BaseDomainShadowsClusterDomain` and skips zone creation entirely. This +prevents the shadowing zone from being created, but the `api` and `oauth` hostnames +from `--external-dns-domain` will not resolve via Private DNS. The cluster must be +recreated with a non-overlapping domain. + ### Condition Debugging If the cluster gets stuck, check the `AzurePrivateLinkService` CR conditions: @@ -481,7 +549,7 @@ oc get azureprivatelinkservices -n clusters-${CLUSTER_NAME} -o jsonpath='{.items | `AzureInternalLoadBalancerAvailable` = False | The `private-router` Service hasn't received an ILB IP yet. Check the Service status and Azure networking. | | `AzurePLSCreated` = False | PLS creation failed. Check NAT subnet policies, credentials, and the HO operator logs. | | `AzurePrivateEndpointAvailable` = False | PE creation failed or connection not approved. Check the PLS auto-approval list and CPO logs. | -| `AzurePrivateDNSAvailable` = False | DNS zone or record creation failed. Check CPO identity permissions in the guest subscription. | +| `AzurePrivateDNSAvailable` = False | DNS zone or record creation failed. If the reason is `BaseDomainShadowsClusterDomain`, the `--external-dns-domain` value overlaps with the cluster domain — the cluster must be recreated with a different value. See [External DNS Domain Must Not Match Cluster Domain](#external-dns-domain-must-not-match-cluster-domain). | ## Related Documentation diff --git a/docs/content/reference/aggregated-docs.md b/docs/content/reference/aggregated-docs.md index bc63d9c122fa..92cd2ce5e069 100644 --- a/docs/content/reference/aggregated-docs.md +++ b/docs/content/reference/aggregated-docs.md @@ -9459,6 +9459,15 @@ ${HYPERSHIFT_BINARY_PATH}/hypershift create cluster azure \ cluster's VNet, `--endpoint-access Private` flag, and HyperShift operator installation with `--private-platform Azure`. +!!! warning "Private Clusters: Avoid DNS Zone Shadowing" + + If creating a **private** Azure HostedCluster, ensure `--external-dns-domain` does + not match `{clusterName}.{baseDomain}` or its parent domain. A matching value + causes an Azure Private DNS zone to shadow `*.apps` resolution, breaking console + and all ingress. This cannot be fixed after creation. See + External DNS Domain Must Not Match Cluster Domain + for details. + ### Configuring Azure Marketplace Images HyperShift supports multiple approaches for configuring Azure Marketplace images for your cluster nodes. The recommended approach varies based on your OpenShift version and requirements. @@ -10072,6 +10081,74 @@ The deletion process automatically cleans up Private Link resources in the corre 1. `.hypershift.local` — synthetic internal zone with `api` and `*.apps` records 2. `` — base domain zone with `api-` and `oauth-` records +### External DNS Domain Must Not Match Cluster Domain + +!!! warning "Azure Private DNS Zone Shadowing" + + On private Azure HostedClusters, do **not** set `--external-dns-domain` to a value + that matches or is a parent domain of `{clusterName}.{baseDomain}`. For example, + if your cluster is named `my-cluster` with base domain `example.com`, do not use + `--external-dns-domain my-cluster.example.com` or `--external-dns-domain example.com`. + + This misconfiguration **cannot be corrected after cluster creation** because the + relevant fields (`spec.services`, `spec.dns.baseDomain`, and `metadata.name`) are + all immutable. The cluster must be destroyed and recreated with a different + `--external-dns-domain` value. + + **Safe example**: If your cluster is `my-cluster` with base domain `example.com`, + use a separate subdomain such as `--external-dns-domain custom-dns.example.com` + that does not overlap with `my-cluster.example.com`. + +#### What Goes Wrong + +Private Azure clusters use two separate routing paths: + +1. **Management-plane router** (`private-router`): An HAProxy pod in the hosted + control plane namespace, fronted by an internal load balancer and exposed to the + guest VNet through Azure Private Link. Worker nodes reach this router via the + Private Endpoint IP. HAProxy uses SNI-based routing and only has ACLs for + `.hypershift.local` hostnames (KAS, ignition, konnectivity, OAuth). Any hostname + that does not match an ACL falls through to the `default_backend kube_api`, which + returns KAS certificates. + +2. **Data-plane router** (`router-default`): The OpenShift ingress controller running + on worker nodes, serving `*.apps.{clusterName}.{baseDomain}` hostnames with the + correct wildcard ingress certificate. + +When `--external-dns-domain` matches the cluster domain, the PLS controller creates a +Private DNS zone named `{clusterName}.{baseDomain}`. This zone becomes authoritative +for **all** queries under that name within the guest VNet, including +`*.apps.{clusterName}.{baseDomain}`. Since the zone only has `api` and `oauth` A +records pointing to the Private Endpoint IP, apps queries either: + +- Return **NXDOMAIN** (if no `*.apps` record exists in the zone), or +- Resolve to the **Private Endpoint IP**, which routes to `private-router` (HAProxy). + Because `*.apps` hostnames do not match any HAProxy SNI ACL, traffic falls through + to `kube_api` and the client receives a **TLS certificate mismatch** (KAS cert + instead of the ingress wildcard cert). + +Neither outcome is usable. The console, OAuth login, and all application routes are +unreachable. + +#### Why the Controller Cannot Self-Heal + +The controller cannot fix this by adding a `*.apps` wildcard record to the shadowing +zone because: + +- The Private Endpoint IP routes to the management-plane `private-router`, not the + data-plane `router-default`. Adding `*.apps → PE IP` would route apps traffic to + HAProxy, which does not serve those hostnames. +- The correct target (the data-plane ingress IP on worker nodes) is not available to + the PLS controller. The controller runs in the control plane and has no client to + the guest cluster. There is no HCP status field that reports the guest ingress IP, + and the HostedCluster Controller Operator (HCCO) does not propagate it back. + +When the controller detects shadowing, it sets `AzurePrivateDNSAvailable=False` with +reason `BaseDomainShadowsClusterDomain` and skips zone creation entirely. This +prevents the shadowing zone from being created, but the `api` and `oauth` hostnames +from `--external-dns-domain` will not resolve via Private DNS. The cluster must be +recreated with a non-overlapping domain. + ### Condition Debugging If the cluster gets stuck, check the `AzurePrivateLinkService` CR conditions: @@ -10085,7 +10162,7 @@ oc get azureprivatelinkservices -n clusters-${CLUSTER_NAME} -o jsonpath='{.items | `AzureInternalLoadBalancerAvailable` = False | The `private-router` Service hasn't received an ILB IP yet. Check the Service status and Azure networking. | | `AzurePLSCreated` = False | PLS creation failed. Check NAT subnet policies, credentials, and the HO operator logs. | | `AzurePrivateEndpointAvailable` = False | PE creation failed or connection not approved. Check the PLS auto-approval list and CPO logs. | -| `AzurePrivateDNSAvailable` = False | DNS zone or record creation failed. Check CPO identity permissions in the guest subscription. | +| `AzurePrivateDNSAvailable` = False | DNS zone or record creation failed. If the reason is `BaseDomainShadowsClusterDomain`, the `--external-dns-domain` value overlaps with the cluster domain — the cluster must be recreated with a different value. See External DNS Domain Must Not Match Cluster Domain. | ## Related Documentation From c5c1341e17ae9c731648d16d4764c2b7a001160f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 19 May 2026 15:54:33 +0000 Subject: [PATCH 06/14] build(deps): bump github.com/go-git/go-git/v5 in /hack/tools Bumps [github.com/go-git/go-git/v5](https://github.com/go-git/go-git) from 5.19.0 to 5.19.1. - [Release notes](https://github.com/go-git/go-git/releases) - [Changelog](https://github.com/go-git/go-git/blob/main/HISTORY.md) - [Commits](https://github.com/go-git/go-git/compare/v5.19.0...v5.19.1) --- updated-dependencies: - dependency-name: github.com/go-git/go-git/v5 dependency-version: 5.19.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] --- hack/tools/go.mod | 2 +- hack/tools/go.sum | 4 +- .../go-git/go-git/v5/config/config.go | 31 +- .../go-git/go-git/v5/config/modules.go | 52 ++++ .../go-git/go-git/v5/config/optbool.go | 82 ++++++ .../go-git/v5/internal/pathutil/dotgit.go | 21 ++ .../go-git/go-git/v5/internal/pathutil/hfs.go | 99 +++++++ .../go-git/v5/internal/pathutil/ntfs.go | 187 +++++++++++++ .../go-git/v5/internal/pathutil/tree.go | 66 +++++ .../go-git/go-git/v5/internal/url/url.go | 37 ++- .../v5/plumbing/format/idxfile/decoder.go | 164 ++++++++++- .../v5/plumbing/format/idxfile/idxfile.go | 29 +- .../v5/plumbing/format/objfile/reader.go | 18 +- .../v5/plumbing/format/packfile/diff_delta.go | 3 - .../v5/plumbing/format/packfile/fsobject.go | 8 +- .../v5/plumbing/format/packfile/packfile.go | 21 +- .../v5/plumbing/format/packfile/parser.go | 72 ++++- .../plumbing/format/packfile/patch_delta.go | 111 +++++--- .../v5/plumbing/format/packfile/scanner.go | 154 +++++++++- .../go-git/go-git/v5/plumbing/object/tree.go | 23 ++ .../v5/plumbing/transport/ssh/common.go | 34 ++- .../github.com/go-git/go-git/v5/repository.go | 13 +- .../v5/storage/filesystem/dotgit/dotgit.go | 19 +- .../github.com/go-git/go-git/v5/submodule.go | 76 ++++- .../go-git/go-git/v5/utils/binary/read.go | 15 + .../github.com/go-git/go-git/v5/worktree.go | 115 +------- .../go-git/go-git/v5/worktree_fs.go | 264 ++++++++++++++++++ .../go-git/go-git/v5/worktree_status.go | 9 + hack/tools/vendor/modules.txt | 3 +- 29 files changed, 1523 insertions(+), 209 deletions(-) create mode 100644 hack/tools/vendor/github.com/go-git/go-git/v5/config/optbool.go create mode 100644 hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/dotgit.go create mode 100644 hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/hfs.go create mode 100644 hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/ntfs.go create mode 100644 hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/tree.go create mode 100644 hack/tools/vendor/github.com/go-git/go-git/v5/worktree_fs.go diff --git a/hack/tools/go.mod b/hack/tools/go.mod index 5b0fa4979ab3..68fca80773f1 100644 --- a/hack/tools/go.mod +++ b/hack/tools/go.mod @@ -106,7 +106,7 @@ require ( github.com/go-critic/go-critic v0.14.3 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/go-billy/v5 v5.9.0 // indirect - github.com/go-git/go-git/v5 v5.19.0 // indirect + github.com/go-git/go-git/v5 v5.19.1 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-logr/zapr v1.3.0 // indirect diff --git a/hack/tools/go.sum b/hack/tools/go.sum index 6bac4750ba6e..a172f7968df5 100644 --- a/hack/tools/go.sum +++ b/hack/tools/go.sum @@ -192,8 +192,8 @@ github.com/go-git/go-billy/v5 v5.9.0 h1:jItGXszUDRtR/AlferWPTMN4j38BQ88XnXKbilmm github.com/go-git/go-billy/v5 v5.9.0/go.mod h1:jCnQMLj9eUgGU7+ludSTYoZL/GGmii14RxKFj7ROgHw= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII= -github.com/go-git/go-git/v5 v5.19.0 h1:+WkVUQZSy/F1Gb13udrMKjIM2PrzsNfDKFSfo5tkMtc= -github.com/go-git/go-git/v5 v5.19.0/go.mod h1:Pb1v0c7/g8aGQJwx9Us09W85yGoyvSwuhEGMH7zjDKQ= +github.com/go-git/go-git/v5 v5.19.1 h1:nX27AnaU43/K5bKktKwgBmR9lawoYVe1Ckg0rgzzN00= +github.com/go-git/go-git/v5 v5.19.1/go.mod h1:Pb1v0c7/g8aGQJwx9Us09W85yGoyvSwuhEGMH7zjDKQ= github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= github.com/go-logr/logr v0.2.0/go.mod h1:z6/tIYblkpsD+a4lm/fGIIU9mZ+XfAiaFtq7xTgseGU= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/config/config.go b/hack/tools/vendor/github.com/go-git/go-git/v5/config/config.go index 33f6e37d269d..3ae6a571e490 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/config/config.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/config/config.go @@ -61,6 +61,16 @@ type Config struct { CommentChar string // RepositoryFormatVersion identifies the repository format and layout version. RepositoryFormatVersion format.RepositoryFormatVersion + // ProtectNTFS controls whether NTFS-specific path protections are + // applied (e.g. rejecting .git trailing spaces/periods, alternate + // data streams, 8.3 short names). When unset, defaults to true on + // Windows. + ProtectNTFS OptBool + // ProtectHFS controls whether HFS+-specific path protections are + // applied (e.g. rejecting .git with Unicode zero-width or + // directional characters that HFS+ would normalize away). + // When unset, defaults to true on macOS. + ProtectHFS OptBool } User struct { @@ -266,6 +276,8 @@ const ( repositoryFormatVersionKey = "repositoryformatversion" objectFormat = "objectformat" mirrorKey = "mirror" + protectNTFSKey = "protectNTFS" + protectHFSKey = "protectHFS" // DefaultPackWindow holds the number of previous objects used to // generate deltas. The value 10 is the same used by git command. @@ -309,6 +321,14 @@ func (c *Config) unmarshalCore() { c.Core.Worktree = s.Options.Get(worktreeKey) c.Core.CommentChar = s.Options.Get(commentCharKey) + + if parsed := parseConfigBool(s.Options.Get(protectNTFSKey)); parsed.IsSet() { + c.Core.ProtectNTFS = parsed + } + + if parsed := parseConfigBool(s.Options.Get(protectHFSKey)); parsed.IsSet() { + c.Core.ProtectHFS = parsed + } } func (c *Config) unmarshalUser() { @@ -379,7 +399,8 @@ func unmarshalSubmodules(fc *format.Config, submodules map[string]*Submodule) { m := &Submodule{} m.unmarshal(sub) - if m.Validate() == ErrModuleBadPath { + if err := m.Validate(); errors.Is(err, ErrModuleBadPath) || + errors.Is(err, ErrModuleBadName) { continue } @@ -436,6 +457,14 @@ func (c *Config) marshalCore() { if c.Core.Worktree != "" { s.SetOption(worktreeKey, c.Core.Worktree) } + + if c.Core.ProtectNTFS.IsSet() { + s.SetOption(protectNTFSKey, c.Core.ProtectNTFS.FormatBool()) + } + + if c.Core.ProtectHFS.IsSet() { + s.SetOption(protectHFSKey, c.Core.ProtectHFS.FormatBool()) + } } func (c *Config) marshalExtensions() { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/config/modules.go b/hack/tools/vendor/github.com/go-git/go-git/v5/config/modules.go index 1c10aa354eb2..5fdd838645fb 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/config/modules.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/config/modules.go @@ -3,8 +3,11 @@ package config import ( "bytes" "errors" + "fmt" "regexp" + "strings" + "github.com/go-git/go-git/v5/internal/pathutil" format "github.com/go-git/go-git/v5/plumbing/format/config" ) @@ -12,6 +15,7 @@ var ( ErrModuleEmptyURL = errors.New("module config: empty URL") ErrModuleEmptyPath = errors.New("module config: empty path") ErrModuleBadPath = errors.New("submodule has an invalid path") + ErrModuleBadName = errors.New("ignoring suspicious submodule name") ) var ( @@ -94,6 +98,10 @@ type Submodule struct { // Validate validates the fields and sets the default values. func (m *Submodule) Validate() error { + if err := validSubmoduleName(m.Name); err != nil { + return fmt.Errorf("%w: %q", ErrModuleBadName, m.Name) + } + if m.Path == "" { return ErrModuleEmptyPath } @@ -109,6 +117,50 @@ func (m *Submodule) Validate() error { return nil } +// validSubmoduleName mirrors canonical Git's check_submodule_name in +// submodule-config.c [1]: reject empty names and any name with a ".." +// path component, using both '/' and '\\' as separators so the rule +// is consistent across platforms. The component check is delegated to +// `pathutil.IsHFSDot` and `pathutil.IsNTFSDot` with `.` as the needle, +// which both cover the bare ".." case and reject components that +// resolve to ".." after HFS+ Unicode normalisation (ignored code +// points, e.g. `..`) or NTFS trailing-space/dot/ADS +// canonicalisation (e.g. `.. `, `..::$INDEX_ALLOCATION`). +// `.gitmodules` is attacker-controlled by definition, so both checks +// run unconditionally regardless of host OS. +// +// The additional checks (bare ".", NUL byte, leading or trailing +// separator, drive-letter prefix) close go-git-specific edge cases +// the canonical loop does not exercise: canonical Git treats names +// as opaque C strings, while Go strings carry NULs through and the +// billy filesystem layer is path-aware in ways Git's working storage +// is not. +// +// [1]: https://github.com/git/git/blob/v2.54.0/submodule-config.c#L214-L237 +func validSubmoduleName(name string) error { + if name == "" || name == "." { + return ErrModuleBadName + } + for _, seg := range strings.FieldsFunc(name, isPathSep) { + if pathutil.IsHFSDot(seg, ".") || pathutil.IsNTFSDot(seg, ".", "") { + return ErrModuleBadName + } + } + // go-git-specific defensive checks beyond canonical Git. + if strings.ContainsRune(name, 0) { + return ErrModuleBadName + } + if isPathSep(rune(name[0])) || isPathSep(rune(name[len(name)-1])) { + return ErrModuleBadName + } + if len(name) >= 2 && name[1] == ':' { + return ErrModuleBadName + } + return nil +} + +func isPathSep(r rune) bool { return r == '/' || r == '\\' } + func (m *Submodule) unmarshal(s *format.Subsection) { m.raw = s diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/config/optbool.go b/hack/tools/vendor/github.com/go-git/go-git/v5/config/optbool.go new file mode 100644 index 000000000000..cb89fbf42bf9 --- /dev/null +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/config/optbool.go @@ -0,0 +1,82 @@ +package config + +import ( + "strconv" + "strings" +) + +// OptBool is a tri-state boolean: unset, explicitly false, or explicitly true. +// Its zero value (OptBoolUnset) means the setting was not specified, which +// allows merge logic based on reflect.Value.IsZero to skip unset fields while +// still letting an explicit "false" override a previously set "true". +type OptBool byte + +const ( + // OptBoolUnset indicates the setting was not specified. + OptBoolUnset OptBool = iota + // OptBoolFalse indicates the setting was explicitly set to false. + OptBoolFalse + // OptBoolTrue indicates the setting was explicitly set to true. + OptBoolTrue +) + +// NewOptBool converts a plain bool into an OptBool. +func NewOptBool(v bool) OptBool { + if v { + return OptBoolTrue + } + return OptBoolFalse +} + +// IsTrue returns whether the value is explicitly true. +func (o OptBool) IsTrue() bool { return o == OptBoolTrue } + +// IsSet returns whether the value was explicitly specified (true or false). +func (o OptBool) IsSet() bool { return o != OptBoolUnset } + +func (o OptBool) String() string { + switch o { + case OptBoolTrue: + return "true" + case OptBoolFalse: + return "false" + default: + return "unset" + } +} + +// FormatBool returns the strconv-formatted value. Only meaningful when IsSet. +func (o OptBool) FormatBool() string { + return strconv.FormatBool(o.IsTrue()) +} + +// parseConfigBool mirrors upstream Git's git_parse_maybe_bool: it +// accepts true/yes/on (→ OptBoolTrue) and false/no/off (→ +// OptBoolFalse) case-insensitively, plus any decimal integer (zero +// → OptBoolFalse, non-zero → OptBoolTrue). Empty or otherwise +// unrecognised values return OptBoolUnset, leaving the caller's +// platform default in place. The empty-string handling is the only +// intentional divergence from upstream, which returns false for +// empty: in our unmarshalCore caller, an empty value means the key +// is unset and the platform default should apply. +// +// Reference: upstream Git git_parse_maybe_bool_text at parse.c +// L157-L173 and git_parse_maybe_bool at parse.c L174-L182 in tag +// v2.54.0[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/parse.c#L157-L182 +func parseConfigBool(v string) OptBool { + switch strings.ToLower(v) { + case "true", "yes", "on": + return OptBoolTrue + case "false", "no", "off": + return OptBoolFalse + } + if i, err := strconv.Atoi(v); err == nil { + if i != 0 { + return OptBoolTrue + } + return OptBoolFalse + } + return OptBoolUnset +} diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/dotgit.go b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/dotgit.go new file mode 100644 index 000000000000..e50ee9ce5d85 --- /dev/null +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/dotgit.go @@ -0,0 +1,21 @@ +package pathutil + +import "strings" + +// IsDotGitName reports whether name is `.git` or its 8.3 NTFS short +// alias `git~1`, case-insensitively. Both are forbidden as path +// components (and as submodule names) because they refer to the +// repository's own metadata directory. +// +// File names that do not conform to the 8.3 format (up to eight +// characters for the basename, three for the file extension) are +// associated with a so-called "short name" on NTFS — at least on +// the `C:` drive by default — which means that `git~1/` is a valid +// way to refer to `.git/`. +func IsDotGitName(name string) bool { + switch strings.ToLower(name) { + case ".git", "git~1": + return true + } + return false +} diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/hfs.go b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/hfs.go new file mode 100644 index 000000000000..66fc12f89195 --- /dev/null +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/hfs.go @@ -0,0 +1,99 @@ +package pathutil + +import "unicode" + +// hfsIgnoredCodepoints contains Unicode code points that HFS+ ignores +// during path normalization. A path component containing these +// characters between the bytes of ".git" (or ".gitmodules", etc.) +// will be treated as that name by HFS+, so they have to be filtered +// out before comparison. +// +// See upstream Git utf8.c next_hfs_char in tag v2.54.0[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/utf8.c#L703-L740 +var hfsIgnoredCodepoints = map[rune]struct{}{ + 0x200c: {}, // ZERO WIDTH NON-JOINER + 0x200d: {}, // ZERO WIDTH JOINER + 0x200e: {}, // LEFT-TO-RIGHT MARK + 0x200f: {}, // RIGHT-TO-LEFT MARK + 0x202a: {}, // LEFT-TO-RIGHT EMBEDDING + 0x202b: {}, // RIGHT-TO-LEFT EMBEDDING + 0x202c: {}, // POP DIRECTIONAL FORMATTING + 0x202d: {}, // LEFT-TO-RIGHT OVERRIDE + 0x202e: {}, // RIGHT-TO-LEFT OVERRIDE + 0x206a: {}, // INHIBIT SYMMETRIC SWAPPING + 0x206b: {}, // ACTIVATE SYMMETRIC SWAPPING + 0x206c: {}, // INHIBIT ARABIC FORM SHAPING + 0x206d: {}, // ACTIVATE ARABIC FORM SHAPING + 0x206e: {}, // NATIONAL DIGIT SHAPES + 0x206f: {}, // NOMINAL DIGIT SHAPES + 0xfeff: {}, // ZERO WIDTH NO-BREAK SPACE +} + +// IsHFSDot reports whether part would be treated as "." on an +// HFS+ filesystem after stripping ignored Unicode code points and +// folding ASCII to lower case. The needle is the lowercase ASCII +// suffix without the leading dot (e.g. "git", "gitmodules"). It +// mirrors upstream Git's is_hfs_dot_generic and is the building +// block of IsHFSDotGit / IsHFSDotGitmodules. +// +// Reference: upstream Git utf8.c is_hfs_dot_generic at L741-L774 and +// the dotgit family at L784-L809 in tag v2.54.0[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/utf8.c#L741-L809 +func IsHFSDot(part, needle string) bool { + runes := []rune(part) + i := 0 + + // skip ignored code points, then expect '.' + for i < len(runes) { + if _, ok := hfsIgnoredCodepoints[runes[i]]; !ok { + break + } + i++ + } + if i >= len(runes) || runes[i] != '.' { + return false + } + i++ + + // match needle case-insensitively, skipping ignored code points + for _, expected := range needle { + for i < len(runes) { + if _, ok := hfsIgnoredCodepoints[runes[i]]; !ok { + break + } + i++ + } + if i >= len(runes) { + return false + } + r := runes[i] + if r > 127 { + return false + } + if unicode.ToLower(r) != expected { + return false + } + i++ + } + + // skip trailing ignored code points + for i < len(runes) { + if _, ok := hfsIgnoredCodepoints[runes[i]]; !ok { + break + } + i++ + } + + // must be at end of component + return i == len(runes) +} + +// IsHFSDotGit reports whether part is an HFS+ equivalent of ".git". +func IsHFSDotGit(part string) bool { return IsHFSDot(part, "git") } + +// IsHFSDotGitmodules reports whether part is an HFS+ equivalent of +// ".gitmodules", catching attempts to plant the file via Unicode +// code points that HFS+ would strip during normalisation. +func IsHFSDotGitmodules(part string) bool { return IsHFSDot(part, "gitmodules") } diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/ntfs.go b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/ntfs.go new file mode 100644 index 000000000000..2ca6c28348b4 --- /dev/null +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/ntfs.go @@ -0,0 +1,187 @@ +package pathutil + +import "strings" + +// IsNTFSDotGit ports upstream Git's is_ntfs_dotgit. It detects path +// components that NTFS would resolve to ".git": the canonical name +// itself and its 8.3 short-name alias "git~1", each followed by any +// number of trailing spaces or periods (which NTFS silently trims) +// and an optional Alternate Data Stream suffix (":"). The +// bare strings ".git" and "git~1" also match, mirroring upstream. +// +// Reference: upstream Git path.c is_ntfs_dotgit at L1415-L1449 +// in tag v2.54.0[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/path.c#L1415-L1449 +func IsNTFSDotGit(part string) bool { + var i int + switch { + case len(part) >= 4 && part[0] == '.' && + asciiToLower(part[1]) == 'g' && + asciiToLower(part[2]) == 'i' && + asciiToLower(part[3]) == 't': + i = 4 + case len(part) >= 5 && + asciiToLower(part[0]) == 'g' && + asciiToLower(part[1]) == 'i' && + asciiToLower(part[2]) == 't' && + part[3] == '~' && part[4] == '1': + i = 5 + default: + return false + } + + for ; i < len(part); i++ { + c := part[i] + if c == ':' { + return true + } + if c != '.' && c != ' ' { + return false + } + } + return true +} + +// WindowsValidPath reports whether part is a valid Windows / NTFS +// path component for the worktree filesystem abstraction. It rejects +// NTFS-disguised variants of `.git` and `git~1` (trailing spaces, +// periods, Alternate Data Streams) and Windows reserved device +// names. Bare `.git` and `git~1` are allowed at this layer; the +// caller decides whether they are permissible at the current path +// position. +func WindowsValidPath(part string) bool { + if IsNTFSDotGit(part) && !IsDotGitName(part) { + return false + } + return !isWindowsReservedName(part) +} + +// windowsReservedNames lists the Windows reserved device names. +// A path component is reserved if its base name (ignoring trailing +// spaces, extensions, and NTFS Alternate Data Streams) matches one of +// these case-insensitively. +// +// See upstream Git compat/mingw.c is_valid_win32_path(). +var windowsReservedNames = []string{ + "CON", "PRN", "AUX", "NUL", + "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", + "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + "CONIN$", "CONOUT$", +} + +func isWindowsReservedName(part string) bool { + for _, name := range windowsReservedNames { + if len(part) < len(name) { + continue + } + if !strings.EqualFold(part[:len(name)], name) { + continue + } + // Exact match or followed by space, dot, colon (ADS), or separator. + if len(part) == len(name) { + return true + } + switch part[len(name)] { + case ' ', '.', ':': + return true + } + } + return false +} + +// IsNTFSDot ports upstream Git's is_ntfs_dot_generic. It detects NTFS +// path-component variants of a dotfile name that attackers can use to +// bypass case-insensitive comparisons against the canonical name on +// Windows. The dotgit parameter is the lowercase name without the +// leading dot (e.g. "gitmodules"); shortnamePrefix is the canonical +// 6-character NTFS short-name prefix used as a fall-back match +// (e.g. "gi7eba" for ".gitmodules"). +// +// Reference: upstream Git path.c is_ntfs_dot_generic at L1451-L1507 +// in tag v2.54.0[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/path.c#L1451-L1507 +func IsNTFSDot(name, dotgit, shortnamePrefix string) bool { + // onlySpacesAndPeriods returns true when the suffix from start + // onwards consists only of trailing spaces and periods, possibly + // terminated by a NTFS Alternate Data Stream colon. Mirrors the + // only_spaces_and_periods label in upstream's is_ntfs_dot_generic. + onlySpacesAndPeriods := func(start int) bool { + for i := start; i < len(name); i++ { + c := name[i] + if c == ':' { + return true + } + if c != ' ' && c != '.' { + return false + } + } + return true + } + + // Pattern 1: "." prefix + trailing spaces / periods / ADS. + if len(name) >= len(dotgit)+1 && name[0] == '.' && + strings.EqualFold(name[1:1+len(dotgit)], dotgit) { + if onlySpacesAndPeriods(len(dotgit) + 1) { + return true + } + } + + // Pattern 2: standard NTFS short name ~[1-4]. + if len(dotgit) >= 6 && len(name) >= 8 && + strings.EqualFold(name[:6], dotgit[:6]) && + name[6] == '~' && name[7] >= '1' && name[7] <= '4' { + if onlySpacesAndPeriods(8) { + return true + } + } + + // Pattern 3: fall-back NTFS short name keyed by shortnamePrefix. + if len(shortnamePrefix) < 6 || len(name) < 8 { + return false + } + sawTilde := false + i := 0 + for i < 8 { + c := name[i] + switch { + case sawTilde: + if c < '0' || c > '9' { + return false + } + case c == '~': + i++ + if i >= len(name) || name[i] < '1' || name[i] > '9' { + return false + } + sawTilde = true + case i >= 6: + return false + case c&0x80 != 0: + return false + default: + if asciiToLower(c) != shortnamePrefix[i] { + return false + } + } + i++ + } + return onlySpacesAndPeriods(8) +} + +// IsNTFSDotGitmodules reports whether part is an NTFS-equivalent of +// ".gitmodules" — the file name (or any of its variants that NTFS +// would resolve to it) that attackers can use to plant submodule +// configuration disguised as a symlink. The 6-character canonical +// short-name prefix "gi7eba" mirrors upstream Git's is_ntfs_dotgitmodules. +func IsNTFSDotGitmodules(part string) bool { + return IsNTFSDot(part, "gitmodules", "gi7eba") +} + +func asciiToLower(c byte) byte { + if c >= 'A' && c <= 'Z' { + return c + ('a' - 'A') + } + return c +} diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/tree.go b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/tree.go new file mode 100644 index 000000000000..e610cd4a8b1c --- /dev/null +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/pathutil/tree.go @@ -0,0 +1,66 @@ +package pathutil + +import ( + "fmt" + "path/filepath" + "strings" +) + +// ErrInvalidPath is returned by ValidTreePath when its argument is +// not a safe path to materialise into the worktree. +var ErrInvalidPath = fmt.Errorf("invalid path") + +// ValidTreePath rejects path strings that, if materialised into a +// worktree, would let an attacker-controlled tree entry escape the +// worktree or rewrite repository metadata. It rejects: +// +// - control characters (< 0x20, 0x7f); +// - empty paths and "." / ".." components; +// - Windows volume name prefixes (e.g. C:); +// - .git, its 8.3 NTFS short-name git~1, plus their HFS+ and NTFS +// variants — at every position, not just the root. +// +// HFS+/NTFS variants of `.git` are always rejected at this layer +// regardless of runtime config: tree paths are canonical UTF-8 with +// no zero-width characters or NTFS short-name forms, so an entry +// that looks like a disguised `.git` is suspicious anywhere. Windows +// reserved device names (CON, NUL, etc.) are not policed here — they +// are legitimate filenames on non-Windows filesystems and upstream +// Git accepts them. The wrapper layer (validPath in package git) +// rejects them at materialisation time when core.protectNTFS is on. +// +// Mirrors upstream Git's verify_path_internal at read-cache.c#L987 +// in tag v2.54.0[1] with protect_hfs / protect_ntfs treated as +// always-on for `.git`-disguise detection (tree paths are not +// application-supplied) and is_valid_win32_path left to the wrapper. +// +// [1]: https://github.com/git/git/blob/v2.54.0/read-cache.c#L987 +func ValidTreePath(p string) error { + for i := 0; i < len(p); i++ { + if p[i] < 0x20 || p[i] == 0x7f { + return fmt.Errorf("%w %q: contains control character", ErrInvalidPath, p) + } + } + + parts := strings.FieldsFunc(p, func(r rune) bool { return r == '\\' || r == '/' }) + if len(parts) == 0 { + return fmt.Errorf("%w: %q", ErrInvalidPath, p) + } + + // Volume names are not supported, in both formats: \\ and :. + if vol := filepath.VolumeName(p); vol != "" { + return fmt.Errorf("%w: %q", ErrInvalidPath, p) + } + + for _, part := range parts { + if part == "." || part == ".." { + return fmt.Errorf("%w %q: cannot use %q", ErrInvalidPath, p, part) + } + + if IsDotGitName(part) || IsHFSDotGit(part) || IsNTFSDotGit(part) { + return fmt.Errorf("%w component: %q", ErrInvalidPath, p) + } + } + + return nil +} diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/internal/url/url.go b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/url/url.go index 266244869374..e40947c90c50 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/internal/url/url.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/internal/url/url.go @@ -2,12 +2,14 @@ package url import ( "regexp" + "runtime" + "strings" ) var ( isSchemeRegExp = regexp.MustCompile(`^[^:]+://`) - // Ref: https://github.com/git/git/blob/master/Documentation/urls.txt#L37 + // Ref: https://github.com/git/git/blob/v2.54.0/Documentation/urls.adoc#L41-L48 scpLikeUrlRegExp = regexp.MustCompile(`^(?:(?P[^@]+)@)?(?P[^:\s]+):(?:(?P[0-9]{1,5}):)?(?P[^\\].*)$`) ) @@ -20,7 +22,38 @@ func MatchesScheme(url string) bool { // MatchesScpLike returns true if the given string matches an SCP-like // format scheme. func MatchesScpLike(url string) bool { - return scpLikeUrlRegExp.MatchString(url) + if !scpLikeUrlRegExp.MatchString(url) { + return false + } + // Mirror canonical Git's url_is_local_not_ssh in connect.c[1] for + // the cases the regex above cannot disambiguate by itself: a URL + // is treated as a local path (not SCP-style SSH) when a `/` + // precedes the first `:` (e.g. `./relative:path`, + // `/abs/with:colon/file`), or — on Windows only — when it has a + // DOS drive prefix like `C:foo` where the host is a single + // ASCII letter. + // + // [1]: https://github.com/git/git/blob/v2.54.0/connect.c#L710-L716 + if before, _, _ := strings.Cut(url, ":"); strings.Contains(before, "/") { + return false + } + if runtime.GOOS == "windows" && hasDosDrivePrefix(url) { + return false + } + return true +} + +// hasDosDrivePrefix reports whether s begins with `:` (a +// Windows drive prefix such as `C:` or `c:`). Mirrors canonical Git's +// win32_has_dos_drive_prefix[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/compat/win32/path-utils.c#L20-L29 +func hasDosDrivePrefix(s string) bool { + if len(s) < 2 || s[1] != ':' { + return false + } + c := s[0] + return ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z') } // FindScpLikeComponents returns the user, host, port and path of the diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/decoder.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/decoder.go index 9e006a72622a..825fad9a584a 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/decoder.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/decoder.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "io/fs" "github.com/go-git/go-git/v5/plumbing/hash" "github.com/go-git/go-git/v5/utils/binary" @@ -25,35 +26,88 @@ const ( objectIDLength = hash.Size ) +// Byte sizes of the idx v2 layout elements, used by the size formula +// in [validateIdxV2Size]. See [gitformat-pack] for the canonical +// layout. +// +// [gitformat-pack]: https://git-scm.com/docs/gitformat-pack +const ( + headerLen = 8 // magic + version + fanoutLen = fanout * 4 // uint32 per bucket + crc32Len = 4 // CRC32 per object + offset32Len = 4 // 32-bit offset per object + offset64Len = 8 // 64-bit overflow offset + trailerHashes = 2 // pack checksum + idx checksum, each hashsz +) + +// statInput is the optional shape the [Decoder] probes for at the +// start of [Decoder.Decode] to learn the on-disk length of the idx +// blob, which it uses to validate the canonical-Git size formula +// before any allocations driven by the fanout table. Callers that +// pass an [*os.File] or a `billy.File` backed by an `*os.File` +// (the production call sites in `storage/filesystem`) satisfy it +// directly; arbitrary [io.Reader]s do not, and decode for them +// retains the pre-existing behaviour of erroring out at the +// truncated-payload boundary instead. +// +// The interface is intentionally unexported so the public +// [NewDecoder] signature stays compatible with v5. +type statInput interface { + Stat() (fs.FileInfo, error) +} + // Decoder reads and decodes idx files from an input stream. type Decoder struct { io.Reader - h hash.Hash + src io.Reader + h hash.Hash } // NewDecoder builds a new idx stream decoder, that reads from r. func NewDecoder(r io.Reader) *Decoder { h := hash.New(crypto.SHA1) tr := io.TeeReader(r, h) - return &Decoder{tr, h} + return &Decoder{tr, r, h} } // Decode reads from the stream and decode the content into the MemoryIndex struct. func (d *Decoder) Decode(idx *MemoryIndex) error { + idxSize := int64(-1) + if in, ok := d.src.(statInput); ok { + fi, err := in.Stat() + if err != nil { + return fmt.Errorf("%w: stat input: %w", ErrMalformedIdxFile, err) + } + idxSize = fi.Size() + } + if err := validateHeader(d); err != nil { return err } - flow := []func(*MemoryIndex, io.Reader) error{ + headerFlow := []func(*MemoryIndex, io.Reader) error{ readVersion, readFanout, + } + for _, f := range headerFlow { + if err := f(idx, d); err != nil { + return err + } + } + + if idxSize >= 0 { + if err := validateIdxV2Size(idx, idxSize); err != nil { + return err + } + } + + bodyFlow := []func(*MemoryIndex, io.Reader) error{ readObjectNames, readCRC32, readOffsets, readPackChecksum, } - - for _, f := range flow { + for _, f := range bodyFlow { if err := f(idx, d); err != nil { return err } @@ -199,3 +253,103 @@ func readIdxChecksum(idx *MemoryIndex, r io.Reader) error { return nil } + +// validateIdxV2Size enforces the size formula used by canonical Git +// load_idx for idx v2 files: the on-disk length must lie within +// [minSize, maxSize] where +// +// perObject = hashsz + crc32Len + offset32Len +// minSize = headerLen + fanoutLen + trailerHashes*hashsz + nr*perObject +// maxSize = minSize + (nr-1)*offset64Len when nr > 0 +// +// with nr taken from the last fanout entry and hashsz fixed at +// [objectIDLength] (SHA-1 in v5). Multiplications use a self-checking +// overflow guard so inputs whose claimed object count overflows the +// formula are rejected rather than wrapping into a smaller value. +func validateIdxV2Size(idx *MemoryIndex, idxSize int64) error { + nr := int64(idx.Fanout[fanout-1]) + hashsz := int64(objectIDLength) + + minSize := minIdxV2Size(nr, hashsz) + maxSize := maxIdxV2Size(nr, hashsz) + if minSize < 0 || maxSize < 0 { + return fmt.Errorf("%w: object count %d is inconsistent with file size", ErrMalformedIdxFile, nr) + } + + if idxSize < minSize || idxSize > maxSize { + return fmt.Errorf("%w: file size %d is inconsistent with object count %d", ErrMalformedIdxFile, idxSize, nr) + } + return nil +} + +// minIdxV2Size returns the minimum on-disk size of an idx v2 file +// holding nr objects with the given hash size, mirroring the +// computation in canonical Git load_idx. Returns -1 when any +// intermediate multiplication or addition would overflow int64. +func minIdxV2Size(nr, hashsz int64) int64 { + perObject := hashsz + crc32Len + offset32Len + fixed := int64(headerLen+fanoutLen) + trailerHashes*hashsz + + objects, ok := mulInt64(nr, perObject) + if !ok { + return -1 + } + sum, ok := addInt64(fixed, objects) + if !ok { + return -1 + } + return sum +} + +// maxIdxV2Size returns the maximum on-disk size of an idx v2 file +// holding nr objects with the given hash size, mirroring the +// computation in canonical Git load_idx. Returns -1 on overflow. +func maxIdxV2Size(nr, hashsz int64) int64 { + minSize := minIdxV2Size(nr, hashsz) + if minSize < 0 { + return -1 + } + if nr == 0 { + return minSize + } + overflow, ok := mulInt64(nr-1, offset64Len) + if !ok { + return -1 + } + sum, ok := addInt64(minSize, overflow) + if !ok { + return -1 + } + return sum +} + +// mulInt64 returns a*b and whether the result fits in an int64 without +// overflow. Negative operands or overflow yield ok=false. The overflow +// check uses the standard self-inverse identity: a*b/b == a only when +// the multiplication did not wrap. +func mulInt64(a, b int64) (int64, bool) { + if a < 0 || b < 0 { + return 0, false + } + if a == 0 || b == 0 { + return 0, true + } + c := a * b + if c/b != a { + return 0, false + } + return c, true +} + +// addInt64 returns a+b and whether the result fits in an int64 without +// overflow. Negative operands or overflow yield ok=false. +func addInt64(a, b int64) (int64, bool) { + if a < 0 || b < 0 { + return 0, false + } + c := a + b + if c < a { + return 0, false + } + return c, true +} diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/idxfile.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/idxfile.go index 136c3e2aca65..f068c25e5736 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/idxfile.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/idxfile/idxfile.go @@ -2,6 +2,7 @@ package idxfile import ( "bytes" + "fmt" "io" "sort" "sync" @@ -126,7 +127,10 @@ func (idx *MemoryIndex) FindOffset(h plumbing.Hash) (int64, error) { return 0, plumbing.ErrObjectNotFound } - offset := idx.getOffset(k, i) + offset, err := idx.getOffset(k, i) + if err != nil { + return 0, err + } // Save the offset for reverse lookup idx.mu.Lock() @@ -141,17 +145,19 @@ func (idx *MemoryIndex) FindOffset(h plumbing.Hash) (int64, error) { const isO64Mask = uint64(1) << 31 -func (idx *MemoryIndex) getOffset(firstLevel, secondLevel int) uint64 { +func (idx *MemoryIndex) getOffset(firstLevel, secondLevel int) (uint64, error) { offset := secondLevel << 2 ofs := encbin.BigEndian.Uint32(idx.Offset32[firstLevel][offset : offset+4]) if (uint64(ofs) & isO64Mask) != 0 { offset := 8 * (uint64(ofs) & ^isO64Mask) - n := encbin.BigEndian.Uint64(idx.Offset64[offset : offset+8]) - return n + if l := uint64(len(idx.Offset64)); l < 8 || offset > l-8 { + return 0, fmt.Errorf("%w: offset64 index out of range", ErrMalformedIdxFile) + } + return encbin.BigEndian.Uint64(idx.Offset64[offset : offset+8]), nil } - return uint64(ofs) + return uint64(ofs), nil } // FindCRC32 implements the Index interface. @@ -209,8 +215,11 @@ func (idx *MemoryIndex) genOffsetHash() error { mappedFirstLevel := idx.FanoutMapping[firstLevel] for secondLevel := uint32(0); i < fanoutValue; i++ { copy(hash[:], idx.Names[mappedFirstLevel][secondLevel*objectIDLength:]) - offset := int64(idx.getOffset(mappedFirstLevel, int(secondLevel))) - offsetHash[offset] = hash + off, err := idx.getOffset(mappedFirstLevel, int(secondLevel)) + if err != nil { + return err + } + offsetHash[int64(off)] = hash secondLevel++ } } @@ -291,7 +300,11 @@ func (i *idxfileEntryIter) Next() (*Entry, error) { mappedFirstLevel := i.idx.FanoutMapping[i.firstLevel] entry := new(Entry) copy(entry.Hash[:], i.idx.Names[mappedFirstLevel][i.secondLevel*objectIDLength:]) - entry.Offset = i.idx.getOffset(mappedFirstLevel, i.secondLevel) + var err error + entry.Offset, err = i.idx.getOffset(mappedFirstLevel, i.secondLevel) + if err != nil { + return nil, err + } entry.CRC32 = i.idx.getCRC32(mappedFirstLevel, i.secondLevel) i.secondLevel++ diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/objfile/reader.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/objfile/reader.go index 621883a67dff..f9842ed9afc6 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/objfile/reader.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/objfile/reader.go @@ -11,9 +11,10 @@ import ( ) var ( - ErrClosed = errors.New("objfile: already closed") - ErrHeader = errors.New("objfile: invalid header") - ErrNegativeSize = errors.New("objfile: negative object size") + ErrClosed = errors.New("objfile: already closed") + ErrHeader = errors.New("objfile: invalid header") + ErrHeaderNotRead = errors.New("objfile: Header must be called before Read") + ErrNegativeSize = errors.New("objfile: negative object size") ) // Reader reads and decodes compressed objfile data from a provided io.Reader. @@ -100,12 +101,23 @@ func (r *Reader) prepareForRead(t plumbing.ObjectType, size int64) { // // If Read encounters the end of the data stream it will return err == io.EOF, // either in the current call if n > 0 or in a subsequent call. +// +// Read returns ErrHeaderNotRead if Header has not been called successfully. func (r *Reader) Read(p []byte) (n int, err error) { + if r.multi == nil { + return 0, ErrHeaderNotRead + } return r.multi.Read(p) } // Hash returns the hash of the object data stream that has been read so far. +// It returns the zero plumbing.Hash if Header has not been called +// successfully — guarding against the nil hasher that prepareForRead has +// not yet allocated. func (r *Reader) Hash() plumbing.Hash { + if r.multi == nil { + return plumbing.ZeroHash + } return r.hasher.Sum() } diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/diff_delta.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/diff_delta.go index 8898e5830e47..a24b63b41641 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/diff_delta.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/diff_delta.go @@ -19,9 +19,6 @@ const ( // https://github.com/git/git/blob/f7466e94375b3be27f229c78873f0acf8301c0a5/diff-delta.c#L428 // Max size of a copy operation (64KB). maxCopySize = 64 * 1024 - - // Min size of a copy operation. - minCopySize = 4 ) // GetDelta returns an EncodedObject of type OFSDeltaObject. Base and Target object, diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/fsobject.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/fsobject.go index 238339daf890..93a6fafca66d 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/fsobject.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/fsobject.go @@ -78,7 +78,13 @@ func (o *FSObject) Reader() (io.ReadCloser, error) { _ = f.Close() return nil, err } - return ioutil.NewReadCloserWithCloser(r, f.Close), nil + // Cap the lazy stream at the resolved object size: well-formed + // content reaches EOF inside the bound, an inflated stream that + // runs past surfaces ErrInflatedSizeMismatch on the byte just + // past the limit. For delta-resolved objects o.size is the + // expanded size, which is what the caller is reading here. + bounded := newBoundedReadCloser(r, o.size) + return ioutil.NewReadCloserWithCloser(bounded, f.Close), nil } r, err := p.getObjectContent(o.offset) if err != nil { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/packfile.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/packfile.go index 68527022578f..f7fb958f9f2b 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/packfile.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/packfile.go @@ -126,11 +126,17 @@ func (p *Packfile) nextObjectHeader() (*ObjectHeader, error) { return h, err } -func (p *Packfile) getDeltaObjectSize(buf *bytes.Buffer) int64 { +func (p *Packfile) getDeltaObjectSize(buf *bytes.Buffer) (int64, error) { delta := buf.Bytes() - _, delta = decodeLEB128(delta) // skip src size - sz, _ := decodeLEB128(delta) - return int64(sz) + _, delta, err := decodeLEB128(delta) // skip src size + if err != nil { + return 0, err + } + sz, _, err := decodeLEB128(delta) + if err != nil { + return 0, err + } + return int64(sz), nil } func (p *Packfile) getObjectSize(h *ObjectHeader) (int64, error) { @@ -145,7 +151,7 @@ func (p *Packfile) getObjectSize(h *ObjectHeader) (int64, error) { return 0, err } - return p.getDeltaObjectSize(buf), nil + return p.getDeltaObjectSize(buf) default: return 0, ErrInvalidObject.AddDetails("type %q", h.Type) } @@ -233,7 +239,10 @@ func (p *Packfile) getNextObject(h *ObjectHeader, hash plumbing.Hash) (plumbing. return nil, err } - size = p.getDeltaObjectSize(buf) + size, err = p.getDeltaObjectSize(buf) + if err != nil { + return nil, err + } if size <= smallObjectThreshold { var obj = new(plumbing.MemoryObject) obj.SetSize(size) diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/parser.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/parser.go index 2659c27e5f7a..7774d2dc1b44 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/parser.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/parser.go @@ -26,6 +26,45 @@ var ( ErrDeltaNotCached = errors.New("delta could not be found in cache") ) +// maxObjectPreallocBytes caps the up-front size hint passed to +// bytes.Buffer.Grow when staging an object's contents, so a malformed length +// cannot trigger a huge or out-of-range allocation. The buffer still grows +// dynamically as data is written; this is purely a hint cap. +const maxObjectPreallocBytes = 1 << 30 // 1 GiB + +// maxObjectsPrealloc caps the up-front capacity reserved from the pack's +// declared object count, so a header advertising an absurd quantity cannot +// trigger a multi-gigabyte allocation. The slice and maps still grow +// organically beyond this hint. +const maxObjectsPrealloc = 1 << 16 // 64 Ki entries + +// Match upstream Git's pack depth ceiling: pack-objects.h OE_DEPTH_BITS, +// enforced in builtin/pack-objects.c as (1 << OE_DEPTH_BITS) - 1. +const maxDeltaChainDepth = 4095 + +// growHint returns a non-negative int64 size, clamped to a sane upper bound, +// suitable for passing to bytes.Buffer.Grow. +func growHint(n int64) int { + switch { + case n <= 0: + return 0 + case n > maxObjectPreallocBytes: + return maxObjectPreallocBytes + default: + return int(n) + } +} + +// objectsHint returns a non-negative count, clamped to maxObjectsPrealloc, +// suitable for passing to make() as the capacity hint for slices or maps +// sized from a pack's declared object count. +func objectsHint(n uint32) int { + if n > maxObjectsPrealloc { + return maxObjectsPrealloc + } + return int(n) +} + // Observer interface is implemented by index encoders. type Observer interface { // OnHeader is called when a new packfile is opened. @@ -166,9 +205,10 @@ func (p *Parser) init() error { } p.count = c - p.oiByHash = make(map[plumbing.Hash]*objectInfo, p.count) - p.oiByOffset = make(map[int64]*objectInfo, p.count) - p.oi = make([]*objectInfo, p.count) + hint := objectsHint(p.count) + p.oiByHash = make(map[plumbing.Hash]*objectInfo, hint) + p.oiByOffset = make(map[int64]*objectInfo, hint) + p.oi = make([]*objectInfo, 0, hint) return nil } @@ -261,7 +301,7 @@ func (p *Parser) indexObjects() error { } if delta && !p.scanner.IsSeekable { buf.Reset() - buf.Grow(int(oh.Length)) + buf.Grow(growHint(oh.Length)) writers = append(writers, buf) } @@ -306,7 +346,7 @@ func (p *Parser) indexObjects() error { } p.oiByOffset[oh.Offset] = ota - p.oi[i] = ota + p.oi = append(p.oi, ota) } return nil @@ -317,8 +357,12 @@ func (p *Parser) resolveDeltas() error { defer sync.PutBytesBuffer(buf) for _, obj := range p.oi { + if err := checkDeltaChainDepth(obj); err != nil { + return err + } + buf.Reset() - buf.Grow(int(obj.Length)) + buf.Grow(growHint(obj.Length)) err := p.get(obj, buf) if err != nil { return err @@ -337,6 +381,9 @@ func (p *Parser) resolveDeltas() error { // create it once and reuse across all children. r := bytes.NewReader(buf.Bytes()) for _, child := range obj.Children { + if err := checkDeltaChainDepth(child); err != nil { + return err + } // Even though we are discarding the output, we still need to read it to // so that the scanner can advance to the next object, and the SHA1 can be // calculated. @@ -356,6 +403,17 @@ func (p *Parser) resolveDeltas() error { return nil } +func checkDeltaChainDepth(o *objectInfo) error { + var depth int + for current := o; current != nil && current.DiskType.IsDelta(); current = current.Parent { + depth++ + if depth > maxDeltaChainDepth { + return fmt.Errorf("%w: delta chain depth exceeds %d", ErrMalformedPackFile, maxDeltaChainDepth) + } + } + return nil +} + func (p *Parser) resolveExternalRef(o *objectInfo) { if ref, ok := p.oiByHash[o.SHA1]; ok && ref.ExternalRef { p.oiByHash[o.SHA1] = o @@ -405,7 +463,7 @@ func (p *Parser) get(o *objectInfo, buf *bytes.Buffer) (err error) { if o.DiskType.IsDelta() { b := sync.GetBytesBuffer() defer sync.PutBytesBuffer(b) - buf.Grow(int(o.Length)) + buf.Grow(growHint(o.Length)) err := p.get(o.Parent, b) if err != nil { return err diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/patch_delta.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/patch_delta.go index a9c6b9b56f2a..4bcb491141de 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/patch_delta.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/patch_delta.go @@ -31,10 +31,15 @@ const ( // premptively made available for a patch operation. maxPatchPreemptionSize uint = 65536 - // minDeltaSize defines the smallest size for a delta. - minDeltaSize = 4 + // minDeltaSize is the smallest valid delta: a 1-byte srcSz LEB128 + // header followed by a 1-byte targetSz LEB128 header (the + // shortest case being targetSz=0 with no operations). + minDeltaSize = 2 ) +// uintBits is the bit width of uint on the current platform (32 or 64). +const uintBits = 32 << (^uint(0) >> 63) + type offset struct { mask byte shift uint @@ -142,7 +147,7 @@ func ReaderFromDelta(base plumbing.EncodedObject, deltaRC io.Reader) (io.ReadClo baseBuf := bufio.NewReader(baseRd) basePos := uint(0) - for { + for remainingTargetSz > 0 { cmd, err := deltaBuf.ReadByte() if err == io.EOF { _ = dstWr.CloseWithError(ErrInvalidDelta) @@ -166,9 +171,9 @@ func ReaderFromDelta(base plumbing.EncodedObject, deltaRC io.Reader) (io.ReadClo return } - if invalidSize(sz, targetSz) || + if invalidSize(sz, remainingTargetSz) || invalidOffsetSize(offset, sz, srcSz) { - _ = dstWr.Close() + _ = dstWr.CloseWithError(ErrInvalidDelta) return } @@ -210,7 +215,7 @@ func ReaderFromDelta(base plumbing.EncodedObject, deltaRC io.Reader) (io.ReadClo case isCopyFromDelta(cmd): sz := uint(cmd) // cmd is the size itself - if invalidSize(sz, targetSz) { + if invalidSize(sz, remainingTargetSz) { _ = dstWr.CloseWithError(ErrInvalidDelta) return } @@ -225,40 +230,48 @@ func ReaderFromDelta(base plumbing.EncodedObject, deltaRC io.Reader) (io.ReadClo _ = dstWr.CloseWithError(ErrDeltaCmd) return } + } - if remainingTargetSz <= 0 { - _ = dstWr.Close() - return - } + // Mirror upstream's `data != top` post-loop check: every byte + // of the delta payload must be consumed. + if _, err := deltaBuf.ReadByte(); err == nil { + _ = dstWr.CloseWithError(ErrInvalidDelta) + return + } else if err != io.EOF { + _ = dstWr.CloseWithError(err) + return } + + _ = dstWr.Close() }() return dstRd, nil } func patchDelta(dst *bytes.Buffer, src, delta []byte) error { - if len(delta) < minCopySize { - return ErrInvalidDelta + srcSz, delta, err := decodeLEB128(delta) + if err != nil { + return fmt.Errorf("%w: %w", ErrInvalidDelta, err) } - - srcSz, delta := decodeLEB128(delta) if srcSz != uint(len(src)) { return ErrInvalidDelta } - targetSz, delta := decodeLEB128(delta) + targetSz, delta, err := decodeLEB128(delta) + if err != nil { + return fmt.Errorf("%w: %w", ErrInvalidDelta, err) + } remainingTargetSz := targetSz - var cmd byte - growSz := min(targetSz, maxPatchPreemptionSize) dst.Grow(int(growSz)) - for { + + for remainingTargetSz > 0 { if len(delta) == 0 { return ErrInvalidDelta } - cmd = delta[0] + cmd := delta[0] delta = delta[1:] switch { @@ -275,16 +288,16 @@ func patchDelta(dst *bytes.Buffer, src, delta []byte) error { return err } - if invalidSize(sz, targetSz) || + if invalidSize(sz, remainingTargetSz) || invalidOffsetSize(offset, sz, srcSz) { - break + return ErrInvalidDelta } dst.Write(src[offset : offset+sz]) remainingTargetSz -= sz case isCopyFromDelta(cmd): sz := uint(cmd) // cmd is the size itself - if invalidSize(sz, targetSz) { + if invalidSize(sz, remainingTargetSz) { return ErrInvalidDelta } @@ -299,10 +312,12 @@ func patchDelta(dst *bytes.Buffer, src, delta []byte) error { default: return ErrDeltaCmd } + } - if remainingTargetSz <= 0 { - break - } + // Mirror upstream's `data != top` post-loop check: every byte of + // the delta payload must be consumed. + if len(delta) != 0 { + return ErrInvalidDelta } return nil @@ -354,7 +369,7 @@ func patchDeltaWriter(dst io.Writer, base io.ReaderAt, delta io.Reader, baselr := io.LimitReader(sr, 0).(*io.LimitedReader) deltalr := io.LimitReader(deltaBuf, 0).(*io.LimitedReader) - for { + for remainingTargetSz > 0 { buf := *bufp cmd, err := deltaBuf.ReadByte() if err == io.EOF { @@ -374,9 +389,9 @@ func patchDeltaWriter(dst io.Writer, base io.ReaderAt, delta io.Reader, return 0, plumbing.ZeroHash, err } - if invalidSize(sz, targetSz) || + if invalidSize(sz, remainingTargetSz) || invalidOffsetSize(offset, sz, srcSz) { - return 0, plumbing.ZeroHash, err + return 0, plumbing.ZeroHash, ErrInvalidDelta } if _, err := sr.Seek(int64(offset), io.SeekStart); err != nil { @@ -389,7 +404,7 @@ func patchDeltaWriter(dst io.Writer, base io.ReaderAt, delta io.Reader, remainingTargetSz -= sz } else if isCopyFromDelta(cmd) { sz := uint(cmd) // cmd is the size itself - if invalidSize(sz, targetSz) { + if invalidSize(sz, remainingTargetSz) { return 0, plumbing.ZeroHash, ErrInvalidDelta } deltalr.N = int64(sz) @@ -399,30 +414,41 @@ func patchDeltaWriter(dst io.Writer, base io.ReaderAt, delta io.Reader, remainingTargetSz -= sz } else { - return 0, plumbing.ZeroHash, err - } - if remainingTargetSz <= 0 { - break + return 0, plumbing.ZeroHash, ErrDeltaCmd } } + // Mirror upstream's `data != top` post-loop check: every byte of + // the delta payload must be consumed. + if _, err := deltaBuf.ReadByte(); err == nil { + return 0, plumbing.ZeroHash, ErrInvalidDelta + } else if err != io.EOF { + return 0, plumbing.ZeroHash, err + } + return targetSz, hasher.Sum(), nil } // Decodes a number encoded as an unsigned LEB128 at the start of some -// binary data and returns the decoded number and the rest of the -// stream. +// binary data and returns the decoded number, the rest of the stream, +// and an error if the encoded value does not fit in a uint. // // This must be called twice on the delta data buffer, first to get the // expected source buffer size, and again to get the target buffer size. -func decodeLEB128(input []byte) (uint, []byte) { +func decodeLEB128(input []byte) (uint, []byte, error) { if len(input) == 0 { - return 0, input + return 0, input, nil } var num, sz uint var b byte for { + // A continuation byte at shift > uintBits-7 cannot contribute + // without overflowing the accumulator. + if sz*7 > uintBits-7 { + return 0, input, ErrLengthOverflow + } + b = input[sz] num |= (uint(b) & payload) << (sz * 7) // concats 7 bits chunks sz++ @@ -432,12 +458,16 @@ func decodeLEB128(input []byte) (uint, []byte) { } } - return num, input[sz:] + return num, input[sz:], nil } func decodeLEB128ByteReader(input io.ByteReader) (uint, error) { var num, sz uint for { + if sz*7 > uintBits-7 { + return 0, ErrLengthOverflow + } + b, err := input.ReadByte() if err != nil { return 0, err @@ -529,8 +559,9 @@ func decodeSize(cmd byte, delta []byte) (uint, []byte, error) { return sz, delta, nil } -func invalidSize(sz, targetSz uint) bool { - return sz > targetSz +// invalidSize reports whether sz exceeds the remaining target size. +func invalidSize(sz, remaining uint) bool { + return sz > remaining } func invalidOffsetSize(offset, sz, srcSz uint) bool { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/scanner.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/scanner.go index 8318aae40dcf..6d2907ecd3d3 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/scanner.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/format/packfile/scanner.go @@ -29,8 +29,100 @@ var ( ErrSeekNotSupported = NewError("not seek support") // ErrMalformedPackFile is returned by the parser when the pack file is corrupted. ErrMalformedPackFile = errors.New("malformed PACK file") + // ErrLengthOverflow is returned when a variable-length integer would not + // fit into its accumulator because the input declares more continuation + // bytes than the type can hold. + ErrLengthOverflow = errors.New("variable-length integer overflow") + // ErrInflatedSizeMismatch is returned when a packfile object inflates to + // more bytes than the size declared in its object header. A well-formed + // packfile never produces more data than the declared size; exceeding it + // indicates a structurally invalid entry. + ErrInflatedSizeMismatch = errors.New("packfile: inflated object exceeds declared size") ) +// boundedWriter passes writes through to w up to limit bytes total, then +// returns ErrInflatedSizeMismatch. It is used to enforce that a packfile +// object's inflated length does not exceed the size declared in its header. +type boundedWriter struct { + w io.Writer + limit int64 + n int64 +} + +// Write forwards p to the underlying writer while keeping the running total +// at or below limit. On overrun it forwards the legal prefix and reports +// the number of bytes actually consumed alongside ErrInflatedSizeMismatch, +// matching the contract in io.Writer. A write error from the underlying +// writer during overrun-handling is joined with ErrInflatedSizeMismatch so +// it is not silently dropped. +func (b *boundedWriter) Write(p []byte) (int, error) { + if b.n+int64(len(p)) > b.limit { + remain := int(b.limit - b.n) + err := error(ErrInflatedSizeMismatch) + if remain > 0 { + n, werr := b.w.Write(p[:remain]) + b.n += int64(n) + if werr != nil { + err = errors.Join(ErrInflatedSizeMismatch, werr) + } + return n, err + } + return 0, err + } + n, err := b.w.Write(p) + b.n += int64(n) + return n, err +} + +// boundedReadCloser wraps a ReadCloser and reports ErrInflatedSizeMismatch +// once more than limit bytes have been read. It is used by the on-demand +// object reader returned from FSObject.Reader so that a lazy Read of a +// packfile object cannot stream past its declared inflated size. +// +// The implementation builds on io.LimitedReader with the standard +// overrun-detection trick: request limit+1 bytes from the underlying so +// that the moment the sentinel byte materializes (LimitedReader.N drops +// to zero) we know the source produced more than limit bytes. +type boundedReadCloser struct { + lr io.LimitedReader + closer io.Closer + overrun bool +} + +// newBoundedReadCloser wraps rc so that the cumulative bytes returned from +// Read never exceed limit. The first call that would have returned a byte +// past limit instead returns ErrInflatedSizeMismatch; subsequent calls +// keep returning the same error. A negative limit is treated as zero, so +// the first byte produced by rc surfaces ErrInflatedSizeMismatch. +func newBoundedReadCloser(rc io.ReadCloser, limit int64) *boundedReadCloser { + if limit < 0 { + limit = 0 + } + return &boundedReadCloser{ + lr: io.LimitedReader{R: rc, N: limit + 1}, + closer: rc, + } +} + +// Read forwards Read up to the configured byte limit. When the underlying +// stream produces the limit+1 sentinel byte, the legal prefix is returned +// alongside ErrInflatedSizeMismatch; on subsequent calls only the error +// is returned. +func (b *boundedReadCloser) Read(p []byte) (int, error) { + if b.overrun { + return 0, ErrInflatedSizeMismatch + } + n, err := b.lr.Read(p) + if b.lr.N == 0 { + b.overrun = true + return n - 1, ErrInflatedSizeMismatch + } + return n, err +} + +// Close closes the underlying ReadCloser. +func (b *boundedReadCloser) Close() error { return b.closer.Close() } + // ObjectHeader contains the information related to the object, this information // is collected from the previous bytes to the content of the object. type ObjectHeader struct { @@ -220,6 +312,13 @@ func (s *Scanner) nextObjectHeader() (*ObjectHeader, error) { return nil, err } + // An OFS-delta references a base object that appears earlier + // in the pack; the negative offset must be strictly positive + // and not larger than the current object's offset. + if no <= 0 || no > h.Offset { + return nil, fmt.Errorf("%w: invalid OFS delta offset", ErrMalformedPackFile) + } + h.OffsetReference = h.Offset - no case plumbing.REFDeltaObject: var err error @@ -303,6 +402,13 @@ func (s *Scanner) readLength(first byte) (int64, error) { shift := firstLengthBits var err error for c&maskContinue > 0 { + // Mirrors unpack_object_header_buffer in canonical Git's + // packfile.c: a continuation byte at shift > 64-7 cannot + // contribute without overflowing an int64. + if shift > 64-lengthBits { + return 0, fmt.Errorf("%w: %w", ErrMalformedPackFile, ErrLengthOverflow) + } + if c, err = s.r.ReadByte(); err != nil { return 0, err } @@ -315,10 +421,18 @@ func (s *Scanner) readLength(first byte) (int64, error) { } // NextObject writes the content of the next object into the reader, returns -// the number of bytes written, the CRC32 of the content and an error, if any +// the number of bytes written, the CRC32 of the content and an error, if any. +// +// When a prior NextObjectHeader has stashed the object header in +// pendingObject, the inflated stream is bounded by the header's declared +// length and surfaces ErrInflatedSizeMismatch on overrun. func (s *Scanner) NextObject(w io.Writer) (written int64, crc32 uint32, err error) { + declaredSize := int64(-1) + if s.pendingObject != nil { + declaredSize = s.pendingObject.Length + } s.pendingObject = nil - written, err = s.copyObject(w) + written, err = s.copyObject(w, declaredSize) s.r.Flush() crc32 = s.crc.Sum32() @@ -327,23 +441,39 @@ func (s *Scanner) NextObject(w io.Writer) (written int64, crc32 uint32, err erro return } -// ReadObject returns a reader for the object content and an error +// ReadObject returns a reader for the object content and an error. +// +// When a prior NextObjectHeader has stashed the object header in +// pendingObject, the returned reader is bounded by the header's declared +// length so callers cannot stream past the declared inflated size; an +// overrun surfaces ErrInflatedSizeMismatch on the byte just past the +// limit. func (s *Scanner) ReadObject() (io.ReadCloser, error) { + declaredSize := int64(-1) + if s.pendingObject != nil { + declaredSize = s.pendingObject.Length + } s.pendingObject = nil zr, err := sync.GetZlibReader(s.r) if err != nil { return nil, fmt.Errorf("zlib reset error: %s", err) } - return ioutil.NewReadCloserWithCloser(zr.Reader, func() error { + rc := ioutil.NewReadCloserWithCloser(zr.Reader, func() error { sync.PutZlibReader(zr) return nil - }), nil + }) + if declaredSize >= 0 { + return newBoundedReadCloser(rc, declaredSize), nil + } + return rc, nil } -// ReadRegularObject reads and write a non-deltified object -// from it zlib stream in an object entry in the packfile. -func (s *Scanner) copyObject(w io.Writer) (n int64, err error) { +// copyObject inflates a non-deltified object's zlib stream into w. When +// declaredSize is non-negative, the write sink is wrapped in a +// boundedWriter so an overrun surfaces ErrInflatedSizeMismatch instead +// of being silently appended. +func (s *Scanner) copyObject(w io.Writer, declaredSize int64) (n int64, err error) { zr, err := sync.GetZlibReader(s.r) defer sync.PutZlibReader(zr) @@ -352,8 +482,14 @@ func (s *Scanner) copyObject(w io.Writer) (n int64, err error) { } defer ioutil.CheckClose(zr.Reader, &err) + + sink := w + if declaredSize >= 0 { + sink = &boundedWriter{w: w, limit: declaredSize} + } + buf := sync.GetByteSlice() - n, err = io.CopyBuffer(w, zr.Reader, *buf) + n, err = io.CopyBuffer(sink, zr.Reader, *buf) sync.PutByteSlice(buf) return } diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/object/tree.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/object/tree.go index d0d0036de64c..3c004f5fdbdf 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/object/tree.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/object/tree.go @@ -10,6 +10,7 @@ import ( "sort" "strings" + "github.com/go-git/go-git/v5/internal/pathutil" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/storer" @@ -118,7 +119,16 @@ func (t *Tree) Tree(path string) (*Tree, error) { } // TreeEntryFile returns the *File for a given *TreeEntry. +// +// The entry's name is validated against pathutil.ValidTreePath for +// the same reason FindEntry validates: TreeEntryFile is a boundary +// where attacker-controlled tree data leaves the trusted store as a +// *File whose Name a caller can hand to filesystem ops. func (t *Tree) TreeEntryFile(e *TreeEntry) (*File, error) { + if err := pathutil.ValidTreePath(e.Name); err != nil { + return nil, err + } + blob, err := GetBlob(t.s, e.Hash) if err != nil { return nil, err @@ -128,7 +138,16 @@ func (t *Tree) TreeEntryFile(e *TreeEntry) (*File, error) { } // FindEntry search a TreeEntry in this tree or any subtree. +// +// The lookup path is validated against pathutil.ValidTreePath to +// prevent attacker-controlled tree contents from leaking past this +// boundary as `.git`-shaped or path-traversal-shaped names. Callers +// that legitimately need to look up unsafe paths should walk the +// tree manually. func (t *Tree) FindEntry(path string) (*TreeEntry, error) { + if err := pathutil.ValidTreePath(path); err != nil { + return nil, err + } if t.t == nil { t.t = make(map[string]*Tree) } @@ -517,6 +536,10 @@ func (w *TreeWalker) Next() (name string, entry TreeEntry, err error) { continue } + if err := pathutil.ValidTreePath(entry.Name); err != nil { + return name, entry, err + } + if entry.Mode == filemode.Dir { obj, err = GetTree(w.s, entry.Hash) } diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/transport/ssh/common.go b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/transport/ssh/common.go index ae6f2174a0c7..647955b2d387 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/transport/ssh/common.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/plumbing/transport/ssh/common.go @@ -252,7 +252,39 @@ func (c *command) setAuthFromEndpoint() error { } func endpointToCommand(cmd string, ep *transport.Endpoint) string { - return fmt.Sprintf("%s '%s'", cmd, ep.Path) + var b strings.Builder + b.WriteString(cmd) + b.WriteByte(' ') + writeShellQuote(&b, ep.Path) + return b.String() +} + +// writeShellQuote writes s to b, wrapped in single quotes with +// embedded single quotes and exclamation marks escaped using the +// POSIX close-escape-reopen idiom: +// +// ' becomes '\'' +// ! becomes '\!' +// +// It is a direct port of canonical Git's sq_quote_buf (quote.c). +// The bang escape keeps the result safe when re-evaluated under +// csh-derived shells that perform history expansion. The output is +// safe to pass as a single argument through any POSIX shell and +// round-trips through git-shell's sq_dequote_to_argv. +func writeShellQuote(b *strings.Builder, s string) { + b.Grow(len(s) + 2) + b.WriteByte('\'') + for i := 0; i < len(s); i++ { + c := s[i] + if c == '\'' || c == '!' { + b.WriteString(`'\`) + b.WriteByte(c) + b.WriteByte('\'') + continue + } + b.WriteByte(c) + } + b.WriteByte('\'') } func overrideConfig(overrides *ssh.ClientConfig, c *ssh.ClientConfig) { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/repository.go b/hack/tools/vendor/github.com/go-git/go-git/v5/repository.go index e0cefc491276..12af1623909c 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/repository.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/repository.go @@ -1530,7 +1530,18 @@ func (r *Repository) Worktree() (*Worktree, error) { return nil, ErrIsBareRepository } - return &Worktree{r: r, Filesystem: r.wt}, nil + protectNTFS := defaultProtectNTFS() + protectHFS := defaultProtectHFS() + if cfg, err := r.Config(); err == nil { + if cfg.Core.ProtectNTFS.IsSet() { + protectNTFS = cfg.Core.ProtectNTFS.IsTrue() + } + if cfg.Core.ProtectHFS.IsSet() { + protectHFS = cfg.Core.ProtectHFS.IsTrue() + } + } + + return &Worktree{r: r, Filesystem: newWorktreeFilesystem(r.wt, protectNTFS, protectHFS)}, nil } func expand_ref(s storer.ReferenceStorer, ref plumbing.ReferenceName) (*plumbing.Reference, error) { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/storage/filesystem/dotgit/dotgit.go b/hack/tools/vendor/github.com/go-git/go-git/v5/storage/filesystem/dotgit/dotgit.go index 72c9ccfc142d..eb85a11454e7 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/storage/filesystem/dotgit/dotgit.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/storage/filesystem/dotgit/dotgit.go @@ -75,6 +75,10 @@ var ( // ErrEmptyRefFile is returned when a reference file is attempted to be read, // but the file is empty ErrEmptyRefFile = errors.New("ref file is empty") + // ErrModuleNameEscape is returned when a submodule name would + // resolve outside the modules/ subtree, mirroring canonical Git's + // "ignoring suspicious submodule name" defence. + ErrModuleNameEscape = errors.New("submodule name escapes modules/ directory") ) // Options holds configuration for the storage. @@ -1127,9 +1131,20 @@ func (d *DotGit) PackRefs() (err error) { return nil } -// Module return a billy.Filesystem pointing to the module folder +// Module returns a billy.Filesystem pointing to the module folder. +// +// As a defence in depth against submodule name path traversal, +// refuse names whose joined path leaves the modules/ subtree once +// cleaned. The config-layer parser also validates submodule names, +// but Module may be reached from any caller that constructs a +// Submodule struct programmatically and so bypasses the parser. func (d *DotGit) Module(name string) (billy.Filesystem, error) { - return d.fs.Chroot(d.fs.Join(modulePath, name)) + p := d.fs.Join(modulePath, name) + cleaned := path.Clean(filepath.ToSlash(p)) + if cleaned != modulePath && !strings.HasPrefix(cleaned, modulePath+"/") { + return nil, ErrModuleNameEscape + } + return d.fs.Chroot(p) } func (d *DotGit) AddAlternate(remote string) error { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/submodule.go b/hack/tools/vendor/github.com/go-git/go-git/v5/submodule.go index afabb6acad4a..2fe4ca2d27e3 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/submodule.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/submodule.go @@ -6,9 +6,12 @@ import ( "errors" "fmt" "path" + "path/filepath" "github.com/go-git/go-billy/v5" "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/internal/pathutil" + giturl "github.com/go-git/go-git/v5/internal/url" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/format/index" "github.com/go-git/go-git/v5/plumbing/transport" @@ -119,6 +122,16 @@ func (s *Submodule) Repository() (*Repository, error) { exists = true } + // s.c.Path is sourced from the worktree's .gitmodules and is + // therefore tree-controlled. Apply the strict tree-path validator + // before chroot — the wrapper's tolerant validPath would let a + // final-position .git component through (e.g. "submodule/.git"), + // which a malicious .gitmodules could use to chroot the submodule + // worktree into the repository's actual .git directory. + if err := pathutil.ValidTreePath(s.c.Path); err != nil { + return nil, err + } + var worktree billy.Filesystem if worktree, err = s.w.Filesystem.Chroot(s.c.Path); err != nil { return nil, err @@ -138,18 +151,25 @@ func (s *Submodule) Repository() (*Repository, error) { return nil, err } - if !path.IsAbs(moduleEndpoint.Path) && moduleEndpoint.Protocol == "file" { - remotes, err := s.w.r.Remotes() + // A relative submodule URL such as "../X.git" must resolve against + // the parent repository's remote URL, not against the process CWD. + // Detect relativity from the raw configured URL because + // transport.NewEndpoint normalizes local paths to absolute form via + // filepath.Abs, which would otherwise mask the relative form here. + if giturl.IsLocalEndpoint(s.c.URL) && + !path.IsAbs(s.c.URL) && !filepath.IsAbs(s.c.URL) { + + base, err := defaultRemote(s.w.r) if err != nil { - return nil, err + return nil, fmt.Errorf("resolving relative submodule URL: %w", err) } - rootEndpoint, err := transport.NewEndpoint(remotes[0].c.URLs[0]) + rootEndpoint, err := transport.NewEndpoint(base.URLs[0]) if err != nil { return nil, err } - rootEndpoint.Path = path.Join(rootEndpoint.Path, moduleEndpoint.Path) + rootEndpoint.Path = path.Join(rootEndpoint.Path, s.c.URL) *moduleEndpoint = *rootEndpoint } @@ -161,6 +181,52 @@ func (s *Submodule) Repository() (*Repository, error) { return r, err } +// defaultRemote returns the remote that relative submodule URLs are +// resolved against, mirroring canonical Git's repo_default_remote +// (remote.c) and resolve_relative_url (builtin/submodule--helper.c): +// +// 1. if HEAD is on a branch with branch..remote configured, +// use that remote; +// 2. else if exactly one remote is configured, use it; +// 3. otherwise fall back to DefaultRemoteName ("origin"). +// +// Each rule falls through unconditionally: a branch lookup that +// finds the branch but with an empty Remote does not short-circuit +// rule (2). Returns an error when the chosen remote is not configured. +func defaultRemote(r *Repository) (*config.RemoteConfig, error) { + cfg, err := r.Config() + if err != nil { + return nil, err + } + + if ref, err := r.Reference(plumbing.HEAD, false); err == nil && + ref.Type() == plumbing.SymbolicReference && + ref.Target().IsBranch() { + if b, ok := cfg.Branches[ref.Target().Short()]; ok && b.Remote != "" { + return lookupRemote(cfg, b.Remote) + } + } + + if len(cfg.Remotes) == 1 { + for name := range cfg.Remotes { + return lookupRemote(cfg, name) + } + } + + return lookupRemote(cfg, DefaultRemoteName) +} + +func lookupRemote(cfg *config.Config, name string) (*config.RemoteConfig, error) { + rc, ok := cfg.Remotes[name] + if !ok { + return nil, fmt.Errorf("remote %q not found", name) + } + if len(rc.URLs) == 0 { + return nil, fmt.Errorf("remote %q has no configured URL", name) + } + return rc, nil +} + // Update the registered submodule to match what the superproject expects, the // submodule should be initialized first calling the Init method or setting in // the options SubmoduleUpdateOptions.Init equals true diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/utils/binary/read.go b/hack/tools/vendor/github.com/go-git/go-git/v5/utils/binary/read.go index b8f9df1a2444..71d9ad607b1d 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/utils/binary/read.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/utils/binary/read.go @@ -5,11 +5,18 @@ package binary import ( "bufio" "encoding/binary" + "errors" "io" + "math" "github.com/go-git/go-git/v5/plumbing" ) +// ErrIntegerOverflow is returned when a Git-format variable-width integer +// would not fit into an int64 because the input declares more continuation +// bytes than the type can hold. +var ErrIntegerOverflow = errors.New("variable-width integer overflow") + // Read reads structured binary data from r into data. Bytes are read and // decoded in BigEndian order // https://golang.org/pkg/encoding/binary/#Read @@ -92,6 +99,14 @@ func ReadVariableWidthInt(r io.Reader) (int64, error) { var v = int64(c & maskLength) for c&maskContinue > 0 { + // Reject input that, after the v++ and shift below, would + // not fit in an int64. With v < (MaxInt64-127)>>7, the + // post-increment v is at most (MaxInt64-127)>>7 and the + // final (v << 7) + (c & 0x7F) stays within int64. + if v >= (math.MaxInt64-int64(maskLength))>>lengthBits { + return 0, ErrIntegerOverflow + } + v++ if err := Read(r, &c); err != nil { return 0, err diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/worktree.go b/hack/tools/vendor/github.com/go-git/go-git/v5/worktree.go index 55d7ebb1b61c..d8ee9fdd138f 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/worktree.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/worktree.go @@ -7,7 +7,6 @@ import ( "io" "os" "path/filepath" - "runtime" "strings" "github.com/go-git/go-billy/v5" @@ -458,10 +457,6 @@ func (w *Worktree) resetWorktree(t *object.Tree, files []string) error { filesMap := buildFilePathMap(files) for _, ch := range changes { - if err := w.validChange(ch); err != nil { - return err - } - if len(files) > 0 { file := "" if ch.From != nil { @@ -489,108 +484,6 @@ func (w *Worktree) resetWorktree(t *object.Tree, files []string) error { return w.r.Storer.SetIndex(idx) } -// worktreeDeny is a list of paths that are not allowed -// to be used when resetting the worktree. -var worktreeDeny = map[string]struct{}{ - // .git - GitDirName: {}, - - // For other historical reasons, file names that do not conform to the 8.3 - // format (up to eight characters for the basename, three for the file - // extension, certain characters not allowed such as `+`, etc) are associated - // with a so-called "short name", at least on the `C:` drive by default. - // Which means that `git~1/` is a valid way to refer to `.git/`. - "git~1": {}, -} - -// validPath checks whether paths are valid. -// The rules around invalid paths could differ from upstream based on how -// filesystems are managed within go-git, but they are largely the same. -// -// For upstream rules: -// https://github.com/git/git/blob/564d0252ca632e0264ed670534a51d18a689ef5d/read-cache.c#L946 -// https://github.com/git/git/blob/564d0252ca632e0264ed670534a51d18a689ef5d/path.c#L1383 -func validPath(paths ...string) error { - for _, p := range paths { - parts := strings.FieldsFunc(p, func(r rune) bool { return (r == '\\' || r == '/') }) - if len(parts) == 0 { - return fmt.Errorf("invalid path: %q", p) - } - - if _, denied := worktreeDeny[strings.ToLower(parts[0])]; denied { - return fmt.Errorf("invalid path prefix: %q", p) - } - - if runtime.GOOS == "windows" { - // Volume names are not supported, in both formats: \\ and :. - if vol := filepath.VolumeName(p); vol != "" { - return fmt.Errorf("invalid path: %q", p) - } - - if !windowsValidPath(parts[0]) { - return fmt.Errorf("invalid path: %q", p) - } - } - - for _, part := range parts { - if part == ".." { - return fmt.Errorf("invalid path %q: cannot use '..'", p) - } - } - } - return nil -} - -// windowsPathReplacer defines the chars that need to be replaced -// as part of windowsValidPath. -var windowsPathReplacer *strings.Replacer - -func init() { - windowsPathReplacer = strings.NewReplacer(" ", "", ".", "") -} - -func windowsValidPath(part string) bool { - if len(part) > 3 && strings.EqualFold(part[:4], GitDirName) { - // For historical reasons, file names that end in spaces or periods are - // automatically trimmed. Therefore, `.git . . ./` is a valid way to refer - // to `.git/`. - if windowsPathReplacer.Replace(part[4:]) == "" { - return false - } - - // For yet other historical reasons, NTFS supports so-called "Alternate Data - // Streams", i.e. metadata associated with a given file, referred to via - // `::`. There exists a default stream - // type for directories, allowing `.git/` to be accessed via - // `.git::$INDEX_ALLOCATION/`. - // - // For performance reasons, _all_ Alternate Data Streams of `.git/` are - // forbidden, not just `::$INDEX_ALLOCATION`. - if len(part) > 4 && part[4:5] == ":" { - return false - } - } - return true -} - -func (w *Worktree) validChange(ch merkletrie.Change) error { - action, err := ch.Action() - if err != nil { - return nil - } - - switch action { - case merkletrie.Delete: - return validPath(ch.From.String()) - case merkletrie.Insert: - return validPath(ch.To.String()) - case merkletrie.Modify: - return validPath(ch.From.String(), ch.To.String()) - } - - return nil -} - func (w *Worktree) checkoutChange(ch merkletrie.Change, t *object.Tree, idx *indexBuilder) error { a, err := ch.Action() if err != nil { @@ -763,10 +656,10 @@ func (w *Worktree) checkoutFile(f *object.File) (err error) { } func (w *Worktree) checkoutFileSymlink(f *object.File) (err error) { - // https://github.com/git/git/commit/10ecfa76491e4923988337b2e2243b05376b40de - if strings.EqualFold(f.Name, gitmodulesFile) { - return ErrGitModulesSymlink - } + // .gitmodules symlink rejection (and its NTFS / HFS variants) is + // enforced by the worktreeFilesystem wrapper's Symlink method via + // validSymlinkName. See https://github.com/git/git/commit/10ecfa7 + // for the upstream rationale. from, err := f.Reader() if err != nil { diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_fs.go b/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_fs.go new file mode 100644 index 000000000000..9bc2fd97dc9b --- /dev/null +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_fs.go @@ -0,0 +1,264 @@ +package git + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + + "github.com/go-git/go-billy/v5" + + "github.com/go-git/go-git/v5/internal/pathutil" +) + +// defaultProtectHFS returns the default value for core.protectHFS +// when not explicitly configured. Matches upstream Git's +// PROTECT_HFS_DEFAULT[1], which the Makefile sets to 1 on Darwin +// and leaves at 0 on every other platform. +// +// [1]: https://github.com/git/git/blob/v2.54.0/config.mak.uname#L146 +func defaultProtectHFS() bool { + return runtime.GOOS == "darwin" +} + +// defaultProtectNTFS returns the default value for core.protectNTFS +// when not explicitly configured. Matches upstream Git's +// PROTECT_NTFS_DEFAULT, which has been 1 on every platform since +// 9102f958ee5 (CVE-2019-1353)[1]: WSL allows Linux processes to +// reach NTFS-mounted worktrees on Windows hosts, so the +// is_ntfs_dotgit guard cannot safely be gated on the runtime OS. +// +// [1]: https://github.com/git/git/commit/9102f958ee5 +func defaultProtectNTFS() bool { + return true +} + +// worktreeFilesystem wraps a billy.Filesystem and validates every path passed +// to a mutating operation. This prevents writing to, or deleting from, +// dangerous locations (e.g. .git/*, ../) regardless of which worktree +// code path triggers the operation. +type worktreeFilesystem struct { + billy.Filesystem + protectNTFS bool + protectHFS bool +} + +func newWorktreeFilesystem(fs billy.Filesystem, protectNTFS, protectHFS bool) *worktreeFilesystem { + return &worktreeFilesystem{Filesystem: fs, protectNTFS: protectNTFS, protectHFS: protectHFS} +} + +func (sfs *worktreeFilesystem) Create(filename string) (billy.File, error) { + if err := sfs.validPath(filename); err != nil { + return nil, fmt.Errorf("create: %w", err) + } + return sfs.Filesystem.Create(filename) +} + +func (sfs *worktreeFilesystem) Open(filename string) (billy.File, error) { + if err := sfs.validReadPath(filename); err != nil { + return nil, fmt.Errorf("open: %w", err) + } + return sfs.Filesystem.Open(filename) +} + +func (sfs *worktreeFilesystem) OpenFile(filename string, flag int, perm os.FileMode) (billy.File, error) { + if err := sfs.validPath(filename); err != nil { + return nil, fmt.Errorf("openfile: %w", err) + } + return sfs.Filesystem.OpenFile(filename, flag, perm) +} + +func (sfs *worktreeFilesystem) Stat(filename string) (os.FileInfo, error) { + if err := sfs.validReadPath(filename); err != nil { + return nil, fmt.Errorf("stat: %w", err) + } + return sfs.Filesystem.Stat(filename) +} + +func (sfs *worktreeFilesystem) Remove(filename string) error { + if err := sfs.validPath(filename); err != nil { + return fmt.Errorf("remove: %w", err) + } + return sfs.Filesystem.Remove(filename) +} + +func (sfs *worktreeFilesystem) Rename(from, to string) error { + if err := sfs.validPath(from, to); err != nil { + return fmt.Errorf("rename: %w", err) + } + return sfs.Filesystem.Rename(from, to) +} + +func (sfs *worktreeFilesystem) ReadDir(path string) ([]os.FileInfo, error) { + if err := sfs.validReadPath(path); err != nil { + return nil, fmt.Errorf("readdir: %w", err) + } + return sfs.Filesystem.ReadDir(path) +} + +func (sfs *worktreeFilesystem) Lstat(filename string) (os.FileInfo, error) { + if err := sfs.validReadPath(filename); err != nil { + return nil, fmt.Errorf("lstat: %w", err) + } + return sfs.Filesystem.Lstat(filename) +} + +func (sfs *worktreeFilesystem) Symlink(target, link string) error { + if err := sfs.validPath(link); err != nil { + return fmt.Errorf("symlink: %w", err) + } + if err := sfs.validSymlinkName(link); err != nil { + return fmt.Errorf("symlink: %w", err) + } + return sfs.Filesystem.Symlink(target, link) +} + +func (sfs *worktreeFilesystem) Readlink(link string) (string, error) { + if err := sfs.validReadPath(link); err != nil { + return "", fmt.Errorf("readlink: %w", err) + } + return sfs.Filesystem.Readlink(link) +} + +func (sfs *worktreeFilesystem) MkdirAll(path string, perm os.FileMode) error { + // MkdirAll on the worktree root is a no-op: the root always exists, + // so there is nothing to materialise. Mirroring the tolerance that + // validReadPath gives to read-side operations avoids breaking callers + // that walk a directory tree and pass the relative-to-root prefix + // ("") through to the worktree FS. + if path == "" || path == "." || path == "/" { + return nil + } + if err := sfs.validPath(path); err != nil { + return fmt.Errorf("mkdirall: %w", err) + } + return sfs.Filesystem.MkdirAll(path, perm) +} + +func (sfs *worktreeFilesystem) TempFile(_, _ string) (billy.File, error) { + return nil, fmt.Errorf("tempfile: %w", errUnsupportedOperation) +} + +func (sfs *worktreeFilesystem) Chroot(path string) (billy.Filesystem, error) { + if err := sfs.validReadPath(path); err != nil { + return nil, fmt.Errorf("chroot: %w", err) + } + return sfs.Filesystem.Chroot(path) +} + +// validReadPath is like validPath but treats the empty string and "." as +// valid references to the worktree root. Read-side operations on the root +// (e.g. ReadDir(""), Lstat(".")) are legitimate; mutating the root itself +// is not, so write-side operations continue to use validPath directly. +func (sfs *worktreeFilesystem) validReadPath(p string) error { + if p == "" || p == "." || p == "/" { + return nil + } + return sfs.validPath(p) +} + +var errUnsupportedOperation = errors.New("unsupported operation") + +// isDotGitVariant reports whether part is .git, git~1, or an HFS+ +// equivalent of .git (when protectHFS is true). NTFS variants of .git +// (e.g. ".git " with trailing space, ".git::$INDEX_ALLOCATION") are +// detected separately by pathutil.WindowsValidPath, which applies +// regardless of position in the path. Both validators reuse this +// helper. +func isDotGitVariant(part string, protectHFS bool) bool { + if pathutil.IsDotGitName(part) { + return true + } + if protectHFS && pathutil.IsHFSDotGit(part) { + return true + } + return false +} + +// validPath checks whether paths are valid for the worktree +// filesystem abstraction. It is intentionally tolerant of .git as +// the final path component of a multi-component path +// (e.g. "submodule/.git"), so that legitimate gitlink pointer files +// can still be Stat'd, Read, and Removed via the wrapper during +// submodule cleanup. Attacker-controlled tree-entry paths are +// validated separately by pathutil.ValidTreePath at the boundaries +// where data leaves the trusted store (Tree.FindEntry, the explicit +// callers in CherryPick and Submodule.Repository). +// +// For upstream rules: +// https://github.com/git/git/blob/v2.54.0/read-cache.c#L987 +// https://github.com/git/git/blob/v2.54.0/path.c#L1419 +func (sfs *worktreeFilesystem) validPath(paths ...string) error { + for _, p := range paths { + for i := 0; i < len(p); i++ { + if p[i] < 0x20 || p[i] == 0x7f { + return fmt.Errorf("invalid path %q: contains control character", p) + } + } + + parts := strings.FieldsFunc(p, func(r rune) bool { return (r == '\\' || r == '/') }) + if len(parts) == 0 { + return fmt.Errorf("invalid path: %q", p) + } + + if sfs.protectNTFS { + // Volume names are not supported, in both formats: \\ and :. + if vol := filepath.VolumeName(p); vol != "" { + return fmt.Errorf("invalid path: %q", p) + } + } + + for i, part := range parts { + if part == "." || part == ".." { + return fmt.Errorf("invalid path %q: cannot use %q", p, part) + } + + // Reject .git (and equivalents) as a path component when it is + // either the first component (root-level .git) or a non-final + // component (traversal into a .git directory, e.g. "a/.git/config"). + // A final non-first .git component (e.g. "submodule/.git") is + // allowed because submodule worktrees contain a .git pointer file. + if isDotGitVariant(part, sfs.protectHFS) && (i == 0 || i < len(parts)-1) { + return fmt.Errorf("invalid path component: %q", p) + } + + if sfs.protectNTFS && !pathutil.WindowsValidPath(part) { + return fmt.Errorf("invalid path: %q", p) + } + } + } + return nil +} + +// validSymlinkName checks the per-component name of a symlink for +// dotfile names that attackers can use to trick a checkout into +// writing a dangerous symlink. Each path component is compared +// against .gitmodules case-insensitively, against its NTFS variants +// (e.g. ".gitmodules .", ".gitmodules::$INDEX_ALLOCATION", or 8.3 +// short-name forms) when protectNTFS is on, and against its HFS+ +// variants (Unicode ignored code points folded into ".gitmodules") +// when protectHFS is on. +// +// Reference: upstream Git verify_path_internal at read-cache.c#L1004-L1024 +// in tag v2.54.0[1]. +// +// [1]: https://github.com/git/git/blob/v2.54.0/read-cache.c#L1004-L1024 +func (sfs *worktreeFilesystem) validSymlinkName(name string) error { + parts := strings.FieldsFunc(name, func(r rune) bool { + return r == '/' || r == '\\' + }) + for _, part := range parts { + if strings.EqualFold(part, gitmodulesFile) { + return ErrGitModulesSymlink + } + if sfs.protectNTFS && pathutil.IsNTFSDotGitmodules(part) { + return ErrGitModulesSymlink + } + if sfs.protectHFS && pathutil.IsHFSDotGitmodules(part) { + return ErrGitModulesSymlink + } + } + return nil +} diff --git a/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_status.go b/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_status.go index e7a60747b7e0..ecc3d7ab8979 100644 --- a/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_status.go +++ b/hack/tools/vendor/github.com/go-git/go-git/v5/worktree_status.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/go-git/go-billy/v5/util" + "github.com/go-git/go-git/v5/internal/pathutil" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/format/gitignore" @@ -545,6 +546,14 @@ func (w *Worktree) addOrUpdateFileToIndex(idx *index.Index, filename string, h p } func (w *Worktree) doAddFileToIndex(idx *index.Index, filename string, h plumbing.Hash) error { + // Mirror upstream's Index.Add gate at the v5 caller boundary: the + // index feeds future trees, so a name that the tree-side + // pathutil.ValidTreePath gate would reject must not enter the + // index in the first place. v5 keeps Index.Add's existing signature + // for API compatibility, so the validation happens here. + if err := pathutil.ValidTreePath(filename); err != nil { + return err + } return w.doUpdateFileToIndex(idx.Add(filename), filename, h) } diff --git a/hack/tools/vendor/modules.txt b/hack/tools/vendor/modules.txt index 4848b0be6362..cdd937fc3b4a 100644 --- a/hack/tools/vendor/modules.txt +++ b/hack/tools/vendor/modules.txt @@ -357,11 +357,12 @@ github.com/go-git/go-billy/v5/helper/polyfill github.com/go-git/go-billy/v5/memfs github.com/go-git/go-billy/v5/osfs github.com/go-git/go-billy/v5/util -# github.com/go-git/go-git/v5 v5.19.0 +# github.com/go-git/go-git/v5 v5.19.1 ## explicit; go 1.25.0 github.com/go-git/go-git/v5 github.com/go-git/go-git/v5/config github.com/go-git/go-git/v5/internal/path_util +github.com/go-git/go-git/v5/internal/pathutil github.com/go-git/go-git/v5/internal/revision github.com/go-git/go-git/v5/internal/url github.com/go-git/go-git/v5/plumbing From 69d9dd838eff52b460e9851a1b257f24311748bb Mon Sep 17 00:00:00 2001 From: enxebre Date: Wed, 20 May 2026 09:56:00 +0200 Subject: [PATCH 07/14] fix(control-plane-operator): set limits for aro.openshift.io/swift-nic in request overrides The resource-request-override annotation only sets requests, but extended resources like aro.openshift.io/swift-nic require limits equal to requests. The API server rejects pods where an extended resource has a request without a matching limit. When the override includes aro.openshift.io/swift-nic, automatically set the limit to the same value as the request. Co-Authored-By: Claude Opus 4.6 --- support/controlplane-component/defaults.go | 17 ++ .../controlplane-component/defaults_test.go | 235 ++++++++++++++++++ 2 files changed, 252 insertions(+) diff --git a/support/controlplane-component/defaults.go b/support/controlplane-component/defaults.go index 5bebe7d8822f..deb0bc2034b9 100644 --- a/support/controlplane-component/defaults.go +++ b/support/controlplane-component/defaults.go @@ -427,15 +427,32 @@ func (c *controlPlaneWorkload[T]) applyRequestsOverrides(podTemplate *corev1.Pod for i, c := range podTemplate.Spec.InitContainers { if res, ok := requestsOverrides[c.Name]; ok { maps.Copy(podTemplate.Spec.InitContainers[i].Resources.Requests, res) + applyNonOvercommitableResourceLimits(&podTemplate.Spec.InitContainers[i], res) } } for i, c := range podTemplate.Spec.Containers { if res, ok := requestsOverrides[c.Name]; ok { maps.Copy(podTemplate.Spec.Containers[i].Resources.Requests, res) + applyNonOvercommitableResourceLimits(&podTemplate.Spec.Containers[i], res) } } } +const aroSwiftNICResource corev1.ResourceName = "aro.openshift.io/swift-nic" + +// applyNonOvercommitableResourceLimits sets limits equal to requests for extended +// resources that cannot be overcommitted, specifically "aro.openshift.io/swift-nic". +// The API server requires limits == requests for these resources. +// https://github.com/kubernetes/kubernetes/blob/621e250502ddeeab8274836e88b506c0c4f57232/pkg/apis/core/validation/validation.go#L7975-L7976 +func applyNonOvercommitableResourceLimits(container *corev1.Container, overrides corev1.ResourceList) { + if quantity, ok := overrides[aroSwiftNICResource]; ok { + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} + } + container.Resources.Limits[aroSwiftNICResource] = quantity + } +} + func parseResourceRequestOverrideAnnotation(value string) corev1.ResourceList { result := corev1.ResourceList{} resourceRequests := strings.Split(value, ",") diff --git a/support/controlplane-component/defaults_test.go b/support/controlplane-component/defaults_test.go index f1c726fe2af7..5859e66fd543 100644 --- a/support/controlplane-component/defaults_test.go +++ b/support/controlplane-component/defaults_test.go @@ -10,6 +10,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" runtime "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" @@ -285,6 +286,240 @@ func generateResources() (map[string]*corev1.Secret, map[string]*corev1.ConfigMa return secrets, configMaps } +func TestApplyRequestsOverrides(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + containers []corev1.Container + initContainers []corev1.Container + expectedContainers []corev1.Container + expectedInitContainers []corev1.Container + }{ + { + name: "When overriding cpu and memory it should only update requests", + annotations: map[string]string{ + "resource-request-override.hypershift.openshift.io/router.router": "cpu=500m,memory=1Gi", + }, + containers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + }, + }, + expectedContainers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + }, + }, + }, + }, + { + name: "When overriding aro.openshift.io/swift-nic it should set both requests and limits", + annotations: map[string]string{ + "resource-request-override.hypershift.openshift.io/router.router": "aro.openshift.io/swift-nic=1", + }, + containers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{}, + }, + }, + }, + expectedContainers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("1"), + }, + Limits: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("1"), + }, + }, + }, + }, + }, + { + name: "When overriding mixed resources it should set limits only for swift-nic", + annotations: map[string]string{ + "resource-request-override.hypershift.openshift.io/router.router": "cpu=500m,aro.openshift.io/swift-nic=1", + }, + containers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }, + expectedContainers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + aroSwiftNICResource: resource.MustParse("1"), + }, + Limits: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("1"), + }, + }, + }, + }, + }, + { + name: "When overriding an init container with swift-nic it should set both requests and limits", + annotations: map[string]string{ + "resource-request-override.hypershift.openshift.io/router.init-router": "aro.openshift.io/swift-nic=2", + }, + initContainers: []corev1.Container{ + { + Name: "init-router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{}, + }, + }, + }, + expectedInitContainers: []corev1.Container{ + { + Name: "init-router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("2"), + }, + Limits: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("2"), + }, + }, + }, + }, + }, + { + name: "When annotation targets a different deployment it should not apply overrides", + annotations: map[string]string{ + "resource-request-override.hypershift.openshift.io/kube-apiserver.kube-apiserver": "cpu=500m", + }, + containers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }, + expectedContainers: []corev1.Container{ + { + Name: "router", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + workload := &controlPlaneWorkload[*appsv1.Deployment]{ + name: "router", + workloadProvider: &deploymentProvider{}, + ComponentOptions: &testComponent{}, + } + hcp := &hyperv1.HostedControlPlane{} + hcp.Annotations = test.annotations + + podTemplate := &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: test.containers, + InitContainers: test.initContainers, + }, + } + + workload.applyRequestsOverrides(podTemplate, hcp) + + if test.expectedContainers != nil { + g.Expect(podTemplate.Spec.Containers).To(Equal(test.expectedContainers)) + } + if test.expectedInitContainers != nil { + g.Expect(podTemplate.Spec.InitContainers).To(Equal(test.expectedInitContainers)) + } + }) + } +} + +func TestApplyNonOvercommitableResourceLimits(t *testing.T) { + tests := []struct { + name string + overrides corev1.ResourceList + existingLimits corev1.ResourceList + expectedLimits corev1.ResourceList + }{ + { + name: "When overriding aro.openshift.io/swift-nic it should set the limit to the same value", + overrides: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("1"), + }, + expectedLimits: corev1.ResourceList{ + aroSwiftNICResource: resource.MustParse("1"), + }, + }, + { + name: "When overriding standard resources it should not set limits", + overrides: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + expectedLimits: nil, + }, + { + name: "When overriding a mix of standard and swift-nic resources it should only set limits for swift-nic", + overrides: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + aroSwiftNICResource: resource.MustParse("2"), + }, + existingLimits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + expectedLimits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("2Gi"), + aroSwiftNICResource: resource.MustParse("2"), + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + container := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: test.existingLimits, + }, + } + applyNonOvercommitableResourceLimits(container, test.overrides) + g.Expect(container.Resources.Limits).To(Equal(test.expectedLimits)) + }) + } +} + func TestSetDefaultOptions(t *testing.T) { g := NewGomegaWithT(t) scheme := runtime.NewScheme() From b42412952e90730cce0699126abad6dd4cf2d592 Mon Sep 17 00:00:00 2001 From: Antoni Segura Puimedon Date: Wed, 20 May 2026 15:51:47 +0200 Subject: [PATCH 08/14] fix(tekton): enable package registry proxy in prefetch-dependencies task The enterprise contract check requires the prefetch-dependencies-oci-ta task to have enable-package-registry-proxy set to true. Co-Authored-By: Claude Opus 4.6 --- .tekton/hypershift-operator-main-tag.yaml | 2 ++ .tekton/pipelines/common-operator-build.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/.tekton/hypershift-operator-main-tag.yaml b/.tekton/hypershift-operator-main-tag.yaml index d6c21a719d90..1446a0d0e6ca 100644 --- a/.tekton/hypershift-operator-main-tag.yaml +++ b/.tekton/hypershift-operator-main-tag.yaml @@ -185,6 +185,8 @@ spec: value: $(params.output-image).prefetch - name: ociArtifactExpiresAfter value: $(params.image-expires-after) + - name: enable-package-registry-proxy + value: "true" runAfter: - clone-repository taskRef: diff --git a/.tekton/pipelines/common-operator-build.yaml b/.tekton/pipelines/common-operator-build.yaml index fc77e0cd8e7c..c221e279fd2b 100644 --- a/.tekton/pipelines/common-operator-build.yaml +++ b/.tekton/pipelines/common-operator-build.yaml @@ -130,6 +130,8 @@ spec: value: $(params.image-expires-after) - name: dev-package-managers value: $(params.dev-package-managers) + - name: enable-package-registry-proxy + value: "true" runAfter: - clone-repository taskRef: From c3cc46cce4bff54e9ca212f452774364175ff360 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 20 May 2026 11:19:02 -0400 Subject: [PATCH 09/14] chore: update Konflux Tekton task bundles Update 38 Tekton task bundle references across pipeline files to their latest versions, including 3 version bumps and 32 digest-only updates. Version bumps: - build-image-index: 0.2 -> 0.3 (removed deprecated COMMIT_SHA and IMAGE_EXPIRES_AFTER params per migration notes) - clamav-scan: 0.3 -> 0.3.1 - rpms-signature-scan: 0.2 -> 0.2.1 Co-Authored-By: Claude Opus 4.6 --- .tekton/hypershift-operator-main-tag.yaml | 44 +++++++++----------- .tekton/pipelines/common-operator-build.yaml | 40 ++++++++---------- 2 files changed, 38 insertions(+), 46 deletions(-) diff --git a/.tekton/hypershift-operator-main-tag.yaml b/.tekton/hypershift-operator-main-tag.yaml index d6c21a719d90..a42a0945846d 100644 --- a/.tekton/hypershift-operator-main-tag.yaml +++ b/.tekton/hypershift-operator-main-tag.yaml @@ -54,7 +54,7 @@ spec: - name: name value: show-sbom - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-show-sbom:0.1@sha256:04994df487ee886adbe60a8a5866647fbdfd53cc26f7b2554272ba51bf7af29e + value: quay.io/konflux-ci/tekton-catalog/task-show-sbom:0.1@sha256:a7346ed61237db4f82ff782e0c9e8b30536e0e67b907ad600341a6d192e80012 - name: kind value: task resolver: bundles @@ -147,7 +147,7 @@ spec: - name: name value: init - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-init:0.4@sha256:288f3106118edc1d0f0c79a89c960abf5841a4dd8bc3f38feb10527253105b19 + value: quay.io/konflux-ci/tekton-catalog/task-init:0.4@sha256:5a423246792ac501ea279229b42ee57da9927da441c04b5c9ff86817b0856b08 - name: kind value: task resolver: bundles @@ -168,7 +168,7 @@ spec: - name: name value: git-clone-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-git-clone-oci-ta:0.1@sha256:2c388d28651457db60bb90287e7d8c3680303197196e4476878d98d81e8b6dc9 + value: quay.io/konflux-ci/tekton-catalog/task-git-clone-oci-ta:0.1@sha256:13d49df7dc9ae301627e45f95a236011422996152f1bea46cd60217b0f057407 - name: kind value: task resolver: bundles @@ -192,7 +192,7 @@ spec: - name: name value: prefetch-dependencies-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-prefetch-dependencies-oci-ta:0.3@sha256:a579d00fe370b6d9a1cb1751c883ecd0ec9f663604344e2fd61e1f6d5bf4e990 + value: quay.io/konflux-ci/tekton-catalog/task-prefetch-dependencies-oci-ta:0.3@sha256:a2efbcdcecfa5293a622eb356a18f5c88e5714046b214fe8730b43b1a7dbb77d - name: kind value: task resolver: bundles @@ -246,7 +246,7 @@ spec: - name: name value: buildah-remote-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-buildah-remote-oci-ta:0.9@sha256:a9ca472e297388d6ef8d1f51ee205abee6076aed7c5356ec0df84f14a2e78ad8 + value: quay.io/konflux-ci/tekton-catalog/task-buildah-remote-oci-ta:0.9@sha256:f667d1146533b1d49829c08097e31faf27db24563da576434a707353de62099f - name: kind value: task resolver: bundles @@ -254,10 +254,6 @@ spec: params: - name: IMAGE value: $(params.output-image) - - name: COMMIT_SHA - value: $(tasks.clone-repository.results.commit) - - name: IMAGE_EXPIRES_AFTER - value: $(params.image-expires-after) - name: ALWAYS_BUILD_INDEX value: $(params.build-image-index) - name: IMAGES @@ -270,7 +266,7 @@ spec: - name: name value: build-image-index - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-build-image-index:0.2@sha256:c7b0f7e1f743040d99a3532abbdfddc9484f80fd559a75171c97499c3eb5d163 + value: quay.io/konflux-ci/tekton-catalog/task-build-image-index:0.3@sha256:b33bfa8dc27dbf459f0779598ba45dcaa490bcc9f8efe1652bcf360ec8cb5582 - name: kind value: task resolver: bundles @@ -291,7 +287,7 @@ spec: - name: name value: source-build-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-source-build-oci-ta:0.3@sha256:362f0475df00e7dfb5f15dea0481d1b68b287f60411718d70a23da3c059a5613 + value: quay.io/konflux-ci/tekton-catalog/task-source-build-oci-ta:0.3@sha256:0917cfc7772e82cb8e74743c2104f43bcf2596aceafe87eec6fce69a8cac5f06 - name: kind value: task resolver: bundles @@ -313,7 +309,7 @@ spec: - name: name value: deprecated-image-check - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-deprecated-image-check:0.5@sha256:5ff16b7e6b4a8aa1adb352e74b9f831f77ff97bafd1b89ddb0038d63335f1a67 + value: quay.io/konflux-ci/tekton-catalog/task-deprecated-image-check:0.5@sha256:e78d0d3baf3c8cfc1a5ad278196b74032d9568b143a87c7a79ab780fedfb296e - name: kind value: task resolver: bundles @@ -335,7 +331,7 @@ spec: - name: name value: clair-scan - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-clair-scan:0.3.2@sha256:3fa03be0280f33d7070ea53f26d53e727199737a7a2b9a59a95071ae40a999ac + value: quay.io/konflux-ci/tekton-catalog/task-clair-scan:0.3.2@sha256:8fad4c2e2f470f82ee43d6b2ac72327b4d9c6e9cb514a678911c1c9359c29894 - name: kind value: task resolver: bundles @@ -360,7 +356,7 @@ spec: - name: name value: ecosystem-cert-preflight-checks - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-ecosystem-cert-preflight-checks:0.2@sha256:b4ac586edea81dcd25dfc17f1bd57899825be2b443e48d572cd05ce058f153bb + value: quay.io/konflux-ci/tekton-catalog/task-ecosystem-cert-preflight-checks:0.2@sha256:9c300728a03f41beee9a689422d66513d32ab5f804664fe561b11cebacd07799 - name: kind value: task resolver: bundles @@ -386,7 +382,7 @@ spec: - name: name value: sast-snyk-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-snyk-check-oci-ta:0.4@sha256:d83becbfefe2aa39971c3d37bdc23489b745e22fd86cf4872455a133f8cb274f + value: quay.io/konflux-ci/tekton-catalog/task-sast-snyk-check-oci-ta:0.4@sha256:8f3ecbeaff579e41b8278f82d7fabac27845db17a8e687ea6c510c0c9aceabbb - name: kind value: task resolver: bundles @@ -413,7 +409,7 @@ spec: - name: name value: clamav-scan - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-clamav-scan:0.3@sha256:9f18b216ce71a66909e7cb17d9b34526c02d73cf12884ba32d1f10614f7b9f5a + value: quay.io/konflux-ci/tekton-catalog/task-clamav-scan:0.3.1@sha256:567cb66bd2e1f4b58b9d4d756f3317fc62479e0b40aa0de66094b1f12d296cfc - name: kind value: task resolver: bundles @@ -458,7 +454,7 @@ spec: - name: name value: sast-coverity-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-coverity-check-oci-ta:0.3@sha256:47f4e2d0881ac8c43a1ea1e2375bb2591dff34b5aa8c7366a043652d1eed499c + value: quay.io/konflux-ci/tekton-catalog/task-sast-coverity-check-oci-ta:0.3@sha256:e92d00ed858233d0096627861192d3e4fc013cf1559c0d0b0ea0657d3377ce75 - name: kind value: task resolver: bundles @@ -479,7 +475,7 @@ spec: - name: name value: coverity-availability-check - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-coverity-availability-check:0.2@sha256:de35caf2f090e3275cfd1019ea50d9662422e904fb4aebd6ea29fb53a1ad57f5 + value: quay.io/konflux-ci/tekton-catalog/task-coverity-availability-check:0.2@sha256:8b501440a960aec446db2ebc6625a49d0317a9fc7bf0f7bd9b18cb63052db7de - name: kind value: task resolver: bundles @@ -505,7 +501,7 @@ spec: - name: name value: sast-shell-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-shell-check-oci-ta:0.1@sha256:6f047f52c04ee6e4d2cb25af46e3ea92b235f6c5e02da540fb7ef0b90718bc0a + value: quay.io/konflux-ci/tekton-catalog/task-sast-shell-check-oci-ta:0.1@sha256:c4ef47e3b4e0508572d266fb745be7e374c29dc02580328cbe9f4d472a8aca57 - name: kind value: task resolver: bundles @@ -531,7 +527,7 @@ spec: - name: name value: sast-unicode-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-unicode-check-oci-ta:0.4@sha256:55006815522c57c1f83451dc0cba723ff7427dbac48553538b75cda7bf886d79 + value: quay.io/konflux-ci/tekton-catalog/task-sast-unicode-check-oci-ta:0.4@sha256:90efa582de7770d55102b74014a765cd16a25a56f2cf644b56a788c70c4dc749 - name: kind value: task resolver: bundles @@ -569,7 +565,7 @@ spec: - name: name value: run-script-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-run-script-oci-ta:0.1@sha256:834a934f1e631a79aea7f2d001162cf90086e664e648c8ca15b69ad9798571ee + value: quay.io/konflux-ci/tekton-catalog/task-run-script-oci-ta:0.1@sha256:0e13a74cc02c945e7119ecd4cc0c9148e7591b50f87e415b212154caad0479c0 - name: kind value: task resolver: bundles @@ -590,7 +586,7 @@ spec: - name: name value: apply-tags - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-apply-tags:0.3@sha256:510b6d2a3b188adeb716e49566b57d611ab36bd69a2794b5ddfc11dbf014c2ca + value: quay.io/konflux-ci/tekton-catalog/task-apply-tags:0.3@sha256:a291081de7fb27f832c6fc3c4b078acf7e6162ca4c085db38b118ca87e8b5b66 - name: kind value: task resolver: bundles @@ -613,7 +609,7 @@ spec: - name: name value: push-dockerfile-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-push-dockerfile-oci-ta:0.3@sha256:1bc2d0f26b89259db090a47bb38217c82c05e335d626653d184adf1d196ca131 + value: quay.io/konflux-ci/tekton-catalog/task-push-dockerfile-oci-ta:0.3@sha256:7855471abfe87de080b914f2f3ca27c59e64f6448a7c2435e51435b764494c71 - name: kind value: task resolver: bundles @@ -630,7 +626,7 @@ spec: - name: name value: rpms-signature-scan - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-rpms-signature-scan:0.2@sha256:35a4ccda7e213d83d9f5e7ea5cad91dd180cbcebcb6c46f8d41a579478dd2072 + value: quay.io/konflux-ci/tekton-catalog/task-rpms-signature-scan:0.2.1@sha256:41720da9dfe26f33b0bdc46bbf8667a27dae4790d8e5c5f4412224658de7b213 - name: kind value: task resolver: bundles diff --git a/.tekton/pipelines/common-operator-build.yaml b/.tekton/pipelines/common-operator-build.yaml index fc77e0cd8e7c..9e25e0647fe0 100644 --- a/.tekton/pipelines/common-operator-build.yaml +++ b/.tekton/pipelines/common-operator-build.yaml @@ -18,7 +18,7 @@ spec: - name: name value: show-sbom - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-show-sbom:0.1@sha256:04994df487ee886adbe60a8a5866647fbdfd53cc26f7b2554272ba51bf7af29e + value: quay.io/konflux-ci/tekton-catalog/task-show-sbom:0.1@sha256:a7346ed61237db4f82ff782e0c9e8b30536e0e67b907ad600341a6d192e80012 - name: kind value: task resolver: bundles @@ -90,7 +90,7 @@ spec: - name: name value: init - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-init:0.4@sha256:288f3106118edc1d0f0c79a89c960abf5841a4dd8bc3f38feb10527253105b19 + value: quay.io/konflux-ci/tekton-catalog/task-init:0.4@sha256:5a423246792ac501ea279229b42ee57da9927da441c04b5c9ff86817b0856b08 - name: kind value: task resolver: bundles @@ -111,7 +111,7 @@ spec: - name: name value: git-clone-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-git-clone-oci-ta:0.1@sha256:2c388d28651457db60bb90287e7d8c3680303197196e4476878d98d81e8b6dc9 + value: quay.io/konflux-ci/tekton-catalog/task-git-clone-oci-ta:0.1@sha256:13d49df7dc9ae301627e45f95a236011422996152f1bea46cd60217b0f057407 - name: kind value: task resolver: bundles @@ -137,7 +137,7 @@ spec: - name: name value: prefetch-dependencies-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-prefetch-dependencies-oci-ta:0.3@sha256:a579d00fe370b6d9a1cb1751c883ecd0ec9f663604344e2fd61e1f6d5bf4e990 + value: quay.io/konflux-ci/tekton-catalog/task-prefetch-dependencies-oci-ta:0.3@sha256:a2efbcdcecfa5293a622eb356a18f5c88e5714046b214fe8730b43b1a7dbb77d - name: kind value: task resolver: bundles @@ -179,7 +179,7 @@ spec: - name: name value: buildah-remote-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-buildah-remote-oci-ta:0.9@sha256:a9ca472e297388d6ef8d1f51ee205abee6076aed7c5356ec0df84f14a2e78ad8 + value: quay.io/konflux-ci/tekton-catalog/task-buildah-remote-oci-ta:0.9@sha256:f667d1146533b1d49829c08097e31faf27db24563da576434a707353de62099f - name: kind value: task resolver: bundles @@ -187,10 +187,6 @@ spec: params: - name: IMAGE value: $(params.output-image) - - name: COMMIT_SHA - value: $(tasks.clone-repository.results.commit) - - name: IMAGE_EXPIRES_AFTER - value: $(params.image-expires-after) - name: ALWAYS_BUILD_INDEX value: "true" - name: IMAGES @@ -203,7 +199,7 @@ spec: - name: name value: build-image-index - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-build-image-index:0.2@sha256:c7b0f7e1f743040d99a3532abbdfddc9484f80fd559a75171c97499c3eb5d163 + value: quay.io/konflux-ci/tekton-catalog/task-build-image-index:0.3@sha256:b33bfa8dc27dbf459f0779598ba45dcaa490bcc9f8efe1652bcf360ec8cb5582 - name: kind value: task resolver: bundles @@ -220,7 +216,7 @@ spec: - name: name value: deprecated-image-check - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-deprecated-image-check:0.5@sha256:5ff16b7e6b4a8aa1adb352e74b9f831f77ff97bafd1b89ddb0038d63335f1a67 + value: quay.io/konflux-ci/tekton-catalog/task-deprecated-image-check:0.5@sha256:e78d0d3baf3c8cfc1a5ad278196b74032d9568b143a87c7a79ab780fedfb296e - name: kind value: task resolver: bundles @@ -242,7 +238,7 @@ spec: - name: name value: clair-scan - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-clair-scan:0.3.2@sha256:3fa03be0280f33d7070ea53f26d53e727199737a7a2b9a59a95071ae40a999ac + value: quay.io/konflux-ci/tekton-catalog/task-clair-scan:0.3.2@sha256:8fad4c2e2f470f82ee43d6b2ac72327b4d9c6e9cb514a678911c1c9359c29894 - name: kind value: task resolver: bundles @@ -266,7 +262,7 @@ spec: - name: name value: ecosystem-cert-preflight-checks - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-ecosystem-cert-preflight-checks:0.2@sha256:b4ac586edea81dcd25dfc17f1bd57899825be2b443e48d572cd05ce058f153bb + value: quay.io/konflux-ci/tekton-catalog/task-ecosystem-cert-preflight-checks:0.2@sha256:9c300728a03f41beee9a689422d66513d32ab5f804664fe561b11cebacd07799 - name: kind value: task resolver: bundles @@ -292,7 +288,7 @@ spec: - name: name value: sast-snyk-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-snyk-check-oci-ta:0.4@sha256:d83becbfefe2aa39971c3d37bdc23489b745e22fd86cf4872455a133f8cb274f + value: quay.io/konflux-ci/tekton-catalog/task-sast-snyk-check-oci-ta:0.4@sha256:8f3ecbeaff579e41b8278f82d7fabac27845db17a8e687ea6c510c0c9aceabbb - name: kind value: task resolver: bundles @@ -318,7 +314,7 @@ spec: - name: name value: clamav-scan - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-clamav-scan:0.3@sha256:9f18b216ce71a66909e7cb17d9b34526c02d73cf12884ba32d1f10614f7b9f5a + value: quay.io/konflux-ci/tekton-catalog/task-clamav-scan:0.3.1@sha256:567cb66bd2e1f4b58b9d4d756f3317fc62479e0b40aa0de66094b1f12d296cfc - name: kind value: task resolver: bundles @@ -358,7 +354,7 @@ spec: - name: name value: sast-coverity-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-coverity-check-oci-ta:0.3@sha256:47f4e2d0881ac8c43a1ea1e2375bb2591dff34b5aa8c7366a043652d1eed499c + value: quay.io/konflux-ci/tekton-catalog/task-sast-coverity-check-oci-ta:0.3@sha256:e92d00ed858233d0096627861192d3e4fc013cf1559c0d0b0ea0657d3377ce75 - name: kind value: task resolver: bundles @@ -379,7 +375,7 @@ spec: - name: name value: coverity-availability-check - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-coverity-availability-check:0.2@sha256:de35caf2f090e3275cfd1019ea50d9662422e904fb4aebd6ea29fb53a1ad57f5 + value: quay.io/konflux-ci/tekton-catalog/task-coverity-availability-check:0.2@sha256:8b501440a960aec446db2ebc6625a49d0317a9fc7bf0f7bd9b18cb63052db7de - name: kind value: task resolver: bundles @@ -405,7 +401,7 @@ spec: - name: name value: sast-shell-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-shell-check-oci-ta:0.1@sha256:6f047f52c04ee6e4d2cb25af46e3ea92b235f6c5e02da540fb7ef0b90718bc0a + value: quay.io/konflux-ci/tekton-catalog/task-sast-shell-check-oci-ta:0.1@sha256:c4ef47e3b4e0508572d266fb745be7e374c29dc02580328cbe9f4d472a8aca57 - name: kind value: task resolver: bundles @@ -431,7 +427,7 @@ spec: - name: name value: sast-unicode-check-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-sast-unicode-check-oci-ta:0.4@sha256:55006815522c57c1f83451dc0cba723ff7427dbac48553538b75cda7bf886d79 + value: quay.io/konflux-ci/tekton-catalog/task-sast-unicode-check-oci-ta:0.4@sha256:90efa582de7770d55102b74014a765cd16a25a56f2cf644b56a788c70c4dc749 - name: kind value: task resolver: bundles @@ -455,7 +451,7 @@ spec: - name: name value: apply-tags - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-apply-tags:0.3@sha256:510b6d2a3b188adeb716e49566b57d611ab36bd69a2794b5ddfc11dbf014c2ca + value: quay.io/konflux-ci/tekton-catalog/task-apply-tags:0.3@sha256:a291081de7fb27f832c6fc3c4b078acf7e6162ca4c085db38b118ca87e8b5b66 - name: kind value: task resolver: bundles @@ -478,7 +474,7 @@ spec: - name: name value: push-dockerfile-oci-ta - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-push-dockerfile-oci-ta:0.3@sha256:1bc2d0f26b89259db090a47bb38217c82c05e335d626653d184adf1d196ca131 + value: quay.io/konflux-ci/tekton-catalog/task-push-dockerfile-oci-ta:0.3@sha256:7855471abfe87de080b914f2f3ca27c59e64f6448a7c2435e51435b764494c71 - name: kind value: task resolver: bundles @@ -495,7 +491,7 @@ spec: - name: name value: rpms-signature-scan - name: bundle - value: quay.io/konflux-ci/tekton-catalog/task-rpms-signature-scan:0.2@sha256:35a4ccda7e213d83d9f5e7ea5cad91dd180cbcebcb6c46f8d41a579478dd2072 + value: quay.io/konflux-ci/tekton-catalog/task-rpms-signature-scan:0.2.1@sha256:41720da9dfe26f33b0bdc46bbf8667a27dae4790d8e5c5f4412224658de7b213 - name: kind value: task resolver: bundles From 83b7c2d860e95276b34cc51bfa1c9832b118a909 Mon Sep 17 00:00:00 2001 From: Max Cao Date: Fri, 15 May 2026 12:45:07 -0700 Subject: [PATCH 10/14] fix(e2e): use public multi-arch image for ARM64 karpenter test quay.io/hypershift/sleep:multiarch is a private image that requires auth not available in CI pull secrets, causing ImagePullBackOff. Use registry.access.redhat.com/ubi10/ubi-minimal:10.1 which is public and multi-arch. Co-authored-by: Cursor Signed-off-by: Max Cao --- test/e2e/karpenter_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/e2e/karpenter_test.go b/test/e2e/karpenter_test.go index ac39964d9280..c9e996d113ef 100644 --- a/test/e2e/karpenter_test.go +++ b/test/e2e/karpenter_test.go @@ -316,7 +316,7 @@ func testARM64Provisioning(ctx context.Context, guestClient crclient.Client, hos {Key: karpenterv1.CapacityTypeLabelKey, Operator: corev1.NodeSelectorOpIn, Values: []string{karpenterv1.CapacityTypeOnDemand}}, } // quay.io/openshift/origin-pod does not support arm64 - armWorkLoads := testWorkloadWithImage("arm-app", 1, map[string]string{karpenterv1.NodePoolLabelKey: armNodePool.Name}, "quay.io/hypershift/sleep:multiarch") + armWorkLoads := testWorkloadWithImage("arm-app", 1, map[string]string{karpenterv1.NodePoolLabelKey: armNodePool.Name}, "registry.access.redhat.com/ubi10/ubi-minimal:10.1") t.Cleanup(func() { _ = guestClient.Delete(ctx, armWorkLoads) @@ -1802,6 +1802,7 @@ func testWorkloadWithImage(name string, replicas int32, nodeSelector map[string] SecurityContext: &corev1.SecurityContext{ AllowPrivilegeEscalation: ptr.To(false), }, + Command: []string{"/bin/sh", "-c", "sleep infinity"}, }}, NodeSelector: nodeSelector, }, From 16b595b06204d6bd0be63b3b4f15242d6225a0e0 Mon Sep 17 00:00:00 2001 From: Yamunadevi Shanmugam Date: Thu, 21 May 2026 08:45:44 +0530 Subject: [PATCH 11/14] fix(pki): requeue certificate revocation to prevent sync timeout Update logic of CertificateRevocationController to request a requeue when encountering stale total-client-ca bundle cache. --- .../certificaterevocationcontroller.go | 4 +- .../certificaterevocationcontroller_test.go | 93 +++++++++++++++++-- 2 files changed, 87 insertions(+), 10 deletions(-) diff --git a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go index 4e64e59ffe7c..6e747aa5f633 100644 --- a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go +++ b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go @@ -733,7 +733,7 @@ func (c *CertificateRevocationController) ensureNewSignerCertificatePropagated(c // that, though, and it's always valid to first check that our certificates have propagated as far // as we can tell in the system before asking the KAS, since that's expensive if len(trustedCertificates(totalClientTrustBundle, []*certificateSecret{{cert: signers[0]}}, now)) == 0 { - return true, nil, false, nil + return true, nil, true, nil } // if the updated trust bundle has propagated as far as we can tell, let's go ahead and ask @@ -1033,7 +1033,7 @@ func (c *CertificateRevocationController) ensureOldSignerCertificateRevoked(ctx // that, though, and it's always valid to first check that our certificates have propagated as far // as we can tell in the system before asking the KAS, since that's expensive if len(trustedCertificates(totalClientTrustBundle, []*certificateSecret{{cert: oldCerts[0]}}, now)) != 0 { - return true, nil, false, nil + return true, nil, true, nil } // if the updated trust bundle has propagated as far as we can tell, let's go ahead and ask diff --git a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go index d8c7b96f010b..ae4c47c70867 100644 --- a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go +++ b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go @@ -463,10 +463,11 @@ func TestCertificateRevocationController_processCertificateRevocationRequest(t * }, }, { - name: "not yet propagated, nothing to do", - now: postRevocationClock.Now, - crrNamespace: "crr-ns", - crrName: "crr-name", + name: "not yet propagated, nothing to do", + now: postRevocationClock.Now, + crrNamespace: "crr-ns", + crrName: "crr-name", + expectedRequeue: true, // New cert not yet in total bundle, requeue to wait for TargetConfigController crr: &certificatesv1alpha1.CertificateRevocationRequest{ ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: "crr-name"}, Spec: certificatesv1alpha1.CertificateRevocationRequestSpec{SignerClass: string(certificates.CustomerBreakGlassSigner)}, @@ -847,10 +848,11 @@ func TestCertificateRevocationController_processCertificateRevocationRequest(t * }, }, { - name: "validating, previous still valid", - now: postRevocationClock.Now, - crrNamespace: "crr-ns", - crrName: "crr-name", + name: "validating, previous still valid", + now: postRevocationClock.Now, + crrNamespace: "crr-ns", + crrName: "crr-name", + expectedRequeue: true, // Old cert still in total bundle, requeue to wait for TargetConfigController crr: &certificatesv1alpha1.CertificateRevocationRequest{ ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: "crr-name"}, Spec: certificatesv1alpha1.CertificateRevocationRequestSpec{SignerClass: string(certificates.CustomerBreakGlassSigner)}, @@ -1033,6 +1035,81 @@ func TestCertificateRevocationController_processCertificateRevocationRequest(t * }, }, }, + { + name: "SRE signer: validating, previous still valid (requeue path)", + now: postRevocationClock.Now, + crrNamespace: "crr-ns", + crrName: "crr-name-sre", + expectedRequeue: true, // Old cert still in total bundle for SRE signer, must requeue + crr: &certificatesv1alpha1.CertificateRevocationRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: "crr-name-sre"}, + Spec: certificatesv1alpha1.CertificateRevocationRequestSpec{SignerClass: string(certificates.SREBreakGlassSigner)}, + Status: certificatesv1alpha1.CertificateRevocationRequestStatus{ + RevocationTimestamp: ptr.To(metav1.NewTime(revocationClock.Now())), + PreviousSigner: &corev1.LocalObjectReference{Name: "1pfcydcz358pa1glirkmc72sdkf5zw21uam4jbnj03pw"}, + Conditions: []metav1.Condition{{ + Type: certificatesv1alpha1.LeafCertificatesRegeneratedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `All leaf certificates are re-generated.`, + }, { + Type: certificatesv1alpha1.RootCertificatesRegeneratedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `Signer certificate crr-ns/sre-system-admin-signer regenerated.`, + }, { + Type: certificatesv1alpha1.NewCertificatesTrustedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `New signer certificate crr-ns/sre-system-admin-signer trusted.`, + }}, + }, + }, + secrets: []*corev1.Secret{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: manifests.SRESystemAdminSigner("").Name, + Annotations: map[string]string{certrotation.CertificateIssuer: "crr-ns_sre-break-glass-signer@1234"}, + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.future.raw.signerCert, + corev1.TLSPrivateKeyKey: data.future.raw.signerKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: "1pfcydcz358pa1glirkmc72sdkf5zw21uam4jbnj03pw", + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.original.raw.signerCert, + corev1.TLSPrivateKeyKey: data.original.raw.signerKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: manifests.SRESystemAdminClientCertSecret("").Name, + Annotations: map[string]string{certrotation.CertificateIssuer: "crr-ns_sre-break-glass-signer@1234"}, + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.future.raw.signedCert, + corev1.TLSPrivateKeyKey: data.future.raw.clientKey, + }, + }}, + cms: []*corev1.ConfigMap{{ + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: manifests.SRESystemAdminSignerCA("").Name}, + Data: map[string]string{ + "ca-bundle.crt": string(data.future.raw.signerCert), + }, + }, { + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: manifests.TotalKASClientCABundle("").Name}, + Data: map[string]string{ + "ca-bundle.crt": string(data.original.raw.signerCert) + string(data.future.raw.signerCert), + }, + }}, + }, } { t.Run(testCase.name, func(t *testing.T) { c := &CertificateRevocationController{ From 141640de610992b0f1639f3f5ce78eea1d040d24 Mon Sep 17 00:00:00 2001 From: Antoni Segura Puimedon Date: Thu, 21 May 2026 16:48:48 +0200 Subject: [PATCH 12/14] ci(workflows): use EFS cache directly instead of copying Point GOCACHE at the read-only EFS mount (/cache/go-build) instead of copying the entire cache into /tmp at job start. Go's build cache handles read-only directories gracefully by skipping writes. This eliminates the per-job cp -a overhead that was adding ~2 minutes to every CI job since the EFS cache was introduced. Co-Authored-By: Claude Opus 4.6 --- .github/actions/warm-go-cache/action.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/actions/warm-go-cache/action.yaml b/.github/actions/warm-go-cache/action.yaml index 26b51b5ab2f7..fedb7abb7805 100644 --- a/.github/actions/warm-go-cache/action.yaml +++ b/.github/actions/warm-go-cache/action.yaml @@ -1,13 +1,13 @@ name: 'Warm Go build cache' -description: 'Set GOCACHE and optionally warm from EFS-backed PV' +description: 'Set GOCACHE to the EFS-backed PV mount (read-only) or a writable fallback' runs: using: composite steps: - shell: bash run: | - echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV" if [ -d /cache/go-build ]; then - mkdir -p /tmp/go-build-cache && \ - timeout 120 cp -a /cache/go-build/. /tmp/go-build-cache/ || \ - echo "::warning::Failed to copy EFS cache, proceeding without cache" + echo "GOCACHE=/cache/go-build" >> "$GITHUB_ENV" + else + mkdir -p /tmp/go-build-cache + echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV" fi From 56fbf18ff1b6fafb38b8b32937f78b440e98b893 Mon Sep 17 00:00:00 2001 From: Antoni Segura Puimedon Date: Thu, 21 May 2026 17:11:20 +0200 Subject: [PATCH 13/14] ci(workflows): use fuse-overlayfs for build cache instead of full copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the per-job `cp -a` of the entire EFS cache with a fuse-overlayfs mount. This gives Go a writable view over the read-only EFS PVC with zero copy overhead — reads hit the EFS mount directly and writes go to a tmpfs upper layer. Falls back to `cp -a` if fuse-overlayfs or /dev/fuse is unavailable, and to an empty cache if both fail. Adds fuse-overlayfs to the runner image. On OpenShift 4.15+ /dev/fuse is available to unprivileged pods without cluster config changes. Co-Authored-By: Claude Opus 4.6 --- .github/actions/warm-go-cache/action.yaml | 21 +++++++++++++++------ Dockerfile.github-actions-runner | 1 + 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/actions/warm-go-cache/action.yaml b/.github/actions/warm-go-cache/action.yaml index fedb7abb7805..993dca6e84fc 100644 --- a/.github/actions/warm-go-cache/action.yaml +++ b/.github/actions/warm-go-cache/action.yaml @@ -1,13 +1,22 @@ name: 'Warm Go build cache' -description: 'Set GOCACHE to the EFS-backed PV mount (read-only) or a writable fallback' +description: 'Set GOCACHE via fuse-overlayfs over the EFS-backed PV or a writable fallback' runs: using: composite steps: - shell: bash run: | - if [ -d /cache/go-build ]; then - echo "GOCACHE=/cache/go-build" >> "$GITHUB_ENV" - else - mkdir -p /tmp/go-build-cache - echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV" + mkdir -p /tmp/go-build-cache + mounted=false + if [ -d /cache/go-build ] && command -v fuse-overlayfs >/dev/null 2>&1 && [ -e /dev/fuse ]; then + mkdir -p /tmp/go-cache-upper /tmp/go-cache-work + if fuse-overlayfs -o lowerdir=/cache/go-build,upperdir=/tmp/go-cache-upper,workdir=/tmp/go-cache-work /tmp/go-build-cache; then + mounted=true + else + echo "::warning::fuse-overlayfs mount failed, falling back to copy" + fi fi + if [ "$mounted" = "false" ] && [ -d /cache/go-build ]; then + timeout 120 cp -a /cache/go-build/. /tmp/go-build-cache/ || \ + echo "::warning::Failed to copy EFS cache, proceeding without cache" + fi + echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV" diff --git a/Dockerfile.github-actions-runner b/Dockerfile.github-actions-runner index 55755caaf5ed..edbf622f822f 100644 --- a/Dockerfile.github-actions-runner +++ b/Dockerfile.github-actions-runner @@ -11,6 +11,7 @@ RUN apt-get update && \ curl \ ca-certificates \ python3-pip \ + fuse-overlayfs \ && rm -rf /var/lib/apt/lists/* ARG TARGETARCH From 20e6157ad1983af2ff274e87c6921f2d97df2957 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Tue, 26 May 2026 13:28:09 -0400 Subject: [PATCH 14/14] ci: add Claude Code WIF auth test workflow Co-Authored-By: Claude Opus 4.6 --- .github/workflows/claude-wif-test.yaml | 37 ++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 .github/workflows/claude-wif-test.yaml diff --git a/.github/workflows/claude-wif-test.yaml b/.github/workflows/claude-wif-test.yaml new file mode 100644 index 000000000000..4643dd6653aa --- /dev/null +++ b/.github/workflows/claude-wif-test.yaml @@ -0,0 +1,37 @@ +name: Test Claude Code WIF Auth +on: + pull_request: + paths: + - .github/workflows/claude-wif-test.yaml + workflow_dispatch: {} + +permissions: + id-token: write + contents: read + +jobs: + test-wif: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - name: Authenticate to GCP via WIF + uses: google-github-actions/auth@c200f3691d83b41bf9bbd8638997a462592937ed # v2 + with: + project_id: hosted-control-planes + service_account: claude-gha@hosted-control-planes.iam.gserviceaccount.com + workload_identity_provider: projects/21066242673/locations/global/workloadIdentityPools/itpc-identity-pool/providers/github-com + + - name: Install Claude Code + run: | + curl -fsSL https://claude.ai/install.sh | sh + echo "$HOME/.local/bin" >> $GITHUB_PATH + + - name: Test Claude Code + env: + CLAUDE_CODE_USE_VERTEX: "1" + CLOUD_ML_REGION: global + ANTHROPIC_VERTEX_PROJECT_ID: hosted-control-planes + run: | + claude --version + claude -p "Say hello in one sentence" --max-turns 1