Skip to content
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.

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

Bypass validate update #253

Conversation

bshephar
Copy link
Collaborator

@bshephar bshephar commented Oct 3, 2023

This change allows for the ValidateUpdate() webhook to be bypassed. This provides functionality for advanced use cases where the user has independently validated that their update won't break the deployment, or orphan any data in an old database.

It's anticipated that this functionality will be required in cases where we deploy the control plane and point it at an existing database. Then perform a database migration, in which case we want to redirect the Heat deployment to the new database.

Since this operation is inherently dangerous. By default, this is an undocumented feature that is intended to facilitate situations where users are confident in the operation they are performing.

@openshift-ci openshift-ci bot requested review from abays and stuggi October 3, 2023 04:02
@openshift-ci openshift-ci bot added the approved label Oct 3, 2023
@bshephar bshephar force-pushed the bypass-validate-update branch 2 times, most recently from da9db43 to 8cb8037 Compare October 3, 2023 04:04
@bshephar bshephar closed this Oct 4, 2023
@bshephar bshephar reopened this Oct 15, 2023
@bshephar bshephar force-pushed the bypass-validate-update branch 2 times, most recently from 4bfef9e to f82748a Compare October 15, 2023 09:11
Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

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

just pasting here for reference on what we did in the infra https://github.com/openstack-k8s-operators/infra-operator/blob/main/apis/network/v1beta1/ipset_webhook.go#L102 to make sure there is a required manual extra step if the ipset was intended to be immutable.

@@ -36,6 +36,8 @@ const (
HeatCfnAPIContainerImage = "quay.io/podified-antelope-centos9/openstack-heat-api-cfn:current-podified"
// HeatEngineContainerImage - default fall-back container image for HeatEngine if associated env var not provided
HeatEngineContainerImage = "quay.io/podified-antelope-centos9/openstack-heat-engine:current-podified"
// HeatSkipValdationsAnnotation - Allows users to bypass the webhook validations
HeatSkipValdationsAnnotation = "heat.openstack.org/skip-validations"
Copy link
Contributor

Choose a reason for hiding this comment

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

if we go this way using an annotation, maybe this should be not a heat specific annotation, so the same can be used on other service operators CRs as this seems to be a functionality we'd then want to have across all service operators?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, my intention here is to establish a design pattern we're happy with. I see infra-operator is using a field in the Spec section to determine this. It feels a lot like the advancedMode proposed on placement-operator. I think I would prefer to be more specific with these regardless of whether we use annotations or the spec fields, see the discussion here:
openstack-k8s-operators/placement-operator#17 (comment)

So I would propose that we either:
a) use the annotations but make them feature specific, which would mean changing the string in this PR to:
HeatDatabaseMigrationAnnotation = "heat.openstack.org/database-migration"

Or b)
Like I mentioned in that comment on the placement-operator, using feature sets that can be enabled or disabled depending on the customers use case:
spec.enabledFeatures.databaseMigration

I'm not sure Immutabe is the best choice of adjective since large portions of the spec will indeed be mutable. It's very specific parts that we would ultimately like to protect. So something more specific to those use cases feels like a better fit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just updated the PR here to change this Const and string to be reflective of my comment.

Both the enabledFeatures and annotations proposals have been established as patterns for user interfaces that change the behaviour of operators in OpenShift as well. With examples being featureSets and Gates:
https://docs.openshift.com/container-platform/4.13/nodes/clusters/nodes-cluster-enabling-features.html

And if you want to delete a specific node from the machine-api, you can use an annotation to do that on the node:
https://github.com/openshift/machine-api-operator/blob/release-4.14/pkg/controller/machineset/delete_policy.go#L32-L34

So I think either of these proposals would be familiar and intuitive for users of OpenShift.

This change adds a bypass for the ValidateUpdate webhook validation.
Without this bypass, all changes to the DatabaseInstance object will be
blocked to prevent data corruption and potential loss of data.

Signed-off-by: Brendan Shephard <bshephar@redhat.com>
Copy link
Contributor

openshift-ci bot commented Jun 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, slagle

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:

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

@openshift-merge-bot openshift-merge-bot bot merged commit 5720bbc into openstack-k8s-operators:main Jun 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants