Skip to content
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

feat: max_idle_conn_time #605

Merged
merged 4 commits into from
Jun 2, 2021
Merged

feat: max_idle_conn_time #605

merged 4 commits into from
Jun 2, 2021

Conversation

ArthurKnoep
Copy link
Contributor

Related issue

This PR is related to #523

Proposed changes

This PR use the new sql database flag max_idle_conn_time to allow configration of https://golang.org/pkg/database/sql/#DB.SetConnMaxIdleTime

Checklist

  • I have read the contributing 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
    [email protected]) 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

This PR is currently a draft as it needs the merge of ory/x#346

@zepatrik
Copy link
Member

I guess the only thing missing is updating ory/x to v0.0.242 and it should work?

@ArthurKnoep
Copy link
Contributor Author

I guess the only thing missing is updating ory/x to v0.0.242 and it should work?

Yep and also updating the doc

I plan to do it tomorrow

@ArthurKnoep ArthurKnoep marked this pull request as ready for review June 1, 2021 07:43
@ArthurKnoep
Copy link
Contributor Author

I've updated the dependency and also, it appears that the related docs is not in this repo but in ory/docs.

So this should be ready for a review

@zepatrik
Copy link
Member

zepatrik commented Jun 1, 2021

Hm the failing CI is caused by some vulnerable outdated deep-dependency introduced in this PR. How should we handle that @aeneasr ? When using trivy instead of nancy, I find other vulnerabilities...

@aeneasr
Copy link
Member

aeneasr commented Jun 1, 2021

If the PR fails sec vuln scanning due to changes it should be fixed in the PR IMO

@aeneasr
Copy link
Member

aeneasr commented Jun 1, 2021

For unfound dependencies (e.g. trivy) these should be addressed in the PR that adds the scanner :)

@ArthurKnoep
Copy link
Contributor Author

So, to my understanding the vulnerabilities are caused by this package alone: golang/k8s.io/[email protected], which is declared as k8s.io/kubernetes in the go.sum.

However go mod why doesn't want to tell why this dependency is needed (from what I understood, go mod why only works for direct and indirect dependencies declared in the go.mod)

So what should I do here to fix the vuln ?

@zepatrik
Copy link
Member

zepatrik commented Jun 2, 2021

The only way I know of that gives you those dependencies is https://github.com/Helcaraxan/gomod
I checked again and master does not have those alerts. Therefore, they are probably caused by some new dependency added in this PR.

@ArthurKnoep
Copy link
Contributor Author

ArthurKnoep commented Jun 2, 2021

So, I've finally been able to fix the vuln dependency. I tried for more than one hour to make gomod graph and go mod graph | modv to work with dot from graphviz but it appears that there are too many dependencies to draw a complete dependency graph in a reasonable amount of time.

So, what I've done is that I've rolled back my changes on go.mod to master, downloaded all the dependencies and cleaned my cache. Then I've re-updated github.com/gobuffalo/pop/v5 to v5.3.4 and github.com/ory/x to v0.0.242 and the vulnerable dependency has now disapeared.
I think it has appeared during the devleopment of my feature and got fixed since I've opened this PR.

We should be good to go now 😄

@aeneasr
Copy link
Member

aeneasr commented Jun 2, 2021

Awesome, thank you so much! :)

@aeneasr aeneasr merged commit 50a8623 into ory:master Jun 2, 2021
@ArthurKnoep ArthurKnoep deleted the feature/max-idle-conn-time branch June 2, 2021 11:51
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.

3 participants