Read-only mode

[dimitropoulos]

closes #1139

I am opening this draft PR just to provide some early context for the direction I'm going with enabling a read-only mode for Flux. I have a lot more to say to fill out the discussion of this post, so I'll be updating this description as more findings arise. I am not only interested in talking about high-level architecture of this feature at this stage so please reserve your comments on anything else until later.

Use Cases

I know of a few but haven't gotten them distilled into text quite yet. If you have some in mind please comment below and I will add them here.

Storing Flux's application state elsewhere

The main challenge with this mode is that Flux uses git for storing Flux's application state. These fall down into two main pieces of data: (1) the "sync-tag" and (2) the usage of git notes.

Replacing the sync-tag

This one, at surface level, looks simple enough. The data held by the sync-tag boils down to a string holding the git reference for the commit that flux has currently synced to. This string can be stored in native kubernetes storage such as:

1. inside a kubernetes Secret as a new key/value pair
   • pro: we already have a secret resource at our disposal
   • con: this data is not secret - and storing it in a Secret (compared to storing it in a ConfigMap) feels like an impedance mismatch
   • pro: the codebase is already familiar with how to work with and use a Secret (I think this should be an extremely minor consideration, but a consideration nonetheless)
   • con: 1MB size limit
2. inside a kubernetes ConfigMap as a new key/value pair
   • con: 1MB size limit
   • meh: requires creation of new resource... but... honestly... due to the popularity of the helm chart it is unlikely this is actually much of an issue at all
   • con: the codebase needs to be taught how to interact with ConfigMaps (not a big deal, I think, but still a consideration)
3. as an annotation on another kubernetes resource, perhaps the flux controller itself or the secret
   • pro: could be a simple option for some very simple data.
   • pro: doesn't require the creation of new kubernetes resources
   • con: 1MB size limit
4. an in-memory Go struct
   • con: not being persistent if the pod shuts down. That's very undesirable - so much so that we should not consider this option further.
5. memcache
   • @squaremo has suggested that we should not scope creep the current role of memcache. Although it's tempting to use as a source-of-truth database for these values, we will not strongly consider this option unless some other factors arise.
6. use data in a CRD
   • I am waiting on @stefanprodan to give me some more information on this, but he tells me there's a something to do with CRD "slice status" that is a new-ish kubernetes feature that will eventually be the resting place for a feature like this.
   • pro: it is likely that in the future we will move many things to a CRD anyway, so might not be bad to get a jump start now.
7. mirrored repo with read-only access
   • This is perhaps simultaneously the most exotic choice on the list as well as the most simple. The idea here with this one would be to basically create a very thin layer on top of the readonly repo (a mirror) that flux does have write access to. All actions on the readonly repo would be mirrored onto the read/write repo and it is there on the read/write repo that flux would use the same state management strategies it uses today. The problem is, this option doesn't scale. It is unrealistic for a cloud provider, for example, to be interested in having to duplicate the entire git tree somewhere just to use flux. Flux should simply be able to watch a repo and snyc the changes (read-only).

Replacing the git notes

This one is a little trickier. All of the above listed possibilities for storing the sync-tag are potentially options here as well. We can store this in a kubernetes resource so long as it doesn't ever have potential to go over 1mb, the limit imposed by kubernetes versions that Flux currently supports. Currently I am unable to determine what the maximum size that the notes might reasonably reach because when I run git notes list on a repository that is using Flux on production, I don't get any results back. if someone can point me in the right direction on this one I would appreciate it.

So far as I can tell the notes are used for webhooks and APIs for returning information about the activity of the repository. It is not possible to release this readonly feature without retaining other primary features, in my opinion.

Currently there are errors associated with git tags that I am still working out in the codebase.

Notes

Yes, I did change some variable names and function signatures in the public API that would constitute a breaking change of some sort. Please ignore the implications any such changes until we get a working strategy for handling the architectural problems described above.

I have left some notes to self under READONLY-NOTE:-prepended comments in the codebase. These will all be removed before merging.

[dimitropoulos]

as part of an effort to understand the notes better (and, more generally how git refs are passed around) I elected to create an opaque type to be used anywhere git refs are passed around.

dimitropoulos@d3d6248

I'd say that (among other things like the sheer amount of change happening here being way too high) this is not something that should be a part of this project. That said, it is somewhat instructive to see how/where refs are used.

[rndstr]

when I run git notes list on a repository that is using Flux on production, I don't get any results back.

You need to fetch them first git fetch origin "refs/notes/*:refs/notes/*" and then select the proper ref (passed in to the agent as --git-notes-ref=flux-prod): git log --show-notes=flux-prod

[squaremo]

Replacing the git notes

I don't think you need to replace these -- they get written to accompany commits, and in read-only mode, fluxd shouldn't be making any commits.

[dimitropoulos]

@squaremo if I don't need to replace git notes... then what are they there for ultimately? Before you answer that please understand that (with this as some evidence) I've tried to get to the precise bottom of that question and haven't been able to fully understand that yet. It appears that, at least in part, they're there for some of the APIs?

All the same, I'll continue, then, on short-circuiting calls to notes when in read-only mode (or ConfigMap state mode, rather).

@stefanprodan the following seems to suggest to me that we're going to be hitting more than 1 megabyte in storage relatively easily:

dimitri@dimitri:~/cloud/src/service-conf$ git fetch origin "refs/notes/*:refs/notes*"
remote: Enumerating objects: 11, done.
remote: Counting objects: 100% (11/11), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 36422 (delta 7), reused 9 (delta 7), pack-reused 36411
Receiving objects: 100% (36422/36422), 6.00 MiB | 8.19 MiB/s, done.
Resolving deltas: 100% (26287/26287), completed with 7 local objects.
From github.com:weaveworks/service-conf
 * [new ref]             refs/notes/flux      -> refs/notesflux
 * [new ref]             refs/notes/flux-prod -> refs/notesflux-prod
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.

Given that.. I'll keep it shimmed out in an in-memory object until we figure out a next course of action.

[squaremo]

if I don't need to replace git notes... then what are they there for ultimately?

It's a read-only mode. You are by definition removing the ability to write to the repo, and not replacing it.

[dimitropoulos]

I think you're misunderstanding what I'm specifically saying. I understand that git notes need to be pushed to work. I'm talking about the underlying information stored in the notes. That information (today encapsulated in a git note but perhaps tomorrow encapsulated elsewhere in the context of a hypothetical read-only mode), I am assuming, is necessary for something to function (otherwise, why would it be done in the first place?). Whatever facility within flux it is that needs the information (today stored in a note) will presumably need that information in the future. That means that I will need to store that information elsewhere for it to be made available for consumption.

[squaremo]

I think you're misunderstanding what I'm specifically saying.

I'm not. I'll elaborate on what I said earlier: git notes are used to annotate commits with fluxd-specific metadata. If you're not making commits or push them, you won't make notes or push them.

You will still need to read notes, because they might be created by some other process (like another fluxd that has write access).

Can you give an example of where you find you need to put the data that goes into a note, somewhere else?

[dimitropoulos]

@squaremo as an update: everything seems to be working. The tests all pass and I have smoke-tested it with readonly mode both on and off. Please take a closer look at the code, now, and let me know what you think about the general direction.

There are two areas in particular I'd like you to look at (in addition to reading the rest):

1. the READONLY-NOTE in VerifySyncTag
2. the READONLY-NOTE in git/state.go above GetNativeSyncMarkerRevision. If there's a way to pass a Checkout to SyncStatus somehow, then that's likely the approach I'm looking for. What I have now works, but it's not an architectural dependency tree (in terms of flow of control) that I'm satisfied with to expose this function in this way.

Probably not a good time to really critique things line by line (I want to write a lot more tests next), but it seems to be straightforward enough to prove out the concept.

[squaremo]

Overall, the ergonomics of this for users will be pretty good.

Before moving out of draft/WIP state, I think these general points (with specifics in the comments) will need to be addressed:

• there's quite a few changes that are not related to what this PR sets out to do
• there's a few gratuitous changes, a small number harmful, most just unnecessary
• the abstraction for storing the sync high water mark is in the wrong place

[dimitropoulos]

@squaremo thank you for taking a look. I'll be going through all of these individually asap.

regarding some of the changes, when I said:

Probably not a good time to really critique things line by line

I was trying to save you the frustration of not having context for some of the changes. Some of them look like random renamings, but only because the context is missing. If there are 8 places in the code that say gitConfig and one that names the variable config, I changed it to match the others - but in a diff like this you can't see why I did that, you only see what looks like a semi-random and semi-useless renaming.

Same goes for :latest on the image tag or committing the private key to the secret. Those are just artifacts of changes I am making while developing that I wasn't trying to suggest should be part of the overall change set.

[dimitropoulos]

ok, I've gone through all the notes and resolved the small/easy ones and left comments on the others. To say it again here: yes, I am aware that there's an architectural problem that needs to be solved, to quote my first message on this topic from before you took a look:

the READONLY-NOTE in git/state.go above GetNativeSyncMarkerRevision. If there's a way to pass a Checkout to SyncStatus somehow, then that's likely the approach I'm looking for. What I have now works, but it's not an architectural dependency tree (in terms of flow of control) that I'm satisfied with to expose this function in this way.

I'll take a look next at extracting the "sync marker" logic to another package and see where we get from there, but it's not clear to me (yet) how that solves the issue of SyncStatus getting the information it needs (that is, the ref of the git tag in git tag state mode, and the hash of the sync marker point in native state mode).

[dimitropoulos]

I have tested the latest push in both Native sync mode and GitTag sync mode. I have one remaining issue that, I can solve, but I'd rather not do as a "workaround". I know I have some tests to work but I'm trying to focus first on finalizing the architecture, which I feel like I'm close to. The issue at hand has to do with this stanza:

	case RepoCloned:
		if !r.IsReadOnly() {
			ctx, cancel := context.WithTimeout(bg, r.timeout)
			err := checkPush(ctx, dir, url)
			cancel()
			if err != nil {
				r.setUnready(RepoCloned, err)
				return false
			}
		}

		r.setReady()
		// Treat every transition to ready as a refresh, so
		// that any listeners can respond in the same way.
		r.refreshed()
		return true

The issue is that this code is actually faulty in its reasoning. It's taken from a switch statement in Repo.step(). The thing that's wrong (took me a while of looking at it and thinking to realize) is that we don't just want to check if the repo is in readonly mode, we also want to check that the state mode is in GitTag mode (or, to say it another way that's also valid today but might not be valid later once other forms of syncing like S3 and so on are available, that the state mode isn't Native mode). The two things are separate since you can perfectly well be syncing with Native mode and still allowing write access because perhaps you want to keep flux's facilities for releases and such, but you don't want it moving a sync tag all over the place all the time.

The Repo type is implemented in the git package. This means that there will be a dependency on the sync package where the GitTagStateMode = "GitTag" and NativeStateSyncMode = "Native" consts currently live. Yes, I could move those out into their own types/entities package that doesn't have any dependencies, but that's not the only problem. Another problem is that the data itself of what sync mode we're currently using is not presently available to Repo, and, frankly, that's a good thing. We don't want to point dependency arrows from the git package up, if you know what I mean. The git package should, ideally, be a lower level construct. I could "dependency inject" my way through this problem and send the variable through via the NewRepo function and store it at that time... but that's still not a good choice.

This PR actually creates a perfect place for this functionality, the SyncProvider. This provider is intended to break the dependency chain and serve as an abstraction where (for example) a function like this wouldn't have to itself know what sync mode we're in. It would simply ask the provider "please verify for me, if you would kind sir, that we can successfully push a tag" and expect a yes or no response. The issue is that this "sync" activity is something that currently exists at the level of the daemon or the sync package, but ideally not at the level of the git package (which Repo is currently a member of). The more time I spend looking at the the more I am starting to think that the functionality currently implemented in Repo.step doesn't belong in the git package. Read the description currently written for that function, for example:

step attempts to advance the repo state machine, and returns true if it has made progress, false otherwise.

To me, that's sync functionality that goes beyond the git package as I currently understand the intended architectural boundaries of the git package. To put it another way, it seems like there are actually two reasons this case statement exists. It's actually asking two questions at once:

1. can we do what we need to do to keep track of the "high-water-mark" state (which today is the flux-sync git tag, but tomorrow will be either that tag or some other native kubernetes state)?
2. can we do what we need to do to push commits?

Previously these boiled down to the same question - "can I push a tag", but today they are separate questions. There seem to be other implications of being in a state where we want flux to know it can, indeed, push commits for releases and the like, but that it should not use git for storing the high water mark state.

I went ahead and implemented what I'm talking about with making Repo.step aware of the state mode just to show what I'm talking about with more clarity.

---

I'm still doing some testing, but aside from this problem we're certainly getting somewhere closer to what resembles a finish line. Once @squaremo you agree with the direction I'm going to turn my attention to refactoring the tests (and writing new ones).

[squaremo]

The issue is that this code is actually faulty in its reasoning.

I don't think it is. A repo is either read-only, or read/write. If you expect to be pushing tags*, make a read/write repo.

*In gitlab at least, there are grades of permission, such that it's possible to be able to push commits to the repo, but not to force-push a tag. If we want to account for that, then the way to do it would be to represent the grades of interest as requirements of the repo, replacing read-only|read/write.

[dimitropoulos]

I have reviewed every line of the diff many times now and successfully tested it and believe the implementation to be very near completion (if not already complete).

From a high level here are the next steps as I see them:

• @squaremo please approve the current implementation (or request any changes)
• I will then refactor all affected tests
• I will re-commit all changes (currently squashed into a wip commit) back into atomic and logical commits
• I will move this pull request out of draft state and it will be ready to merge

Please respond to each of the following in addition to above request:

1. The SyncMarkerAction struct in provider.go is precisely the same as the git.Commit struct. It was exactly this way before with git.TagAction, so I'm not sure if that's intentional but it seems like it might make more sense to be more forthright about what SyncMarkerAction really represents... a Commit. On the other hand, the two concepts (a sync marker action and a commit) could be reasonably considered different things despite having identical data so I can see it either way. please tell me whether this duplication was intentional (and why)
2. Prior to now the "native" storage has been mocked with an in-memory object. As I understand it @squaremo you want me to put the data for the "Native-mode" sync marker as an annotation on the secret. please verify that this is what you want.
3. NativeSyncProvider.DeleteMarker doesn't actually remove the annotation itself. I struggled to find a way to accomplish it otherwise. I read every example, the docs, github issues of other people struggling with the same thing, the source code for the patch strategies, all usages of the k8sObject.GetAnnotations method to see if somewhere else in the app deletes an annotation key in this way, the filterObject function in save_cmd.go which deletes the annotations entirely (which is not what I'm attemption to do) and none got me my answer. I don't want to grab the whole annotations as a struct, delete that key, and then post it back again because (in my mind) that sounds like something that would require a lock of some kind. please make any suggestions you are aware of on how to delete just the key. Strictly speaking... it's probably fine if it never really gets deleted but I'd like to be as cleanly as possible (if possible).
4. Please take note that you will see the reasonably-kludgy map[string]map[string]map[string]string because I am following a nearly identical usage already in place (same context but one fewer level deep) in sshkeyring.go. I would have done map[string]interface{} or something if not for trying to stick to the standards already in place. This same reason goes for using the go client direction as the sshkeyring code does rather than some similar facilities available in the kubernetes flux package. I'm telling you more so you don't say: "what was he thinking" and you are aware I'm just trying to introduce as little personality as humanly possible.
5. I followed your advice to work around the issue of the sync provider having access to the right working directory by passing a pointer to a Repo to the sync provider (just as Daemon requires). That does work, but it means calling Repo.Dir() any time the provider is interacted with. The other option seems to be to give Repo the facilities to do the "heavy lifting" of creating, reading, updating, deleting, and verifying the GPG signature of the tag. To me, this seems like a leaky abstraction if it depends on Repo in that way when, after all, nothing else in repo.go has anything to do with the git tag (nor should it since that information is now abstracted away through the sync provider). Still, if you would like me to move that logic to Repo, I am happy to do so. please just let me know what you decide. Perhaps I'm making something out of nothing since all Repo.Dir() does after all is extremely simple and perhaps not worth worrying about calling on every SyncProvider interaction if it keeps the code better organized:

   func (r *Repo) Dir() string {
   	r.mu.RLock()
   	defer r.mu.RUnlock()
   	return r.dir
   }

[dimitropoulos]

one of @2opremio / @hiddeco / @stefanprodan do you think you could take a look at the above ? I would appreciate that greatly if so. You don't have to go full-bore on it line-by-line (unless you want to, which would be great in my book), but an overall "looks good but maybe change x to be like y" might save some time when @squaremo has time to get to it. I want to keep it moving if at all possible.

[hiddeco]

#1791 is going to give you conflicts once it has been merged.

I have taken a look at this PR and compared it to changes I made in the mentioned PR and these are the things that stand out to me:

1. I factored out the whole doLoop() from loop.go into a daemon/sync.go. You will probably need to re-add changes made to this loop into sync.go, and reincorporate the SyncProvider.
2. The git verification feature sports a new ratchet mechanism to control the revision it applies (daemon/loop.go). This mechanism calls daemon.latestValidRevision() which also validates the tag in git, this should be disabled when in read-only modus (I think SyncProvider could take care of this by no-oping out on VerifyMarker or something).
3. Any other verifyTag action that I missed should be taken account for.

[squaremo]

• The SyncMarkerAction struct in provider.go is precisely the same as the git.Commit struct. It was exactly this way before with git.TagAction, so I'm not sure if that's intentional but it seems like it might make more sense to be more forthright about what SyncMarkerAction really represents... a Commit. On the other hand, the two concepts (a sync marker action and a commit) could be reasonably considered different things despite having identical data so I can see it either way. please tell me whether this duplication was intentional (and why)

I don't see why the SyncMarkerAction exists, to be quite honest. All the SyncProvider cares about is moving the high water mark. It doesn't need a message or a signing key to do that. Those things are specific to moving a tag, which is a git operation, not a SyncProvider operation.

• Prior to now the "native" storage has been mocked with an in-memory object. As I understand it @squaremo you want me to put the data for the "Native-mode" sync marker as an annotation on the secret. please verify that this is what you want.

I think that's a reasonable design, yes. The secret is mandatory, at least at present; we know its name because it's used for the keyring; and, it's related to the git repo.

• NativeSyncProvider.DeleteMarker doesn't actually remove the annotation itself. I struggled to find a way to accomplish it otherwise. I read every example, the docs, github issues of other people struggling with the same thing, the source code for the patch strategies, all usages of the k8sObject.GetAnnotations method to see if somewhere else in the app deletes an annotation key in this way, the filterObject function in save_cmd.go which deletes the annotations entirely (which is not what I'm attemption to do) and none got me my answer. I don't want to grab the whole annotations as a struct, delete that key, and then post it back again because (in my mind) that sounds like something that would require a lock of some kind. please make any suggestions you are aware of on how to delete just the key. Strictly speaking... it's probably fine if it never really gets deleted but I'd like to be as cleanly as possible (if possible).

The Kubernetes API does have optimistic locking, if you're concerned about that. But setting it to an empty string, and treating an empty string as equivalent to a missing value, is fine I would think.

• Please take note that you will see the reasonably-kludgy map[string]map[string]map[string]string because I am following a nearly identical usage already in place (same context but one fewer level deep) in sshkeyring.go. I would have done map[string]interface{} or something if not for trying to stick to the standards already in place. This same reason goes for using the go client direction as the sshkeyring code does rather than some similar facilities available in the kubernetes flux package. I'm telling you more so you don't say: "what was he thinking" and you are aware I'm just trying to introduce as little personality as humanly possible.

OK.

• I followed your advice to work around the issue of the sync provider having access to the right working directory by passing a pointer to a Repo to the sync provider (just as Daemon requires). That does work, but it means calling Repo.Dir() any time the provider is interacted with. The other option seems to be to give Repo the facilities to do the "heavy lifting" of creating, reading, updating, deleting, and verifying the GPG signature of the tag. To me, this seems like a leaky abstraction if it depends on Repo in that way when, after all, nothing else in repo.go has anything to do with the git tag (nor should it since that information is now abstracted away through the sync provider). Still, if you would like me to move that logic to Repo, I am happy to do so. please just let me know what you decide. Perhaps I'm making something out of nothing since all Repo.Dir() does after all is extremely simple and perhaps not worth worrying about calling on every SyncProvider interaction if it keeps the code better organized:

  func (r *Repo) Dir() string {
  	r.mu.RLock()
  	defer r.mu.RUnlock()
  	return r.dir
  }

It'd be fine if the Repo, or otherwise the git package, had methods for dealing with tags. That would mean you can avoid putting the SyncProvider implementation in git and thereby introducing a dependency there on sync.

[dimitropoulos]

to be clear, I'm waiting on the first step of the above to be completed:

From a high level here are the next steps as I see them:

1. @squaremo please approve the current implementation (or request any changes)

[squaremo]

@squaremo please approve the current implementation (or request any changes)

My guidance remains as above:

• I don't think you need SyncMarkerAction since it contains things that aren't pertinent to sync marker actions (only to a particular implementation)
• move the SyncProvider out of the git package so that the latter doesn't require a dependency on the sync package

In addition, take Hidde's notes into consideration (you may want to look at the ratchet mechanism in #1791), since it's likely you'll need to rebase on that at some point.

[luxas]

Any progress on this? Would be exciting to use :)

[stefanprodan]

LGTM

Tested with stefanprodan/flux:readonlygit-fbc3cc98 and a readonly deploy key.
Works great! Thanks Michael 🥇

[hiddeco]

Nitpick:

Suggested change

	ReadOnlyROMode ReadOnlyReason = "ReadonlyMode"
	ReadOnlyROMode ReadOnlyReason = "ReadOnlyMode"

[dimitropoulos]

I went back and forth on this many times. There are examples of both in the wild. Both are used in the Go codebase about evenly. Both variants will feel weird to different people at for different reasons (if not simultaneously).

[hiddeco]

👍 for naming