feat: make SpaceBindingRequest.Spec.MasterUserRecord immutable #470
Conversation
mfrancisc
requested review from
sbryzak,
xcoulon,
alexeykazakov,
MatousJobanek,
rajivnathan and
ranakan19
|
/retest updated e2e PR |
| port: 443 | ||
| matchPolicy: Equivalent | ||
| rules: | ||
| - operations: ["CREATE", "UPDATE"] |
There was a problem hiding this comment.
we also need to cover PATCH, right?
There was a problem hiding this comment.
I remember adding PATCH initially but was failing, since apparently there is not PATCH operations 🤷♂️ :https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-rules
operations lists one or more operations to match. Can be "CREATE", "UPDATE", "DELETE", "CONNECT", or "*" to match all.
There was a problem hiding this comment.
ah, OK. My bad - I automatically expected that Patch would be one of the operations as well :-)
Thanks for the link and sorry for confusion 👍
There was a problem hiding this comment.
No worries, I expected the same and I'm still a bit confused of why are those the "operations". What is connect for example 😕
make/go.mk
Outdated
| @rm ./pkg/webhook/deploy/templates/template_assets.go 2>/dev/null || true | ||
| @$(GO_BINDATA) -pkg templates -o ./pkg/webhook/deploy/templates/template_assets.go -nocompress -prefix deploy/webhook deploy/webhook |
There was a problem hiding this comment.
wouldn't webhook (or in plural form webhooks), or webhook_assest be more appropriate?
There was a problem hiding this comment.
I've renamed it to webhooks plz check : 9ddde44
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/client-go/kubernetes/scheme" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| ) | ||
|
|
||
| func TestHandleValidateRolebBndingAdmissionRequestBlocked(t *testing.T) { | ||
| func TestHandleValidateRolebBindingAdmissionRequestBlocked(t *testing.T) { |
There was a problem hiding this comment.
is the extra b in Roleb expected?
it's the same also in other names of the test functions
There was a problem hiding this comment.
While trying to fix a typo I've introduced a new one 🤦♂️ . Thanks it should be fixed in 9ddde44
| Labels: map[string]string{ | ||
| toolchainv1alpha1.ProviderLabelKey: toolchainv1alpha1.ProviderLabelValue, | ||
| }, |
There was a problem hiding this comment.
why do we need this label set?
There was a problem hiding this comment.
for the sake of the test we don't , was a copy paste left over I guess. Removed in 9ddde44
Co-authored-by: Matous Jobanek <[email protected]>
Co-authored-by: Matous Jobanek <[email protected]>
|
@MatousJobanek thanks for your review. I've addressed your comments, when you have time PTAL. I've also fixed/updated some unit tests which apparently I've missed initially. |
|
/retest I haven't saw this in a while. |
|
/retest updated e2e PR |
|
/retest infra |
1 similar comment
|
/retest infra |
|
/retest infra |
Codecov Report
@@ Coverage Diff @@
## master #470 +/- ##
==========================================
- Coverage 82.31% 82.02% -0.29%
==========================================
Files 29 30 +1
Lines 3393 3444 +51
==========================================
+ Hits 2793 2825 +32
- Misses 455 474 +19
Partials 145 145
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
/retest infra |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, mfrancisc 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 |
|
/retest infra |
|
/retest infra |
This PR introduces a new validating webhook for SpaceBindingRequests. Specifically the webhook makes sure that once an SBR is created the MasterUserRecord field cannot be changed anymore. Changing the MUR field in SBR will generate an discrepancy with the name of SpaceBinding that will still contain the old MUR.
Jira: https://issues.redhat.com/browse/ASC-426
Paired with: codeready-toolchain/toolchain-e2e#792