Disable Image Scanning with a flag [take 2] #2745

2opremio · 2020-01-15T10:40:47Z

Overrides #2524

I made sure that the memcached client is not initialized when scanning is disabled. Also, I added proper errors when querying the registry if scanning is disabled.

hiddeco

Can you add the newly added flag to the documentation? Probably with a highlight about the fact that this also disables the fluxctl automation features.

hiddeco · 2020-01-15T10:56:20Z

pkg/update/release_image.go

@@ -196,6 +196,9 @@ func (s ReleaseImageSpec) calculateImageUpdates(rc ReleaseContext, candidates []
 		ref, err = s.ImageSpec.AsRef()
 		if err == nil {
 			singleRepo = ref.CanonicalName()
+			// FIXME(fons): we probably want to allow this operation even if image
+			//              scanning is disabled. We could either avoid the validation
+			//              or use an uncached registry.


The uncached registry is not an option, Michael shared insights about this in the superseded PR (Flux had this in the beginning but it lead to timeouts).

That code only validates that the images exists, I think we can use an uncached registry for that. Otherwise releasing a specific image won't work.

stefanprodan · 2020-01-15T11:05:41Z

If read-only is enabled then I think it should also disable image scanning as for a read-only Flux it doesn't make sense to maintain a a cache of images or depend on memcached. Furthermore I would make the read-only flag in the fluxctl install command omit the memcached deployment from the manifests output. The same applies for the Helm chart, when git.readonly=true memcached deployment should be omitted and Flux internally would set disable-image-scan=true.

2opremio · 2020-01-15T11:17:26Z

@stefanprodan note that listing images doesn't work when disabling image scanning but it works in read-only. There may be other things on top of that that I haven't thought of.

2opremio · 2020-01-15T13:31:57Z

I changed --disable-image-scan to --registry-scanning. Also, --registry-scanning=false implies --read-only=false

docs/references/daemon.md

Co-Authored-By: Stefan Prodan <[email protected]>

stefanprodan

LGTM

…ifests

2opremio · 2020-01-15T14:00:49Z

PTAL, I added --registry-scanning to fluxctl install and made sure that it doesn't output the memcached manifests when using --registry-scanning=false or --git-readonly=true

pkg/install/templates/flux-deployment.yaml.tmpl

Co-Authored-By: Stefan Prodan <[email protected]>

squaremo · 2020-01-20T14:14:54Z

If read-only is enabled then I think it should also disable image scanning as for a read-only Flux it doesn't make sense to maintain a a cache of images or depend on memcached

Those two things are related, but not coupled. You may very well want to query the image metadata while not wanting to write to the git repo.

Also, --registry-scanning=false implies --read-only=false

What, turning registry scanning off forces it to treat the repo as writable? That doesn't make any sense.

2opremio · 2020-01-20T21:54:05Z

Also, --registry-scanning=false implies --read-only=false

What, turning registry scanning off forces it to treat the repo as writable? That doesn't make any sense.

I meant --read-only=true implies --registry-scanning=false (that's what the code does)

Those two things are related, but not coupled. You may very well want to query the image metadata while not wanting to write to the git repo.

I was on the fence about this one, @stefanprodan convinced me about not making much sense to have the scanning just for list-images. Maybe we are wrong ....

2opremio · 2020-01-20T21:55:48Z

We can still revert that, nothing was released yet.

squaremo · 2020-01-28T10:17:59Z

cmd/fluxctl/install_cmd.go

@@ -33,21 +33,23 @@ fluxctl install --git-url '[email protected]:<your username>/flux-get-started' | ku
 	cmd.Flags().StringVar(&opts.GitBranch, "git-branch", "master",
 		"Git branch to be used by Flux")
 	cmd.Flags().StringSliceVar(&opts.GitPaths, "git-paths", []string{},
-		"Relative paths within the Git repo for Flux to locate Kubernetes manifests")
+		"relative paths within the Git repo for Flux to locate Kubernetes manifests")


How come these all got lowercased, en passant .. looking at the other subcommands, the usage messages tend to start with capitals

Ops, I did it after fluxd's flags (wrongly assuming that the rest of the fluxctl commands followed the as pattern)

I am happy to revert that, but I think fluxd and fluxctl should be consistent

I noticed it because it caused a merge conflict (easily solved, I just need to know which direction to solve it in ..)

Good point on being consistent with fluxd; I'll resolve the conflict with the least change, then do them all at once in another PR. (i.e., don't worry about reverting!)

Great, thanks!

texasbobs · 2020-01-31T18:36:31Z

When you mention --read-only=false are you referring to--git-readonly or is this a new parm?

We would have --git-readonly=false but would not want any access attempts to memcached.

2opremio · 2020-01-31T18:44:42Z

We would have --git-readonly-false but would not want any access attempts to memcached.

That's what the flag added in this PR does.

When you mention --read-only=false are you referring to--git-readonly or is this a new parm?

We were referring to `--git-readonly|

texasbobs · 2020-01-31T18:54:09Z

We would have --git-readonly-false but would not want any access attempts to memcached.

That's what the flag added in this PR does.

Ok. Thanks. I guess I wasn't reading it that way. We would like the following:

--registry-scanning=false and --git-readonly=false

But the comments seem to me to indicate if --registry-scanning=false then --git-readonly=true should be used.

registryScanning     = fs.Bool("registry-scanning", true, "scan container image registries to fill in the registry cache; --registry-scanning=false implies --read-only=true")

`

2opremio · 2020-01-31T20:45:39Z

That's really interesting. There is discussion about that at #2790

Would you mind explaining there why would you need --registry-scanning=false and --git-readonly=false?

--registry-scanning=false disables a lot of functionality also disabled implicitly by --git-readonly=true

texasbobs · 2020-02-01T15:39:06Z

That's really interesting. There is discussion about that at #2790

Would you mind explaining there why would you need --registry-scanning=false and --git-readonly=false?

--registry-scanning=false disables a lot of functionality also disabled implicitly by --git-readonly=true

Perhaps we do not understand the functions but here is what we are thinking. We want to use --git-readonly=false so that Flux can keep track of what has changed in the repo and what it has already applied. We want to use --registry-scanning=false because we are not using the feature pertaining to image versions.

So perhaps we don't understand what the --git-readonly is for. Is the only time Flux writes back to the git repo when it needs to update an image tag?

Based on the following, we thought that Flux needed to write back to the repo to keep track of what it had applied.

  # Set to `true` if you intend for Flux to not be able to push changes to git.
  # Also configure state.mode to `secret` since storing state in a git tag will no longer be possible.
  readonly: false
  # use `.sync.state: secret` to store flux's state as an annotation on the secret (instead of a git tag)
  state: git

2opremio · 2020-02-03T13:52:15Z

So perhaps we don't understand what the --git-readonly is for. Is the only time Flux writes back to the git repo when it needs to update an image tag?

--git-readonly prevents Flux from writing back to the Git repository. This happens when updating images, when updating policies (annotation metadata) and when pushing a tag (specified through --git-label).

We want to use --git-readonly=false so that Flux can keep track of what has changed in the repo and what it has already applied.

So, what you mean is that you what --git-readonly=false in order to keep letting Flux use an git tag to keep track of what it synced?

texasbobs · 2020-02-03T14:11:12Z

So, what you mean is that you what --git-readonly=false in order to keep letting Flux use an git tag to keep track of what it synced?

That is correct. From what I can tell in the doc, that is the purpose of the flag. I see that it would also control the updates for new images, but it seems you could have readonly=false and not apply new images.

Flux keeps track of the last commit that it’s applied to the cluster, by pushing a tag (controlled by the command-line flags --git-sync-tag and --git-label) to the git repository. This gives it a persistent high water mark, so even if it is restarted from scratch, it will be able to tell where it got to.

texasbobs · 2020-02-03T14:14:53Z

Looking at #2790, I would think that git-readonly=true implies registry-scanning=false is right. Why scan registry entries if you can't update the repo. However
registry-scanning=false does not imply git-readonly=true because you still might want to use the sync tag option.

squaremo · 2020-02-03T14:18:38Z

Why scan registry entries if you can't update the repo.

Because there are API methods for querying workloads and the images they use, and those depend on having scanned registry entries, and have nothing to do with whether you can write to git or not.

texasbobs · 2020-02-03T14:27:53Z

Why scan registry entries if you can't update the repo.

Because there are API methods for querying workloads and the images they use, and those depend on having scanned registry entries, and have nothing to do with whether you can write to git or not.

So in that case, the two parms are totally unrelated.

2opremio · 2020-02-03T16:19:59Z

I would think that git-readonly=true implies registry-scanning=false is right. Why scan registry entries if you can't update the repo. However
registry-scanning=false does not imply git-readonly=true because you still might want to use the sync tag option.

@texasbobs note that (regardless of what the help messages say, they are wrong right now) as it is in master right now --git-readonly=true implies --registry-scanning=false and not the other way around. So, you could still have --registry-scanning=false and --git-readonly=false and your usecase would be covered.

2opremio requested review from stefanprodan and hiddeco January 15, 2020 10:40

2opremio mentioned this pull request Jan 15, 2020

Disable Image Scanning with a flag #2524

Closed

2opremio force-pushed the disable-image-scan branch from 1e47ab0 to 957132c Compare January 15, 2020 10:43

hiddeco reviewed Jan 15, 2020

View reviewed changes

stefanprodan reviewed Jan 15, 2020

View reviewed changes

docs/references/daemon.md Outdated Show resolved Hide resolved

John Sushant Sundharam and others added 3 commits January 15, 2020 14:33

Disable Image Scanning with a flag

66d55f5

Disable memcached together scanning and add proper errors

7fc401a

Move to --registry-scanning flag and imply --read-only=false when false

3a77351

2opremio force-pushed the disable-image-scan branch from 269dd8b to 3a77351 Compare January 15, 2020 13:33

Update docs/references/daemon.md

6a186bf

Co-Authored-By: Stefan Prodan <[email protected]>

stefanprodan approved these changes Jan 15, 2020

View reviewed changes

Remove unused ImageScanEnabled field

b8b54a5

stefanprodan mentioned this pull request Jan 15, 2020

chart: Disable memcached when Flux is in readonly mode #2746

Merged

hiddeco approved these changes Jan 15, 2020

View reviewed changes

Add --registry-scanning to fluxctl install and remove memcached man…

844c760

…ifests

stefanprodan suggested changes Jan 15, 2020

View reviewed changes

pkg/install/templates/flux-deployment.yaml.tmpl Outdated Show resolved Hide resolved

Update pkg/install/templates/flux-deployment.yaml.tmpl

1698b7c

Co-Authored-By: Stefan Prodan <[email protected]>

stefanprodan approved these changes Jan 15, 2020

View reviewed changes

2opremio added 2 commits January 15, 2020 16:39

Update generated install template files

110fdbd

Fix tests

c7af8e0

2opremio added the enhancement label Jan 15, 2020

2opremio added this to the 1.18.0 milestone Jan 15, 2020

2opremio merged commit 992f6b3 into fluxcd:master Jan 15, 2020

2opremio deleted the disable-image-scan branch January 15, 2020 16:36

2opremio mentioned this pull request Jan 16, 2020

Turn off the docker scan option completely. #2411

Closed

2opremio mentioned this pull request Jan 16, 2020

Update FAQ with --registry-scanning #2753

Merged

squaremo reviewed Jan 28, 2020

View reviewed changes

2opremio mentioned this pull request Feb 3, 2020

Reformulate --registry-scanning #2790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable Image Scanning with a flag [take 2] #2745

Disable Image Scanning with a flag [take 2] #2745

2opremio commented Jan 15, 2020

hiddeco left a comment

hiddeco Jan 15, 2020 •

edited

Loading

2opremio Jan 15, 2020 •

edited

Loading

stefanprodan commented Jan 15, 2020 •

edited

Loading

2opremio commented Jan 15, 2020 •

edited

Loading

2opremio commented Jan 15, 2020

stefanprodan left a comment

2opremio commented Jan 15, 2020

squaremo commented Jan 20, 2020

2opremio commented Jan 20, 2020

2opremio commented Jan 20, 2020

squaremo Jan 28, 2020

2opremio Jan 28, 2020

2opremio Jan 28, 2020

squaremo Jan 28, 2020

2opremio Jan 28, 2020

texasbobs commented Jan 31, 2020 •

edited

Loading

2opremio commented Jan 31, 2020

texasbobs commented Jan 31, 2020

2opremio commented Jan 31, 2020 •

edited

Loading

texasbobs commented Feb 1, 2020 •

edited by 2opremio

Loading

2opremio commented Feb 3, 2020

texasbobs commented Feb 3, 2020

texasbobs commented Feb 3, 2020

squaremo commented Feb 3, 2020

texasbobs commented Feb 3, 2020

2opremio commented Feb 3, 2020

Disable Image Scanning with a flag [take 2] #2745

Disable Image Scanning with a flag [take 2] #2745

Conversation

2opremio commented Jan 15, 2020

hiddeco left a comment

Choose a reason for hiding this comment

hiddeco Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

2opremio Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

stefanprodan commented Jan 15, 2020 • edited Loading

2opremio commented Jan 15, 2020 • edited Loading

2opremio commented Jan 15, 2020

stefanprodan left a comment

Choose a reason for hiding this comment

2opremio commented Jan 15, 2020

squaremo commented Jan 20, 2020

2opremio commented Jan 20, 2020

2opremio commented Jan 20, 2020

squaremo Jan 28, 2020

Choose a reason for hiding this comment

2opremio Jan 28, 2020

Choose a reason for hiding this comment

2opremio Jan 28, 2020

Choose a reason for hiding this comment

squaremo Jan 28, 2020

Choose a reason for hiding this comment

2opremio Jan 28, 2020

Choose a reason for hiding this comment

texasbobs commented Jan 31, 2020 • edited Loading

2opremio commented Jan 31, 2020

texasbobs commented Jan 31, 2020

2opremio commented Jan 31, 2020 • edited Loading

texasbobs commented Feb 1, 2020 • edited by 2opremio Loading

2opremio commented Feb 3, 2020

texasbobs commented Feb 3, 2020

texasbobs commented Feb 3, 2020

squaremo commented Feb 3, 2020

texasbobs commented Feb 3, 2020

2opremio commented Feb 3, 2020

hiddeco Jan 15, 2020 •

edited

Loading

2opremio Jan 15, 2020 •

edited

Loading

stefanprodan commented Jan 15, 2020 •

edited

Loading

2opremio commented Jan 15, 2020 •

edited

Loading

texasbobs commented Jan 31, 2020 •

edited

Loading

2opremio commented Jan 31, 2020 •

edited

Loading

texasbobs commented Feb 1, 2020 •

edited by 2opremio

Loading