ROSAENG-8224: refactor(ho): improve --hcp-egress-block-cidrs validation

cmd/install/install.go

@@ -20,7 +20,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"net"
 	"os"
 	"strconv"
 	"strings"
@@ -201,19 +200,16 @@ func (o *Options) Validate() error {
 	errs = append(errs, o.validateScaleFromZeroConfig()...)
 	errs = append(errs, o.validateMonitoringConfig()...)
 	errs = append(errs, o.validateMiscConfig()...)
-	errs = append(errs, o.validateHCPEgressBlockCIDRs()...)
+	errs = append(errs, o.validateHCPEgressBlockCIDRs())
 
 	return errors.NewAggregate(errs)
 }
 
-func (o *Options) validateHCPEgressBlockCIDRs() []error {
-	var errs []error
-	for _, cidr := range o.HCPEgressBlockCIDRs {
-		if _, _, err := net.ParseCIDR(cidr); err != nil {
-			errs = append(errs, fmt.Errorf("invalid --hcp-egress-block-cidrs value %q: %w", cidr, err))
-		}
+func (o *Options) validateHCPEgressBlockCIDRs() error {
+	if err := util.ValidateIPv4CIDRs(o.HCPEgressBlockCIDRs); err != nil {
+		return fmt.Errorf("invalid --hcp-egress-block-cidrs: %w", err)
 	}
-	return errs
+	return nil
 }
 
 func (o *Options) validatePlatformConfig() []error {
@@ -515,7 +511,7 @@ func NewCommand() *cobra.Command {
 	cmd.PersistentFlags().StringVar(&opts.ScaleFromZeroCreds, "scale-from-zero-creds", opts.ScaleFromZeroCreds, "Path to credentials file for scale-from-zero instance type queries")
 	cmd.PersistentFlags().StringVar(&opts.ScaleFromZeroCredentialsSecret, "scale-from-zero-secret", opts.ScaleFromZeroCredentialsSecret, "Name of existing secret containing scale-from-zero credentials (alternative to --scale-from-zero-creds)")
 	cmd.PersistentFlags().StringVar(&opts.ScaleFromZeroCredentialsSecretKey, "scale-from-zero-secret-key", opts.ScaleFromZeroCredentialsSecretKey, "Key within the scale-from-zero credentials secret (default: credentials)")
-	cmd.PersistentFlags().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.")
+	cmd.PersistentFlags().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. Only IPv4 CIDRs are supported. May be specified multiple times.")
 	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/util/util.go

@@ -2,11 +2,33 @@ package util
 
 import (
 	"fmt"
+	"net"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// ValidateIPv4CIDRs validates that each CIDR in the slice is a valid IPv4 CIDR.
+// It returns an error listing all invalid entries.
+func ValidateIPv4CIDRs(cidrs []string) error {
+	var msgs []string
+	for _, cidr := range cidrs {
+		_, ipNet, err := net.ParseCIDR(cidr)
+		if err != nil {
+			msgs = append(msgs, fmt.Sprintf("%q: %v", cidr, err))
+			continue
+		}
+		if ipNet.IP.To4() == nil {
+			msgs = append(msgs, fmt.Sprintf("%q: IPv6 CIDRs are not supported", cidr))
+		}
+	}
+	if len(msgs) > 0 {
+		return fmt.Errorf("%s", strings.Join(msgs, "; "))
+	}
+	return nil
+}
+
 // ValidateRequiredOption returns a cobra style error message when the flag value is empty
 func ValidateRequiredOption(flag string, value string) error {
 	if len(value) == 0 {

cmd/util/util_test.go

@@ -0,0 +1,71 @@
+package util
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestValidateIPv4CIDRs(t *testing.T) {
+	tests := []struct {
+		name        string
+		cidrs       []string
+		wantErr     bool
+		errContains []string
+	}{
+		{
+			name:    "When cidrs is empty, it should return no error",
+			cidrs:   nil,
+			wantErr: false,
+		},
+		{
+			name:    "When cidrs contains a valid IPv4 CIDR, it should return no error",
+			cidrs:   []string{"10.0.0.0/16"},
+			wantErr: false,
+		},
+		{
+			name:    "When cidrs contains multiple valid IPv4 CIDRs, it should return no error",
+			cidrs:   []string{"10.0.0.0/16", "172.16.0.0/12", "192.168.0.0/24"},
+			wantErr: false,
+		},
+		{
+			name:        "When cidrs contains an invalid CIDR string, it should return an error",
+			cidrs:       []string{"not-a-cidr"},
+			wantErr:     true,
+			errContains: []string{`"not-a-cidr"`},
+		},
+		{
+			name:        "When cidrs contains an IPv6 CIDR, it should return an error",
+			cidrs:       []string{"fd00::/64"},
+			wantErr:     true,
+			errContains: []string{`"fd00::/64": IPv6 CIDRs are not supported`},
+		},
+		{
+			name:        "When cidrs contains multiple CIDRs including an IPv6 one, it should return an error for the IPv6 CIDR",
+			cidrs:       []string{"10.0.0.0/16", "fd00::/64"},
+			wantErr:     true,
+			errContains: []string{`"fd00::/64": IPv6 CIDRs are not supported`},
+		},
+		{
+			name:        "When cidrs contains multiple invalid CIDRs, it should return an error mentioning each one",
+			cidrs:       []string{"not-a-cidr", "fd00::/64"},
+			wantErr:     true,
+			errContains: []string{`"not-a-cidr"`, `"fd00::/64": IPv6 CIDRs are not supported`},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			err := ValidateIPv4CIDRs(tt.cidrs)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				for _, substr := range tt.errContains {
+					g.Expect(err.Error()).To(ContainSubstring(substr))
+				}
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+			}
+		})
+	}
+}

hypershift-operator/main.go

@@ -19,14 +19,14 @@ import (
 	"crypto/tls"
 	"encoding/json"
 	"fmt"
-	"net"
 	"os"
 	"strings"
 	"time"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
 	awsutil "github.com/openshift/hypershift/cmd/infra/aws/util"
 	"github.com/openshift/hypershift/cmd/install/assets"
+	cmdutil "github.com/openshift/hypershift/cmd/util"
 	cpofeaturegate "github.com/openshift/hypershift/control-plane-operator/featuregates"
 	pkiconfig "github.com/openshift/hypershift/control-plane-pki-operator/config"
 	etcdrecovery "github.com/openshift/hypershift/etcd-recovery"
@@ -213,11 +213,11 @@ func NewStartCommand() *cobra.Command {
 	cmd.Flags().StringVar(&opts.ScaleFromZeroProvider, "scale-from-zero-provider", opts.ScaleFromZeroProvider, "Platform type for scale-from-zero autoscaling (aws)")
 	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.OTELEndpoint, "otel-endpoint", os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT"), "OpenTelemetry collector endpoint (OTLP/gRPC). Empty disables tracing.")
 	cmd.Flags().StringVar(&opts.OTELSampler, "otel-sampler", os.Getenv("OTEL_TRACES_SAMPLER"), "Trace sampler type (default: parentbased_always_on)")
 	cmd.Flags().StringVar(&opts.OTELSamplerArg, "otel-sampler-arg", os.Getenv("OTEL_TRACES_SAMPLER_ARG"), "Trace sampler argument (e.g. ratio 0.0-1.0)")
 	cmd.Flags().StringVar(&opts.OTELCorrelationAttrs, "otel-correlation-attrs", os.Getenv("OTEL_CORRELATION_ATTRS"), "Comma-separated span attribute names for cross-service correlation (e.g. cs.cluster.id). Each key is set to the cluster infraID on reconcile spans. Empty disables correlation.")
+	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. Only IPv4 CIDRs are supported. 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).")
 
 	// 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.
@@ -245,13 +245,6 @@ func NewStartCommand() *cobra.Command {
 			os.Exit(1)
 		}
 
-		for _, cidr := range opts.HCPEgressBlockCIDRs {
-			if _, _, err := net.ParseCIDR(cidr); err != nil {
-				fmt.Fprintf(os.Stderr, "invalid --hcp-egress-block-cidrs value %q: %v\n", cidr, err)
-				os.Exit(1)
-			}
-		}
-
 		if err := run(ctx, &opts, ctrl.Log.WithName("setup")); err != nil {
 			fmt.Println(err)
 			os.Exit(1)
@@ -408,6 +401,13 @@ func validateStartOptions(opts *StartOptions, log logr.Logger) error {
 		return fmt.Errorf("--etcd-backup-max-count must be at least 1, got %d", opts.EtcdBackupMaxCount)
 	}
 
+	if err := cmdutil.ValidateIPv4CIDRs(opts.HCPEgressBlockCIDRs); err != nil {
+		return fmt.Errorf("invalid --hcp-egress-block-cidrs: %w", err)
+	}
+	if len(opts.HCPEgressBlockCIDRs) > 0 {
+		log.Info("Static HCP egress block CIDRs configured", "cidrs", opts.HCPEgressBlockCIDRs)
+	}
+
 	supportedProviders := set.New("aws", "azure")
 	if opts.ScaleFromZeroCreds != "" {
 		if opts.ScaleFromZeroProvider == "" {