-
Notifications
You must be signed in to change notification settings - Fork 25
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
VC-36547: Remove gabs library as a dependency #598
Conversation
Signed-off-by: Tim Ramlot <[email protected]>
@@ -18,10 +18,10 @@ func TestSelect(t *testing.T) { | |||
"metadata": map[string]interface{}{ | |||
"name": "example", | |||
"namespace": "example", | |||
"annotations": map[string]string{ | |||
"annotations": map[string]interface{}{ |
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.
We can expect all maps to be of type map[string]interface{}
, that is the behavior for Kubernetes objects unmarshalled in unstructured objects. The helper unstructured functions will also make sure this stays true (eg. https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/unstructured/helpers.go#L259).
"metadata.uid", | ||
"type", | ||
"/data/tls.crt", | ||
"/data/ca.crt", |
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.
Why did this original code use /
as path separator for these two fields?
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 @inteon
- I see that this is part of a plan that you and @maelvls agreed upon.
- Good to remove this unnecessary dependency and the resulting code is more readable.
- I ran the
./hack/e2e/test.sh
script and it passed. - I noticed some comments and code in
pkg.datagatherere.k8s.dynamic.go:redactList
which might be obsoleted by this change. Ignore this comment if you disagree.// all objects fetched from sharedIndexInformers is now redacted // removing the managedFields and `kubectl.kubernetes.io/last-applied-configuration` annotation
// During the internal marshal/unmarshal the runtime.Object the metav1.TypeMeta seems to be lost // this section reassigns the TypeMeta to the resource
jetstack-secure/pkg/datagatherer/k8s/dynamic.go
Lines 349 to 409 in 2ebec9e
func redactList(list []*api.GatheredResource) error { | |
for i := range list { | |
if item, ok := list[i].Resource.(*unstructured.Unstructured); ok { | |
// Determine the kind of items in case this is a generic 'mixed' list. | |
gvks, _, err := scheme.Scheme.ObjectKinds(item) | |
if err != nil { | |
return errors.WithStack(err) | |
} | |
resource := item | |
// Redact item if it is a: | |
for _, gvk := range gvks { | |
// secret object | |
if gvk.Kind == "Secret" && (gvk.Group == "core" || gvk.Group == "") { | |
Select(SecretSelectedFields, resource) | |
// route object | |
} else if gvk.Kind == "Route" && gvk.Group == "route.openshift.io" { | |
Select(RouteSelectedFields, resource) | |
} | |
} | |
// remove managedFields from all resources | |
Redact(RedactFields, resource) | |
continue | |
} | |
// objectMeta interface is used to give resources from sharedIndexInformers, (core.Pod|apps.Deployment), a common interface | |
// with access to the metav1.Object | |
type objectMeta interface{ GetObjectMeta() metav1.Object } | |
// all objects fetched from sharedIndexInformers is now redacted | |
// removing the managedFields and `kubectl.kubernetes.io/last-applied-configuration` annotation | |
if item, ok := list[i].Resource.(objectMeta); ok { | |
item.GetObjectMeta().SetManagedFields(nil) | |
delete(item.GetObjectMeta().GetAnnotations(), "kubectl.kubernetes.io/last-applied-configuration") | |
resource := item.(runtime.Object) | |
gvks, _, err := scheme.Scheme.ObjectKinds(resource) | |
if err != nil { | |
return errors.WithStack(err) | |
} | |
// During the internal marshal/unmarshal the runtime.Object the metav1.TypeMeta seems to be lost | |
// this section reassigns the TypeMeta to the resource | |
for _, gvk := range gvks { | |
if len(gvk.Kind) == 0 { | |
continue | |
} | |
if len(gvk.Version) == 0 || gvk.Version == runtime.APIVersionInternal { | |
continue | |
} | |
resource.GetObjectKind().SetGroupVersionKind(gvk) | |
break | |
} | |
continue | |
} | |
} | |
return nil | |
} |
/approve
/lgtm
It is still useful to remove annotations and managed fields here: jetstack-secure/pkg/datagatherer/k8s/dynamic.go Lines 383 to 384 in 2ebec9e
Because the |
Instead of using the
gabs
library and an unnecessary marshal + unmarshal roundtrip, we can just use theunstructured
helpers.This is the first step in the plan outlined here: https://venafi.atlassian.net/browse/VC-36216?focusedCommentId=439503