HOSTEDCP-1090: Use statically configured haproxy for router #2778

csrwng · 2023-07-06T22:06:31Z

What this PR does / why we need it:
Replace the internal router deployment with a statically configured instance of haproxy. Remove any additional privileges required by control plane workloads.

Which issue(s) this PR fixes:
Fixes #HOSTEDCP-1090

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci-robot · 2023-07-06T22:06:34Z

@csrwng: This pull request references HOSTEDCP-1090 which is a valid jira issue.

Details

In response to this:

What this PR does / why we need it:
Replace the internal router deployment with a statically configured instance of haproxy. Remove any additional privileges required by control plane workloads.

Which issue(s) this PR fixes:
Fixes #HOSTEDCP-1090

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

openshift-ci · 2023-07-06T22:06:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-07-06T22:06:37Z

@csrwng: This pull request references HOSTEDCP-1090 which is a valid jira issue.

Details

In response to this:

What this PR does / why we need it:
Replace the internal router deployment with a statically configured instance of haproxy. Remove any additional privileges required by control plane workloads.

Which issue(s) this PR fixes:
Fixes #HOSTEDCP-1090

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

csrwng · 2023-07-10T12:55:30Z

/test verify
/test e2e-aws

csrwng · 2023-07-11T12:18:28Z

/test e2e-aws

csrwng · 2023-07-11T15:17:29Z

/test e2e-aws

csrwng · 2023-07-11T18:41:35Z

/test e2e-aws

enxebre · 2023-07-13T08:39:38Z

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

-		}
-
-		sa := ignitionserver.ProxyServiceAccount(controlPlaneNamespace)
-		if result, err := createOrUpdate(ctx, c, sa, func() error {


why is this going away?

Because the ignition proxy does not need any special permissions on the management cluster. There's no point in having a dedicated service account with role/rolebinding. This was here because we were granting the service account permission to use the host network SCC. We really don't need that. Everything now runs as restricted.

enxebre · 2023-07-13T09:05:38Z

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

+{{ $ns := .Namespace }}
+{{- range .Backends }}
+backend {{ .Name }}
+  server {{ .Name }} {{ .DestinationService }}.{{ $ns }}.svc.cluster.local:{{.DestinationPort}} check resolvers coredns init-addr last,libc,none


Do we need the ns and .cluster.local?
Couldn't we just use the name?
If so couldn't this actually be a static hardcoded list?

We don't need either, but that's the full dns name without relying on a search suffix, which I think is less error prone.
I didn't use a static list because we don't always use routes for all services, so I wanted to build the list of things we route to based on existing route resources and rely on the logic that creates routes to tell me which things I should route to here.

csrwng · 2023-07-13T17:52:10Z

/hold
Both router and ignition-server-proxy need to move to request serving nodes when using that isolation

Replace the internal router deployment with a statically configured instance of haproxy. Remove any additional privileges required by control plane workloads.

netlify · 2023-07-13T22:14:08Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`bb74adf`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/64b077050fc0d40008d7c30d
😎 Deploy Preview	https://deploy-preview-2778--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 configuration.

csrwng · 2023-07-13T22:15:26Z

/hold cancel
Updated deployments for request serving

bryan-cox

/lgtm

openshift-ci · 2023-07-14T15:42:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bryan-cox, csrwng

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [csrwng]

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

sjenning · 2023-07-14T15:45:10Z

already tagged but lgtm as well 👍

muraee · 2023-07-14T15:48:31Z

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

+			manifests.KubeAPIServerExternalPublicRoute("").Name,
+			manifests.KubeAPIServerExternalPrivateRoute("").Name:
+			p.HasKubeAPI = true
+			continue


why do we need continue here?

just curious, otherwise lgtm

we don't need it :)
I had that there before that section was inside the case stmt but forgot to remove. It doesn't hurt, but I will remove it in a follow up

openshift-ci-robot · 2023-07-14T19:18:49Z

/retest-required

Remaining retests: 0 against base HEAD 42afe9a and 2 for PR HEAD bb74adf in total

openshift-ci-robot · 2023-07-18T03:03:15Z

/retest-required

Remaining retests: 0 against base HEAD 9134f1d and 1 for PR HEAD bb74adf in total

openshift-ci · 2023-07-18T07:28:53Z

@csrwng: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

csrwng · 2023-07-31T12:53:02Z

Need to consult with IBM Cloud team to determine whether an override for the PSA labels is needed before backporting
cc @rtheis

Introduces an annotation to override the pod security admission label for hosted control plane namespaces. This is a follow up to openshift#2778 where the default PSA is now changed to 'Restricted'. For some consumers like IBM, this label may be too restrictive for other workloads they may want to run in the control plane namespace.

rtheis · 2023-08-11T18:57:11Z

@csrwng we ran some tests and confirmed that baseline PSA labels will work for us but not restricted. If you all intend to proceed with restricted PSA labels then we will need #2886 to override the default.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 6, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2023

csrwng force-pushed the static_router branch from 3d5ae84 to bd929c3 Compare July 11, 2023 12:17

csrwng force-pushed the static_router branch from 4829e68 to eec6eea Compare July 12, 2023 02:30

csrwng marked this pull request as ready for review July 12, 2023 02:31

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023

openshift-ci bot requested review from enxebre and sjenning July 12, 2023 02:31

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

csrwng force-pushed the static_router branch from eec6eea to ed9dfb8 Compare July 12, 2023 20:06

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

enxebre reviewed Jul 13, 2023

View reviewed changes

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

Use statically configured haproxy for router

bb74adf

Replace the internal router deployment with a statically configured instance of haproxy. Remove any additional privileges required by control plane workloads.

csrwng force-pushed the static_router branch from ed9dfb8 to bb74adf Compare July 13, 2023 22:13

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

bryan-cox approved these changes Jul 14, 2023

View reviewed changes

openshift-ci bot assigned bryan-cox Jul 14, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2023

muraee reviewed Jul 14, 2023

View reviewed changes

openshift-merge-robot merged commit d4a0c95 into openshift:main Jul 18, 2023

DavidHurta mentioned this pull request Jul 18, 2023

OTA-855: Enable CVO to evaluate conditional updates on self-managed HyperShift deployed on OpenShift #2807

Merged

4 tasks

csrwng mentioned this pull request Aug 8, 2023

Allow overriding pod security admission label #2886

Merged

1 task

csrwng deleted the static_router branch November 15, 2023 02:10

sjenning mentioned this pull request Nov 21, 2023

OCPBUGS-23472: unset ServiceAccount on ignition-server-proxy #3209

Merged

HOSTEDCP-1090: Use statically configured haproxy for router #2778

HOSTEDCP-1090: Use statically configured haproxy for router #2778

Uh oh!

Conversation

csrwng commented Jul 6, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 6, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 6, 2023

Uh oh!

openshift-ci-robot commented Jul 6, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csrwng commented Jul 10, 2023

Uh oh!

csrwng commented Jul 11, 2023

Uh oh!

csrwng commented Jul 11, 2023

Uh oh!

csrwng commented Jul 11, 2023

Uh oh!

enxebre Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

csrwng Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

enxebre Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

csrwng Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

csrwng commented Jul 13, 2023

Uh oh!

netlify bot commented Jul 13, 2023

✅ Deploy Preview for hypershift-docs ready!

Uh oh!

csrwng commented Jul 13, 2023

Uh oh!

bryan-cox left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 14, 2023

Uh oh!

sjenning commented Jul 14, 2023

Uh oh!

muraee Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muraee Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

csrwng Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 14, 2023

Uh oh!

openshift-ci-robot commented Jul 18, 2023

Uh oh!

openshift-ci bot commented Jul 18, 2023

Uh oh!

csrwng commented Jul 31, 2023

Uh oh!

rtheis commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

csrwng commented Jul 6, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 6, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 6, 2023 •

edited by openshift-ci bot

Loading

muraee Jul 14, 2023 •

edited

Loading