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

Add git2go option to enable Azure DevOps #213

Merged
merged 13 commits into from
Dec 8, 2020

Conversation

phillebaba
Copy link
Member

@phillebaba phillebaba commented Nov 21, 2020

So this change has been a long time coming, and has had its ups and downs, but now lets hope that this will be the PR that fixes a major blocker for Azure DevOps users.

The git2go library works with Azure DevOps as it has greater support for more complex capabilities in the git wire protocol. A downside of the library is that it does not support shallow cloning of a git repository, meaning that it needs to fetch whole repository every time. Something that could in theory be expensive both in time and transfer data for very large repositories. As this is a very large downside the compromise solution is to implement a option in the GitRepository resource called gitImplementation to enable the use of the git2go library. This is turned off to default meaning that source-controller will default to using the usual go-git library people are familiar with.

Eventually when shallow cloning is supported in git2go we could consider fully migrating to only using git2go.
libgit2/libgit2#5254

There are a couple of things that have to be done before merging this:

  • Know Host validation in v2
  • Transport tests in v2
  • Update flux2 docs explaining when to enable v2

@stefanprodan @hiddeco I would appreciate any early feedback in case there is some major design flaws.

Fixes #104

An additional selling point for this change is that git2go could in the future allow us to do self signed cert verification from memory as it has a certificate validation callback.

@phillebaba
Copy link
Member Author

In general i think we need better ssh protocol validation for both v1 and v2 implementations

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

We need to install libgit2 on the CI VM:

- name: Install libgit2
  run: sudo apt-get install -y libgit2-dev

@phillebaba
Copy link
Member Author

The libgit2 version in apt is very old so we need to find another way to install the latest version.

@stefanprodan
Copy link
Member

Maybe this one will work https://packages.debian.org/experimental/libgit2-1.0

@hiddeco
Copy link
Member

hiddeco commented Nov 23, 2020

That version is too old, see the table here. I am afraid we may have to build it ourselves.

pkg/git/v2/transport.go Outdated Show resolved Hide resolved
@phillebaba
Copy link
Member Author

I have been looking around and it seems like you are right @hiddeco. I really like yak shaving 😄

@hiddeco
Copy link
Member

hiddeco commented Nov 25, 2020

Stefan and I discussed another option: testing from within a Docker container, that would allow us to piggyback on the package that is available for Alpine images.

@relu
Copy link
Member

relu commented Nov 26, 2020

Running the whole job in a container is fairly simple with GHA, removing the actions/setup-go@v2 step and adding this to the job section should do it:

container:
  image: golang:1.15-alpine

I think it's the best approach to keep the build and test environment as close as possible, it should probably be done for all the other components too.

@stefanprodan
Copy link
Member

@relu if we use the container image than we can no longer cache go modules and all the builds will take 5+ minutes. We don't use CGO expect in here, so I don't see why would we give up caching everywhere.

@relu
Copy link
Member

relu commented Nov 26, 2020

I don't think it will break caching, at least I remember that was not the case in a previous project I worked on, although it was npm caching that was used rather than go modules, I think the mechanism is the same though.

@stefanprodan
Copy link
Member

Are you saying that GitHub mounts the Go cache from host into the golang:1.15-alpine container? If so that's news to me

@relu
Copy link
Member

relu commented Nov 26, 2020

Well... technically the cache itself is not on the host, it's kept in GHA caches (limited to 5GB) and restored over the network (really fast on GitHub-operated runners). We use caching even on self-hosted runners and it works the same, though a bit slower as you can imagine. Let me put it to the test, gonna try it out in a separate PR.

@stefanprodan
Copy link
Member

@relu we can't run the whole thing in a container, we need Kubernetes Kind, docker buildx, etc

@relu
Copy link
Member

relu commented Nov 26, 2020

Yes, and we probably don't want to mess with dind. Failure log is here: https://github.com/fluxcd/source-controller/pull/216/checks?check_run_id=1458681336 (at least the caching works, so that's something).

@stefanprodan
Copy link
Member

@relu I think we should move only the make test into a container.

@relu
Copy link
Member

relu commented Nov 26, 2020

@stefanprodan I agree, I've tested multiple solutions for running everything inside a container and it doesn't look good. It is worth noting that it is possible to get all the tooling installed and caching works just fine, but it seems to fail for some reason during test execution and I think it's not worth digging deeper as long as we can simply just build and run a container for the test step alone. I can help out with that if you don't already have a working example.

@stefanprodan
Copy link
Member

@relu if you can, please open a PR that moves the unit tests and git dirty check inside a container.

@phillebaba
Copy link
Member Author

I can probably free up some time during the weekend but if @relu is already on it I can just rebase when his PR gets merged.

@phillebaba phillebaba force-pushed the feature/git2go branch 3 times, most recently from 34d61e3 to ebc3ce8 Compare November 26, 2020 20:12
@phillebaba phillebaba force-pushed the feature/git2go branch 2 times, most recently from d4d2d1e to 3dd0312 Compare November 29, 2020 21:05
@phillebaba
Copy link
Member Author

So I rebased the latest changes to run the test in a container. Could someone have a look and check if there is something else missing before this can be merged?

.github/actions/run-tests/Dockerfile Outdated Show resolved Hide resolved
pkg/git/common/common.go Outdated Show resolved Hide resolved
pkg/git/v2/trasnport_test.go Outdated Show resolved Hide resolved
Philip Laine and others added 9 commits December 2, 2020 20:18
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@gmail.com>
Signed-off-by: Philip Laine <philip.laine@gmail.com>
Signed-off-by: Philip Laine <philip.laine@gmail.com>
@phillebaba
Copy link
Member Author

Just to make sure that we have discussed this point.

libgit2 will default to OpenSSL for HTTPS transport (except on Windows and macOS, as mentioned above). On any system, mbedTLS may be optionally enabled as the security provider. OpenSSL is thread-safe starting at version 1.1.0. If your copy of libgit2 is linked against that version, you do not need to take any further steps.
https://github.com/libgit2/libgit2/blob/master/docs/threading.md

Looking at alpine it should include openssl 1.1.1 so this should not be an issue.
https://pkgs.alpinelinux.org/packages?name=openssl&branch=v3.12

Could I just have someone else verify my thinking.

@relu
Copy link
Member

relu commented Dec 2, 2020

I just checked the latest golang:1.15-alpine image, it looks like the package you're looking for is actually libssl1.1, which seems to be installed by default. Indeed its version matches 1.1.1 so it should be just fine according to the documentation.

# apk info libssl1.1
WARNING: Ignoring APKINDEX.2c4ac24e.tar.gz: No such file or directory
WARNING: Ignoring APKINDEX.40a3604f.tar.gz: No such file or directory
libssl1.1-1.1.1g-r0 description:
SSL shared libraries

libssl1.1-1.1.1g-r0 webpage:
https://www.openssl.org/

libssl1.1-1.1.1g-r0 installed size:
540672

Philip Laine added 2 commits December 3, 2020 12:56
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Just a couple of naming nits from me

api/v1beta1/gitrepository_types.go Outdated Show resolved Hide resolved
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package git
package v1
Copy link
Member

Choose a reason for hiding this comment

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

I reckon name the subpackages for the implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean naming the package go-git instead of v1? Would that not create confusion as there would be two packages with the same name.

@@ -39,7 +39,7 @@ deploy: manifests
kustomize build config/default | kubectl apply -f -

# Deploy controller dev image in the configured Kubernetes cluster in ~/.kube/config
dev-deploy: manifests
Copy link
Member

Choose a reason for hiding this comment

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

Why was this omitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Manifest gen requires libgit2 to be installed which wont work in the CI as this is run outside of the CI container. That is why it had to be removed.

docs/spec/v1beta1/gitrepositories.md Outdated Show resolved Hide resolved
Signed-off-by: Philip Laine <philip.laine@xenit.se>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @phillebaba 🥇

Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

LGTM 🦸

Signed-off-by: Philip Laine <philip.laine@xenit.se>
// +kubebuilder:validation:Enum=go-git;libgit2
// +kubebuilder:default:=go-git
// +optional
GitImplementation string `json:"gitImplementation,omitempty"`
Copy link

Choose a reason for hiding this comment

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

I really like this new property!

@dirien
Copy link

dirien commented Dec 9, 2020

Question regarding the off the shelve (OTS) function from kustomize: Will this still work?

We use this quite heavily in our applications and found out in e.g. ArgoCD that even their switch to the native CLI it is still breaking in the off the shelve part of the kustomize.

Maybe it is more of a concern for kustomize cli.... ?

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.

Azure DevOps compatibility issues
7 participants