⚠️ Move Workspace.Status.{Cluster, URL} to Spec #2557
Conversation
openshift-ci
bot
added
the
kind/api-change
|
/assign @sttts |
pkg/reconciler/tenancy/workspace/workspace_reconcile_shard_url.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/tenancy/workspace/workspace_reconcile_scheduling_test.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/tenancy/workspace/workspace_reconcile_scheduling.go
Outdated
Show resolved
Hide resolved
vincepri
changed the title
pkg/admission/workspace/admission.go
Outdated
| if old.Status.Cluster != "" && cw.Status.Cluster == "" { | ||
| return admission.NewForbidden(a, errors.New("status.cluster cannot be unset")) | ||
| if old.Spec.Cluster != "" && cw.Spec.Cluster == "" { | ||
| return admission.NewForbidden(a, errors.New("spec.cluster cannot be unset")) | ||
| } | ||
|
|
||
| if cw.Status.Phase != corev1alpha1.LogicalClusterPhaseScheduling { |
There was a problem hiding this comment.
this is still a pretty hard condition on status. Not sure it leads to problems.
There was a problem hiding this comment.
👍 I'll keep an eye out for errors in the e2e, although this isn't ideal to check for the Phase. We could just remove these checks instead?
| @@ -374,28 +380,28 @@ func TestValidate(t *testing.T) { | |||
| Annotations: map[string]string{"experimental.tenancy.kcp.io/owner": "{}"}, | |||
| }, | |||
| Spec: tenancyv1beta1.WorkspaceSpec{ | |||
| Cluster: "somewhere", | |||
There was a problem hiding this comment.
missing a test that rejects changes for non-privileged users
pkg/apis/tenancy/v1beta1/types.go
Outdated
| @@ -58,6 +58,8 @@ type Workspace struct { | |||
| } | |||
|
|
|||
| // WorkspaceSpec holds the desired state of the ClusterWorkspace. | |||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.URL) || has(self.URL)",message="URL cannot be unset" | |||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.cluster) || has(self.cluster)",message="cluster is immutable" | |||
There was a problem hiding this comment.
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.cluster) || has(self.cluster)",message="cluster is immutable" | |
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.cluster) || has(self.cluster)",message="cluster cannot be unset" |
?
There was a problem hiding this comment.
Isn't this one correct? The cannot be unset for URL seems to be meaning that it can't be empty, but it could be potentially be changed. Meanwhile the Cluster is set once?
There was a problem hiding this comment.
Yeah, it is immutable. This validation doesn't enforce that, though. This passes validation on update if:
- old.cluster is empty, or
- old.cluster is set and new.cluster is set
There is a separate validation on the Cluster field itself to ensure immutability:
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="cluster is immutable"
There was a problem hiding this comment.
Ah ok, in this specific validation piece I can see how the message should be different, thanks!
openshift-ci
bot
added
lgtm
|
/lgtm cancel |
|
Flake #2501 |
In order to preserve the URL field, given that Status may be wiped under a number of conditions and most times URL cannot be easily or at all reconstructured, this changeset moves the field under Spec. In addition, now the admission code also checks that any modification to the URL is performed by a system user. Signed-off-by: Vince Prignano <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc 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 |
Summary
In order to preserve the URL field, given that Status may be wiped under a number of conditions and most times URL cannot be easily or at all reconstructured, this changeset moves the field under Spec.
In addition, now the admission code also checks that any modification to the URL is performed by a system user.
Signed-off-by: Vince Prignano [email protected]
Related issue(s)
Fixes #