Skip to content
Merged
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
4 changes: 4 additions & 0 deletions cmd/install/assets/hypershift_operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,7 @@ type HyperShiftOperatorDeployment struct {
ScaleFromZeroSecret *corev1.Secret
ScaleFromZeroSecretKey string
ScaleFromZeroProvider string
HCPEgressBlockCIDRs []string
Comment thread
Ajpantuso marked this conversation as resolved.
}

func (o HyperShiftOperatorDeployment) Build() *appsv1.Deployment {
Expand Down Expand Up @@ -756,6 +757,9 @@ func (o HyperShiftOperatorDeployment) buildArgs() []string {
fmt.Sprintf("--enable-ci-debug-output=%t", o.EnableCIDebugOutput),
fmt.Sprintf("--private-platform=%s", o.PrivatePlatform),
}
for _, cidr := range o.HCPEgressBlockCIDRs {
args = append(args, fmt.Sprintf("--hcp-egress-block-cidrs=%s", cidr))
}
if o.RegistryOverrides != "" {
args = append(args, fmt.Sprintf("--registry-overrides=%s", o.RegistryOverrides))
}
Expand Down
18 changes: 18 additions & 0 deletions cmd/install/assets/hypershift_operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,24 @@ func TestBuildArgs(t *testing.T) {
fmt.Sprintf("--private-platform=%s", hyperv1.AWSPlatform),
},
},
{

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jparrill added the additional test case for flag propagation.

name: "When HCPEgressBlockCIDRs is set, it should include one flag per CIDR",
deployment: HyperShiftOperatorDeployment{
PrivatePlatform: string(hyperv1.NonePlatform),
HCPEgressBlockCIDRs: []string{"10.0.0.0/16", "10.1.0.0/16"},
},
expectContains: []string{
"--hcp-egress-block-cidrs=10.0.0.0/16",
"--hcp-egress-block-cidrs=10.1.0.0/16",
},
},
{
name: "When HCPEgressBlockCIDRs is empty, it should not include the flag",
deployment: HyperShiftOperatorDeployment{
PrivatePlatform: string(hyperv1.NonePlatform),
},
expectNotContains: []string{"--hcp-egress-block-cidrs"},
},
}

for _, tc := range tests {
Expand Down
15 changes: 15 additions & 0 deletions cmd/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"fmt"
"io"
"net"
"os"
"strconv"
"strings"
Expand Down Expand Up @@ -156,6 +157,7 @@ type Options struct {
ScaleFromZeroCredentialsSecret string
ScaleFromZeroCredentialsSecretKey string
RenderSensitive bool
HCPEgressBlockCIDRs []string
}

func (o *Options) Validate() error {
Expand All @@ -173,10 +175,21 @@ 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()...)

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))
}
}
return errs
}

func (o *Options) validatePlatformConfig() []error {
var errs []error
switch hyperv1.PlatformType(o.PrivatePlatform) {
Expand Down Expand Up @@ -439,6 +452,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.")
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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.

The operator's main.go validates these with net.ParseCIDR at startup, but hypershift install doesn't validate them in Options.Validate(). That means hypershift install --hcp-egress-block-cidrs=not-a-cidr renders a Deployment that will CrashLoopBackOff. Worth adding a validation guard in Validate() so the user gets immediate feedback before the manifest is applied.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the validations for the install command options.


cmd.RunE = func(cmd *cobra.Command, args []string) error {
return InstallHyperShiftOperator(cmd.Context(), cmd.OutOrStdout(), opts)
Expand Down Expand Up @@ -1294,6 +1308,7 @@ func setupOperatorResources(opts Options, userCABundleCM *corev1.ConfigMap, trus
ScaleFromZeroSecret: scaleFromZeroSecret,
ScaleFromZeroSecretKey: opts.ScaleFromZeroCredentialsSecretKey,
ScaleFromZeroProvider: opts.ScaleFromZeroProvider,
HCPEgressBlockCIDRs: opts.HCPEgressBlockCIDRs,
}.Build()
operatorService := assets.HyperShiftOperatorService{
Namespace: operatorNamespace,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,12 @@ type HostedClusterReconciler struct {
// via the shared ingress. Defaults to probeSharedIngressEndpoint. Override
// in tests to avoid real network calls.
ProbeSharedIngressEndpoint func(context context.Context, serviceIP string, servicePort int, kasHostname string) bool
// HCPEgressBlockCIDRs, when non-empty, provides a static list of CIDRs to
// block in HCP namespace egress NetworkPolicies. These replace the
// dynamically-discovered management cluster KAS endpoint IPs, eliminating
// NetworkPolicy churn during KAS rolling restarts that can trigger OVN
// port-group reconciliation races and cause traffic drops to HCP routers.
HCPEgressBlockCIDRs []string
}

// +kubebuilder:rbac:groups=hypershift.openshift.io,resources=hostedclusters,verbs=get;list;watch;create;update;patch;delete
Expand Down
57 changes: 30 additions & 27 deletions hypershift-operator/controllers/hostedcluster/network_policies.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,19 @@ func (r *HostedClusterReconciler) reconcileNetworkPolicies(ctx context.Context,
return fmt.Errorf("failed to reconcile kube-apiserver network policy: %w", err)
}

//nolint:staticcheck // SA1019: corev1.Endpoints is intentionally used for backward compatibility
kubernetesEndpoint := &corev1.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "kubernetes", Namespace: "default"}}
if err := r.Get(ctx, client.ObjectKeyFromObject(kubernetesEndpoint), kubernetesEndpoint); err != nil {
return fmt.Errorf("failed to get management cluster network config: %w", err)
var kasBlock []string
if len(r.HCPEgressBlockCIDRs) > 0 {
kasBlock = append(kasBlock, r.HCPEgressBlockCIDRs...)
} else {
//nolint:staticcheck // SA1019: corev1.Endpoints is intentionally used for backward compatibility
kubernetesEndpoint := &corev1.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "kubernetes", Namespace: "default"}}
if err := r.Get(ctx, client.ObjectKeyFromObject(kubernetesEndpoint), kubernetesEndpoint); err != nil {
return fmt.Errorf("getting management cluster kubernetes endpoints: %w", err)
}
kasBlock = kasEndpointsToCIDRs(kubernetesEndpoint)
}

if err := r.reconcileManagementKASPolicies(ctx, createOrUpdate, hcluster, hcp, controlPlaneNamespaceName, managementClusterNetwork, kubernetesEndpoint, controlPlaneOperatorAppliesManagementKASNetworkPolicyLabel); err != nil {
if err := r.reconcileManagementKASPolicies(ctx, createOrUpdate, hcluster, hcp, controlPlaneNamespaceName, managementClusterNetwork, kasBlock, controlPlaneOperatorAppliesManagementKASNetworkPolicyLabel); err != nil {
return err
}

Expand All @@ -96,7 +102,7 @@ func (r *HostedClusterReconciler) reconcileNetworkPolicies(ctx context.Context,
return fmt.Errorf("failed to reconcile monitoring network policy: %w", err)
}

if err := r.reconcilePlatformNetworkPolicies(ctx, log, createOrUpdate, hcluster, kubernetesEndpoint, managementClusterNetwork, version, controlPlaneNamespaceName); err != nil {
if err := r.reconcilePlatformNetworkPolicies(ctx, log, createOrUpdate, hcluster, kasBlock, managementClusterNetwork, version, controlPlaneNamespaceName); err != nil {
return err
}

Expand All @@ -114,15 +120,14 @@ func (r *HostedClusterReconciler) getManagementClusterNetwork(ctx context.Contex
return managementClusterNetwork, nil
}

//nolint:staticcheck // SA1019: corev1.Endpoints is intentionally used for backward compatibility
func (r *HostedClusterReconciler) reconcileManagementKASPolicies(ctx context.Context, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, hcp *hyperv1.HostedControlPlane, controlPlaneNamespaceName string, managementClusterNetwork *configv1.Network, kubernetesEndpoint *corev1.Endpoints, controlPlaneOperatorAppliesManagementKASNetworkPolicyLabel bool) error {
func (r *HostedClusterReconciler) reconcileManagementKASPolicies(ctx context.Context, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, hcp *hyperv1.HostedControlPlane, controlPlaneNamespaceName string, managementClusterNetwork *configv1.Network, kasBlock []string, controlPlaneOperatorAppliesManagementKASNetworkPolicyLabel bool) error {
if !controlPlaneOperatorAppliesManagementKASNetworkPolicyLabel || hcluster.Spec.Platform.Type != hyperv1.AWSPlatform {
return nil
}

policy := networkpolicy.ManagementKASNetworkPolicy(controlPlaneNamespaceName)
if _, err := createOrUpdate(ctx, r.Client, policy, func() error {
return reconcileManagementKASNetworkPolicy(policy, managementClusterNetwork, kubernetesEndpoint, r.ManagementClusterCapabilities.Has(capabilities.CapabilityDNS))
return reconcileManagementKASNetworkPolicy(policy, managementClusterNetwork, kasBlock, r.ManagementClusterCapabilities.Has(capabilities.CapabilityDNS))
}); err != nil {
return fmt.Errorf("failed to reconcile kube-apiserver network policy: %w", err)
}
Expand All @@ -139,14 +144,13 @@ func (r *HostedClusterReconciler) reconcileManagementKASPolicies(ctx context.Con
return nil
}

//nolint:staticcheck // SA1019: corev1.Endpoints is intentionally used for backward compatibility
func (r *HostedClusterReconciler) reconcilePlatformNetworkPolicies(ctx context.Context, log logr.Logger, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, kubernetesEndpoint *corev1.Endpoints, managementClusterNetwork *configv1.Network, version semver.Version, controlPlaneNamespaceName string) error {
func (r *HostedClusterReconciler) reconcilePlatformNetworkPolicies(ctx context.Context, log logr.Logger, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, kasBlock []string, managementClusterNetwork *configv1.Network, version semver.Version, controlPlaneNamespaceName string) error {
switch hcluster.Spec.Platform.Type {
case hyperv1.AWSPlatform, hyperv1.AzurePlatform, hyperv1.GCPPlatform:
policy := networkpolicy.PrivateRouterNetworkPolicy(controlPlaneNamespaceName)
ingressOnly := version.Major == 4 && version.Minor < 14
if _, err := createOrUpdate(ctx, r.Client, policy, func() error {
return reconcilePrivateRouterNetworkPolicy(policy, hcluster, kubernetesEndpoint, r.ManagementClusterCapabilities.Has(capabilities.CapabilityDNS), managementClusterNetwork, ingressOnly)
return reconcilePrivateRouterNetworkPolicy(policy, hcluster, kasBlock, r.ManagementClusterCapabilities.Has(capabilities.CapabilityDNS), managementClusterNetwork, ingressOnly)
}); err != nil {
return fmt.Errorf("failed to reconcile private router network policy: %w", err)
}
Expand Down Expand Up @@ -290,8 +294,7 @@ func reconcileKASNetworkPolicy(policy *networkingv1.NetworkPolicy, hcluster *hyp
return nil
}

//nolint:staticcheck // SA1019: corev1.Endpoints is intentionally used for backward compatibility
func reconcilePrivateRouterNetworkPolicy(policy *networkingv1.NetworkPolicy, _ *hyperv1.HostedCluster, kubernetesEndpoint *corev1.Endpoints, isOpenShiftDNS bool, managementClusterNetwork *configv1.Network, ingressOnly bool) error {
func reconcilePrivateRouterNetworkPolicy(policy *networkingv1.NetworkPolicy, _ *hyperv1.HostedCluster, kasBlockExceptions []string, isOpenShiftDNS bool, managementClusterNetwork *configv1.Network, ingressOnly bool) error {
httpPort := intstr.FromInt(8080)
httpsPort := intstr.FromInt(8443)
protocol := corev1.ProtocolTCP
Expand Down Expand Up @@ -330,11 +333,12 @@ func reconcilePrivateRouterNetworkPolicy(policy *networkingv1.NetworkPolicy, _ *
}
}

// Allow to any destination not on the management cluster service network
// i.e. block all inter-namespace egress not allowed by other rules.
// Also do not allow Kubernetes endpoint IPs explicitly
// i.e. block access to management cluster KAS.
exceptions := append(kasEndpointsToCIDRs(kubernetesEndpoint), clusterNetworks...)
// Allow to any destination not on the management cluster pod network and
// not on the KAS block CIDRs (either KAS endpoint /32s or the MC machine
// network CIDR depending on operator configuration).
exceptions := make([]string, 0, len(kasBlockExceptions)+len(clusterNetworks))
exceptions = append(exceptions, kasBlockExceptions...)
exceptions = append(exceptions, clusterNetworks...)
policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{
{
To: []networkingv1.NetworkPolicyPeer{
Expand Down Expand Up @@ -815,9 +819,7 @@ func reconcileSameNamespaceNetworkPolicy(policy *networkingv1.NetworkPolicy) err

// reconcileManagementKASNetworkPolicy selects pods excluding the ones having NeedManagementKASAccessLabel and specific operands.
// It denies egress traffic to the management cluster clusterNetwork and to the KAS endpoints.
//
//nolint:staticcheck // SA1019: corev1.Endpoints is intentionally used for backward compatibility
func reconcileManagementKASNetworkPolicy(policy *networkingv1.NetworkPolicy, managementClusterNetwork *configv1.Network, kubernetesEndpoint *corev1.Endpoints, isOpenShiftDNS bool) error {
func reconcileManagementKASNetworkPolicy(policy *networkingv1.NetworkPolicy, managementClusterNetwork *configv1.Network, kasBlockExceptions []string, isOpenShiftDNS bool) error {
// Allow traffic to same namespace
policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{
{
Expand All @@ -837,11 +839,12 @@ func reconcileManagementKASNetworkPolicy(policy *networkingv1.NetworkPolicy, man
}
}

// Allow to any destination not on the management cluster service network
// i.e. block all inter-namespace egress not allowed by other rules.
// Also do not allow Kubernetes endpoint IPs explicitly
// i.e. block access to management cluster KAS.
exceptions := append(kasEndpointsToCIDRs(kubernetesEndpoint), clusterNetworks...)
// Allow to any destination not on the management cluster pod network and
// not on the KAS block CIDRs (either KAS endpoint /32s or the MC machine
// network CIDR depending on operator configuration).
exceptions := make([]string, 0, len(kasBlockExceptions)+len(clusterNetworks))
exceptions = append(exceptions, kasBlockExceptions...)
exceptions = append(exceptions, clusterNetworks...)
policy.Spec.Egress = append(policy.Spec.Egress,
networkingv1.NetworkPolicyEgressRule{
To: []networkingv1.NetworkPolicyPeer{
Expand Down
Loading