Bug 1832232: Gather image registry config by mfojtik · Pull Request #100 · openshift/insights-operator

mfojtik · 2020-05-05T13:34:21Z

The config is valuable as we can drive deprecation of some fields inside it, but also valuable to get insights into how image registry is configured in clusters.

/cc @bparees @dmage

bparees · 2020-05-05T13:42:36Z

images is more about the external registries they are using. we probably also want configs.imageregistry.operator.openshift.io which is the operator config resource for how the internal registry is configured (e.g. what storage it uses) if we're not already getting that somehow.

mfojtik · 2020-05-05T13:46:26Z

@bparees where is the client/api for that resource?

bparees · 2020-05-05T13:57:14Z

@bparees where is the client/api for that resource?

https://github.com/openshift/api/blob/master/imageregistry/v1/types.go
https://github.com/openshift/client-go/tree/master/imageregistry

dmage · 2020-05-05T13:57:19Z

@mfojtik you have a typo in the commit message, you grab config.imageregistry.operator.openshift.io/cluster

mfojtik · 2020-05-05T14:00:42Z

@dmage fixed

dmage · 2020-05-05T14:08:57Z

/lgtm

mfojtik · 2020-05-05T14:09:48Z

/hold

for CCX team approval

iNecas · 2020-05-05T14:55:40Z

Going to play a bad 👮‍♂️ here, but mind adding some tests to https://github.com/openshift/insights-operator/blob/master/pkg/gather/clusterconfig/clusterconfig_test.go, having some example input and anonymized output would make me sleep better at night :)

martinkunc · 2020-05-06T05:12:17Z

@mfojtik Could you you please add a bugzilla case to know to which versions it would have to be backported ?

martinkunc · 2020-05-06T05:37:21Z

+
+// Marshal implements serialization of Ingres.Spec.Domain with anonymization
+func (a ImageRegistryAnonymizer) Marshal(_ context.Context) ([]byte, error) {
+	a.Spec.HTTPSecret = anonymizeString(a.Spec.HTTPSecret)


What if we would just remove all Storage ? Can here be any other types, like pvc ?
And what about HttpSecret, is it something to remove as well ?

If we remove Storage, we won't be able to know if someone uses a storage parameter.

openshift-ci-robot · 2020-05-06T11:17:06Z

@mfojtik: This pull request references Bugzilla bug 1832232, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1832232: Gather image registry config

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.

mfojtik · 2020-05-06T11:17:31Z

@martinkunc I expect this to be backported to 4.4 at least.

mfojtik · 2020-05-06T11:57:49Z

@iNecas test pushed. PTAL.

mfojtik · 2020-05-06T11:58:11Z

/bugzilla refresh

openshift-ci-robot · 2020-05-06T11:58:18Z

@mfojtik: This pull request references Bugzilla bug 1832232, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

/bugzilla refresh

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.

iNecas · 2020-05-06T12:30:31Z

+				},
+			},
+			evalOutput: func(t *testing.T, obj *imageregistryv1.Config) {
+				if obj.Spec.Storage.GCS.Bucket == "foo" {


s/foo/bucket, or maybe just checking it contains noething but /x*/ would be better (and reusable for all keys to prevent this kind of copy/paste problems)

it does not matter :-) this just checks the Bucket value was modified by anonymyzer.

And if you read it carefully, you will find this test actually would catch even if the anonymizer was not there, becuase the strings are different (foo vs. bucket)

mfojtik · 2020-05-11T12:14:22Z

@iNecas @martinkunc all updated, are we fine here?

iNecas · 2020-05-11T12:26:56Z

Letting @martinkunc to have the final word, but not other comments from me. Thanks

martinkunc · 2020-05-11T12:39:47Z

 	}
 }

+// GatherClusterImageRegistry fetches the cluster Image Registry configuration


Could you please also run make gen-doc ? That should update the doc/gathered-data.md with this new method.

martinkunc · 2020-05-11T13:05:39Z

 )

+func init() {
+	utilruntime.Must(registryv1.AddToScheme(registryScheme))


Is is neccessary to register the registryv1 explicitly ? I would think it would be registered automatically by importing the package, like it is happening with openshift/kubernetes types. Or am I missing something ?

it is always better to make your own local scheme than poisoning or depending on some "automatic" behavior...

Is is neccessary to register the registryv1 explicitly ? I would think it would be registered automatically by importing the package, like it is happening with openshift/kubernetes types. Or am I missing something ?

Automatic registration is a bug, not a feature because it assumes a single unified scheme. As we evolved our API, we developed disjoint schemes focused on their individual purposes. This improves reliability because it avoids action at a distance. It is more appropriate to install the types you're using in a local scheme like this.

Ok, nice, thanks for the explanation both !

martinkunc · 2020-05-11T16:38:13Z

/lgtm

martinkunc · 2020-05-11T16:40:21Z

/approve

openshift-ci-robot · 2020-05-11T16:40:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, martinkunc, mfojtik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [martinkunc,mfojtik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mfojtik · 2020-05-11T16:57:13Z

/hold cancel

openshift-ci-robot · 2020-05-11T17:01:57Z

@mfojtik: All pull requests linked via external trackers have merged: openshift/insights-operator#100. Bugzilla bug 1832232 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1832232: Gather image registry config

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.

mfojtik · 2020-05-11T17:03:27Z

/cherrypick release-4.4

openshift-cherrypick-robot · 2020-05-11T17:03:36Z

@mfojtik: #100 failed to apply on top of branch "release-4.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Patch failed at 0001 bump(*): get openshift/client-go for image registry

Details

In response to this:

/cherrypick release-4.4

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.

openshift-ci-robot requested review from bparees and dmage May 5, 2020 13:35

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2020

dmage reviewed May 5, 2020

View reviewed changes

Comment thread pkg/gather/clusterconfig/clusterconfig.go

Comment thread pkg/gather/clusterconfig/clusterconfig.go

mfojtik force-pushed the collect-image-registry branch from 6b761dd to 5e29518 Compare May 5, 2020 13:59

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2020

openshift-ci-robot assigned dmage May 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020

martinkunc reviewed May 6, 2020

View reviewed changes

mfojtik changed the title ~~Gather image registry config~~ Bug 1832232: Gather image registry config May 6, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 6, 2020

bump(*): get openshift/client-go for image registry

9da47ea

mfojtik force-pushed the collect-image-registry branch from 5e29518 to bb51e9d Compare May 6, 2020 11:57

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 6, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 6, 2020

iNecas reviewed May 6, 2020

View reviewed changes

mfojtik force-pushed the collect-image-registry branch 2 times, most recently from ae8d271 to dc3dff6 Compare May 11, 2020 12:13

martinkunc approved these changes May 11, 2020

View reviewed changes

gather: grab config.imageregistry.operator.openshift.io/cluster

79019f7

mfojtik force-pushed the collect-image-registry branch from dc3dff6 to 79019f7 Compare May 11, 2020 12:55

martinkunc approved these changes May 11, 2020

View reviewed changes

openshift-ci-robot assigned martinkunc May 11, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2020

openshift-merge-robot merged commit 66179bb into openshift:master May 11, 2020

This was referenced May 11, 2020

Bug 1820633: [release-4.4] Gather image config #101

Closed

[release-4.4] Bug 1834677: Gather image registry config #103

Closed

martinkunc mentioned this pull request Jun 29, 2020

[release-4.4] Bug 1834677: Gather image registry config (refresh) #131

Merged

Conversation

mfojtik commented May 5, 2020

Uh oh!

bparees commented May 5, 2020

Uh oh!

mfojtik commented May 5, 2020

Uh oh!

Uh oh!

Uh oh!

bparees commented May 5, 2020

Uh oh!

dmage commented May 5, 2020

Uh oh!

mfojtik commented May 5, 2020

Uh oh!

dmage commented May 5, 2020

Uh oh!

mfojtik commented May 5, 2020

Uh oh!

iNecas commented May 5, 2020

Uh oh!

martinkunc commented May 6, 2020

Uh oh!

martinkunc May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

mfojtik commented May 6, 2020

Uh oh!

mfojtik commented May 6, 2020

Uh oh!

mfojtik commented May 6, 2020

Uh oh!

openshift-ci-robot commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented May 11, 2020

Uh oh!

iNecas commented May 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinkunc commented May 11, 2020

Uh oh!

martinkunc commented May 11, 2020

Uh oh!

openshift-ci-robot commented May 11, 2020

Uh oh!

mfojtik commented May 11, 2020

Uh oh!

openshift-ci-robot commented May 11, 2020

Uh oh!

mfojtik commented May 11, 2020

Uh oh!

openshift-cherrypick-robot commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

martinkunc May 6, 2020 •

edited

Loading