From a67baf5a4daae320e2877d7e6f2178c55a1b647d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Budai?= Date: Fri, 2 Oct 2020 11:38:24 +0200 Subject: [PATCH] upload/koji: use the new API of kolo/xmlrpc by default Fedora 33 ships the new API so let's do the switch now. But... this would break older Fedoras because they only have the old API, right? We have the following options: 1) Ship xmlrpc compat package to Fedora 33+. This would mean that we delay the API switch till F32 EOL. This would be the most elegant solution, yet it has two issues: a) We will surely not be able to deliver the compat package before F33 Final Freeze. b) It's an extra and annoying work. 2) Downstream patch. No. 3) Use build constraints and have two versions of our code for both different API. I chose solution #3. It has an issue though: %gobuild macro already passes -tags argument to go build. Therefore the following line fails because it's not possible to use -tags more than once: %gobuild -tags kolo_xmlrpc_oldapi ... Therefore I had to come up with manual tinkering with the build constraints in the spec file. This is pretty ugly but I like that: 1) Go code is actually clean, no weird magic is happening there. 2) We can still ship our software to Fedora/RHEL as we used to (no downstream patches) 3) All downstreams can use the upstream spec file directly. Note that this doesn't affect RHEL in any way as it uses vendored libraries. --- go.mod | 2 +- go.sum | 4 +- golang-github-osbuild-composer.spec | 14 ++++ .../upload/koji/xmlrpc-response-oldapi.go | 4 +- internal/upload/koji/xmlrpc-response.go | 34 ++++++++++ osbuild-composer.spec | 14 ++++ vendor/github.com/kolo/xmlrpc/README.md | 3 +- vendor/github.com/kolo/xmlrpc/client.go | 65 ++++++++----------- vendor/github.com/kolo/xmlrpc/decoder.go | 5 +- vendor/github.com/kolo/xmlrpc/encoder.go | 32 +++++---- vendor/github.com/kolo/xmlrpc/is_zero.go | 44 +++++++++++++ vendor/github.com/kolo/xmlrpc/response.go | 50 ++++++-------- vendor/github.com/kolo/xmlrpc/test_server.rb | 6 +- vendor/github.com/kolo/xmlrpc/xmlrpc.go | 19 ------ vendor/modules.txt | 2 +- 15 files changed, 190 insertions(+), 108 deletions(-) create mode 100644 vendor/github.com/kolo/xmlrpc/is_zero.go delete mode 100644 vendor/github.com/kolo/xmlrpc/xmlrpc.go diff --git a/go.mod b/go.mod index 0ad87091c..099f43ff8 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/google/uuid v1.1.1 github.com/gophercloud/gophercloud v0.11.0 github.com/julienschmidt/httprouter v1.2.0 - github.com/kolo/xmlrpc v0.0.0-20190417161013-de6d879202d7 + github.com/kolo/xmlrpc v0.0.0-20200310150728-e0350524596b github.com/labstack/echo/v4 v4.1.11 github.com/pkg/errors v0.9.1 // indirect github.com/stretchr/testify v1.5.1 diff --git a/go.sum b/go.sum index 1dbf11c1b..cbc3e81b4 100644 --- a/go.sum +++ b/go.sum @@ -74,8 +74,8 @@ github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af h1:pmfjZENx5i github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k= github.com/julienschmidt/httprouter v1.2.0 h1:TDTW5Yz1mjftljbcKqRcrYhd4XeOoI98t+9HbQbYf7g= github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= -github.com/kolo/xmlrpc v0.0.0-20190417161013-de6d879202d7 h1:kL2yi3DjwkRWFgKwD5COyl4XMLKhfOvqck4xyis7EIw= -github.com/kolo/xmlrpc v0.0.0-20190417161013-de6d879202d7/go.mod h1:o03bZfuBwAXHetKXuInt4S7omeXUu62/A845kiycsSQ= +github.com/kolo/xmlrpc v0.0.0-20200310150728-e0350524596b h1:DzHy0GlWeF0KAglaTMY7Q+khIFoG8toHP+wLFBVBQJc= +github.com/kolo/xmlrpc v0.0.0-20200310150728-e0350524596b/go.mod h1:o03bZfuBwAXHetKXuInt4S7omeXUu62/A845kiycsSQ= github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= diff --git a/golang-github-osbuild-composer.spec b/golang-github-osbuild-composer.spec index 3d771a1fd..eeec12f5d 100644 --- a/golang-github-osbuild-composer.spec +++ b/golang-github-osbuild-composer.spec @@ -64,6 +64,20 @@ Provides: weldr %goprep %endif +%if 0%{?fedora} && 0%{?fedora} <= 32 +# Fedora 32 and older ships a different kolo/xmlrpc API. We cannot specify +# build tags in gobuild macro because the macro itself specifies build tags. +# and -tags argument cannot be used more than once. +# Therefore, this ugly hack with build tags switcharoo is required. +# Remove when F32 is EOL. + +# Remove the build constraint from the wrapper of the old API +sed -i "s$// +build kolo_xmlrpc_oldapi$// +build !kolo_xmlrpc_oldapi$" internal/upload/koji/xmlrpc-response-oldapi.go + +# Add a build constraint to the wrapper of the new API +sed -i "s$// +build !kolo_xmlrpc_oldapi$// +build kolo_xmlrpc_oldapi$" internal/upload/koji/xmlrpc-response.go +%endif + %build %if 0%{?rhel} GO_BUILD_PATH=$PWD/_build diff --git a/internal/upload/koji/xmlrpc-response-oldapi.go b/internal/upload/koji/xmlrpc-response-oldapi.go index 24b4bc732..3b52a4372 100644 --- a/internal/upload/koji/xmlrpc-response-oldapi.go +++ b/internal/upload/koji/xmlrpc-response-oldapi.go @@ -1,4 +1,6 @@ -// This files provides a wrapper around kolo/xmlrpc response handling. +// +build kolo_xmlrpc_oldapi +// +// This file provides a wrapper around kolo/xmlrpc response handling. // // Commit e3ad6d89 of the xmlrpc library changed the API of response handling. // This means that different APIs are available in Fedora 32 and 33 (it does diff --git a/internal/upload/koji/xmlrpc-response.go b/internal/upload/koji/xmlrpc-response.go index e69de29bb..dc4f9be11 100644 --- a/internal/upload/koji/xmlrpc-response.go +++ b/internal/upload/koji/xmlrpc-response.go @@ -0,0 +1,34 @@ +// +build !kolo_xmlrpc_oldapi +// +// This file provides a wrapper around kolo/xmlrpc response handling. +// +// Commit e3ad6d89 of the xmlrpc library changed the API of response handling. +// This means that different APIs are available in Fedora 32 and 33 (it does +// not matter for RHEL as uses vendored libraries). +// This wrapper allows us to use both xmlrpc's APIs using buildflags. +// +// This file is a wrapper for xmlrpc equal or newer than e3ad6d89. + +package koji + +import ( + "fmt" + + "github.com/kolo/xmlrpc" +) + +// processXMLRPCResponse is a wrapper around kolo/xmlrpc +func processXMLRPCResponse(body []byte, reply interface{}) error { + resp := xmlrpc.Response(body) + + if resp.Err() != nil { + return fmt.Errorf("xmlrpc server returned an error: %v", resp.Err()) + } + + err := resp.Unmarshal(reply) + if err != nil { + return fmt.Errorf("cannot unmarshal the xmlrpc response: %v", err) + } + + return nil +} diff --git a/osbuild-composer.spec b/osbuild-composer.spec index cf9fb5533..e7d364d8f 100644 --- a/osbuild-composer.spec +++ b/osbuild-composer.spec @@ -87,6 +87,20 @@ Provides: golang-github-osbuild-composer = %{version}-%{release} %goprep %endif +%if 0%{?fedora} && 0%{?fedora} <= 32 +# Fedora 32 and older ships a different kolo/xmlrpc API. We cannot specify +# build tags in gobuild macro because the macro itself specifies build tags. +# and -tags argument cannot be used more than once. +# Therefore, this ugly hack with build tags switcharoo is required. +# Remove when F32 is EOL. + +# Remove the build constraint from the wrapper of the old API +sed -i "s$// +build kolo_xmlrpc_oldapi$// +build !kolo_xmlrpc_oldapi$" internal/upload/koji/xmlrpc-response-oldapi.go + +# Add a build constraint to the wrapper of the new API +sed -i "s$// +build !kolo_xmlrpc_oldapi$// +build kolo_xmlrpc_oldapi$" internal/upload/koji/xmlrpc-response.go +%endif + %build %if 0%{?rhel} GO_BUILD_PATH=$PWD/_build diff --git a/vendor/github.com/kolo/xmlrpc/README.md b/vendor/github.com/kolo/xmlrpc/README.md index 8113cfcc3..fecfcd839 100644 --- a/vendor/github.com/kolo/xmlrpc/README.md +++ b/vendor/github.com/kolo/xmlrpc/README.md @@ -44,11 +44,12 @@ Data types encoding rules: * xmlrpc.Base64 encoded to base64; * slice encoded to array; -Structs decoded to struct by following rules: +Structs encoded to struct by following rules: * all public field become struct members; * field name become member name; * if field has xmlrpc tag, its value become member name. +* for fields tagged with `",omitempty"`, empty values are omitted; Server method can accept few arguments, to handle this case there is special approach to handle slice of empty interfaces (`[]interface{}`). diff --git a/vendor/github.com/kolo/xmlrpc/client.go b/vendor/github.com/kolo/xmlrpc/client.go index 3aa86ce2c..643dc1c10 100644 --- a/vendor/github.com/kolo/xmlrpc/client.go +++ b/vendor/github.com/kolo/xmlrpc/client.go @@ -31,7 +31,7 @@ type clientCodec struct { responses map[uint64]*http.Response mutex sync.Mutex - response *Response + response Response // ready presents channel, that is used to link request and it`s response. ready chan uint64 @@ -43,16 +43,16 @@ type clientCodec struct { func (codec *clientCodec) WriteRequest(request *rpc.Request, args interface{}) (err error) { httpRequest, err := NewRequest(codec.url.String(), request.ServiceMethod, args) + if err != nil { + return err + } + if codec.cookies != nil { for _, cookie := range codec.cookies.Cookies(codec.url) { httpRequest.AddCookie(cookie) } } - if err != nil { - return err - } - var httpResponse *http.Response httpResponse, err = codec.httpClient.Do(httpRequest) @@ -75,43 +75,39 @@ func (codec *clientCodec) WriteRequest(request *rpc.Request, args interface{}) ( func (codec *clientCodec) ReadResponseHeader(response *rpc.Response) (err error) { var seq uint64 - select { case seq = <-codec.ready: case <-codec.close: return errors.New("codec is closed") } - - codec.mutex.Lock() - httpResponse := codec.responses[seq] - codec.mutex.Unlock() - - if httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 { - return fmt.Errorf("request error: bad status code - %d", httpResponse.StatusCode) - } - - respData, err := ioutil.ReadAll(httpResponse.Body) - - if err != nil { - return err - } - - httpResponse.Body.Close() - - resp := NewResponse(respData) - - if resp.Failed() { - response.Error = fmt.Sprintf("%v", resp.Err()) - } - - codec.response = resp - response.Seq = seq codec.mutex.Lock() + httpResponse := codec.responses[seq] delete(codec.responses, seq) codec.mutex.Unlock() + defer httpResponse.Body.Close() + + if httpResponse.StatusCode < 200 || httpResponse.StatusCode >= 300 { + response.Error = fmt.Sprintf("request error: bad status code - %d", httpResponse.StatusCode) + return nil + } + + body, err := ioutil.ReadAll(httpResponse.Body) + if err != nil { + response.Error = err.Error() + return nil + } + + resp := Response(body) + if err := resp.Err(); err != nil { + response.Error = err.Error() + return nil + } + + codec.response = resp + return nil } @@ -119,12 +115,7 @@ func (codec *clientCodec) ReadResponseBody(v interface{}) (err error) { if v == nil { return nil } - - if err = codec.response.Unmarshal(v); err != nil { - return err - } - - return nil + return codec.response.Unmarshal(v) } func (codec *clientCodec) Close() error { diff --git a/vendor/github.com/kolo/xmlrpc/decoder.go b/vendor/github.com/kolo/xmlrpc/decoder.go index 61712d8aa..d4dcb19ad 100644 --- a/vendor/github.com/kolo/xmlrpc/decoder.go +++ b/vendor/github.com/kolo/xmlrpc/decoder.go @@ -130,8 +130,9 @@ func (dec *decoder) decodeValue(val reflect.Value) error { ismap = true } else if checkType(val, reflect.Interface) == nil && val.IsNil() { var dummy map[string]interface{} - pmap = reflect.New(reflect.TypeOf(dummy)).Elem() - valType = pmap.Type() + valType = reflect.TypeOf(dummy) + pmap = reflect.New(valType).Elem() + val.Set(pmap) ismap = true } else { return err diff --git a/vendor/github.com/kolo/xmlrpc/encoder.go b/vendor/github.com/kolo/xmlrpc/encoder.go index d585a7d3f..7ab271aa5 100644 --- a/vendor/github.com/kolo/xmlrpc/encoder.go +++ b/vendor/github.com/kolo/xmlrpc/encoder.go @@ -7,9 +7,13 @@ import ( "reflect" "sort" "strconv" + "strings" "time" ) +// Base64 represents value in base64 encoding +type Base64 string + type encodeFunc func(reflect.Value) ([]byte, error) func marshal(v interface{}) ([]byte, error) { @@ -80,28 +84,34 @@ func encodeValue(val reflect.Value) ([]byte, error) { return []byte(fmt.Sprintf("%s", string(b))), nil } -func encodeStruct(val reflect.Value) ([]byte, error) { +func encodeStruct(structVal reflect.Value) ([]byte, error) { var b bytes.Buffer b.WriteString("") - t := val.Type() - for i := 0; i < t.NumField(); i++ { - b.WriteString("") - f := t.Field(i) + structType := structVal.Type() + for i := 0; i < structType.NumField(); i++ { + fieldVal := structVal.Field(i) + fieldType := structType.Field(i) - name := f.Tag.Get("xmlrpc") - if name == "" { - name = f.Name + name := fieldType.Tag.Get("xmlrpc") + // if the tag has the omitempty property, skip it + if strings.HasSuffix(name, ",omitempty") && isZero(fieldVal) { + continue + } + name = strings.TrimSuffix(name, ",omitempty") + if name == "" { + name = fieldType.Name } - b.WriteString(fmt.Sprintf("%s", name)) - p, err := encodeValue(val.FieldByName(f.Name)) + p, err := encodeValue(fieldVal) if err != nil { return nil, err } - b.Write(p) + b.WriteString("") + b.WriteString(fmt.Sprintf("%s", name)) + b.Write(p) b.WriteString("") } diff --git a/vendor/github.com/kolo/xmlrpc/is_zero.go b/vendor/github.com/kolo/xmlrpc/is_zero.go new file mode 100644 index 000000000..65276d04a --- /dev/null +++ b/vendor/github.com/kolo/xmlrpc/is_zero.go @@ -0,0 +1,44 @@ +package xmlrpc + +import ( + "math" + . "reflect" +) + +func isZero(v Value) bool { + switch v.Kind() { + case Bool: + return !v.Bool() + case Int, Int8, Int16, Int32, Int64: + return v.Int() == 0 + case Uint, Uint8, Uint16, Uint32, Uint64, Uintptr: + return v.Uint() == 0 + case Float32, Float64: + return math.Float64bits(v.Float()) == 0 + case Complex64, Complex128: + c := v.Complex() + return math.Float64bits(real(c)) == 0 && math.Float64bits(imag(c)) == 0 + case Array: + for i := 0; i < v.Len(); i++ { + if !isZero(v.Index(i)) { + return false + } + } + return true + case Chan, Func, Interface, Map, Ptr, Slice, UnsafePointer: + return v.IsNil() + case String: + return v.Len() == 0 + case Struct: + for i := 0; i < v.NumField(); i++ { + if !isZero(v.Field(i)) { + return false + } + } + return true + default: + // This should never happens, but will act as a safeguard for + // later, as a default value doesn't makes sense here. + panic(&ValueError{"reflect.Value.IsZero", v.Kind()}) + } +} diff --git a/vendor/github.com/kolo/xmlrpc/response.go b/vendor/github.com/kolo/xmlrpc/response.go index 6742a1c74..18e6d366c 100644 --- a/vendor/github.com/kolo/xmlrpc/response.go +++ b/vendor/github.com/kolo/xmlrpc/response.go @@ -1,6 +1,7 @@ package xmlrpc import ( + "fmt" "regexp" ) @@ -8,43 +9,32 @@ var ( faultRx = regexp.MustCompile(`(\s|\S)+`) ) -type failedResponse struct { - Code int `xmlrpc:"faultCode"` - Error string `xmlrpc:"faultString"` +// FaultError is returned from the server when an invalid call is made +type FaultError struct { + Code int `xmlrpc:"faultCode"` + String string `xmlrpc:"faultString"` } -func (r *failedResponse) err() error { - return &xmlrpcError{ - code: r.Code, - err: r.Error, +// Error implements the error interface +func (e FaultError) Error() string { + return fmt.Sprintf("Fault(%d): %s", e.Code, e.String) +} + +type Response []byte + +func (r Response) Err() error { + if !faultRx.Match(r) { + return nil } -} - -type Response struct { - data []byte -} - -func NewResponse(data []byte) *Response { - return &Response{ - data: data, - } -} - -func (r *Response) Failed() bool { - return faultRx.Match(r.data) -} - -func (r *Response) Err() error { - failedResp := new(failedResponse) - if err := unmarshal(r.data, failedResp); err != nil { + var fault FaultError + if err := unmarshal(r, &fault); err != nil { return err } - - return failedResp.err() + return fault } -func (r *Response) Unmarshal(v interface{}) error { - if err := unmarshal(r.data, v); err != nil { +func (r Response) Unmarshal(v interface{}) error { + if err := unmarshal(r, v); err != nil { return err } diff --git a/vendor/github.com/kolo/xmlrpc/test_server.rb b/vendor/github.com/kolo/xmlrpc/test_server.rb index 1b1ff8760..1ccfc9ac4 100644 --- a/vendor/github.com/kolo/xmlrpc/test_server.rb +++ b/vendor/github.com/kolo/xmlrpc/test_server.rb @@ -15,9 +15,9 @@ class Service x + y end - def error - raise XMLRPC::FaultException.new(500, "Server error") - end + def error + raise XMLRPC::FaultException.new(500, "Server error") + end end server = XMLRPC::Server.new 5001, 'localhost' diff --git a/vendor/github.com/kolo/xmlrpc/xmlrpc.go b/vendor/github.com/kolo/xmlrpc/xmlrpc.go deleted file mode 100644 index 8766403af..000000000 --- a/vendor/github.com/kolo/xmlrpc/xmlrpc.go +++ /dev/null @@ -1,19 +0,0 @@ -package xmlrpc - -import ( - "fmt" -) - -// xmlrpcError represents errors returned on xmlrpc request. -type xmlrpcError struct { - code int - err string -} - -// Error() method implements Error interface -func (e *xmlrpcError) Error() string { - return fmt.Sprintf("error: \"%s\" code: %d", e.err, e.code) -} - -// Base64 represents value in base64 encoding -type Base64 string diff --git a/vendor/modules.txt b/vendor/modules.txt index 48cd3997f..55011c733 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -119,7 +119,7 @@ github.com/gophercloud/gophercloud/pagination github.com/jmespath/go-jmespath # github.com/julienschmidt/httprouter v1.2.0 github.com/julienschmidt/httprouter -# github.com/kolo/xmlrpc v0.0.0-20190417161013-de6d879202d7 +# github.com/kolo/xmlrpc v0.0.0-20200310150728-e0350524596b github.com/kolo/xmlrpc # github.com/kr/pretty v0.1.0 github.com/kr/pretty