-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@sebastianroesch: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think at the moment the behaviour options are:
- 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
- non-namespaced mode: Controller will reconcile GitTracks and ClusterGitTracks, ClusterGitTracks can own resources in both namespace and cluster scope, GitTracks can only own namespaced scope
I think what we want by the end of this PR is the following three modes:
- Cluster wide mode: Only reconcile ClusterGitTracks, which manage namespaced and non-namespaced resources
- Namespaced mode: Reconcile GitTracks which can only manage Namespaced resources
- ClusterScope mode: Reconcile ClusterGitTracks which can only manage Cluster scoped resources
You should use one ClusterScope controller in every cluster and a Namespaced controller in every namespace when you want to have the separation. Or you just use one Cluster wide controller which manages everything, but in this mode, what should the behaviour for GitTracks be? Does it manage them still? I guess it probably could without issue as long as they don't cross namespace boundaries or manage cluster scoped resources
This is a fairly complex change so I think it would benefit from having a proper design document written up, I'll see if I can get something written next week, I'm off on holiday for a few days now.
In the meantime post questions about what I've said here and we can answer them and copy the contents into the doc
Also, you have checked in the kustomize
binary, can we remove that and add it to the gitignore maybe?
A namespaced GitTrack, however, should never own cluster-scoped resources or | ||
resources in other namespaces. | ||
|
||
To maintain backward-compatibility, the following flag defaults to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we know this can destroy clusters, I'm very tempted to not maintain this backwards compatibility.
We are on an alpha version and I think, given the severity of this change, we will skip the 0.4.0
release and go straight to 0.5.0
. People will expect breaking changes and I think there are better ways to handle this.
I discussed with @mthssdrbrg yesterday about having the GitTrackObject
controller look for OwnerReferences pointing to the wrong namespace and "orphaning" the GitTracks. This could then be exposed as a metric which we can note in the release people should look for and make sure it goes to zero, what do you think to that approach instead? Easy migration vs backwards compatiblity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it would be better to break the current behavior in this case.
I like the idea of exposing the metric and breaking the owner reference.
// +kubebuilder:printcolumn:name="Resources Ignored",type="integer",JSONPath=".status.objectsIgnored" | ||
// +kubebuilder:printcolumn:name="Children In Sync",type="integer",JSONPath=".status.objectsInSync" | ||
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" | ||
type ClusterGitTrack struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be a separate API group. Adding a new type doesn't break backwards compatibility and the new API group really complicates this PR. I would prefer if we just moved this into v1alpha1
, as it is the first version of this resource, it shouldn't be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, makes it a lot easier.
|
||
// GetNamespacedName implementes the GitTrack interface | ||
func (g *ClusterGitTrack) GetNamespacedName() string { | ||
return fmt.Sprintf("%s/%s", g.Namespace, g.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespaced name for a Cluster scope resource should just be g.Name
return fmt.Sprintf("%s/%s", g.Namespace, g.Name) | |
return g.Name |
@@ -103,6 +104,11 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { | |||
return err | |||
} | |||
|
|||
err = c.Watch(&source.Kind{Type: &farosv1alpha2.ClusterGitTrack{}}, &handler.EnqueueRequestForObject{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the controller is running in namespaced mode, we don't want to be managing ClusterGitTracks
right? Likewise, we will need another flag to suggest managing only ClusterGitTrack
, any ideas for how we can solve this @sebastianroesch @mthssdrbrg ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, the different cases have only been checked in the function that ignores the resources if their namespace doesn't match, the controllers would still watch all resources.
According to my comment about the namespaced mode, we would have to watch all resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to be able to switch off the watching of everything as in certain modes it shouldn't be necessary and, with fewer watches, would allow people to reduce their RBAC scope too.
According to my comment about the namespaced mode, we would have to watch all resources.
My thought here was that you will likely have multiple "namespaced" instances of the controller (one per namespace) and then a separate "cluster" instance running to manage the cluster scoped resources. So the individual namespaced controllers should be able to completely ignore ClusterGitTracks
and ClusterGitTrackObjects
in theory unless they are running both the Namespaced
and ClusterScoped
modes within the same controller instance
if namespaced && farosflags.Namespace != "" && farosflags.Namespace != u.GetNamespace() { | ||
r.log.V(1).Info("Object not in namespace", "object namespace", u.GetNamespace(), "managed namespace", farosflags.Namespace) | ||
return true, fmt.Sprintf("namespace `%s` is not managed by this Faros", u.GetNamespace()), nil | ||
if namespaced { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a check to make sure ClusterGitTracks don't manage namespaced objects when running in cluster only mode? (I realise this is yet to be defined)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, if we decide that we want to add this mode as per your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have this mode so there can be definite separation between who is owning/who should be owning what
GitTrack
for namespaced stuff
ClusterGitTrack
for cluster scoped (in cluster scoped mode)
ClusterGitTrack
for everything if not using GitTracks
@@ -63,20 +65,34 @@ func (p OwnerInNamespacePredicate) Generic(e event.GenericEvent) bool { | |||
// When it is restricted to a namespace this should only be the GitTracks | |||
// in the namespace the controller is managing. | |||
func (p OwnerInNamespacePredicate) ownerInNamespace(ownerRefs []metav1.OwnerReference) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this makes sense for ClusterGitTrack
s? This is used to check if a GTO or CGTO has an owner managed by this controller right? If the controller is running in cluster wide or cluster scope only mode, then all it needs check is that the OwnerRef points to a ClusterGitTrack
, I'm not sure this predicate is required in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't entirely sure about what's going on here, but I think I had to make this change to receive the events of the GTO
s owned by the ClusterGitTrack
.
waitForInstanceCreated(key) | ||
}) | ||
|
||
It("does not create ClusterGitTrackObject", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test here needs updating, you've updated the description but the test case does not make sense with the current description
I'll first answer your questions to the current behavior of this change:
If a Regarding the proposed modes: I am unsure if the modes you describe cover all cases. One other case I see is with the 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 In this case, we don't need the Accordingly, I agree that we would need a new flag that controls the mode of the |
@sebastianroesch: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@JoelSpeed any thoughts on my previous comment? |
@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. |
@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 |
@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? |
@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. |
@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 |
So you're aware, I have created a project for trying to resolve this issue https://github.com/pusher/faros/projects/1 |
@JoelSpeed I'd be happy to help out with some of the tasks over the next days. Anything that can be easily broken out? |
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 theallow-cross-namespace-ownership=true
is set. This defaults totrue
to remain backward-compatible, but it is recommended to set it tofalse
and either use oneGitTrack
per namespace or use the newv1alpha2.ClusterGitTrack
instead.For Faros to be able to access the Git credentials, this also adds the
secretNamespace
property to theGitTrack
resources. It is required when using a cluster-scopedv1alpha2.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.