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(app): switch git proxy to golang #993

Merged
merged 10 commits into from
Apr 11, 2022
Merged

feat(app): switch git proxy to golang #993

merged 10 commits into from
Apr 11, 2022

Conversation

olevski
Copy link
Member

@olevski olevski commented Apr 4, 2022

Found a bit better maintained proxy library in Go. This will resolve all the critical security vulnerabilities we have with the current node js proxy.

I also added tests for the proxy that ensure that the Git credentials are added in the right circumstances and edited the CI pipeline so that these tests run on every push.

@olevski olevski requested a review from a team as a code owner April 4, 2022 21:02
@olevski olevski force-pushed the replace-git-proxy branch 6 times, most recently from 9d51b9b to d93c288 Compare April 4, 2022 22:07
@olevski
Copy link
Member Author

olevski commented Apr 5, 2022

The integration tests are failing because the git-proxy shuts down before the autosave can be created when a session is stopped. This is exactly the problem that #951 will address. Therefore to keep the diffs sane I think that we should merge this PR first and then rebase #951 to work with this version of the git proxy.

This occurs more rarely in practice (but it still does) because the old version of the git proxy does not handle the SIGTERM signal properly and waits until it receives SIGKILL from k8s 30 seconds later. This is enough in most cases to pass the integration tests.

@olevski
Copy link
Member Author

olevski commented Apr 6, 2022

The integration tests fully pass in #951. So this can be merged and then #951 can be reviewed and merged. Or we can just close this PR and merge just #951.

vigsterkr
vigsterkr previously approved these changes Apr 6, 2022
Copy link
Contributor

@vigsterkr vigsterkr left a comment

Choose a reason for hiding this comment

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

some total nitpicking, nothing major. feel free to ignore those

git-https-proxy/main.go Outdated Show resolved Hide resolved
git-https-proxy/main.go Outdated Show resolved Hide resolved
git-https-proxy/main.go Outdated Show resolved Hide resolved
git-https-proxy/main.go Outdated Show resolved Hide resolved
git-https-proxy/main.go Outdated Show resolved Hide resolved
git-https-proxy/main.go Outdated Show resolved Hide resolved
olevski and others added 3 commits April 8, 2022 11:25
Co-authored-by: Viktor Gal <vigsterkr@gmail.com>
Co-authored-by: Viktor Gal <vigsterkr@gmail.com>
@olevski olevski requested a review from vigsterkr April 8, 2022 09:35
Copy link
Contributor

@vigsterkr vigsterkr left a comment

Choose a reason for hiding this comment

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

:shipit:

@olevski olevski merged commit 3f0f965 into master Apr 11, 2022
@olevski olevski deleted the replace-git-proxy branch April 11, 2022 09:22
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.

None yet

2 participants