diff --git a/cmd/install/assets/hypershift_operator.go b/cmd/install/assets/hypershift_operator.go index 791ca2ea79df..abb975a26dc7 100644 --- a/cmd/install/assets/hypershift_operator.go +++ b/cmd/install/assets/hypershift_operator.go @@ -7,8 +7,10 @@ import ( "fmt" "io" "io/fs" + "net" "path/filepath" "slices" + "strconv" "strings" "time" @@ -559,6 +561,7 @@ type HyperShiftOperatorDeployment struct { ScaleFromZeroSecretKey string ScaleFromZeroProvider string HCPEgressBlockCIDRs []string + PprofAddr string } func (o HyperShiftOperatorDeployment) Build() *appsv1.Deployment { @@ -723,6 +726,12 @@ func (o HyperShiftOperatorDeployment) Build() *appsv1.Deployment { }, } + if port, ok := o.pprofContainerPort(); ok { + deployment.Spec.Template.Spec.Containers[0].Ports = append( + deployment.Spec.Template.Spec.Containers[0].Ports, port, + ) + } + // Azure Workload Identity requires this pod label for the webhook to inject // federated tokens (AZURE_CLIENT_ID, AZURE_TENANT_ID, AZURE_FEDERATED_TOKEN_FILE). if o.AzurePLSManagedIdentityClientID != "" { @@ -794,6 +803,9 @@ func (o HyperShiftOperatorDeployment) buildArgs() []string { if o.RegistryOverrides != "" { args = append(args, fmt.Sprintf("--registry-overrides=%s", o.RegistryOverrides)) } + if _, ok := o.pprofContainerPort(); ok { + args = append(args, "--pprof-addr="+o.PprofAddr) + } return args } @@ -950,6 +962,28 @@ func (o HyperShiftOperatorDeployment) addScaleFromZeroResources(args *[]string, }) } +// pprofContainerPort parses o.PprofAddr and returns the corresponding +// ContainerPort when the address is non-empty and valid. The installer +// validates the address before Build is called, so errors here are a safety net. +func (o HyperShiftOperatorDeployment) pprofContainerPort() (corev1.ContainerPort, bool) { + if o.PprofAddr == "" { + return corev1.ContainerPort{}, false + } + _, portStr, err := net.SplitHostPort(o.PprofAddr) + if err != nil { + return corev1.ContainerPort{}, false + } + port, err := strconv.Atoi(portStr) + if err != nil || port < 1 || port > 65535 { + return corev1.ContainerPort{}, false + } + return corev1.ContainerPort{ + Name: "pprof", + ContainerPort: int32(port), + Protocol: corev1.ProtocolTCP, + }, true +} + func (o HyperShiftOperatorDeployment) resolveImage() string { image := o.OperatorImage if mapImage, ok := o.Images["hypershift-operator"]; ok { diff --git a/cmd/install/assets/hypershift_operator_test.go b/cmd/install/assets/hypershift_operator_test.go index 14c19c9de251..7fa1be12271a 100644 --- a/cmd/install/assets/hypershift_operator_test.go +++ b/cmd/install/assets/hypershift_operator_test.go @@ -599,6 +599,85 @@ func TestHyperShiftOperatorDeployment_Build(t *testing.T) { } } +func TestHyperShiftOperatorDeployment_Build_PprofAddr(t *testing.T) { + testNamespace := "hypershift" + testOperatorImage := "myimage" + baseParams := func() HyperShiftOperatorDeployment { + return HyperShiftOperatorDeployment{ + Namespace: &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: testNamespace}, + }, + OperatorImage: testOperatorImage, + ServiceAccount: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "hypershift"}, + }, + Replicas: 1, + PrivatePlatform: "None", + } + } + + tests := []struct { + name string + pprofAddr string + expectArg bool + expectedArgValue string + expectPort bool + expectedPort int32 + }{ + { + name: "When pprof-addr is empty, it should not include pprof arg or container port", + pprofAddr: "", + }, + { + name: "When pprof-addr is set with port only, it should include pprof arg and container port", + pprofAddr: ":6060", + expectArg: true, + expectedArgValue: "--pprof-addr=:6060", + expectPort: true, + expectedPort: 6060, + }, + { + name: "When pprof-addr is set with host and port, it should include pprof arg and container port", + pprofAddr: "0.0.0.0:9999", + expectArg: true, + expectedArgValue: "--pprof-addr=0.0.0.0:9999", + expectPort: true, + expectedPort: 9999, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + params := baseParams() + params.PprofAddr = tc.pprofAddr + deployment := params.Build() + args := deployment.Spec.Template.Spec.Containers[0].Args + ports := deployment.Spec.Template.Spec.Containers[0].Ports + + if tc.expectArg { + g.Expect(args).To(ContainElement(tc.expectedArgValue)) + } else { + for _, arg := range args { + g.Expect(arg).NotTo(HavePrefix("--pprof-addr")) + } + } + + if tc.expectPort { + g.Expect(ports).To(ContainElement(corev1.ContainerPort{ + Name: "pprof", + ContainerPort: tc.expectedPort, + Protocol: corev1.ProtocolTCP, + })) + } else { + for _, p := range ports { + g.Expect(p.Name).NotTo(Equal("pprof")) + } + } + }) + } +} + func TestExternalDNSDeployment_Build(t *testing.T) { baseDeployment := func() ExternalDNSDeployment { return ExternalDNSDeployment{ diff --git a/cmd/install/install.go b/cmd/install/install.go index 48a2d05d7908..b01322323b16 100644 --- a/cmd/install/install.go +++ b/cmd/install/install.go @@ -166,6 +166,7 @@ type Options struct { HCPEgressBlockCIDRs []string InstallScope string DisableCAPIMigration bool + OperatorPprofAddr string } func (o *Options) Complete() error { @@ -199,6 +200,7 @@ func (o *Options) Validate() error { errs = append(errs, o.validateMonitoringConfig()...) errs = append(errs, o.validateMiscConfig()...) errs = append(errs, o.validateHCPEgressBlockCIDRs()...) + errs = append(errs, o.validateOperatorPprofAddr()...) return errors.NewAggregate(errs) } @@ -213,6 +215,25 @@ func (o *Options) validateHCPEgressBlockCIDRs() []error { return errs } +func (o *Options) validateOperatorPprofAddr() []error { + if o.OperatorPprofAddr == "" { + return nil + } + _, portStr, err := net.SplitHostPort(o.OperatorPprofAddr) + if err != nil { + return []error{fmt.Errorf("invalid --operator-pprof-addr value %q: %w", o.OperatorPprofAddr, err)} + } + port, err := strconv.Atoi(portStr) + if err != nil || port < 1 || port > 65535 { + return []error{fmt.Errorf("invalid --operator-pprof-addr port %q: must be an integer between 1 and 65535", portStr)} + } + switch port { + case 9000, 9443: + return []error{fmt.Errorf("invalid --operator-pprof-addr port %q: conflicts with an existing operator listener", portStr)} + } + return nil +} + func (o *Options) validatePlatformConfig() []error { var errs []error switch hyperv1.PlatformType(o.PrivatePlatform) { @@ -515,6 +536,7 @@ func NewCommand() *cobra.Command { cmd.PersistentFlags().StringVar(&opts.AWSRoleCredentialSource, "aws-role-credential-source", aws.CredentialSourceWebIdentity, "Credential source for AWS role ARN flags: 'web-identity' (uses projected SA token) or 'ec2-instance-metadata' (uses EC2 instance metadata)") cmd.PersistentFlags().StringVar(&opts.AWSOperatorRolesFile, "aws-operator-roles-file", "", "Path to JSON output file from 'hypershift create operator-roles aws' (sets all three role ARN flags at once)") cmd.Flags().StringVar(&opts.InstallScope, "install-scope", string(OutputAll), "Scope of installation: 'all' installs CRDs and resources (default), 'crds' installs only CRDs, 'resources' installs only resources assuming CRDs were installed previously (operator deployment and RBAC)") + cmd.PersistentFlags().StringVar(&opts.OperatorPprofAddr, "operator-pprof-addr", "", "The address the HyperShift Operator pprof endpoint binds to (e.g. :6060). Disabled when empty.") cmd.RunE = func(cmd *cobra.Command, args []string) error { return InstallHyperShiftOperator(cmd.Context(), cmd.OutOrStdout(), opts) @@ -1394,6 +1416,7 @@ func setupOperatorResources(opts Options, userCABundleCM *corev1.ConfigMap, trus ScaleFromZeroSecretKey: opts.ScaleFromZeroCredentialsSecretKey, ScaleFromZeroProvider: opts.ScaleFromZeroProvider, HCPEgressBlockCIDRs: opts.HCPEgressBlockCIDRs, + PprofAddr: opts.OperatorPprofAddr, }.Build() operatorService := assets.HyperShiftOperatorService{ Namespace: operatorNamespace, diff --git a/cmd/install/install_test.go b/cmd/install/install_test.go index 2b8075dc0abb..d1056c90be07 100644 --- a/cmd/install/install_test.go +++ b/cmd/install/install_test.go @@ -2647,3 +2647,73 @@ func TestComplete(t *testing.T) { }) } } + +func TestValidateOperatorPprofAddr(t *testing.T) { + tests := []struct { + name string + opts Options + expectError bool + }{ + { + name: "When operator-pprof-addr is empty, it should pass", + opts: Options{}, + expectError: false, + }, + { + name: "When operator-pprof-addr is set with port only, it should pass", + opts: Options{OperatorPprofAddr: ":6060"}, + expectError: false, + }, + { + name: "When operator-pprof-addr is set with host and port, it should pass", + opts: Options{OperatorPprofAddr: "0.0.0.0:6060"}, + expectError: false, + }, + { + name: "When operator-pprof-addr is set with named host and port, it should pass", + opts: Options{OperatorPprofAddr: "localhost:6060"}, + expectError: false, + }, + { + name: "When operator-pprof-addr has no port separator, it should fail", + opts: Options{OperatorPprofAddr: "6060"}, + expectError: true, + }, + { + name: "When operator-pprof-addr has a non-numeric port, it should fail", + opts: Options{OperatorPprofAddr: ":pprof"}, + expectError: true, + }, + { + name: "When operator-pprof-addr has port 0, it should fail", + opts: Options{OperatorPprofAddr: ":0"}, + expectError: true, + }, + { + name: "When operator-pprof-addr has port greater than 65535, it should fail", + opts: Options{OperatorPprofAddr: ":65536"}, + expectError: true, + }, + { + name: "When operator-pprof-addr uses the metrics port, it should fail", + opts: Options{OperatorPprofAddr: ":9000"}, + expectError: true, + }, + { + name: "When operator-pprof-addr uses the manager port, it should fail", + opts: Options{OperatorPprofAddr: ":9443"}, + expectError: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + errs := tc.opts.validateOperatorPprofAddr() + if tc.expectError { + g.Expect(errs).NotTo(BeEmpty()) + } else { + g.Expect(errs).To(BeEmpty()) + } + }) + } +} diff --git a/hypershift-operator/main.go b/hypershift-operator/main.go index 00362ae3a634..97facc94be73 100644 --- a/hypershift-operator/main.go +++ b/hypershift-operator/main.go @@ -162,6 +162,7 @@ type StartOptions struct { ScaleFromZeroCreds string EtcdBackupMaxCount int HCPEgressBlockCIDRs []string + PprofAddr string } func NewStartCommand() *cobra.Command { @@ -203,6 +204,7 @@ func NewStartCommand() *cobra.Command { cmd.Flags().StringVar(&opts.ScaleFromZeroCreds, "scale-from-zero-creds", opts.ScaleFromZeroCreds, "Path to credentials file for scale-from-zero instance type queries") cmd.Flags().IntVar(&opts.EtcdBackupMaxCount, "etcd-backup-max-count", 5, "Maximum number of completed HCPEtcdBackup CRs to retain per HostedControlPlane") cmd.Flags().StringArrayVar(&opts.HCPEgressBlockCIDRs, "hcp-egress-block-cidrs", nil, "Static CIDRs to block in HCP namespace egress NetworkPolicies instead of dynamically-discovered hosting cluster KAS endpoint IPs. When specified, eliminates NetworkPolicy churn during hosting cluster KAS rolling restarts and avoids OVN port-group reconciliation races that can drop traffic to HCP routers. May be specified multiple times (e.g. --hcp-egress-block-cidrs=10.0.0.0/16 --hcp-egress-block-cidrs=10.1.0.0/16).") + cmd.Flags().StringVar(&opts.PprofAddr, "pprof-addr", "", "The address the pprof endpoint binds to. Disabled when empty.") // Attempt to determine featureset prior to adding featuregate flags. // It is safe to get the empty string from this as the empty string is the default featureset. @@ -433,7 +435,8 @@ func createManager(restConfig *rest.Config, webhookOptions webhook.Options, opts Metrics: metricsserver.Options{ BindAddress: opts.MetricsAddr, }, - WebhookServer: webhook.NewServer(webhookOptions), + PprofBindAddress: opts.PprofAddr, + WebhookServer: webhook.NewServer(webhookOptions), Client: crclient.Options{ Cache: &crclient.CacheOptions{ Unstructured: true,