request: Making ocm-sdk easily packageable

[croissanne]

https://github.com/osbuild/osbuild-composer/ is a project that's aiming to run on api.openshift.com in future. However it also releases regularly into fedora/epel etc... And in order to depend on the ocm-sdk a few minor changes could help us greatly.

Most of ocm-sdk's dependencies are release into these platforms already, except:

• github.com/grokify/html-strip-tags-go:
  alternative https://github.com/microcosm-cc/bluemonday is packaged, plus it's much better suited to handle untrusted html.
• https://gitlab.com/c0b/go-ordered-json:
  a bit of a weird one, doesn't have go module support and seems really unmaintained (last code update was 3 years ago).
  We could package it alongside, but it has bugs (the tests don't work). Could also just copy the needed file into this project with a reference to it's upstream.

---

There's also the issue that https://github.com/dgrijalva/jwt-go is unmaintained. https://github.com/golang-jwt/jwt is a maintained fork, which sadly isn't yet packaged, but hopefully will be soon.

---

I'm happy to do the work on this, but want to discuss if it would be acceptable from your side first.

[vkareh]

Another option is to vendor in the dependencies instead of relying on packaged versions of those modules. That way the release cycle of ocm-sdk-go is not tied to the release cycle of third-party modules.

[croissanne]

Yea that's what composer ends up doing for most distributions other than fedora, though vendoring has it's own drawbacks. composer is tied to release cycles of distributions and thus the packages available on those distros, and I guess I'm trying to reconcile it being both tied to distribution's release cycles and being able to run it on a.o.c.

That said we'd package ocm-sdk, so I wouldn't be asking for openshift/ocm to actually package/release it into fedora.

Regardless some of the issues mentioned in the ticket re:deprecation are still valid I think. But feel free to close this ticket if that's best.

[jhernand]

@Gundersanne I think this is a very reasonable request. Some of the changes we should do regardless of packaging. I am reopening and starting working on it.

[croissanne]

Awesome, if you want me to take on one of them, like the tag stripping one, I'm very happy to do so.

[jhernand]

@Gundersanne yes, please, take the tag stripping. I will start with migrating to the new JWT library.

[croissanne]

How do you feel about the ordered map copy https://github.com/Gundersanne/ocm-sdk-go/tree/ordered-map ? If the copying over is a bit weird, I can try to open a MR on gitlab to fix the test and try to get it in that way.

[jhernand]

I don't like very much the idea of copying the code. Not that I have a concrete reason, just a feeling that it isn't OK.

I'd prefer to either switch to a different library or else write the required logic from scratch.

Note that we are only using that ordered map to avoid changing the order of the JSON written to the debug log. I took a quick look to github.com/tidwall/pretty. It is already packaged for Fedora, but I am not 100% sure if it preserves the order. Would you want to check that and maybe change the code to use it if it preserves the order?

[croissanne]

Yea I have to agree copying is a bit weird. https://github.com/tidwall/pretty seems to have a SortKeys option, so i'll give it a go :)

[jhernand]

Great, thanks. Note that the point is not changing the order of the JSON received/sent from the SDK. For example, if we receive { "b": "B", "a": "A" } we want to write to the log that formatted for humans, but without changing the order of the fields:

{
  "b": "B",
  "a": "A"
}

The reason for not changing the order is that some times the order is (or will be, in the future) important. For example, results of collections should have the "size" field before the array of items because that way the SDK can allocate the right array in advance. If the order in the debug log is different to what was actually received it is harder to debug errors in that logic.

So I guess (but I am not sure) that we shouldn't use the SortKeys option.

[croissanne]

Ah right, indeed SortKeys isn't it in that case.

[croissanne]

seems promising :)

[jhernand]

Yes, that is what we need, thanks!

[croissanne]

it doesn't really allow you to iterate over a json struct though, so the redacting becomes a lot harder.

[jhernand]

I think that for redacting we can use a plain map, something like this:

--- a/dump.go
+++ b/dump.go
@@ -30,8 +30,6 @@ import (
        "sort"
        "strings"
 
-       "gitlab.com/c0b/go-ordered-json"
-
        "github.com/openshift-online/ocm-sdk-go/logging"
 )
 
@@ -269,7 +267,7 @@ func (d *dumpRoundTripper) dumpForm(ctx context.Context, data []byte) {
 // dumpJSON tries to parse the given data as a JSON document. If that works, then it dumps it
 // indented, otherwise dumps it as is.
 func (d *dumpRoundTripper) dumpJSON(ctx context.Context, data []byte) {
-       parsed := ordered.NewOrderedMap()
+       var parsed map[string]interface{}
        err := json.Unmarshal(data, parsed)
        if err != nil {
                d.logger.Debug(ctx, "%s", data)
@@ -292,15 +290,10 @@ func (d *dumpRoundTripper) dumpBytes(ctx context.Context, data []byte) {
 }
 
 // redactSensitive replaces sensitive fields within a response with redactionStr.
-func (d *dumpRoundTripper) redactSensitive(body *ordered.OrderedMap) {
-       iterator := body.EntriesIter()
-       for {
-               pair, ok := iterator()
-               if !ok {
-                       break
-               }
-               if redactFields[pair.Key] {
-                       body.Set(pair.Key, redactionStr)
+func (d *dumpRoundTripper) redactSensitive(body map[string]interface{}) {
+       for key, _ := range body {
+               if redactFields[key] {
+                       body[key] = redactionStr
                }
        }
 }

Then we can use the new library to write to the log the original slice of bytes.

[jhernand]

Ah, we can't do that because we want to write out the modified/redacted map, not the original, sorry.

[jhernand]

@Gundersanne we may be able to do what we want using the JSON iterator that we already use for converting JSON documents to objects. I will take a look at that.

[jhernand]

Meanwhile, if you need an urgent solution for packaging in Fedora you may want to apply a patch similar to the one I shared. That would not respect the order in debug output, but that isn't the end of the world. Once we have a fix for the issue you can just remove the patch.

[croissanne]

It's not quite that urgent.

Hm would that work for nested values though? Not sure if the redacted fields are ever nested.

I guess we wanna avoid manipulating the byte slice directly. At first glance json-iter would work with constructing a new stream, writing everything except the redacted fields.

[croissanne]

I took a swing at it #423, not sure if this is what you had in mind. If this is in the right direction, might need to consider having a max depth on the recursion, and a test of course.

[jhernand]

Yes, that is what I was considering. Looks good to me as is.

[jhernand]

This issue has been addressed in release 0.1.199.