[HOSTEDCP-1041] Defaulting webhook for self managed HCP #2864

davidvossel · 2023-07-31T21:12:39Z

Previously, the hostedCluster.Spec.Release.Image was required and the hcp or hypershift cli defaulted that value client side. This posed several issues though.

The defaulting did not take into consideration what the max and min version the deployed hypershift operator supports, which will eventually lead to the client defaulting unsupported versions.
This logic was all client side, which meant that it couldn't be used in a gitops flow or reused by the console web UI.

The new logic performs defaulting the release image on the backend using an optional mutating webhook. The logic takes into account the latest/min supported versions and picks the most recent version that falls within those constraints.

A webhook was chosen over performing defaults in the controller's reconcile loop due to the following reasons.

The Webhook requires no api changes (such as making the release image optional)
Error discovery occurs immediately at creation time (http Post response) rather than after admission via condition

openshift-ci · 2023-07-31T21:13:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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~~ [davidvossel]

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

sjenning · 2023-08-01T13:26:03Z

Needs a make update

sjenning · 2023-08-01T13:27:14Z

api/v1beta1/hostedcluster_types.go

@@ -1659,7 +1659,7 @@ type ManagedEtcdStorageSpec struct {
 	//
 	// +optional
 	// +immutable
-	RestoreSnapshotURL []string `json:"restoreSnapshotURL"`


Why this change? Seems unrelated.

i know... I thought so too. but it actually matters in a kind of dumb way (that I only half understand)

The issue is with how the decoding/encoding of the object works in the webhook. If we don't add omitempty here, then the decode/encode logic results in the mutation webhook trying to patch RestoreSnapshotURL to Null, then we get an API error during the validation phase about trying to set a []string to a Null value... or something along those lines.

sjenning · 2023-08-01T13:27:46Z

cmd/cluster/core/create.go

@@ -440,7 +432,11 @@ func CreateCluster(ctx context.Context, opts *CreateOptions, platformSpecificApp
 func defaultNetworkType(ctx context.Context, opts *CreateOptions, releaseProvider releaseinfo.Provider, readFile func(string) ([]byte, error)) error {
 	if opts.NetworkType != "" {
 		return nil
+	} else if opts.ReleaseImage == "" {


I think this change is causing the unit test failure of TestDefaultNetworkType in github.com/openshift/hypershift/cmd/cluster/core

ah, yep, fixed

sjenning · 2023-08-01T13:28:44Z

support/supportedversion/version.go

+		return "", err
+	}
+	return version.PullSpec, nil
+


nit: extra line

enxebre · 2023-08-01T13:40:52Z

hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go

+
+	if np.Spec.Release.Image != "" {
+		return nil
+	} else if np.Spec.ClusterName == "" {


Why is the defaulter throwing this validation error? the .ClusterName is already required and immutable

Two reasons

I don't think ClusterName required to be a non empty string. I believe it is only required to be immutable. this is the validation rule for cluster name +kubebuilder:validation:XValidation:rule="self == oldSelf", message="ClusterName is immutable"

The mutation webhook runs before the validation, which is why we can default the empty release image even though it is required to not be empty via this validation rule +kubebuilder:validation:Pattern=^(\w+\S+)$

So, i'm kind of in a weird spot with the cluster name here.

I don't think ClusterName required to be a non empty string

Yeh it is, every API field is required unless marked as optional via kubebuilder marker or a pointer with omitempty tag. So I think we can drop this check completely unless you prove me wrong, in which case that's a bug and we should fix the field to be a required non empty string at the API level and address separately from the release defaulting.

you're right though this ensures presence but it would accept an explicitly set zero value for the string, and as you mentioned if we enforced != "" validation at the crd schema it would run after this, so I'm fine either way.

enxebre · 2023-08-01T13:50:23Z

support/supportedversion/version.go

+}
+
+func LookupLatestSupportedRelease(ctx context.Context) (string, error) {
+	prefix := "https://multi.ocp.releases.ci.openshift.org/api/v1/releasestream/4-stable-multi/latest"


Did we make a decision of defaulting to multiarch? This has an impact particularly for disconnected environments https://issues.redhat.com/browse/MIXEDARCH-292 / https://issues.redhat.com/browse/MIXEDARCH-255

At minimum I think we should document and communicate this clearly in docs?

The cli defaulted to multi, so i brought that over to the webhook.

hypershift/cmd/version/version.go

Line 22 in 1fc3c32

defaultReleaseStream = "4-stable-multi"

If someone is using disconnected, I doubt they'd be using the release image defaulting.

enxebre · 2023-08-01T13:51:43Z

support/supportedversion/version.go

+	PullSpec    string `json:"pullSpec"`
+	DownloadURL string `json:"downloadURL"`
+}
+


would you mind document this func? e.g // LookupLatestSupportedRelease picks the latest multi-arch image supported by this Hypershift Operator...

enxebre · 2023-08-01T13:52:09Z

it'd be nice to exercise this in the kubevirt job

davidvossel · 2023-08-01T20:47:59Z

it'd be nice to exercise this in the kubevirt job

yep, I think it will take a few steps for us to get to that point though. We'll need to enable the webhook during install (which involves messing with openshift/release ci code), then we can add a e2e test case that ensures the HC/NP defaults the release image during creation when kubevirt platform is in use.

enxebre · 2023-08-02T08:35:23Z

hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go

+
+	err := defaulter.client.Get(ctx, client.ObjectKeyFromObject(hc), hc)
+	if err != nil {
+		return fmt.Errorf("Error retrieving HostedCluster named [%s], %v", np.Spec.ClusterName, err)


Cap letter here makes verify fail.

oops, thanks, fixed

Signed-off-by: David Vossel <davidvossel@gmail.com>

netlify · 2023-08-02T17:43:02Z

✅ Deploy Preview for hypershift-docs ready!

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

davidvossel · 2023-08-04T18:38:39Z

/retest-required

openshift-ci · 2023-08-04T20:09:44Z

@davidvossel: The following test 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-ibmcloud-iks	`d3e9893`	link	false	`/test e2e-ibmcloud-iks`

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.

sjenning · 2023-08-07T18:28:09Z

/lgtm

sjenning · 2023-08-07T18:29:16Z

/retest-required

davidvossel · 2023-08-08T13:48:20Z

/override "Pipelines as Code CI"

openshift-ci · 2023-08-08T13:48:36Z

@davidvossel: Overrode contexts on behalf of davidvossel: Pipelines as Code CI

In response to this:

/override "Pipelines as Code CI"

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-robot · 2023-08-08T20:04:44Z

/retest-required

Remaining retests: 0 against base HEAD b802382 and 2 for PR HEAD d3e9893 in total

openshift-ci bot added the do-not-merge/needs-area label Jul 31, 2023

openshift-ci bot requested review from enxebre and hasueki July 31, 2023 21:12

openshift-ci bot added the area/cli Indicates the PR includes changes for CLI label Jul 31, 2023

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-area labels Jul 31, 2023

sjenning reviewed Aug 1, 2023

View reviewed changes

enxebre reviewed Aug 1, 2023

View reviewed changes

davidvossel force-pushed the defaulting-webhook-v1 branch from d830c81 to 7677e88 Compare August 1, 2023 20:46

enxebre reviewed Aug 2, 2023

View reviewed changes

davidvossel mentioned this pull request Aug 2, 2023

[HOSTEDCP-1041] Accurately default latest release image using hypershift-operator supported versions #2830

Closed

Defaulting webhook for self managed HCP

d3e9893

Signed-off-by: David Vossel <davidvossel@gmail.com>

davidvossel force-pushed the defaulting-webhook-v1 branch from 7677e88 to d3e9893 Compare August 2, 2023 17:42

openshift-ci bot assigned sjenning Aug 7, 2023

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

openshift-merge-robot merged commit 569f67c into openshift:main Aug 9, 2023
13 of 15 checks passed

sjenning mentioned this pull request Aug 9, 2023

Revert "[HOSTEDCP-1041] Defaulting webhook for self managed HCP" #2889

Merged

deepsm007 mentioned this pull request Aug 9, 2023

Revert "[HOSTEDCP-1041] Defaulting webhook for self managed HCP" #2890

Closed

davidvossel mentioned this pull request Aug 9, 2023

[HOSTEDCP-1041] Re-introducing defaulting webhook for self managed HCP #2892

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOSTEDCP-1041] Defaulting webhook for self managed HCP #2864

[HOSTEDCP-1041] Defaulting webhook for self managed HCP #2864

davidvossel commented Jul 31, 2023 •

edited

openshift-ci bot commented Jul 31, 2023

sjenning commented Aug 1, 2023 •

edited

sjenning Aug 1, 2023 •

edited

davidvossel Aug 1, 2023

sjenning Aug 1, 2023 •

edited

davidvossel Aug 1, 2023

sjenning Aug 1, 2023

enxebre Aug 1, 2023 •

edited

davidvossel Aug 1, 2023

enxebre Aug 2, 2023 •

edited

enxebre Aug 2, 2023

enxebre Aug 1, 2023 •

edited

davidvossel Aug 1, 2023

enxebre Aug 1, 2023

enxebre commented Aug 1, 2023

davidvossel commented Aug 1, 2023

enxebre Aug 2, 2023

davidvossel Aug 2, 2023

netlify bot commented Aug 2, 2023

davidvossel commented Aug 4, 2023

openshift-ci bot commented Aug 4, 2023 •

edited

sjenning commented Aug 7, 2023

sjenning commented Aug 7, 2023

davidvossel commented Aug 8, 2023

openshift-ci bot commented Aug 8, 2023

openshift-ci-robot commented Aug 8, 2023

[HOSTEDCP-1041] Defaulting webhook for self managed HCP #2864

[HOSTEDCP-1041] Defaulting webhook for self managed HCP #2864

Conversation

davidvossel commented Jul 31, 2023 • edited

openshift-ci bot commented Jul 31, 2023

sjenning commented Aug 1, 2023 • edited

sjenning Aug 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning Aug 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Aug 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Aug 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Aug 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Aug 1, 2023

davidvossel commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Aug 2, 2023

✅ Deploy Preview for hypershift-docs ready!

davidvossel commented Aug 4, 2023

openshift-ci bot commented Aug 4, 2023 • edited

sjenning commented Aug 7, 2023

sjenning commented Aug 7, 2023

davidvossel commented Aug 8, 2023

openshift-ci bot commented Aug 8, 2023

openshift-ci-robot commented Aug 8, 2023

davidvossel commented Jul 31, 2023 •

edited

sjenning commented Aug 1, 2023 •

edited

sjenning Aug 1, 2023 •

edited

sjenning Aug 1, 2023 •

edited

enxebre Aug 1, 2023 •

edited

enxebre Aug 2, 2023 •

edited

enxebre Aug 1, 2023 •

edited

openshift-ci bot commented Aug 4, 2023 •

edited