pusher / faros

Faros is a CRD based GitOps controller
Apache License 2.0
99 stars 15 forks source link

Added ClusterGitTrack #145

Closed sebastianrosch closed 4 years ago

sebastianrosch commented 5 years ago

In order to resolve #143 we added a v1alpha2.ClusterGitTrack resource which is cluster-scoped and can therefore own resources that are cluster-scoped or in any namespace.

The v1alpha1.GitTrack can own resources in it's own namespace, or, to stay backward-compatible, can own resources in any namespace when the allow-cross-namespace-ownership=true is set. This defaults to true to remain backward-compatible, but it is recommended to set it to false and either use one GitTrack per namespace or use the new v1alpha2.ClusterGitTrack instead.

For Faros to be able to access the Git credentials, this also adds the secretNamespace property to the GitTrack resources. It is required when using a cluster-scoped v1alpha2.ClusterGitTrack, as Faros would otherwise look up the secret in the same namespace, but secrets in Kubernetes are always namespace-scoped.

Looking for your feedback on this approach.

pusher-ci commented 5 years ago

Hi @sebastianroesch. Thanks for your PR.

I'm waiting for a pusher member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
JoelSpeed commented 5 years ago

/ok-to-test

pusher-ci commented 5 years ago

@sebastianroesch: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-faros-test-1.13 3587162178c43433eeff99ff7c721c036a755bc3 link /test 1.13
pull-faros-test-1.11 3587162178c43433eeff99ff7c721c036a755bc3 link /test 1.11

Full PR test history

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
sebastianrosch commented 5 years ago

I'll first answer your questions to the current behavior of this change:

  • namespaced mode: Reconcile GitTracks and restrict resources to being in the same namespace, what's the behaviour for ClusterGitTracks here? I think it still reconciles them

If a --namespace is set, the behavior would be as GitTrack before. ClusterGitTracks would create resources in the specified namespace or cluster-scoped resources. GitTrack would only create resources in the specified namespace, unless cross-namespace ownerships is allowed (as per the new flag).

Regarding the proposed modes:

I am unsure if the modes you describe cover all cases. One other case I see is with the --namespace flag set, which could allow a Faros instance to manage cluster-scoped resources and resources in this namespace.

But thinking about it, I get the feeling that there's actually only two modes:

The namespaced mode you mentioned is the default and only behavior for GitTracks. It can always be used in combination with the other two.

In this case, we don't need the --namespace flag anymore, as this would be achieved by creating the appropriate GitTrack.

Accordingly, I agree that we would need a new flag that controls the mode of the ClusterGitTracks.

pusher-ci commented 5 years ago

@sebastianroesch: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
sebastianrosch commented 5 years ago

@JoelSpeed any thoughts on my previous comment?

JoelSpeed commented 5 years ago

@sebastianroesch As this is a fairly substantial change, I'd like to get some more detailed design done and make sure that we have an understanding of the desired behaviour before we go ahead on the work. I'm working on a document (that I will share with you later in the week) on what my understanding of the problem and the possible operational modes of Faros that we need to support are.

JoelSpeed commented 5 years ago

@sebastianroesch Could you please take a look at and add comments to this doc that I've come up with https://paper.dropbox.com/doc/Introducing-ClusterGitTracks--AixMM37BMMhcugDznYFKsuD8AQ-XslbpUtYZeUTkigSSV4SS

I think this outlines the new behaviour that we want and the different ways that I envisage people using Faros, let me know if you need any clarification on any of it

JoelSpeed commented 5 years ago

@sebastianroesch @wimdec Do either of you have any further comments or questions on the proposal document or are we all agreed on the design to go forward with?

sebastianrosch commented 5 years ago

@JoelSpeed I think it's a good proposal that we can go ahead with. I think it's rather complex compared to what I hoped for, but I understand that this might be necessary to support all those different use cases.

JoelSpeed commented 5 years ago

@sebastianroesch I appreciate it's reasonably complicated but I know there are different people using the project in different ways and this change is going to affect them all

I'd like to propose creating an issue for each of the bullet points at the bottom of the document and then people can claim the issues and can open a PR for each in turn and make this a more collaborative effort? I think the first couple of them can be cherry-picked from this PR's branch pretty much

JoelSpeed commented 5 years ago

So you're aware, I have created a project for trying to resolve this issue https://github.com/pusher/faros/projects/1

sebastianrosch commented 5 years ago

@JoelSpeed I'd be happy to help out with some of the tasks over the next days. Anything that can be easily broken out?

JoelSpeed commented 5 years ago

@sebastianroesch I'm going to try and get #151 #152 and #154 done today/tomorrow morning, once they are done everything else should be parallelisable (that said #154 could quite easily be done separately)