Skip to content

OCPBUGS-20161: Remove unncessary kas port exposure on service #3139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
enxebre wants to merge 1 commit into from
Closed

OCPBUGS-20161: Remove unncessary kas port exposure on service #3139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion control-plane-operator/controllers/hostedcontrolplane/cno/clusternetworkoperator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func NewParams(hcp *hyperv1.HostedControlPlane, version string, releaseImageProv
p.DeploymentConfig.SetDefaultSecurityContext = setDefaultSecurityContext
if util.IsPrivateHCP(hcp) {
p.APIServerAddress = fmt.Sprintf("api.%s.hypershift.local", hcp.Name)
p.APIServerPort = util.InternalAPIPortWithDefault(hcp, config.DefaultAPIServerPort)
p.APIServerPort = 443
} else {
p.APIServerAddress = hcp.Status.ControlPlaneEndpoint.Host
p.APIServerPort = hcp.Status.ControlPlaneEndpoint.Port
Expand Down
4 changes: 2 additions & 2 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,7 +1401,7 @@ func (r *HostedControlPlaneReconciler) reconcileHCPRouterServices(ctx context.Co
if util.IsPrivateHCP(hcp) {
svc := manifests.PrivateRouterService(hcp.Namespace)
if _, err := createOrUpdate(ctx, r.Client, svc, func() error {
return ingress.ReconcileRouterService(svc, util.InternalAPIPortWithDefault(hcp, config.DefaultAPIServerPort), true, true)
return ingress.ReconcileRouterService(svc, true, true)
}); err != nil {
return fmt.Errorf("failed to reconcile private router service: %w", err)
}
Expand All @@ -1423,7 +1423,7 @@ func (r *HostedControlPlaneReconciler) reconcileHCPRouterServices(ctx context.Co
// When Public access endpoint we need to create a Service type LB external for the KAS.
if util.IsPublicHCP(hcp) && exposeKASThroughRouter {
if _, err := createOrUpdate(ctx, r.Client, pubSvc, func() error {
return ingress.ReconcileRouterService(pubSvc, util.InternalAPIPortWithDefault(hcp, config.DefaultAPIServerPort), false, util.IsPrivateHCP(hcp))
return ingress.ReconcileRouterService(pubSvc, false, util.IsPrivateHCP(hcp))
}); err != nil {
return fmt.Errorf("failed to reconcile router service: %w", err)
}
Expand Down
1 change: 0 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,6 @@ func TestReconcileHCPRouterServices(t *testing.T) {
Selector: map[string]string{"app": "private-router"},
Ports: []corev1.ServicePort{
{Name: "https", Port: 443, TargetPort: intstr.FromString("https"), Protocol: corev1.ProtocolTCP},
{Name: "kube-apiserver", Port: 6443, TargetPort: intstr.FromString("https"), Protocol: corev1.ProtocolTCP},
},
},
}
Expand Down
22 changes: 2 additions & 20 deletions control-plane-operator/controllers/hostedcontrolplane/ingress/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func buildHCPRouterContainerMain(image string) func(*corev1.Container) {
}
}

func ReconcileRouterService(svc *corev1.Service, kasPort int32, internal, crossZoneLoadBalancingEnabled bool) error {
func ReconcileRouterService(svc *corev1.Service, internal, crossZoneLoadBalancingEnabled bool) error {
if svc.Annotations == nil {
svc.Annotations = map[string]string{}
}
Expand All @@ -247,25 +247,14 @@ func ReconcileRouterService(svc *corev1.Service, kasPort int32, internal, crossZ
svc.Spec.Type = corev1.ServiceTypeLoadBalancer
svc.Spec.Selector = hcpRouterLabels()
foundHTTPS := false
foundKAS := false

// TODO (alberto): why this criteria?
// Introduced here https://github.com/openshift/hypershift/pull/1614/files#diff-62c16653415b8d89921cb26796abc479c31da1654095f7c46b551b470533d66dR368-R372.
if kasPort == 443 {
foundKAS = true
}
Comment on lines -250 to -256

@mjlshen mjlshen Oct 26, 2023

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.

I'm not familiar with the use-case, but is this so that someone could separate router/KAS traffic onto different ports? If so, this would just be a guard for the ROSA HCP use-case where we use the same port for both. We would then need the if !foundKas block further down at line 279

@enxebre enxebre Oct 26, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not familiar with the use-case, but is this so that someone could separate router/KAS traffic onto different ports? If so, this would just be a guard for the ROSA HCP use-case where we use the same port for both. We would then need the if !foundKas block further down at line 279

Might be the original reason, I'm not aware of this use atm.
But also the if kasPort == 443 {foundKAS = true} is not a valid criteria any more for that potential use case.

for i, port := range svc.Spec.Ports {
switch port.Name {
case "https":
svc.Spec.Ports[i].Port = 443
svc.Spec.Ports[i].TargetPort = intstr.FromString("https")
svc.Spec.Ports[i].Protocol = corev1.ProtocolTCP
foundHTTPS = true
case "kube-apiserver":
svc.Spec.Ports[i].Port = kasPort
svc.Spec.Ports[i].TargetPort = intstr.FromString("https")
svc.Spec.Ports[i].Protocol = corev1.ProtocolTCP
foundKAS = true
Comment on lines -264 to -268

@mjlshen mjlshen Oct 26, 2023

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.

I think removing this is correct

}
}
if !foundHTTPS {
Expand All @@ -276,14 +265,7 @@ func ReconcileRouterService(svc *corev1.Service, kasPort int32, internal, crossZ
Protocol: corev1.ProtocolTCP,
})
}
if !foundKAS {
svc.Spec.Ports = append(svc.Spec.Ports, corev1.ServicePort{
Name: "kube-apiserver",
Port: kasPort,
TargetPort: intstr.FromString("https"),
Protocol: corev1.ProtocolTCP,
})
}

return nil
}

Expand Down
3 changes: 2 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/kas/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,8 +382,9 @@ func (p *KubeAPIServerParams) ExternalURL() string {
return fmt.Sprintf("https://%s:%d", p.ExternalAddress, p.ExternalPort)
}

// InternalURL is used by ReconcileBootstrapKubeconfigSecret.
func (p *KubeAPIServerParams) InternalURL() string {
return fmt.Sprintf("https://%s:%d", p.InternalAddress, p.InternalPort)
return fmt.Sprintf("https://%s:%d", p.InternalAddress, 443)
}

func (p *KubeAPIServerParams) ExternalKubeconfigKey() string {
Expand Down
2 changes: 1 addition & 1 deletion hypershift-operator/controllers/nodepool/haproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (r *NodePoolReconciler) reconcileHAProxyIgnitionConfig(ctx context.Context,

if util.IsPrivateHC(hcluster) {
apiServerExternalAddress = fmt.Sprintf("api.%s.hypershift.local", hcluster.Name)
apiServerExternalPort = util.InternalAPIPortFromHostedClusterWithDefault(hcluster, config.DefaultAPIServerPort)
apiServerExternalPort = 443
} else {
if hcluster.Status.KubeConfig == nil {
return "", true, nil
Expand Down
8 changes: 4 additions & 4 deletions hypershift-operator/controllers/nodepool/haproxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ kind: Config`
hc.Spec.Networking.ServiceNetwork = []hyperv1.ServiceNetworkEntry{{CIDR: *ipnet.MustParseCIDR("192.168.1.0/24")}}
}),

expectedHAProxyConfigContent: []string{"api." + hc().Name + ".hypershift.local:6443"},
expectedHAProxyConfigContent: []string{"api." + hc().Name + ".hypershift.local:443"},
},
{
name: "private cluster uses .local address and custom apiserver port",
Expand All @@ -104,7 +104,7 @@ kind: Config`
hc.Spec.Networking.ServiceNetwork = []hyperv1.ServiceNetworkEntry{{CIDR: *ipnet.MustParseCIDR("192.168.1.0/24")}}
}),

expectedHAProxyConfigContent: []string{"api." + hc().Name + ".hypershift.local:6443"},
expectedHAProxyConfigContent: []string{"api." + hc().Name + ".hypershift.local:443"},
},
{
name: "public and private cluster uses .local address and custom apiserver port",
Expand All @@ -126,11 +126,11 @@ kind: Config`
other: []crclient.Object{&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Name: "kk", Namespace: hc().Namespace},
Data: map[string][]byte{
"kubeconfig": []byte(kubeconfig(6443)),
"kubeconfig": []byte(kubeconfig(443)),
},
}},

expectedHAProxyConfigContent: []string{"kubeconfig-host:6443"},
expectedHAProxyConfigContent: []string{"kubeconfig-host:443"},
},
{
name: "public cluster uses address from kubeconfig and custom port",
Expand Down
2 changes: 1 addition & 1 deletion ...ta/zz_fixture_TestReconcileHAProxyIgnitionConfig_private_cluster_uses_.local_address.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ backend remote_apiserver
option httpchk GET /version
option log-health-checks
default-server inter 10s fall 3 rise 3
server controlplane api.hc.hypershift.local:6443
server controlplane api.hc.hypershift.local:443
2 changes: 1 addition & 1 deletion ...re_TestReconcileHAProxyIgnitionConfig_public_and_private_cluster_uses_.local_address.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ backend remote_apiserver
option httpchk GET /version
option log-health-checks
default-server inter 10s fall 3 rise 3
server controlplane api.hc.hypershift.local:6443
server controlplane api.hc.hypershift.local:443
2 changes: 1 addition & 1 deletion ...xture_TestReconcileHAProxyIgnitionConfig_public_cluster_uses_address_from_kubeconfig.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ backend remote_apiserver
option httpchk GET /version
option log-health-checks
default-server inter 10s fall 3 rise 3
server controlplane kubeconfig-host:6443
server controlplane kubeconfig-host:443
9 changes: 0 additions & 9 deletions support/util/networking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,6 @@ func InternalAPIPortWithDefault(hcp *hyperv1.HostedControlPlane, defaultValue in
return defaultValue
}

// InternalAPIPortFromHostedClusterWithDefault will retrieve the port to use to contact the APIServer over the Kubernetes service domain
// kube-apiserver.NAMESPACE.svc.cluster.local:INTERNAL_API_PORT
func InternalAPIPortFromHostedClusterWithDefault(hc *hyperv1.HostedCluster, defaultValue int32) int32 {
if hc.Spec.Networking.APIServer != nil && hc.Spec.Networking.APIServer.Port != nil {
return *hc.Spec.Networking.APIServer.Port
}
return defaultValue
}

func AdvertiseAddress(hcp *hyperv1.HostedControlPlane) *string {
if hcp != nil && hcp.Spec.Networking.APIServer != nil {
return hcp.Spec.Networking.APIServer.AdvertiseAddress
Expand Down