Skip to content

OCPBUGS-7840: Untangle kas port #2964

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

Merged
openshift-merge-robot merged 2 commits into from
Sep 6, 2023
Merged

OCPBUGS-7840: Untangle kas port #2964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af1fbc7
Comment state of the world
enxebre Aug 30, 2023
4a5fb87
Drop default kas port to 443 and endpoint reconciliation for new clus…
enxebre Aug 30, 2023
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
7 changes: 5 additions & 2 deletions api/v1beta1/hostedcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,8 +635,11 @@ type APIServerNetworking struct {
AdvertiseAddress *string `json:"advertiseAddress,omitempty"`

// Port is the port at which the APIServer is exposed inside a node. Other
// pods using host networking cannot listen on this port. If not specified,
// 6443 is used.
// pods using host networking cannot listen on this port.
// If unset 6443 is used.
// This is useful to choose a port other than the default one which might interfere with customer environments e.g. https://github.com/openshift/hypershift/pull/356.
// Setting this to 443 is possible only for backward compatibility reasons and it's discouraged.
// Doing so, it would result in the controller overriding the KAS endpoint in the guest cluster having a discrepancy with the KAS Pod and potentially causing temporarily network failures.
Port *int32 `json:"port,omitempty"`

// AllowedCIDRBlocks is an allow list of CIDR blocks that can access the APIServer
Expand Down
9 changes: 8 additions & 1 deletion cmd/install/assets/hypershift-operator/hypershift.openshift.io_hostedclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5961,7 +5961,14 @@ spec:
port:
description: Port is the port at which the APIServer is exposed
inside a node. Other pods using host networking cannot listen
on this port. If not specified, 6443 is used.
on this port. If unset 6443 is used. This is useful to choose
a port other than the default one which might interfere
with customer environments e.g. https://github.com/openshift/hypershift/pull/356.
Setting this to 443 is possible only for backward compatibility
reasons and it's discouraged. Doing so, it would result
in the controller overriding the KAS endpoint in the guest
cluster having a discrepancy with the KAS Pod and potentially
causing temporarily network failures.
format: int32
type: integer
type: object
Expand Down
9 changes: 8 additions & 1 deletion cmd/install/assets/hypershift-operator/hypershift.openshift.io_hostedcontrolplanes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5929,7 +5929,14 @@ spec:
port:
description: Port is the port at which the APIServer is exposed
inside a node. Other pods using host networking cannot listen
on this port. If not specified, 6443 is used.
on this port. If unset 6443 is used. This is useful to choose
a port other than the default one which might interfere
with customer environments e.g. https://github.com/openshift/hypershift/pull/356.
Setting this to 443 is possible only for backward compatibility
reasons and it's discouraged. Doing so, it would result
in the controller overriding the KAS endpoint in the guest
cluster having a discrepancy with the KAS Pod and potentially
causing temporarily network failures.
format: int32
type: integer
type: object
Expand Down
2 changes: 1 addition & 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 @@ -180,7 +180,7 @@ func TestReconcileAPIServerService(t *testing.T) {
{
Protocol: corev1.ProtocolTCP,
Port: apiPort,
TargetPort: intstr.FromInt(6443),
TargetPort: intstr.FromInt(int(apiPort)),
},
},
LoadBalancerSourceRanges: allowCIDRString,
Expand Down
2 changes: 2 additions & 0 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 @@ -249,6 +249,8 @@ func ReconcileRouterService(svc *corev1.Service, kasPort int32, internal, crossZ
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
}
Expand Down
27 changes: 17 additions & 10 deletions 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 @@ -40,18 +40,23 @@ type KubeAPIServerParams struct {
CloudProviderConfig *corev1.LocalObjectReference `json:"cloudProviderConfig"`
CloudProviderCreds *corev1.LocalObjectReference `json:"cloudProviderCreds"`

ServiceAccountIssuer string `json:"serviceAccountIssuer"`
ServiceCIDRs []string `json:"serviceCIDRs"`
ClusterCIDRs []string `json:"clusterCIDRs"`
AdvertiseAddress string `json:"advertiseAddress"`
ExternalAddress string `json:"externalAddress"`
ExternalPort int32 `json:"externalPort"`
InternalAddress string `json:"internalAddress"`
InternalPort int32 `json:"internalPort"`
ServiceAccountIssuer string `json:"serviceAccountIssuer"`
ServiceCIDRs []string `json:"serviceCIDRs"`
ClusterCIDRs []string `json:"clusterCIDRs"`
AdvertiseAddress string `json:"advertiseAddress"`
ExternalAddress string `json:"externalAddress"`
// ExternalPort is the port coming from the status of the SVC which is exposing the KAS, e.g. common router LB, dedicated private/public/ LB...
// This is used to build kas urls for generated internal kubeconfigs for example.
ExternalPort int32 `json:"externalPort"`
InternalAddress string `json:"internalAddress"`
// InternalPort is the port that was used to expose the KAS SVC.
// This is used to build kas urls for generated external kubeconfigs for example.
InternalPort int32 `json:"internalPort"`
// APIServerPort is port to expose the KAS Pod.
APIServerPort int32 `json:"apiServerPort"`
ExternalOAuthAddress string `json:"externalOAuthAddress"`
ExternalOAuthPort int32 `json:"externalOAuthPort"`
EtcdURL string `json:"etcdAddress"`
APIServerPort int32 `json:"apiServerPort"`
KubeConfigRef *hyperv1.KubeconfigSecretRef `json:"kubeConfigRef"`
AuditWebhookRef *corev1.LocalObjectReference `json:"auditWebhookRef"`
ConsolePublicURL string `json:"consolePublicURL"`
Expand All @@ -65,7 +70,9 @@ type KubeAPIServerParams struct {
}

type KubeAPIServerServiceParams struct {
APIServerPort int
// APIServerPort is the port used for the SVC.
APIServerPort int
// APIServerListenPort is the port used for the TargetPort.
APIServerListenPort int
AllowedCIDRBlocks []string
OwnerReference *metav1.OwnerReference
Expand Down
2 changes: 1 addition & 1 deletion control-plane-operator/controllers/hostedcontrolplane/kas/params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestNewAPIServerParamsAPIAdvertiseAddressAndPort(t *testing.T) {
port: pointer.Int32(6789),
serviceNetworkCIDR: "10.0.0.0/24",
expectedAddress: config.DefaultAdvertiseIPv4Address,
expectedPort: config.DefaultAPIServerPort,
expectedPort: 6789,
},
{
name: "port set for NodePort service Publishing Strategy",
Expand Down
39 changes: 15 additions & 24 deletions control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ import (
"context"
"crypto/md5"
"fmt"
"net/url"
"reflect"
"strconv"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -99,7 +97,6 @@ type reconciler struct {
oauthPort int32
versions map[string]string
operateOnReleaseImage string
apiServerPort int32
}

// eventHandler is the handler used throughout. As this controller reconciles all kind of different resources
Expand All @@ -116,18 +113,6 @@ func Setup(opts *operator.HostedClusterConfigOperatorConfig) error {
return fmt.Errorf("failed to add to scheme: %w", err)
}

apiServerPort := int32(443)
apiServerURL, err := url.Parse(opts.Manager.GetConfig().Host)
if err != nil {
return fmt.Errorf("failed to parse apiserver host %s as url: %w", opts.Manager.GetConfig().Host, err)
}
if p := apiServerURL.Port(); p != "" {
numericPort, err := strconv.Atoi(p)
if err != nil {
return fmt.Errorf("failed to parse apiserver port string %s as int: %w", p, err)
}
apiServerPort = int32(numericPort)
}
uncachedClient, err := client.New(opts.Manager.GetConfig(), client.Options{
Scheme: opts.Manager.GetScheme(),
Mapper: opts.Manager.GetRESTMapper(),
Expand Down Expand Up @@ -169,7 +154,6 @@ func Setup(opts *operator.HostedClusterConfigOperatorConfig) error {
oauthPort: opts.OAuthPort,
versions: opts.Versions,
operateOnReleaseImage: opts.OperateOnReleaseImage,
apiServerPort: apiServerPort,
}})
if err != nil {
return fmt.Errorf("failed to construct controller: %w", err)
Expand Down Expand Up @@ -259,16 +243,23 @@ func (r *reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
errs = append(errs, fmt.Errorf("failed to reconcile crds: %w", err))
}

log.Info("reconciling kubernetes.default endpoints")
endpoints := manifests.APIServerEndpoints()
if _, err := r.CreateOrUpdate(ctx, r.client, endpoints, func() error {
if len(endpoints.Subsets) == 0 || len(endpoints.Subsets[0].Ports) == 0 {
// We only keep reconciling the endpoint for existing clusters that are relying on this for nodes haproxy to work.
// Otherwise, changing the haproxy config to !=443 would result in a NodePool rollout which want to avoid for existing clusters.
// Existing clusters are given the *hcp.Spec.Networking.APIServer.Port == 443 semantic as we were enforcing this default previously,
// and it's a now a forbidden operation.
if hcp.Spec.Networking.APIServer != nil && hcp.Spec.Networking.APIServer.Port != nil &&
*hcp.Spec.Networking.APIServer.Port == 443 {
log.Info("reconciling kubernetes.default endpoints")
endpoints := manifests.APIServerEndpoints()
if _, err := r.CreateOrUpdate(ctx, r.client, endpoints, func() error {
if len(endpoints.Subsets) == 0 || len(endpoints.Subsets[0].Ports) == 0 {
return nil
}
endpoints.Subsets[0].Ports[0].Port = 443
return nil
}); err != nil {
errs = append(errs, fmt.Errorf("failed to reconcile kubernetes.default endpoints: %w", err))
}
endpoints.Subsets[0].Ports[0].Port = r.apiServerPort
return nil
}); err != nil {
errs = append(errs, fmt.Errorf("failed to reconcile kubernetes.default endpoints: %w", err))
}

log.Info("reconciling install configmap")
Expand Down
7 changes: 5 additions & 2 deletions docs/content/reference/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,8 +864,11 @@ int32
</td>
<td>
<p>Port is the port at which the APIServer is exposed inside a node. Other
pods using host networking cannot listen on this port. If not specified,
6443 is used.</p>
pods using host networking cannot listen on this port.
If unset 6443 is used.
This is useful to choose a port other than the default one which might interfere with customer environments e.g. <a href="https://github.com/openshift/hypershift/pull/356">https://github.com/openshift/hypershift/pull/356</a>.
Setting this to 443 is possible only for backward compatibility reasons and it&rsquo;s discouraged.
Doing so, it would result in the controller overriding the KAS endpoint in the guest cluster having a discrepancy with the KAS Pod and potentially causing temporarily network failures.</p>
</td>
</tr>
<tr>
Expand Down
22 changes: 18 additions & 4 deletions hack/app-sre/saas_template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35495,8 +35495,15 @@ objects:
port:
description: Port is the port at which the APIServer is
exposed inside a node. Other pods using host networking
cannot listen on this port. If not specified, 6443 is
used.
cannot listen on this port. If unset 6443 is used. This
is useful to choose a port other than the default one
which might interfere with customer environments e.g.
https://github.com/openshift/hypershift/pull/356. Setting
this to 443 is possible only for backward compatibility
reasons and it's discouraged. Doing so, it would result
in the controller overriding the KAS endpoint in the guest
cluster having a discrepancy with the KAS Pod and potentially
causing temporarily network failures.
format: int32
type: integer
type: object
Expand Down Expand Up @@ -43204,8 +43211,15 @@ objects:
port:
description: Port is the port at which the APIServer is
exposed inside a node. Other pods using host networking
cannot listen on this port. If not specified, 6443 is
used.
cannot listen on this port. If unset 6443 is used. This
is useful to choose a port other than the default one
which might interfere with customer environments e.g.
https://github.com/openshift/hypershift/pull/356. Setting
this to 443 is possible only for backward compatibility
reasons and it's discouraged. Doing so, it would result
in the controller overriding the KAS endpoint in the guest
cluster having a discrepancy with the KAS Pod and potentially
causing temporarily network failures.
format: int32
type: integer
type: object
Expand Down
16 changes: 0 additions & 16 deletions hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4174,22 +4174,6 @@ func (r *HostedClusterReconciler) defaultAPIPortIfNeeded(ctx context.Context, hc
if hcluster.Spec.Networking.APIServer != nil && hcluster.Spec.Networking.APIServer.Port != nil {
return nil
}
for _, publishingStrategy := range hcluster.Spec.Services {
if publishingStrategy.Service != hyperv1.APIServer {
continue
}
if publishingStrategy.Type == hyperv1.Route {
if hcluster.Spec.Networking.APIServer == nil {
hcluster.Spec.Networking.APIServer = &hyperv1.APIServerNetworking{}
}

hcluster.Spec.Networking.APIServer.Port = k8sutilspointer.Int32(443)
if err := r.Update(ctx, hcluster); err != nil {
return fmt.Errorf("failed to update hostedcluster after defaulting the apiserver port: %w", err)
}
}
break
}

if !r.ManagementClusterCapabilities.Has(capabilities.CapabilityInfrastructure) {
return nil
Expand Down
6 changes: 2 additions & 4 deletions hypershift-operator/controllers/nodepool/haproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,8 @@ func (r *NodePoolReconciler) reconcileHAProxyIgnitionConfig(ctx context.Context,
apiServerInternalAddress = config.DefaultAdvertiseIPv6Address
}

//TODO: in order to prevent periodic kube-apiserver network blimps in the LoadBalancer
//publish strategy this should change.
//However: will need API changes for service publishing strategy. Best function to call:
//apiServerInternalPort := util.BindAPIPortWithDefaultFromHostedCluster(hcluster, config.DefaultAPIServerPort)
// TODO (alberto): Technically this should call util.BindAPIPortWithDefaultFromHostedCluster and let 443 be an invalid value.
// How ever we allow it here to keep backward compatibility with existing clusters which defaulted .port to 443.
apiServerInternalPort := haproxyFrontendListenAddress(hcluster, config.DefaultAPIServerPort)
if hcluster.Spec.Networking.APIServer != nil {
if hcluster.Spec.Networking.APIServer.AdvertiseAddress != nil {
Expand Down
32 changes: 18 additions & 14 deletions support/util/networking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,33 +59,37 @@ func APIPort(hcp *hyperv1.HostedControlPlane) *int32 {
return nil
}

// BindAPIPortWithDefault will retrieve the port the kube-apiserver binds on locally in the pod
// BindAPIPortWithDefault will retrieve the port the kube-apiserver binds on locally in the pod.
// This comes from hcp.Spec.Networking.APIServer.Port if set and != 443
func BindAPIPortWithDefault(hcp *hyperv1.HostedControlPlane, defaultValue int32) int32 {
if port := APIPort(hcp); port != nil {
for _, svc := range hcp.Spec.Services {
if svc.Service == hyperv1.APIServer && svc.Type == hyperv1.NodePort {
return *port
}
}
// Binding on 443 is not allowed. So returning default for that case.
// This provides backward compatibility for existing clusters which were defaulting to that value, ignoring it here and
// enforcing it in the data plane proxy by reconciling the endpoint. 443 API input is not allowed now.
// https://github.com/openshift/hypershift/pull/2964
if hcp.Spec.Networking.APIServer != nil && hcp.Spec.Networking.APIServer.Port != nil && *hcp.Spec.Networking.APIServer.Port != 443 {
return *hcp.Spec.Networking.APIServer.Port
}
return defaultValue
}

// BindAPIPortWithDefaultFromHostedCluster will retrieve the port the kube-apiserver binds on locally in the pod
// BindAPIPortWithDefaultFromHostedCluster will retrieve the port the kube-apiserver binds on locally in the pod.
// This comes from hcp.Spec.Networking.APIServer.Port if set and != 443
func BindAPIPortWithDefaultFromHostedCluster(hc *hyperv1.HostedCluster, defaultValue int32) int32 {
for _, svc := range hc.Spec.Services {
if svc.Service == hyperv1.APIServer {
if svc.Type == hyperv1.NodePort && hc.Spec.Networking.APIServer != nil && hc.Spec.Networking.APIServer.Port != nil {
return *hc.Spec.Networking.APIServer.Port
}
}
// Binding on 443 is not allowed. So returning default for that case.
// This provides backward compatibility for existing clusters which were defaulting to that value, ignoring it here and
// enforcing it in the data plane proxy by reconciling the endpoint. 443 API input is not allowed now.
// https://github.com/openshift/hypershift/pull/2964
if hc.Spec.Networking.APIServer != nil && hc.Spec.Networking.APIServer.Port != nil && *hc.Spec.Networking.APIServer.Port != 443 {
return *hc.Spec.Networking.APIServer.Port
}
return defaultValue
}

// InternalAPIPortWithDefault 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 InternalAPIPortWithDefault(hcp *hyperv1.HostedControlPlane, defaultValue int32) int32 {
// TODO (alberto): Why is the exposed port for the SVC coming from .Spec.Networking.APIServer.Port?
// The API input is meant to be just the KAS Pod Port (and so the nodes haproxy).
if port := APIPort(hcp); port != nil {
return *port
}
Expand Down