Add by pod status for mutators #1260
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1260 +/- ##
==========================================
- Coverage 49.86% 48.47% -1.40%
==========================================
Files 65 68 +3
Lines 4522 4879 +357
==========================================
+ Hits 2255 2365 +110
- Misses 1956 2161 +205
- Partials 311 353 +42
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
maxsmythe
force-pushed
the
mutation-status
branch
from
3bb6528
to
d632c3b
Compare
maxsmythe
force-pushed
the
mutation-status
branch
2 times, most recently
from
2474e19
to
2b530f7
Compare
maxsmythe
changed the title
maxsmythe
force-pushed
the
mutation-status
branch
from
204513d
to
1927b3d
Compare
| @@ -162,7 +162,7 @@ install: manifests | |||
| kustomize build config/crd | kubectl apply -f - | |||
|
|
|||
| deploy-mutation: patch-image | |||
| @grep -q -v 'enable-mutation' ./config/overlays/dev_mutation/manager_image_patch.yaml && sed -i '/- --operation=webhook/a \ \ \ \ \ \ \ \ - --enable-mutation=true' ./config/overlays/dev_mutation/manager_image_patch.yaml | |||
| @grep -q -v 'enable-mutation' ./config/overlays/dev_mutation/manager_image_patch.yaml && sed -i '/- --operation=webhook/a \ \ \ \ \ \ \ \ - --enable-mutation=true' ./config/overlays/dev_mutation/manager_image_patch.yaml && sed -i '/- --operation=status/a \ \ \ \ \ \ \ \ - --operation=mutation-status' ./config/overlays/dev_mutation/manager_image_patch.yaml | |||
There was a problem hiding this comment.
TODO: need to update this in https://github.com/open-policy-agent/gatekeeper/blob/master/deploy/experimental/gatekeeper-mutation.yaml and helm chart when we have this in the next release.
There was a problem hiding this comment.
Do we have a process for this? Should that be part of this PR?
There was a problem hiding this comment.
Until #1190 is fixed, these updates need to done manually when we cut a release. Not part of this PR. For now, let's open an issue to track it and tag the upcoming milestone/release.
| return nil, err | ||
| } | ||
| key := apiTypes.NamespacedName{Name: sName, Namespace: util.GetNamespace()} | ||
| if err := r.Get(context.TODO(), key, statusObj); err != nil { |
There was a problem hiding this comment.
Should we use r.reader.Get here?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := r.Create(context.TODO(), statusObj); err != nil { |
| tracker.Observe(assignMetadata) | ||
| tracker.TryCancelExpect(assignMetadata) | ||
| status.Status.Errors = append(status.Status.Errors, statusv1beta1.Error{Message: err.Error()}) | ||
| if err2 := r.Update(ctx, status); err != nil { |
There was a problem hiding this comment.
This reconcile struct doesn't have a reader/writer.
That was a change Oren made to certain controllers a while back "because the manager's default client bypasses the cache for unstructured resources" (per a comment he left).
This is not a concern for Assign/AssignMetadata as we are working with the typed client (not the unstructured client).
Default kubebuilder-generated code follows the present model of embedding the client in the reconciler struct. Since there is no functional difference, I'm ambivalent as to which is better. Do you prefer aligning with Kubebuilder or our other controllers?
There was a problem hiding this comment.
This is not a concern for Assign/AssignMetadata as we are working with the typed client (not the unstructured client).
Agreed. Since this is not unstructured client, there's no functional difference. Let's keep it as is then.
| log.Error(err, "Insert failed", "resource", request.NamespacedName) | ||
| tracker.TryCancelExpect(assignMetadata) | ||
| status.Status.Errors = append(status.Status.Errors, statusv1beta1.Error{Message: err.Error()}) | ||
| if err2 := r.Update(ctx, status); err != nil { |
| func (r *Reconciler) defaultGetPod() (*corev1.Pod, error) { | ||
| // require injection of GetPod in order to control what client we use to | ||
| // guarantee we don't inadvertently create a watch | ||
| panic("GetPod must be injected") |
There was a problem hiding this comment.
Panic writes a core dump to stderr. Is there something specific you want to see with a log line.
FWIW this is a sanity check that the object is initialized properly, the pod wont run and our unit tests/e2e tests (should) fail if we hit this point
There was a problem hiding this comment.
yea I was thinking at least log the pod name but yea the pod won't even come up so maybe it's a moot point.
|
Thanks for the reviews! Comments resolved except for those I had follow-up questions on. |
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
maxsmythe
force-pushed
the
mutation-status
branch
from
2376959
to
1870602
Compare
Signed-off-by: Max Smythe <smythe@google.com>
maxsmythe
force-pushed
the
mutation-status
branch
from
1870602
to
dc695d7
Compare
* Add by pod status for mutators Signed-off-by: Max Smythe <smythe@google.com> * Only enable mutation status if mutation is enabled Signed-off-by: Max Smythe <smythe@google.com> * Add tests Signed-off-by: Max Smythe <smythe@google.com> * Address PR comments Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe smythe@google.com
What this PR does / why we need it:
This adds byPod status for mutators. Testing is what's WIP, feel free to review the code itself.
It also refactors the mutation package some to avoid import cycles.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer: