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
added namespace validation for custom resources #1523
Conversation
952325b
to
9d16c96
Compare
Hi @fedepaol , I've attempted a fix for namespace validation. Can you please review when you get a chance and suggest if any changes? |
sure thing, and thanks for the contribution! ❤️ |
Looks good! A couple of quick nits:
|
Sure @fedepaol. I'll try and resolve your changes by today. |
no rush! |
Just an update on status: |
Sure thing! Again, no rush :-) |
9d16c96
to
c890960
Compare
@fedepaol, I've added e2tests and pushed another commit (BFDProfile test was failing earlier because its webhook config did NOT have a CREATE hook registered, I've added it now, hope that's fine). As for clear commit message, I currently have 2 separate commits which I've pushed already. Would you recommend that I rebase/squash both commits into one single commit and force push in its current branch (I was reluctant to try it without checking first, because it's usually not advised)? OR perhaps a new PR with just the changes applied as a single commit with a clear commit message? Please advise. As for the message content, I think your description on the issue was sufficiently verbose. Based on your advise on rebase/squash or squash/merge we can use the below message accordingly (with your changes if any).
|
e2etest/webhookstests/webhooks.go
Outdated
}) | ||
|
||
ginkgo.Context("For BFDProfile", func() { | ||
ginkgo.It("Should reject creating BFDProfile in a different namespace", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could use a Table test here, providing resources as a table entry. This will avoid duplication, since we always expect the same behaviour.
See
metallb/e2etest/bgptests/bgp.go
Line 152 in 65a54f3
table.DescribeTable("A service of protocol load balancer should work with ETP=local", func(pairingIPFamily ipfamily.Family, poolAddresses []string, tweak testservice.Tweak) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Wasn't aware of it. I've refactored it now to use DescribeTable.
e2etest/webhookstests/webhooks.go
Outdated
var _ = ginkgo.Describe("Webhooks", func() { | ||
ginkgo.BeforeEach(func() { | ||
ginkgo.By("Clearing any previous configuration") | ||
err := ConfigUpdaterOtherNS.Clean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect there's no need for cleaning before or after, as all these tests won't add anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version doesn't use setup code for namespace validation.
e2etest/webhookstests/webhooks.go
Outdated
@@ -381,3 +383,175 @@ var _ = ginkgo.Describe("Webhooks", func() { | |||
}) | |||
}) | |||
}) | |||
|
|||
var _ = ginkgo.Describe("Webhooks", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add it under the same describe, just as a different Context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing Describe has setup code which we don't need for the new specs. So I've retained it as a separate set of specs under a separate DescribeTable (since we don't need the setup code for it). Is that ok?
--- | ||
apiVersion: apiextensions.k8s.io/v1 | ||
kind: CustomResourceDefinition | ||
metadata: | ||
annotations: | ||
controller-gen.kubebuilder.io/version: v0.7.0 | ||
controller-gen.kubebuilder.io/version: v0.9.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the same kubebuilder versoin? I know it's pretty old, but I'd be deliberate in changing the CRDs for that (I guess that's what is removing the status and adding x-kubernetes-map-type: atomic
below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the exact version of controller-gen and retested it in the latest changes.
Commit message is fine. We normally split the changes with a commit for the changes and another one for the tests. |
Add a mention to metallb#1523
74a971d
to
90ed280
Compare
Let me check the e2e failures and fix, certain scenarios are failing. |
Add a mention to metallb#1523
90ed280
to
0ded6e5
Compare
My bad that I hadn't run all test possibilities (am exploring the use of The helm based e2etests were failing. I had to make 2 changes:
|
The operator based e2etest required a change in the metallb-operator repo (to enable validation webhook for BFDProfile create op). Tracking that change under metallb/metallb-operator#240 (from what I understand, it fetches configs from this repo and that's causing its manifest validation tests to fail, looking into it). |
Currently, there's nothing preventing the CRs to be created in a namespace different from the one metallb is deployed to. This change adds namespace validation to prevent creation of CRs in a different namespace.
Add a mention to metallb#1523
Yep, this is something that we can definitely improve. Regarding the 'other namespace', why not creating one? Despite it's reasonable to expect |
Add a mention to metallb#1523
392daca
to
6c78ab3
Compare
I've made these two changes:
|
when we deal with this chicken-egg situations, it's fine to have the operator lane temporary failing. I'd go that way, to avoid the need of reverting again. |
Got it. In that case, let me remove that commit (6c78ab3) and push it again. I've tested the above commit already for all tests passing in my forked repo (mgsh#1), to cross-check that it was the only missing dependency. So once the metallb-operator changes are sorted out e2e-use-operator tests will be automatically fixed. |
6c78ab3
to
ae1ed4e
Compare
@@ -134,11 +137,24 @@ var _ = ginkgo.BeforeSuite(func() { | |||
updater, err = testsconfig.UpdaterForCRs(clientconfig, metallb.Namespace) | |||
framework.ExpectNoError(err) | |||
|
|||
// for testing namespace validation, we need an existing namespace that's different from the | |||
// metallb installation namespace | |||
otherNamespace := fmt.Sprintf("%s-other", metallb.Namespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you delete this in aftersuite?
Also, ignore the err if it is AlreadyExists, so if we interrupt the tests before aftersuite the new run won't fail.
e2etest/webhookstests/webhooks.go
Outdated
var _ = table.DescribeTable("Webhooks namespace validation", | ||
func(resources *metallbconfig.ClusterResources) { | ||
err := ConfigUpdaterOtherNS.Update(*resources) | ||
framework.ExpectError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is not necessary given the next row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean that framework.ExpectError
is redundant because of the following Expect(err.Error()).To(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if err is nil that row will complain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fedepaol ☝🏽 if that was your suggestion, won't err.Error()
panic if it's nil
and we remove the framework.ExpectError
before it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. I misread the string checking line.
I suggest using
Expect(err).To(MatchError(ContainSubstring(....))
That will avoid adding the double check
Couple of small nits, and I think this is good to land! |
Add a mention to metallb#1523
15dd689
to
c2e3f26
Compare
Add a mention to metallb#1523
c2e3f26
to
445abc8
Compare
Add a mention to metallb#1523
445abc8
to
9016108
Compare
For each CR, we try creating a resource in a namespace other than the metallb namespace and expect a failure.
Add a mention to metallb#1523
9016108
to
86f0a49
Compare
LGTM, thanks! |
Add a mention to metallb#1523
Add a mention to metallb#1523
Add a mention to metallb#1523
Add a mention to metallb/metallb#1523
Fixed #1517