Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions cmd/install/assets/hypershift_operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import (
"fmt"
"io"
"io/fs"
"net"
"path/filepath"
"slices"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -559,6 +561,7 @@ type HyperShiftOperatorDeployment struct {
ScaleFromZeroSecretKey string
ScaleFromZeroProvider string
HCPEgressBlockCIDRs []string
PprofAddr string
}

func (o HyperShiftOperatorDeployment) Build() *appsv1.Deployment {
Expand Down Expand Up @@ -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,
)
}
Comment on lines +729 to +733

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major

Reject pprof port 9000 in the shared validation helper.

buildArgs() always passes --metrics-addr=:9000 at Line [794], but pprofContainerPort() accepts :9000; both deployment generation and argument propagation then configure conflicting listeners, causing operator startup failure when upstream validation is bypassed. Reject the reserved metrics port here and keep the installer validator aligned.

Suggested fix
-	if err != nil || port < 1 || port > 65535 {
+	if err != nil || port < 1 || port > 65535 || port == 9000 {

Also applies to: 806-808, 965-986

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/install/assets/hypershift_operator.go` around lines 729 - 733, Update the
shared pprof port validation used by pprofContainerPort() to reject port 9000,
matching the installer validator and the fixed --metrics-addr=:9000
configuration in buildArgs(). Ensure invalid :9000 input does not add a
container port or propagate pprof arguments, while preserving existing behavior
for valid pprof ports.


// 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 != "" {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Comment on lines +965 to +985

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major

Still reject pprof addresses that collide with the metrics listener.

buildArgs() always passes --metrics-addr=:9000 at Line [794], but this helper accepts :9000; the operator can then fail when both listeners bind the same port. Reject the reserved metrics port in both installer validation and this safety-net helper. The prior parse-error concern is fixed, but this collision remains unresolved.

🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 981-981: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(port)
Note: [CWE-190] Integer Overflow or Wraparound.

(integer-overflow-narrowing-conversion-go)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/install/assets/hypershift_operator.go` around lines 965 - 985, Update the
pprof address validation used by buildArgs and
HyperShiftOperatorDeployment.pprofContainerPort to reject port 9000, matching
the fixed metrics listener configured by --metrics-addr=:9000. Preserve existing
empty, parse-error, and out-of-range rejection behavior while ensuring the
helper also returns false for the reserved metrics port.


func (o HyperShiftOperatorDeployment) resolveImage() string {
image := o.OperatorImage
if mapImage, ok := o.Images["hypershift-operator"]; ok {
Expand Down
79 changes: 79 additions & 0 deletions cmd/install/assets/hypershift_operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
23 changes: 23 additions & 0 deletions cmd/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ type Options struct {
HCPEgressBlockCIDRs []string
InstallScope string
DisableCAPIMigration bool
OperatorPprofAddr string
}

func (o *Options) Complete() error {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func (o *Options) validatePlatformConfig() []error {
var errs []error
switch hyperv1.PlatformType(o.PrivatePlatform) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
70 changes: 70 additions & 0 deletions cmd/install/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
})
}
}
5 changes: 4 additions & 1 deletion hypershift-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ type StartOptions struct {
ScaleFromZeroCreds string
EtcdBackupMaxCount int
HCPEgressBlockCIDRs []string
PprofAddr string
}

func NewStartCommand() *cobra.Command {
Expand Down Expand Up @@ -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.")
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// 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.
Expand Down Expand Up @@ -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,
Expand Down