Use a per-hcp router for all hcp ingress traffic #1614

alvaroaleman · 2022-07-25T21:41:21Z

This change adds a new mode to the cpo where it will set up a router and use it for all controlplane traffic, ref: https://issues.redhat.com/browse/HOSTEDCP-485

This is done by:

Making the CLI decide based on the presence of a value for --external-dns-domain if it configures Route or the existing LoadBalancer as publishingStrategy for the KAS
If the KAS publishing strategy is route, the CPO will:
- Not set up a LB but a clusterIP service for the KAS
- Create an external route if the cluster is public
- Create an internal route
- Configure the router with a custom template that uses the KAS as default backend, this is needed because in-cluster traffic accesses the KAS through an IP, but SNI does not support IP addreses
- If the cluster is private, the AWS private link controller will set up two hypershift.local DNS entries for the router, one for the KAS, one for the apps domain

/label tide/merge-method-squash

openshift-ci · 2022-07-25T21:41:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enxebre · 2022-07-26T10:54:15Z

api/v1alpha1/endpointservice_types.go

@@ -51,6 +51,10 @@ type AWSEndpointServiceStatus struct {
 	// +optional
 	DNSName string `json:"dnsName,omitempty"`

+	// DNSName are the names for the records created in the hypershift private zone


how is this different from the above field? does the above needs to be dropped/deprecated?

It is different in that it can hold more than one value ([]string vs string)

so the semantic is the same, I don't think we need two different fields to represent 1 vs multiple, do we? Can we drop DNSName and keep only []DNSName where you can have 1..N?

Yeah, the semantic is the same. We can deprecate the field, but we have to keep it around for or as long as we support an OCP version where the HCP controller does not have this change

If you are deprecating the single field you should add a // Deprecated comment to it.

enxebre · 2022-07-26T11:12:22Z

Making the CLI decide based on the presence of a value for --external-dns-domain if it configures Route or the existing LoadBalancer as publishingStrategy for the KAS

Why is external dns the discriminator, why don't always go router?

Can we please capture this in https://github.com/openshift/enhancements/tree/master/enhancements/hypershift/networking

alvaroaleman · 2022-07-26T12:58:30Z

Why is external dns the discriminator, why don't always go router?

Because it is not going to work without external DNS, as we need it to be able to do name based virtual hosting

csrwng

did an initial pass, testing to better understand new behavior

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

control-plane-operator/controllers/hostedcontrolplane/ingress/router.template

csrwng · 2022-07-26T18:55:20Z

api/v1alpha1/endpointservice_types.go

@@ -51,6 +51,10 @@ type AWSEndpointServiceStatus struct {
 	// +optional
 	DNSName string `json:"dnsName,omitempty"`

+	// DNSName are the names for the records created in the hypershift private zone


If you are deprecating the single field you should add a // Deprecated comment to it.

control-plane-operator/controllers/hostedcontrolplane/oauth/route.go

csrwng · 2022-07-26T19:00:17Z

hypershift-operator/controllers/platform/aws/controller.go

@@ -302,15 +302,15 @@ func (r *AWSEndpointServiceReconciler) reconcileAWSEndpointServiceStatus(ctx con
 	hasPrivateRouterEPService := false
 	hasPrivateIngressControllerEPService := false
 	for _, eps := range endpointServices.Items {
-		// If a private-router AWSEndpointService exists, it means that
+		// If a router AWSEndpointService exists, it means that


it means that ?

No idea, I had just deleted the reference to private, whoever originally created the comment forgot to finish the sentence 🙃

csrwng · 2022-07-26T19:56:11Z

@alvaroaleman so in the public use case I would still expect the ignition, konnectivity, oauth and ovnsdb routes to go through the one router as long as a hostname is specified for them. That doesn't look to be the case with the current code.

alvaroaleman · 2022-07-26T21:53:11Z

@alvaroaleman so in the public use case I would still expect the ignition, konnectivity, oauth and ovnsdb routes to go through the one router as long as a hostname is specified for them. That doesn't look to be the case with the current code.

Updated that and added an e2e verification.The only exception for this is currently the ovn route, which is managed by the CNO. I will fix it there once this has merged.

netlify · 2022-07-28T23:33:41Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`70f0406`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/62e9258dba03c20008ae408b
😎 Deploy Preview	https://deploy-preview-1614--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

alvaroaleman · 2022-07-29T14:24:54Z

/retest-required

alvaroaleman · 2022-07-29T16:05:16Z

/retest-required

alvaroaleman · 2022-07-29T18:37:03Z

@csrwng @sjenning so in order to save on LB ports to in turn save on security group rules I added defaulting for the KAS to listen on port 443 when the Route strategy is used. This required to:

Change things to hardcode the kas to listen on 6443 and do the 443->6443 translation in the Service object, because the KAS can not bind to 443 as it doesn't run as root
Make the internal LB in private clusters respect the apiserver port, everything that uses it is hardcoded to use port 6443

Those two changes could be extracted from this PR, should I do that?

alvaroaleman · 2022-07-29T19:06:25Z

/retest-required

csrwng · 2022-07-29T20:09:20Z

Those two changes could be extracted from this PR, should I do that?

Only if you think it will help to make progress quicker. I wouldn't spend too much time extracting it from here.

alvaroaleman · 2022-07-29T21:15:41Z

Tests are finally green 🎉

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

enxebre · 2022-08-01T08:48:09Z

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -818,10 +822,36 @@ func (r *HostedControlPlaneReconciler) reconcileAPIServerService(ctx context.Con
 		return fmt.Errorf("failed to reconcile API server service: %w", err)
 	}

-	if util.IsPrivateHCP(hcp) {
+	if serviceStrategy.Type == hyperv1.Route {


nit: I'd find easier to read and understand all the possible casuistic with a swtich case for serviceStrategy.Type here, then in default with util.IsPrivateHCP(hcp) {...

The issue is that we have three logical branches:

Uses hyperv1. Router: Always create internal route.

subbranch is public: Create public route

Uses hyperv1.LB and is private: Create private LB

The hyperv1.LB for the last condition is implicit, because private AWS clusters can only have route or LB apiserver expose strategy. I've added it explicitly to the condition to make that more obvious, the check doesn't really cost anything.

enxebre · 2022-08-01T09:03:26Z

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

+	kasServiceStrategy := servicePublishingStrategyByType(hostedControlPlane, hyperv1.APIServer)
+	if util.IsPrivateHCP(hostedControlPlane) || kasServiceStrategy.Type == hyperv1.Route {
+		r.Log.Info("Reconciling router")
+		if err = r.reconcileRouter(ctx, hostedControlPlane, releaseImage, createOrUpdate); err != nil {


why does this only happen when is private || kasServiceStrategy.Type == hyperv1.Route? I though part of the goal was "The per-hostedcluster router is always created, not only in the private case"

I.e shouldn't the other components also be accounted for, e.g konnectivity serviceStrategy...?

Also does it makes sense to let the HC backend validate and reject if the combination of serviceStrategy is not supported?

I'll respond below

enxebre · 2022-08-01T09:24:49Z

Why is external dns the discriminator, why don't always go router?
Because it is not going to work without external DNS, as we need it to be able to do name based virtual hosting

Would it work if management cluster DNS is used when no-external DNS is set?

To keep solidifying and simplifying this, I'd expect:

We agree on more opinionated valid setups and let the HC backend validate input and fail early in condition otherwise. E.g all components can be set to strategy router but not only some...
Always create the router (regardless private or public setup).
Let the CLI always set router strategy (regardless external DNS).

PR lgtm overall, dropped some nits and questions but no objections to proceed and follow up later.

…rough route" This reverts commit 219b149.

This entails having to listen on a different port than what is used externally, as the KAS doesn't run as route and thus can not bind to a port < 1025.

…posed through route"" This reverts commit 9f52874.

alvaroaleman · 2022-08-02T13:33:29Z

We agree on more opinionated valid setups and let the HC backend validate input and fail early in condition otherwise. E.g all components can be set to strategy router but not only some...

No objections to that, but so far we didn't make any statements about which combination of publishing strategies we do or do not support

Always create the router (regardless private or public setup).

We can not do that, because there are existing clusters that use the LB strategy, which is why we have to keep supporting it. This entail that we need a feature gating mechanism that tells us if a cluster is a LB cluster or a route cluster. I went for using the api server expose strategy itself to detect that.

On top of needing to support existing clusters, the other advantage of keeping on supporting it is that we have fewer dependencies, as the route strategy can not work without external DNS. This doesn't matter for the SD use case, but might be nice for the ACM use case.

Let the CLI always set router strategy (regardless external DNS).

This will result in a broken cluster if there is no external DNS. We need at minimum DNS entries for ignition, oauth and the KAS. Without distinct SNI/Host headers, the router can not do name based virtual hosting.

enxebre · 2022-08-02T13:37:44Z

/lgtm
/hold
for @csrwng to have the chance to have another look

openshift-ci · 2022-08-02T15:46:17Z

@alvaroaleman: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws	`2104ac0`	link	false	`/test e2e-aws`
ci/prow/capi-provider-agent-sanity	`70f0406`	link	false	`/test capi-provider-agent-sanity`
ci/prow/e2e-kubevirt-gcp-ovn	`70f0406`	link	false	`/test e2e-kubevirt-gcp-ovn`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

alvaroaleman · 2022-08-03T13:55:18Z

@csrwng could you have a look, please?

csrwng · 2022-08-03T14:10:57Z

/hold cancel
/lgtm

In openshift#1614 the KAS was changed to always listen on port 6443 in oder to support privileged external ports by doing the mapping external port -> internal port in the service object. This resulted in an invalid KAS networkpolicy if the external port is not 6443, as it would allowlist the external port rather than 6443 where the KAS is actually listening.

* Expose through router * KAS service reporting: Report on router service if exposed through route * Revert "KAS service reporting: Report on router service if exposed through route" This reverts commit 219b149. * Default KAS port to 443 when exposing through router This entails having to listen on a different port than what is used externally, as the KAS doesn't run as route and thus can not bind to a port < 1025. * Create router earlier so lb services get provisioned asap * Revert "Revert "KAS service reporting: Report on router service if exposed through route"" This reverts commit 9f52874. * Rebase * Be a bit more explicit

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 25, 2022

openshift-ci bot requested review from csrwng and sjenning July 25, 2022 21:41

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2022

enxebre reviewed Jul 26, 2022

View reviewed changes

csrwng reviewed Jul 26, 2022

View reviewed changes

alvaroaleman force-pushed the changes branch from d4c715d to acbaf48 Compare July 28, 2022 23:32

alvaroaleman force-pushed the changes branch from acbaf48 to 1a9b95f Compare July 29, 2022 03:21

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2022

alvaroaleman force-pushed the changes branch from d65676f to d5c4d3a Compare July 31, 2022 16:12

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2022

enxebre reviewed Aug 1, 2022

View reviewed changes

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go Outdated Show resolved Hide resolved

enxebre reviewed Aug 1, 2022

View reviewed changes

alvaroaleman added 3 commits August 2, 2022 09:10

Expose through router

5f0aac5

KAS service reporting: Report on router service if exposed through route

c20381e

Revert "KAS service reporting: Report on router service if exposed th…

86e8c88

…rough route" This reverts commit 219b149.

alvaroaleman added 5 commits August 2, 2022 09:10

Default KAS port to 443 when exposing through router

79817d4

This entails having to listen on a different port than what is used externally, as the KAS doesn't run as route and thus can not bind to a port < 1025.

Create router earlier so lb services get provisioned asap

ade79db

Revert "Revert "KAS service reporting: Report on router service if ex…

d472029

…posed through route"" This reverts commit 9f52874.

Rebase

37787c9

Be a bit more explicit

70f0406

alvaroaleman force-pushed the changes branch from d5c4d3a to 70f0406 Compare August 2, 2022 13:24

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2022

openshift-ci bot assigned enxebre Aug 2, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2022

openshift-ci bot assigned csrwng Aug 3, 2022

openshift-ci bot merged commit 0b95b70 into openshift:main Aug 3, 2022

alvaroaleman mentioned this pull request Aug 18, 2022

Fix clusters that use LB expose strategy with a custom port #1683

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a per-hcp router for all hcp ingress traffic #1614

Use a per-hcp router for all hcp ingress traffic #1614

alvaroaleman commented Jul 25, 2022

openshift-ci bot commented Jul 25, 2022

enxebre Jul 26, 2022 •

edited

Loading

alvaroaleman Jul 26, 2022 •

edited

Loading

enxebre Jul 26, 2022 •

edited

Loading

alvaroaleman Jul 26, 2022

csrwng Jul 26, 2022

enxebre commented Jul 26, 2022

alvaroaleman commented Jul 26, 2022

csrwng left a comment

csrwng Jul 26, 2022

csrwng Jul 26, 2022

alvaroaleman Jul 26, 2022

csrwng commented Jul 26, 2022

alvaroaleman commented Jul 26, 2022

netlify bot commented Jul 28, 2022 •

edited

Loading

alvaroaleman commented Jul 29, 2022

alvaroaleman commented Jul 29, 2022

alvaroaleman commented Jul 29, 2022 •

edited

Loading

alvaroaleman commented Jul 29, 2022

csrwng commented Jul 29, 2022

alvaroaleman commented Jul 29, 2022

enxebre Aug 1, 2022

alvaroaleman Aug 2, 2022

enxebre Aug 1, 2022 •

edited

Loading

alvaroaleman Aug 2, 2022

enxebre commented Aug 1, 2022

alvaroaleman commented Aug 2, 2022

enxebre commented Aug 2, 2022

openshift-ci bot commented Aug 2, 2022

alvaroaleman commented Aug 3, 2022

csrwng commented Aug 3, 2022

Use a per-hcp router for all hcp ingress traffic #1614

Use a per-hcp router for all hcp ingress traffic #1614

Conversation

alvaroaleman commented Jul 25, 2022

openshift-ci bot commented Jul 25, 2022

enxebre Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

alvaroaleman Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

enxebre Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Jul 26, 2022

alvaroaleman commented Jul 26, 2022

csrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Jul 26, 2022

alvaroaleman commented Jul 26, 2022

netlify bot commented Jul 28, 2022 • edited Loading

✅ Deploy Preview for hypershift-docs ready!

alvaroaleman commented Jul 29, 2022

alvaroaleman commented Jul 29, 2022

alvaroaleman commented Jul 29, 2022 • edited Loading

alvaroaleman commented Jul 29, 2022

csrwng commented Jul 29, 2022

alvaroaleman commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Aug 1, 2022

alvaroaleman commented Aug 2, 2022

enxebre commented Aug 2, 2022

openshift-ci bot commented Aug 2, 2022

alvaroaleman commented Aug 3, 2022

csrwng commented Aug 3, 2022

enxebre Jul 26, 2022 •

edited

Loading

alvaroaleman Jul 26, 2022 •

edited

Loading

enxebre Jul 26, 2022 •

edited

Loading

netlify bot commented Jul 28, 2022 •

edited

Loading

alvaroaleman commented Jul 29, 2022 •

edited

Loading

enxebre Aug 1, 2022 •

edited

Loading