MAISTRA-1071 Add manifest namespace validation #462

dgn · 2020-06-11T15:08:12Z

This is two parts:

validatingWebhook code to give users early and comprehensible feedback on why they can't use a namespace
reconciler.validateManifests function that makes sure we'll never deploy resources into namespaces that are not part of the mesh

pkg/controller/servicemesh/controlplane/reconciler.go

rcernich · 2020-06-11T15:43:01Z

pkg/controller/servicemesh/controlplane/reconciler.go

+	if err := r.Client.Get(context.TODO(), client.ObjectKey{Namespace: r.Instance.GetNamespace(), Name: common.MemberRollName}, smmr); err != nil {
+		if !apierrors.IsNotFound(err) {
+			// log error, but don't fail validation: we'll just assume that the control plane namespace is the only namespace for now
+			log.Error(err, "failed to retrieve SMMR for SMCP")


It's probably worth returning the error and letting the reconciliation restart. If there's a gateway that references a namespace that is legitimate, then we'll error when we validate, which will be a false error message.

So you want to restart reconciliation without logging an error in this case?

The thing is that this will probably only happen rarely, and even if it happens, it will only have an impact if you have a gateway outside the control plane namespace. The idea with just logging and continuing was that 99% of the time it'll be alright, because the gateway is in the cp namespace anyway

I believe you can just return the error from the reconcile loop and let it do its thing. I don't remember if we need to log it and return it, or just return it. @luksa?

I agree that it's probably unlikely that it will fail and unlikely that a namespace other than the control plane will be used. Maybe keep track of the fact that the smmr lookup failed and then if there's a validation failure, you can return the smmr error instead. wdyt?

Yeah, I think that makes sense

I fixed it now. While at it, I found another issue; I had forgotten to split the manifests (by "---") so was only looking at the first manifest in the output of each helm template (which for the gateways are multiple).

dgn · 2020-06-16T12:20:55Z

FYI I ran the validation function 1k times and it takes 21ms on average on my machine, so should be okay

luksa

LGTM but I'm wondering why we need to consider the member namespaces when checking the manifests. Aren't all resources deployed in the control plane namespace? If yes, then we could perform the validation when we render the charts.

pkg/controller/servicemesh/controlplane/reconciler_test.go

dgn · 2020-06-22T14:58:46Z

LGTM but I'm wondering why we need to consider the member namespaces when checking the manifests. Aren't all resources deployed in the control plane namespace? If yes, then we could perform the validation when we render the charts.

The gateways are an exception in that they can be deployed to any namespace by providing a
istio.gateways.some-gateway.namespace: someNamespace; this is meant to allow for multiple ingress/egress gateways in separate namespaces.

The reason I added this in a generic fashion is that things like this could come up again in future versions of Istio, and it might not just be restricted to gateways

dgn · 2020-06-22T15:16:47Z

Adding a hold as this will go into 1.1.5

dgn · 2020-07-13T08:55:05Z

/cherry-pick maistra-1.2

maistra-bot · 2020-07-13T08:55:20Z

@dgn: #462 failed to apply on top of branch "maistra-1.2":

Using index info to reconstruct a base tree...
M	pkg/controller/common/util.go
M	pkg/controller/servicemesh/controlplane/reconciler.go
M	pkg/controller/servicemesh/controlplane/reconciler_test.go
M	pkg/controller/servicemesh/webhooks/validation/controlplane_test.go
A	pkg/controller/servicemesh/webhooks/validation/controlplane_util.go
A	pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/versions/util.go
CONFLICT (content): Merge conflict in pkg/controller/versions/util.go
CONFLICT (modify/delete): pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go deleted in HEAD and modified in MAISTRA-1071 Add manifest namespace validation. Version MAISTRA-1071 Add manifest namespace validation of pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go left in tree.
Auto-merging pkg/controller/servicemesh/webhooks/validation/controlplane_test.go
CONFLICT (content): Merge conflict in pkg/controller/servicemesh/webhooks/validation/controlplane_test.go
Auto-merging pkg/controller/servicemesh/controlplane/reconciler_test.go
CONFLICT (content): Merge conflict in pkg/controller/servicemesh/controlplane/reconciler_test.go
Auto-merging pkg/controller/servicemesh/controlplane/reconciler.go
Auto-merging pkg/controller/common/util.go
CONFLICT (content): Merge conflict in pkg/controller/common/util.go
error: Failed to merge in the changes.
Patch failed at 0001 MAISTRA-1071 Add manifest namespace validation

In response to this:

/cherry-pick maistra-1.2

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.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

maistra-bot added the size/L label Jun 11, 2020

rcernich reviewed Jun 11, 2020

View reviewed changes

dgn force-pushed the MAISTRA-1071 branch 3 times, most recently from 733f97e to 5c3beb2 Compare June 16, 2020 11:36

rcernich approved these changes Jun 16, 2020

View reviewed changes

dgn requested a review from luksa June 22, 2020 11:48

luksa approved these changes Jun 22, 2020

View reviewed changes

pkg/controller/servicemesh/controlplane/reconciler_test.go Show resolved Hide resolved

mergify bot added the ready to merge label Jun 22, 2020

MAISTRA-1071 Add manifest namespace validation

e68c00e

dgn force-pushed the MAISTRA-1071 branch from 5c3beb2 to e68c00e Compare June 22, 2020 15:09

dgn added the do-not-merge/work-in-progress label Jun 22, 2020

knrc added okay to merge and removed do-not-merge/work-in-progress labels Jul 10, 2020

mergify bot removed the ready to merge label Jul 10, 2020

Merge branch 'maistra-1.1' into MAISTRA-1071

58dddbf

mergify bot merged commit 5db8548 into maistra:maistra-1.1 Jul 10, 2020

dgn deleted the MAISTRA-1071 branch July 13, 2020 08:54

dgn added a commit to dgn/istio-operator that referenced this pull request Jul 14, 2020

MAISTRA-1071 Add manifest namespace validation (maistra#462)

5ede35f

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

dgn added a commit to dgn/istio-operator that referenced this pull request Jul 14, 2020

MAISTRA-1071 Add manifest namespace validation (maistra#462)

e23d01d

dgn mentioned this pull request Jul 14, 2020

MAISTRA-1071 Add manifest namespace validation #479

Merged

tatianab mentioned this pull request Nov 8, 2023

x/vulndb: potential Go vuln in github.com/maistra/istio-operator: CVE-2020-14306 golang/vulndb#2277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAISTRA-1071 Add manifest namespace validation #462

MAISTRA-1071 Add manifest namespace validation #462

dgn commented Jun 11, 2020 •

edited

Loading

rcernich Jun 11, 2020

dgn Jun 11, 2020

dgn Jun 11, 2020

rcernich Jun 11, 2020

rcernich Jun 11, 2020

dgn Jun 16, 2020

dgn Jun 16, 2020

dgn commented Jun 16, 2020 •

edited

Loading

luksa left a comment

dgn commented Jun 22, 2020

dgn commented Jun 22, 2020

dgn commented Jul 13, 2020

maistra-bot commented Jul 13, 2020

MAISTRA-1071 Add manifest namespace validation #462

MAISTRA-1071 Add manifest namespace validation #462

Conversation

dgn commented Jun 11, 2020 • edited Loading

rcernich Jun 11, 2020

Choose a reason for hiding this comment

dgn Jun 11, 2020

Choose a reason for hiding this comment

dgn Jun 11, 2020

Choose a reason for hiding this comment

rcernich Jun 11, 2020

Choose a reason for hiding this comment

rcernich Jun 11, 2020

Choose a reason for hiding this comment

dgn Jun 16, 2020

Choose a reason for hiding this comment

dgn Jun 16, 2020

Choose a reason for hiding this comment

dgn commented Jun 16, 2020 • edited Loading

luksa left a comment

Choose a reason for hiding this comment

dgn commented Jun 22, 2020

dgn commented Jun 22, 2020

dgn commented Jul 13, 2020

maistra-bot commented Jul 13, 2020

dgn commented Jun 11, 2020 •

edited

Loading

dgn commented Jun 16, 2020 •

edited

Loading