Skip to content

wip: update to the recent x version#925

Closed
dadrus wants to merge 9 commits intoory:masterfrom
innoq:x_update
Closed

wip: update to the recent x version#925
dadrus wants to merge 9 commits intoory:masterfrom
innoq:x_update

Conversation

@dadrus
Copy link
Contributor

@dadrus dadrus commented Feb 16, 2022

@zepatrik: This the work in progress PR, you helped me today with.

Related issue(s)

None I'm aware of.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I am following the contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security. vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Work in progress. ;)

Current status: Does not compile as following needs still to be fixed:

github.com/ory/oathkeeper/driver/health
../driver/health/event_manager_default.go:64:27: cannot use listener.Validate (type func() error) as type healthx.ReadyChecker in assignment

github.com/ory/oathkeeper/driver/configuration
../driver/configuration/provider_viper.go:332:28: undefined: wiperx

@dadrus dadrus requested a review from aeneasr as a code owner February 16, 2022 19:12
@dadrus
Copy link
Contributor Author

dadrus commented Feb 16, 2022

@zepatrik: It would be great if you could take a look at event_manager_default.go:64:27. I'll try to resolve the other one by myself

if cfg.Api.Retry != nil {
maxRetryDelay := time.Second
giveUpAfter := time.Millisecond * 50
// giveUpAfter := time.Millisecond * 50 this was previously used as maxelapsedtime for the old transport implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeneasr : Can you please help with this one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean whether this default is ok?

}

client.Transport = httpx.NewResilientRoundTripper(a.client.Transport, maxRetryDelay, giveUpAfter)
// TODO: for the below block (related to the todo above)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeneasr : and with this one please ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean exactly 😅

@zepatrik zepatrik self-requested a review February 21, 2022 09:40
@CLAassistant
Copy link

CLAassistant commented Mar 10, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ dadrus
✅ zepatrik
❌ phaus
You have signed the CLA already but the status is still pending? Let us recheck it.

@dadrus
Copy link
Contributor Author

dadrus commented Mar 10, 2022

Even the updated code compiles, there is an issue with the updated golang version (required by the current x version and also for making use the embed directive). Starting with go 1.13 a change in the "flag" package has been introduced, which makes flags.Parse panic if a flag was already defined. This means also that flag.Parse is not allowed to be used in the Init function. Unfortunately a couple of oathkeeper (indirect) dependencies (even in the latest version) do that. This results in a panic saying that the v flag is redefined

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc00007c180, {0x1c912c8, 0xc000268ee0}, {0x1827032, 0x1}, {0x18312f8, 0x9})
	/opt/golang-v1.17.5/src/flag/flag.go:879 +0x2f4
flag.(*FlagSet).IntVar(...)
	/opt/golang-v1.17.5/src/flag/flag.go:658
flag.(*FlagSet).Int(0xc0008094c0, {0x1827032, 0x1}, 0x0, {0x18312f8, 0x9})
	/opt/golang-v1.17.5/src/flag/flag.go:671 +0x7e
flag.Int(...)
	/opt/golang-v1.17.5/src/flag/flag.go:678
github.com/google/martian/v3.init()
	/home/dimi/go/pkg/mod/github.com/google/martian/v3@v3.2.1/init.go:24 +0x65

when trying to start any of the tests or trying to start the compiled oathkeeper binary. The above panic relates to google/martian#309. Other dependencies, which have the same issue are github.com/Azure/go-autorest/autorest/adal and github.com/Azure/go-autorest/autorest.

An approach recommended at many places, like having a separate file in the main module with following contents

package main

import (
	"github.com/google/martian/v3"
)

var _ = func() bool {
	martian.Init()
	return true
}()

doesn't resolve the issue, as we then run into

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc00007c2a0, {0x1c91790, 0x28b40e8}, {0x182702a, 0x1}, {0x1844519, 0x14})
	/opt/golang-v1.17.5/src/flag/flag.go:879 +0x2f4
flag.Var(...)
	/opt/golang-v1.17.5/src/flag/flag.go:894
github.com/golang/glog.init.0()
	/home/dimi/go/pkg/mod/github.com/golang/glog@v1.0.0/glog.go:401 +0xc5

which doesn't call flag.Parse in its init function, but does also define the v flag and the panic message is again about the redefinition of the v flag. Applying the same pattern as above is not possible due to the private level of the init function in glog.
The glog, as well as the martian dependencies are indirect

I really appreciate if somebody knows how to resolve this

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm no idea, sorry. Maybe if you bump to ory/x v0.0.356 the dependency is actually gone? I was able to bring down the number of dependencies quite a lot there.

@aeneasr
Copy link
Member

aeneasr commented Mar 25, 2022

Sorry for replying so late. This is the root cause of the problem: dgraph-io/ristretto#292

I fixed this with this rewrite:

replace github.com/dgraph-io/ristretto => github.com/ory/ristretto fix-log

@aeneasr
Copy link
Member

aeneasr commented Sep 7, 2022

Superseded by #999

@aeneasr aeneasr closed this Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants