Check podID and containerID function arguments #163

jodh-intel · 2017-03-21T16:50:00Z

This branch switches the logic around so that rather than passing blank podID and containerID's around and then magically expanding them at quite a low level, the podID and containerID are set as soon as possible by virtc. This allows all functions to validate the parameters are not blank and return an error if they are [1].

The logic is complicated by the fact that in certain contexts -- for example when creating particular podResource's -- containerID may be blank. To handle this, I've created a new function called resourceNeedsContainerID().

Note that this is a proof-of-concept branch - the tests have not been updated so will fail.

[1] - This allows "fail-fast" behaviour which is less confusing when attempting to debug issues.

sboeuf · 2017-03-21T20:30:50Z

errors.go

+)
+
+// common error objects used for argument checking
+var (


Those ones could be const instead, right ?

That would be good, but you can't create const errors afaik.

Ok too bad...

sboeuf · 2017-03-21T20:41:43Z

hack/virtc/main.go

@@ -227,7 +228,14 @@ func buildPodConfig(context *cli.Context) (vc.PodConfig, error) {
 		Memory: vmMemory,
 	}

+	id := context.String("pod-id")


I am fine with this change, but could you create the corresponding flag ? Indeed, there is no pod-id flag for the create command. And could you call it "id" instead of "pod-id" ? It is not needed to use pod-id for the naming because we are in the command pod ... thus it is pretty obvious.

Added. This raises the question though of what is considered a valid podID and containerID. Since these end up being encoded in pathnames, we should take care here.

Currently containerID (via --id=) is free-form, whereas podID default to a uuid (although this branch would potentially allow that to change).

@sameo - Could you clarify what the approach should be here by answering a couple of questions:

should podID be restricted to being a uuid? I'm guessing yes. If so, maybe we shouldn't allow a user to specify a uuid value manually?

should containerID be restricted to being a uuid? I'm guessing no, but how about we restrict the name to only be able to contain characters from the set [a-ZA-Z0-9-_.]? (Do we want unicode support?)

@jodh-intel podID should be a UUID, yes. I agree they should just be assigned by virtcontainers and not exported.

I think containerID should also be a UUID, not to be confused with a container name (That we don't have atm).

So overall we should stop exporting the ID field from both ContainerConfig and PodConfig and only generate valid UUIDs for those.
And we should add a Name exported field to both ContainerConfig and PodConfig that ican be defined by the caller or we'd generate a readable one. Here only [a-ZA-Z0-9-_.] would be acceptable. This can be done as a separate PR.

@sameo according to your comment, the generation of the UUID in virtc done in this PR is not valid, right ?

@sameo - In terms of the command-line API then, are you saying that we should not have a --id= for pod create or container create? In other words, we should have the following behaviour:

virtc pod create
(generates pod uuid automatically).

virtc container create --pod-id=$pod_id
(generates container uuid automatically).

Yes, but he also means that virtcontainers should be responsible for this, and we could stop to export ID in PodConfig.
type PodConfig struct {
id
....
}

virtc pod create
(generates pod uuid automatically).

Correct.

virtc container create --pod-id=$pod_id
(generates container uuid automatically).

Correct.

And obviously the UUID generation should happen in virtcontainers.

I've raised #171 to make these ID fields private.

sboeuf · 2017-03-21T21:08:36Z

filesystem.go

+		// pod-specific resources
+		return false
+	default:
+		if podSpecific == true {


You could simplify this with "return !podSpecific"

sboeuf · 2017-03-21T21:11:30Z

pod.go

@@ -280,19 +287,26 @@ type PodConfig struct {

 // valid checks that the pod configuration is valid.
 func (podConfig *PodConfig) valid() bool {
-	if _, err := newHypervisor(podConfig.HypervisorType); err != nil {
-		podConfig.HypervisorType = QemuHypervisor
+	if podConfig == nil {


Can it happen ? I don't think so because you need podConfig to be valid (not nil) to be able to call this function, right ?

Oops - how did that get in there? :)

sboeuf · 2017-03-21T21:16:14Z

@jodh-intel I have few comments, let me know what you think about. And as a global comment, could you please add some new lines after all the new closing brackets corresponding to new if statements ?

jodh-intel · 2017-03-22T10:34:11Z

@sboeuf - volontiers! I thought the crunched up approach was "the golang way" - I much prefer letting the code breathe! 😄

jodh-intel · 2017-03-22T17:58:33Z

Added a new function commonResourceChecks() to reduce the cyclomatic complexity of storeResource() and thus hopefully appease the gods of Travis.

sboeuf · 2017-03-22T18:20:13Z

@jodh-intel Yeah but you have to fix unit tests now ;)

jodh-intel · 2017-03-23T09:06:12Z

@sboeuf - Yep - on it ;)

wcwxyz · 2017-04-05T09:37:23Z

errors.go

 	ErrNeedPodID       = errors.New("Pod ID cannot be empty")
 	ErrNeedContainerID = errors.New("Container ID cannot be empty")
+	ErrNeedFile        = errors.New("file cannot be empty")


Maybe "file" should be "File"?

actually, it seems usual to have errors starting with a lower case character in golang, so the other ones should be changed if any (a separate commit though). For reference I grepped through github.com/golang/go:

$ git grep -p 'errors.New.*"[A-Z]' src/ | wc -l 96 $ git grep -p 'errors.New.*"[a-z]' src/ | wc -l 1364

Hi @wcwxyz - thanks for reminding me about this. I had planned for these errors to refer to the name of the object originally. file didn't fit because that error is used for a file path. But looking at this list again, it's all rather inconsistent since it should be containerID and podID. I think as you say, just capitalising file would be the simplest fix ;)

Thanks for looking into it. I figure it's a typo.

@dlespiau - interesting, but looking at the virtcontainers code:

$ git grep -p 'fmt.Errorf.*"[A-Z]' | wc -l 220 $ git grep -p 'fmt.Errorf.*"[a-z]' | wc -l 40

Maybe an opportunity here to write a new linter for Error's to plug into https://github.com/alecthomas/gometalinter? 😄

Return error if argument checks fail. Signed-off-by: James O. D. Hunt <[email protected]>

Added additional variables for standard error conditions. Signed-off-by: James O. D. Hunt <[email protected]>

Previously, blank containerID was handled by ContainerConfig.valid() and a blank podID was handled by PodConfig.valid(). However, this meant function arguments could not be checked reliably (since some functions accepted blank values, knowning that they would be expanded by a lower level). The behaviour was also rather confusing since valid() connotes isValid(), not makeValid(). This change Ensures that both podID and containerID are set at the highest level (in virtc) which will allow all lower-level functions to assert these values are non-blank in future. It also adds an explicit "--id=" for "pod create". Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel · 2017-04-12T15:49:21Z

Ready for re-review.

jodh-intel · 2017-04-12T16:12:13Z

Note that once we've decided on a strategy for #187, I can raise a follow-up PR to simplify the test changes (using a test setup fixture).

Where possible, standard errors are used. Note that a few parameter names have changed slightly to ensure the returned standard errors make sense. Signed-off-by: James O. D. Hunt <[email protected]>

Explain which podResource types relate only to pods. Signed-off-by: James O. D. Hunt <[email protected]>

Updated usage to show that if the user does not specify a container name using "--id=", one will be chosen automatically. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel · 2017-04-12T16:47:10Z

Updated cleanUp() function to stop TestQemuAppendImage from failing due to a missing testImage file. This fix is minimal: I plan to raise follow-up PRs to improve the tests after this PR lands.

coveralls · 2017-04-12T16:52:28Z

Coverage decreased (-1.05%) to 67.245% when pulling ac92c3e on jodh-intel:check-args into 9c5273c on containers:master.

sboeuf · 2017-04-12T16:55:36Z

lgtm

sboeuf reviewed Mar 21, 2017

View reviewed changes

jodh-intel force-pushed the check-args branch 3 times, most recently from e4e73d7 to 62dda78 Compare March 22, 2017 17:57

jodh-intel added the do-not-merge label Mar 23, 2017

jodh-intel force-pushed the check-args branch 2 times, most recently from 76f1770 to b41245d Compare March 23, 2017 16:17

jodh-intel mentioned this pull request Mar 24, 2017

Don't export PodConfig.ID and ContainerConfig.ID #171

Closed

jodh-intel force-pushed the check-args branch from b41245d to 8ec8473 Compare April 4, 2017 10:22

wcwxyz reviewed Apr 5, 2017

View reviewed changes

jodh-intel force-pushed the check-args branch 4 times, most recently from 0cbc28c to 7c3c612 Compare April 10, 2017 16:48

jodh-intel mentioned this pull request Apr 10, 2017

Switch tests to use a testing framework that provides fixtures (test setup / tear down functions). #187

Open

jodh-intel added 3 commits April 12, 2017 15:10

network: Add argument checks.

73f5dff

Return error if argument checks fail. Signed-off-by: James O. D. Hunt <[email protected]>

errors: Add additional standard errors.

bb8fe0d

Added additional variables for standard error conditions. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel force-pushed the check-args branch from 7c3c612 to f9e4819 Compare April 12, 2017 15:47

jodh-intel removed the do-not-merge label Apr 12, 2017

jodh-intel force-pushed the check-args branch from f9e4819 to 7df4e03 Compare April 12, 2017 16:10

jodh-intel changed the title ~~RFC: Check podID and containerID function arguments~~ Check podID and containerID function arguments Apr 12, 2017

jodh-intel added 3 commits April 12, 2017 17:45

general: Add explicit checks on all possible function arguments.

17726a8

Where possible, standard errors are used. Note that a few parameter names have changed slightly to ensure the returned standard errors make sense. Signed-off-by: James O. D. Hunt <[email protected]>

filesystem: Improve comments on podResource.

f74bd13

Explain which podResource types relate only to pods. Signed-off-by: James O. D. Hunt <[email protected]>

virtc: Explain the "--id=" is set automatically by default.

ac92c3e

Updated usage to show that if the user does not specify a container name using "--id=", one will be chosen automatically. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel force-pushed the check-args branch from 7df4e03 to ac92c3e Compare April 12, 2017 16:45

sboeuf merged commit ba80514 into containers:master Apr 12, 2017

jodh-intel mentioned this pull request Apr 27, 2018

virtcontainers: always pass sandbox as a pointer kata-containers/runtime#263

Merged

bergwolf mentioned this pull request May 1, 2018

check sandbox pointers in virtcontainers internal functions kata-containers/runtime#280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check podID and containerID function arguments #163

Check podID and containerID function arguments #163

jodh-intel commented Mar 21, 2017

sboeuf Mar 21, 2017

jodh-intel Mar 22, 2017

sboeuf Mar 22, 2017

sboeuf Mar 21, 2017

jodh-intel Mar 22, 2017 •

edited

Loading

sameo Mar 23, 2017

sboeuf Mar 23, 2017

jodh-intel Mar 23, 2017

sboeuf Mar 23, 2017

sameo Mar 23, 2017 •

edited

Loading

jodh-intel Mar 24, 2017 •

edited

Loading

sboeuf Mar 21, 2017

jodh-intel Mar 22, 2017

sboeuf Mar 21, 2017

jodh-intel Mar 22, 2017

sboeuf commented Mar 21, 2017

jodh-intel commented Mar 22, 2017

jodh-intel commented Mar 22, 2017

sboeuf commented Mar 22, 2017

jodh-intel commented Mar 23, 2017

wcwxyz Apr 5, 2017

dlespiau Apr 5, 2017

jodh-intel Apr 5, 2017

wcwxyz Apr 5, 2017

jodh-intel Apr 5, 2017

jodh-intel commented Apr 12, 2017

jodh-intel commented Apr 12, 2017

jodh-intel commented Apr 12, 2017

coveralls commented Apr 12, 2017 •

edited

Loading

sboeuf commented Apr 12, 2017 •

edited by dlespiau

Loading

Check podID and containerID function arguments #163

Check podID and containerID function arguments #163

Conversation

jodh-intel commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameo Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

jodh-intel Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf commented Mar 21, 2017

jodh-intel commented Mar 22, 2017

jodh-intel commented Mar 22, 2017

sboeuf commented Mar 22, 2017

jodh-intel commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Apr 12, 2017

jodh-intel commented Apr 12, 2017

jodh-intel commented Apr 12, 2017

coveralls commented Apr 12, 2017 • edited Loading

sboeuf commented Apr 12, 2017 • edited by dlespiau Loading

jodh-intel Mar 22, 2017 •

edited

Loading

sameo Mar 23, 2017 •

edited

Loading

jodh-intel Mar 24, 2017 •

edited

Loading

coveralls commented Apr 12, 2017 •

edited

Loading

sboeuf commented Apr 12, 2017 •

edited by dlespiau

Loading