Revision Tag Support #413

dgn · 2024-10-11T17:16:26Z

Fixes #380

codecov · 2024-10-18T12:55:53Z

Codecov Report

Attention: Patch coverage is 86.65208% with 61 lines in your changes missing coverage. Please review.

Project coverage is 78.77%. Comparing base (0fa4f24) to head (a9519cd).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...rs/istiorevisiontag/istiorevisiontag_controller.go	86.21%	36 Missing and 15 partials ⚠️
cmd/main.go	0.00%	6 Missing ⚠️
...trollers/istiorevision/istiorevision_controller.go	94.23%	2 Missing and 1 partial ⚠️
pkg/validation/namespace.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   76.01%   78.77%   +2.75%     
==========================================
  Files          36       38       +2     
  Lines        1864     2309     +445     
==========================================
+ Hits         1417     1819     +402     
- Misses        372      401      +29     
- Partials       75       89      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

controllers/istiorevision/istiorevision_controller.go

luksa · 2024-10-21T12:18:50Z

controllers/istiorevision/istiorevision_controller.go

+		return false, fmt.Errorf("failed to list IstioRevisionTags: %w", err)
+	}
+	for _, tag := range revisionTagList.Items {
+		if tag.Status.IstioRevision == rev.Name && tag.Status.GetCondition(v1alpha1.IstioRevisionTagConditionInUse).Status == metav1.ConditionTrue {


Is it correct to check whether the tag is in use? As a user, if I create a tag and point it to a revision, I don't want that revision to be deleted by the operator. It doesn't matter if there are no pods/namespaces referencing that tag.

Also, is it correct to check status.istioRevision instead of spec.targetRef? What if the spec was just updated to point to the revision, but the controller hasn't yet updated the status?

Is it correct to check whether the tag is in use? As a user, if I create a tag and point it to a revision, I don't want that revision to be deleted by the operator. It doesn't matter if there are no pods/namespaces referencing that tag.

I thought about that, too. Yeah, maybe referencing a revision in a tag should be treated just like a reference from a pod or namespace and prevent deletion. I can implement that.

Also, is it correct to check status.istioRevision instead of spec.targetRef? What if the spec was just updated to point to the revision, but the controller hasn't yet updated the status?

Well, the status shows what is actually referenced ie what the webhooks will inject. So I would consider that to take precedent over what is in the spec. Apart from that, it's not obvious from the spec which IstioRevision is actually referenced, eg if the targetRef points to an Istio resource. I think adding this logic here would mean the IstioRevision controller knowing too many technical details about IstioRevisionTag. But I'm happy to be convinced otherwise.

luksa · 2024-10-21T12:20:13Z

You also need to add IstioRevisionTag to the PROJECT file. I believe some make target(s) only update certain files if the CRD is listed in the PROJECT file.

nrfox · 2024-10-21T12:44:58Z

api/v1alpha1/istiorevisiontags_types.go

+	// Kind is the kind of the target resource.
+	//
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	Kind string `json:"kind"`


nit: add enum validation for Istio and IstioRevision?

nrfox · 2024-10-21T12:47:44Z

api/v1alpha1/istiorevisiontags_types.go

+	// IstioRevision stores the name of the referenced IstioRevision
+	IstioRevision string `json:"istioRevision"`


Do you also need Istio here in cases where an Istio is referenced instead of an IstioRevision? In that case maybe this should be ,omitempty so you don't have a blank istioRevision when you reference and Istio.

no, actually the reasoning behind this field is exactly the fact that we could be referencing an Istio resource. The IstioRevision controller needs some easy way to know which IstioRevision is being referenced (even if technically it is referencing an Istio, it will always resolve to an IstioRevision), so instead of duplicating the resolution logic behind that, I'm just doing it at reconcile time in the IstioRevisionTag controller and write the result into this field

chart/samples/istio-sample-gw-api.yaml

chart/samples/istio-sample-revisionbased.yaml

controllers/istiorevisiontag/istiorevisiontag_controller.go

nrfox · 2024-10-21T17:08:03Z

controllers/istiorevision/istiorevision_controller.go

@@ -409,6 +417,18 @@ func (r *Reconciler) isRevisionReferencedByWorkloads(ctx context.Context, rev *v
 	log := logf.FromContext(ctx)
 	nsList := corev1.NamespaceList{}
 	nsMap := map[string]corev1.Namespace{}
+	// if we're referenced by an in-use revisionTag, we're done. we can outsource the checking to the IstioRevisionTagController


Is the revision tag controller also checking pods? What if you have created a tag but are still using the rev label and not the tag name to label your namespace?

The only thing the tag adds, if it exists, is an additional condition for the namespace. Now it could be either istio.io/rev: <rev> or istio.io/rev: <tag>.

actually... I was previously checking the pods but that doesn't make sense, as their annotation will always have the actual revision name, not the tag. so we don't need to check pods in the revisiontag controller

okay I removed the annotation checking, but I'm now checking the label to identify injection intent

api/v1alpha1/istiorevisiontags_types.go

nrfox · 2024-10-21T17:27:36Z

controllers/istiorevisiontag/istiorevisiontag_controller.go

+	podList := corev1.PodList{}
+	if err := r.Client.List(ctx, &podList); err != nil { // TODO: can we optimize this by specifying a label selector
+		return false, fmt.Errorf("failed to list pods: %w", err)
+	}


I wonder if this controller should be checking for pods at all or if it can defer to the istiorevision controller for since that controller is already checking for the pod annotation that is independent of tag? Basically if this controller's istiorev ref is in use then this tag should be considered in use?

Main question I have is this ^^. Rest is basically nits.

hmm. yes that is an optimization, at least for the pods. for the namespaces we'd still need to check.

actually no it doesn't work. I just made a change that means that now, any tag referencing a revision will mean that revision is considered InUse. We'd have to read from the status whether it was specifically a pod/ns/tag that triggered the status.

dgn · 2024-10-24T09:45:21Z

Okay I refactored (and simplified) the InUse detection quite a bit and I'm going to outline it here (will also update the design doc accordingly):

An IstioRevisionTag is considered InUse when

there's a pod or namespace that explicitly references the IstioRevisionTag in an istio.io/rev label
the name of the IstioRevisionTag name is 'default' and there's a pod or namespace with the istio-injection: enabled label

Note that a pod's istio.io/rev annotation will not be considered as that will always have the name of the referenced IstioRevision rather than the name of the IstioRevisionTag! The labels however are added by users and reflect usage intent, ie the user will use the name of the IstioRevisionTag.

Even if the referenced IstioRevision of an IstioRevisionTag is considered InUse, that does not suffice to make the IstioRevisionTag considered InUse by the operator! It is considered an unused alias for an InUse IstioRevision.

An IstioRevision is considered InUse when (new parts in bold)

there's a pod that explicitly references the IstioRevision in an istio.io/rev annotation or label
there's a namespace that explicitly references the IstioRevision in an istio.io/rev label
the name of the IstioRevision name is 'default' and there's a pod or namespace with the istio-injection: enabled label
there's an IstioRevisionTag referencing this IstioRevision

Note specifically the fourth item here: being referenced by an IstioRevisionTag will now always lead to being considered InUse! It does not matter if the IstioRevisionTag is itself considered InUse!

I think this is the cleanest approach with the least amount of circular dependencies.

nrfox

When I first create the sample this is what I get:

kubectl get istios
NAME      REVISIONS   READY   IN USE   ACTIVE REVISION   STATUS    VERSION   AGE
default   1           1       1        default-v1-23-2   Healthy   v1.23.2   113s

kubectl get istiorevisions.sailoperator.io
NAME              TYPE    READY   STATUS    IN USE   VERSION   AGE
default-v1-23-2   Local   True    Healthy   True     v1.23.2   2m5s

kubectl get istiorevisiontags.sailoperator.io
NAME      STATUS                    IN USE   REVISION          AGE
default   NotReferencedByAnything   False    default-v1-23-2   114s

Notice that the Istio and IstioRevision are both IN USE = true but the tag is not? That rightfully changes when I deploy a sample workload that uses the tag but the istio/rev both seem to have a different definition of IN USE than the tag does.

edit: Re-reading the above comment outlining when each resource is considered IN USE, this is actually the expected behavior and I think makes sense.

edit2: I wonder if it would help to have short human readable message for the istiorevtag printer columns like "Istio Tag is not referenced by a pod or a namespace." to make it more clear why the tag is considered not IN USE?

nrfox

Just nits but otherwise LGTM. Everything works well and is nice to use.

docs/api-reference/sailoperator.io.md

tests/integration/api/istiorevisiontag_test.go

api/v1alpha1/istiorevisiontags_types.go

sridhargaddam · 2024-10-29T09:42:09Z

controllers/istiorevisiontag/istiorevisiontag_controller.go

+	IstioInjectionLabel        = "istio-injection"
+	IstioInjectionEnabledValue = "enabled"
+	IstioRevLabel              = "istio.io/rev"
+	IstioSidecarInjectLabel    = "sidecar.istio.io/inject"


We can probably move these to some common constants package as we have similar constants in istiorevistion_controller.go file.

good catch. done!

sridhargaddam · 2024-10-29T09:56:58Z

controllers/istiorevisiontag/istiorevisiontag_controller.go

+	if revision != "" {
+		return revision
+	}
+	// TODO: if .Values.sidecarInjectorWebhook.enableNamespacesByDefault is true, then all namespaces except system namespaces should use the "default" revision


Do we need this TODO here as we are checking for the enableNamespacesByDefault in Line 396?

yeah- this function is also called when the namespace watch triggers, so enableNamespacesByDefault is not fully supported yet (just like in our other controllers I'm afraid)

sridhargaddam · 2024-10-29T13:12:23Z

@dgn, does this PR implement the following from the SEP?

When the very first `IstioRevision` is created in a cluster from a `RevisionBased` Istio resource, the Sail Operator will create a `IstioRevisionTag` with the name `default`, referencing that `IstioRevision`. This way, the standard namespace injection label `istio-injection=enabled` will work out of the box for RevisionBased deployments

sridhargaddam · 2024-10-29T13:15:24Z

chart/samples/istio-sample-revisionbased.yaml

+  updateStrategy:
+    type: RevisionBased
+---
+apiVersion: sailoperator.io/v1alpha1


@dgn JFYI, I am seeing the following reconcile error (which is probably expected for the first time until IstioRevision is created - Can we mask it some how?) in the Sail Operator logs when applying this yaml.

sail-operator 2024-10-29T12:40:01Z ERROR ctrlr.revtag Reconciler error {"IstioRevisionTag": "default", "reconcileID": "1ff96be2-388e-41e6-a437-fed29156cc84", "error": "failed to retrieve IstioRevision for IstioRevisionTag \"default\": referenced Istio has no active revision"} sail-operator sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler sail-operator sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316 sail-operator sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem sail-operator sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:263 sail-operator sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2 sail-operator sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:224 sail-operator 2024-10-29T12:40:01Z INFO ctrlr.istio Reconciling {"Istio": "default", "reconcileID": "0580456b-b418-4041-a6f8-cd15d7d8d6a3"}

Yeah that is a problem. I'll check if I can make it look less like an error

should be fixed now!

dgn · 2024-11-06T09:59:04Z

@dgn, does this PR implement the following from the SEP?

When the very first `IstioRevision` is created in a cluster from a `RevisionBased` Istio resource, the Sail Operator will create a `IstioRevisionTag` with the name `default`, referencing that `IstioRevision`. This way, the standard namespace injection label `istio-injection=enabled` will work out of the box for RevisionBased deployments

As mentioned in Slack, that part will be left out (I moved it to Alternatives Considered) and might be picked up again as part of #439.

nrfox

1 comment but non-blocking. LGTM

nrfox · 2024-11-07T13:35:20Z

controllers/istiorevisiontag/istiorevisiontag_controller.go

+	return enqueuelogger.WrapIfNecessary(v1alpha1.IstioRevisionTagKind, logger, handler)
+}
+
+func UnpackReconcilerError(err error) error {


This is basically what Unwrap does?

fjglira · 2024-11-08T17:50:57Z

Do you think we should cover the use of the revision tag also in the e2e test? By adding some test cases related to revision based update

I also added a bunch of integration tests. Documentation is still to come. There are more scenarios that might need testing, we'll need to figure that out as we go. Signed-off-by: Daniel Grimm <[email protected]>

luksa · 2024-11-14T10:09:37Z

api/v1alpha1/istiorevisiontags_types.go

+// IstioRevisionTagSpec defines the desired state of IstioRevisionTag
+type IstioRevisionTagSpec struct {
+	// +kubebuilder:validation:Required
+	TargetRef IstioRevisionTagTargetReference `json:"targetRef"`


If an IstioRevisionTag can reference an Istio, doesn't that make the name "revision tag" confusing? Should it be called IstioTag?

Also, why exactly do we need the option to reference Istio? How do the two reference options fit into the fully-automatic updates? The plan was for the operator to automatically change the istio.io/rev label from the old to the new revision, so it should probably also change the targetRef in IstioRevisionTag. If we decide against the latter, then we should also not implement the former, but then the automatic update wouldn't actually do anything.

We should discuss this and go over all possible scenarios.

dgn requested a review from a team as a code owner October 11, 2024 17:16

istio-testing added needs-rebase size/XXL labels Oct 11, 2024

dgn added do-not-merge/work-in-progress and removed size/XXL needs-rebase labels Oct 11, 2024

dgn force-pushed the revision-tags branch from b82f3de to 9ef2b1e Compare October 11, 2024 17:17

istio-testing added size/XXL and removed do-not-merge/work-in-progress labels Oct 11, 2024

dgn force-pushed the revision-tags branch from 9ef2b1e to 88dc9d6 Compare October 11, 2024 17:21

istio-testing added the needs-rebase label Oct 12, 2024

dgn force-pushed the revision-tags branch from 88dc9d6 to 4d3f94d Compare October 18, 2024 12:52

istio-testing removed the needs-rebase label Oct 18, 2024

dgn force-pushed the revision-tags branch 3 times, most recently from 1f1ceca to 57a01bd Compare October 18, 2024 13:50

istio-testing added the needs-rebase label Oct 21, 2024

dgn force-pushed the revision-tags branch 2 times, most recently from 526451f to fb2e3ec Compare October 21, 2024 08:33

istio-testing removed the needs-rebase label Oct 21, 2024

dgn force-pushed the revision-tags branch from fb2e3ec to b26de7b Compare October 21, 2024 09:26

dgn changed the title ~~[DRAFT] Revision Tag Support~~ Revision Tag Support Oct 21, 2024

istio-ecosystem deleted a comment from istio-testing Oct 21, 2024

luksa reviewed Oct 21, 2024

View reviewed changes

controllers/istiorevision/istiorevision_controller.go Outdated Show resolved Hide resolved

luksa reviewed Oct 21, 2024

View reviewed changes

dgn force-pushed the revision-tags branch from b26de7b to d51800c Compare October 21, 2024 14:07

longmuir mentioned this pull request Oct 21, 2024

Add OSSM 2 vs 3 differences doc openshift-service-mesh/sail-operator#145

Merged

nrfox reviewed Oct 21, 2024

View reviewed changes

dgn force-pushed the revision-tags branch from 59cf6ae to ded0247 Compare October 23, 2024 15:23

istio-testing removed the needs-rebase label Oct 23, 2024

dgn force-pushed the revision-tags branch 2 times, most recently from da8d69e to 28be30b Compare October 24, 2024 09:34

nrfox reviewed Oct 28, 2024

View reviewed changes

docs/api-reference/sailoperator.io.md Outdated Show resolved Hide resolved

tests/integration/api/istiorevisiontag_test.go Outdated Show resolved Hide resolved

tests/integration/api/istiorevisiontag_test.go Outdated Show resolved Hide resolved

istio-testing added the needs-rebase label Oct 29, 2024

sridhargaddam reviewed Oct 29, 2024

View reviewed changes

dgn force-pushed the revision-tags branch 2 times, most recently from bd5db41 to 869dd43 Compare October 29, 2024 15:02

istio-testing removed the needs-rebase label Oct 29, 2024

dgn force-pushed the revision-tags branch 2 times, most recently from 3a2ec18 to a9b3523 Compare November 4, 2024 15:44

istio-testing added the needs-rebase label Nov 6, 2024

dgn force-pushed the revision-tags branch 2 times, most recently from 0b6b7b6 to 85879f2 Compare November 6, 2024 09:45

istio-testing removed the needs-rebase label Nov 6, 2024

dgn force-pushed the revision-tags branch 3 times, most recently from b4432ca to 47bb472 Compare November 6, 2024 15:47

nrfox reviewed Nov 7, 2024

View reviewed changes

IstioRevisionTag implementation

a9519cd

I also added a bunch of integration tests. Documentation is still to come. There are more scenarios that might need testing, we'll need to figure that out as we go. Signed-off-by: Daniel Grimm <[email protected]>

dgn force-pushed the revision-tags branch from 47bb472 to a9519cd Compare November 9, 2024 22:16

luksa reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revision Tag Support #413

Revision Tag Support #413

dgn commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

luksa Oct 21, 2024

dgn Oct 21, 2024

luksa commented Oct 21, 2024

nrfox Oct 21, 2024

nrfox Oct 21, 2024

dgn Oct 23, 2024

nrfox Oct 21, 2024

nrfox Oct 21, 2024

dgn Oct 24, 2024

dgn Oct 24, 2024

nrfox Oct 21, 2024 •

edited

Loading

nrfox Oct 21, 2024

dgn Oct 23, 2024 •

edited

Loading

dgn commented Oct 24, 2024 •

edited

Loading

nrfox left a comment •

edited

Loading

nrfox left a comment

sridhargaddam Oct 29, 2024

dgn Nov 6, 2024

sridhargaddam Oct 29, 2024

dgn Oct 29, 2024

sridhargaddam commented Oct 29, 2024

sridhargaddam Oct 29, 2024 •

edited

Loading

dgn Nov 6, 2024

dgn Nov 6, 2024

dgn commented Nov 6, 2024

nrfox left a comment

nrfox Nov 7, 2024

fjglira commented Nov 8, 2024

luksa Nov 14, 2024

		// IstioRevision stores the name of the referenced IstioRevision
		IstioRevision string `json:"istioRevision"`

Revision Tag Support #413

Are you sure you want to change the base?

Revision Tag Support #413

Conversation

dgn commented Oct 11, 2024 • edited Loading

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luksa commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrfox Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgn Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

dgn commented Oct 24, 2024 • edited Loading

nrfox left a comment • edited Loading

Choose a reason for hiding this comment

nrfox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sridhargaddam commented Oct 29, 2024

sridhargaddam Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgn commented Nov 6, 2024

nrfox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjglira commented Nov 8, 2024

Choose a reason for hiding this comment

dgn commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

nrfox Oct 21, 2024 •

edited

Loading

dgn Oct 23, 2024 •

edited

Loading

dgn commented Oct 24, 2024 •

edited

Loading

nrfox left a comment •

edited

Loading

sridhargaddam Oct 29, 2024 •

edited

Loading