cnabio / cnab-go Goto Github PK

I think we can use the same Brigade instance from Duffle.
Once @vdice confirms, and creates a new project for us, I'll go ahead and add basic config to test this package.

Currently, bundles don't have digests for invocation images (and other images). The cnabio/duffle#355 PR is skipping validation when there is no digest present. Bundles should have digests and we should instead enforce the validation. This would be a breaking change at the moment (given state of bundles) so once cnabio/duffle#391 is completed, swap digest validation to be required, unless explicitly disabled.

As it turns out, the master branch of this repository is not protected, and should probably be.

This issue tracks the addition of branch protections for master.

A last minute spec clarification was put in that affects the runtime implementation in cnab-go. cnabio/cnab-spec#270

All outputs that apply to a specified action are considered to be required. If an output is missing at the end of an action, and a default is defined, the runtime should write the default to the file specified by the path attribute. Otherwise the runtime should report the missing output as an error to the user.

Right now we expect all outputs to exist when we fetch them and return an error when one doesn't exist. The change needed is to detect when the output file is missing, and to create it with the default defined on the output, if specified.

See original comment from @glyn.

I've been going through the Docker driver code and I realized that it currently has no testing, and we should probably add some.

@silvin-lubecki - is there a fake Docker client / CLI / standard way of testing the creation of containers, adding volumes and other typical operations we might execute with the Docker driver?

Consider the following duffle.json:

{
    "description": "A short description of your bundle",
    "credentials": {
        "username": {
            "description": "this is a cred",
            "env": "HOST_KEY"
        }
    },
	"invocationImages": {
		"cnab": {
			"builder": "docker",
			"configuration": {
				"registry": "jeremyrickard"
			},
			"name": "cnab"
		}
	},
	"keywords": ["test", "cnab", "tutorial"],
	"maintainers": [{
		"email": "[email protected]",
		"name": "John Doe",
		"url": "https://example.com"
	}, {
		"email": "[email protected]",
		"name": "Jane Doe",
		"url": "https://example.com"
	}],
	"definitions": {
		"foo": {
			"type": "string"
		}
	},
	"parameters": {
		"foo": {
			"definition": "foo",
			"required": false,
			"destination": {
				"path": "",
				"env": "APP_URL"
			}
		},
		"manifest_file": {
            "definition": "foo",
            "required": false,
			"destination": {
                "path" : "",
				"env": "MANIFEST_FILE"
			}
		}
	},
	"name": "test",
	"schemaVersion": "v1.0.0-WD",
	"version": "0.1.0"
}

This produces a bundle that looks like:

{"credentials":{"username":{"description":"this is a cred","env":"HOST_KEY"}},"definitions":{"foo":{"type":"string"}},"description":"A short description of your bundle","invocationImages":[{"image":"jeremyrickard/test-cnab:7612a24756ad13e91aa1ff2d7795039045b43462","imageType":"docker"}],"keywords":["test","cnab","tutorial"],"maintainers":[{"email":"[email protected]","name":"John Doe","url":"https://example.com"},{"email":"[email protected]","name":"Jane Doe","url":"https://example.com"}],"name":"test","parameters":{"foo":{"definition":"foo","destination":{"env":"APP_URL"}},"manifest_file":{"definition":"foo","destination":{"env":"MANIFEST_FILE"}}},"schemaVersion":"v1.0.0-WD","version":"0.1.0"}

In this case, the spec is clear that the credential is not required. We currently fail installation if any credentials are defined though:

$ duffle install blah test:0.1.0
Error: bundle requires credential for username

Per the spec, we MUST fail if the credential is required. I believe if a credential is optional we should fail fast. The bundle may or may not need the credential and we should defer to the bundle author to
a.) tell the bundle consumers that it is required by setting it to be required
b.) handle optional credentials in their bundle.

This allows use cases like cnabio/duffle#574

According to the CNAB spec, the parameters/required field in a bundle is optional:

required: A list of required parameters. MUST be an array of strings.(OPTIONAL)

The bundle schema confirms this by not declaring the field to be required.

Yet attempting to install a bundle without an explicit empty array for parameters/required results in SIGSEGV e.g.:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x13580aa]

goroutine 1 [running]:
github.com/deislabs/duffle/vendor/github.com/deislabs/cnab-go/bundle.ValuesOrDefaults(0xc00036b410, 0xc0000da540, 0xc0002c7a30, 0x0, 0xc00036b440)
	/Users/gnormington/go/src/github.com/deislabs/duffle/vendor/github.com/deislabs/cnab-go/bundle/bundle.go:139 +0x9a
main.calculateParamValues(0xc0000da540, 0x0, 0x0, 0x34e4b08, 0x0, 0x0, 0x34e4b08, 0x0, 0x0, 0x3, ...)
	/Users/gnormington/go/src/github.com/deislabs/duffle/cmd/duffle/install.go:264 +0x3dd
main.(*installCmd).run(0xc0000da240, 0x1a, 0x24beb40)
	/Users/gnormington/go/src/github.com/deislabs/duffle/cmd/duffle/install.go:139 +0x41e
main.newInstallCmd.func1(0xc0001dcf00, 0xc0000e6e40, 0x2, 0x2, 0x0, 0x0)
	/Users/gnormington/go/src/github.com/deislabs/duffle/cmd/duffle/install.go:85 +0xcf
github.com/deislabs/duffle/vendor/github.com/spf13/cobra.(*Command).execute(0xc0001dcf00, 0xc0000e6e00, 0x2, 0x2, 0xc0001dcf00, 0xc0000e6e00)
	/Users/gnormington/go/src/github.com/deislabs/duffle/vendor/github.com/spf13/cobra/command.go:762 +0x465
github.com/deislabs/duffle/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc0003c3b80, 0x0, 0xc0003c3b80, 0xc00009a058)
	/Users/gnormington/go/src/github.com/deislabs/duffle/vendor/github.com/spf13/cobra/command.go:852 +0x2ec
github.com/deislabs/duffle/vendor/github.com/spf13/cobra.(*Command).Execute(...)
	/Users/gnormington/go/src/github.com/deislabs/duffle/vendor/github.com/spf13/cobra/command.go:800
main.main()
	/Users/gnormington/go/src/github.com/deislabs/duffle/cmd/duffle/main.go:32 +0x48

Steps to reproduce

1. Unless cnabio/duffle#795 has been fixed, edit examples/helloworld/duffle.json to ensure it has a schema version, e.g.

   "schemaVersion" : "v1.0.0-WD",
   

2. duffle build ./examples/helloworld
3. docker push the resultant invocation image
4. duffle install helloworld helloworld
5. Observe SIGSEGV

The code to inject parameters into the invocation image does not handle object or array types correctly. For example, if I include an object in my parameters section then it appears inside the invocation image as map[string]interface{"key": "value"} instead of {"key": "value"}.

The code referenced above should marshal the parameter values to JSON.

cc @youreddy

Revisit Brigade CI (brigade.js) and update/refresh according to latest best practices, in addition to including issue comment handling (edit: done in #45) and individual check run execution.

The docker driver is passed on the Operation struct a writer for any output, but instead it is writing directly to stdout.

As part of Porter we are implementing a verbose flag and would like to be able to show/hide this output, but it is printing directly so we can't control it. Would you be up for a pull request that switches the driver over to using op.Out?

PR cnabio/duffle#428 will add the equivalent of a docker push when doing a build, and optionally when doing a docker sign in order to get the digests of the images for the bundle.

This isn't a great UX though, as it requires the push on build. A better solution might be to look at buildkit and containerd to do all of this locally w/o using the Docker libraries.

Per conversation with dmcg:

containerd uses the digest, not the image id to refer to images. If you pull with containerd, the image digest used is the manifest hash. Containerd itself does no create content, so you will always know the digest before pushing. If using buildkit, buildkit can create the content but it will create the full manifest, rather than just image id with uncompressed layers

The manifest digest refers to compressed layers, so Docker doesn't know that identifier until after push since it calculates it on push. After we replace the image backend in Docker, that will work a little differently, we will be able to keep the compressed image hashes that were pulled or built

Related to multiple identifiers, it is always possible to create images that are  the "same" but only differ by metadata, compression, encryption, or anything else

However, we are trying to move to a world where that original content is always used, so changes to the identifier actually represent a change to the image, rather than a side effect of pulling and pushing an image from a different docker version

The image ID does not have a the compressed hash, which tends to be what is needed to fetch the image from a repository or the byte size of the fetch-able artifacts

We should add support for Section 500.

The CNAB spec was updated to introduce a required attribute on credentials. More generally, we don't properly model the credential as it also has a description. We should update cnab-go to reflect that, and introduce a Credential struct that models this correctly, instead of just using the Location type.

The Command driver should implement output handling.

Since the command driver works by exec-ing a local binary, maybe the way to get output files would be to agree on a folder where the binary would leave them. I'm not sure where the interface between the Command driver and the binary it calls is defined.

(cnabio/cnab-spec#227) in https://github.com/deislabs/cnab-spec introduced some changes to how parameters are defined, so we need to update cnab-go to reflect that.

Now that we have updated Duffle to use the new cnab-go, we are seeing an issue related to outputs cnabio/duffle#839

These bundles don't have outputs, so we likely need to handle some edge cases here.

There is an optional license field in the bundle definition:

license: The license under which this bundle is covered. This SHOULD use one of the SPDX License Identifiers whenever possible (OPTIONAL)

And the JSON schema bits:

"license": {
      "description": "The SPDX license code or proprietary license name for this bundle",
      "type": "string"
}

We don't currently model that in our Bundle struct, so we should add it.

Now that duffle no longer uses viper, we've stripped the mapstructure annotations out of duffle and we can do the same for cnab-go unless some other user of cnab-go depends on these annotations. There may be only one way to find out...

Our BaseImage struct has JSON marshalling tags on the Digest field as follow:

Digest    string            `json:"digest,omitempty" mapstructure:"digest"`

The spec defines invocation images and images to have a contentDigest field, not a digest. We need to update to this.

There are some fields in the bundle that are optional per the CNAB spec:

• Images
• Credentials
• Parameters
• Outputs

We should update the bundle.go code appropriately to ensure that these can be left out of the resulting JSON without things like:

{"credentials":null,"parameters":null}

The current bundle marshalling code in cnab-go does not add a newline unlike the bundle marshalling code in duffle build:

func marshalBundle(bf *bundle.Bundle) ([]byte, string, error) {
	data, err := json.MarshalCanonical(bf)
        ...
	data = append(data, '\n') //TODO: why?

	digest, err := digest.OfBuffer(data)
        ...
	return data, digest, nil
}

Note: the newline is added before the digest is calculated.

The newline seems to have originated in some earlier code which signed the data first and then added the newline before the bundle was written to file, thus:

func (b *buildCmd) writeBundle(loc string, bf *bundle.Bundle) error {
        ...
	data, err := sign.Clearsign(bf)
	data = append(data, '\n')
	...
	return ioutil.WriteFile(loc, data, 0644)
}

Now that we are planning to move bundle marshalling to cnab-go, we need to decide whether to add the newline or not.

This project currently uses the qri-io/jsonschema library for parameter validation, as in https://github.com/deislabs/cnab-go/blob/master/bundle/definition/validation.go.

However, this underlying library currently only defines a subset of valid schema properties (see https://github.com/qri-io/jsonschema/blob/master/validate.go#L58-L107) and will fail validation (actually in unmarshal-ing prior to) for properties not in the list and not supplied via a custom validator.

One such property that would trigger this failure is contentEncoding. As an example, the validation then fails with:

Error: could not list bundle instances: error unmarshaling contentEncoding from json: json: cannot unmarshal string into Go value of type jsonschema._schema

Ways to avoid these validation failures would be:

• Add a custom validator to the jsonschema library before unmarshal-ing/validation. We'd have to do this for any/all missing/desired properties. See #118 as a first example.
• Add/PR applicable validators upstream (depending on project status, etc.)
• Explore use of a different JSON Schema validation library with full/desired support
• Other?

For a very long time now duffle, and now cnab-go has been relying on magic commits in a few packages.

https://github.com/deislabs/cnab-go/blob/2b49a93c113c94ccd6f13ca11509319a16b0a3b1/Gopkg.toml#L13-L28

https://github.com/deislabs/cnab-go/blob/2b49a93c113c94ccd6f13ca11509319a16b0a3b1/Gopkg.toml#L46-L60

This is a problem. It means that while yes, our own library works, for every downstream consumer of the library, they have to figure out what that magic commit is and use an override in their Gopkg.toml to get their code to work properly.

It also guarantees that as soon as there is a release or change in these dependencies that we apply to our Gopkg.toml, everyone who relies on cnab-go is going to have dep ensure fail when they update. 😅

It also locks the entire dependency graph onto a single commit, which makes resolution difficult if they are using the package elsewhere their code and need any other version but that commit.

The ideal configuration that we should be striving towards with a 1.0 release is that a consumer of our library can add a constraint for just cnab-go, and they inherit all of our constraints and they just work. If we are using any overrides or requires, then that won't work (which is our current state). Moving to modules would change some of the UX problems described here but not the fundamental graph solving problems.

I am not sure why those commits have remained unreleased in those packages for so long? But it's worth figuring out and coming up with a plan to get onto tagged versions.

Since bundles are written using canonical json, and canonical json does not allow for decimal numbers only integers, our current use of float64 to store properties such as Maximum, Minimum, etc is a problem. Any bundles with any of those fields set is unwritable with an error like json: unsupported value: 0.5.

Other fields like Default allow you to store a decimal number and should also gracefully be handled as part of any fix for the other fields.

One fix would be to switch from float64 to json.Number. Then in WriteTo we could check if fields like Default which store an interface{} if they contain a non-integer float and stringify it.

cnab-spec recently merged cnabio/cnab-spec#212, which adds an immutable flag to the parameter definition. We need to update the struct here to reflect this.

We're adding a dependency on a JSON Schema validator as part of #34. We can also use this to validate the bundle by enhancing the existing bundle Validate(..) method.

PR #82 makes a modification to the Makefile and tests to introduce some integration tests:

.PHONY: integration-test
integration-test:
	go test -tags integration ./...

Those are not currently run as part of CI, so we need to enhance CI (azure pipelines and brigade) to handle running the integration tests as part of a CI run. Integration requests will require DinD.

We should stop relying on revisions for importing this repo and start using semantic versions.

@carolynvs suggested we start with a v0.1.0.

Thoughts?

Couple of outstanding issues with Parameter definitions:

• The top level parameters element of the bundle.json requires a required array to indicate what parameters are required for the bundle.
• Simple types should not have a required attribute
• Our types are not correct (bool => boolean)
• JSON schema for complex objects

Some drivers can\will be able to execute invocation images for multiple platforms and architectures, the interface between duffle and drivers should allow for this - see cnabio/cnab-spec#179

As discussed in the public channel + meeting, the Duffle maintainers should also be maintainers for this project.
Adding a CODEOWNERS file to reflect that.

cnabio/cnab-spec#220 in https://github.com/deislabs/cnab-spec introduced the concept of requiredExtensions. We should implement appropriate handling of this within cnab-go by adding it to the Bundle struct.

A PR in cnab-spec changed from apply-to => applyTo, we need to update to that.

As in the duffle repo, when a tag is pushed, Brigade should automate the creation of a github release.

/cc @vdice

as per the bundle.json description and the outputs PR on the cnab spec

Now that we have mostly added all the relevant output definitions and claims elements, we should complete the picture and implement output handling in the Kubernetes driver. The Kubernetes driver currently handles placing parameter and credentials into files for the invocation image, so we should extend the driver to handle obtaining them from the invocation image, maybe by adding a volume to store the outputs and reading from that?

I think we should leave what happens to them after they are read from the invocation image up to the end tooling, but we should make the driver able to fetch them.

See original comment from @glyn.

As discussed in cnabio/duffle#680, we want to extract various parts of Duffle as separate packages to be used in other projects.

As @michelleN pointed, as we are refactoring parts of Duffle, we don't want to extract everything right now -- but rather on an actual need basis, and after we agree about the package split.

As we are working on cnab-to-oci, it's clear we first need the github.com/deislabs/duffle/pkg/bundle package as a separate package, that we can import in both cnab-to-oci and Duffle.

This issue tracks the extraction of this package -- feel free to comment if you have thoughts about the split.

The PR accompanying this issue should have a single package, github.com/deislabs/cnab-go/bundle.

As @silvin-lubecki pointed out in #8, utils is not an ideal package name - moreover, the package also mixes an interface declaration for a CRUD store, with its filesystem implementation.

While this approach mostly made sense for Duffle, now that the package has moved out of Duffle and is intended to be imported by other projects, we should refactor the entire package. Here's what I had in mind:

• move the filesystem implementation of the crud.Store package back into Duffle.
• move the current crud.Store interface into a top-level crud package in this package - not 100% sure this is a good idea though, but at the same time, I also really like this interface being exported, such as consumers can directly use claimstore.Store and pass an implementation for this interface.

What do you think?

The cnab-spec recently merged cnabio/cnab-spec#219. This removed the platform object from both the imageMap image definitions and the invocationImage definitions and replaces it with a general purpose key/value label system. We need to update the structs here to support this.

https://github.com/deislabs/cnab-go/blob/master/bundle/bundle.go#L210-L218

This could be helpful for consumers of this library.

cc @silvin-lubecki

We currently have the omitEmpty tag on the Path field in OutputDefinition:

Path        string   `json:"path,omitemtpty" mapstructure:"path,omitempty"`

The bundle schema in cnab-spec indicates this is a required field, however.

 "output": {
      "description": "A value that is produced by running an invocation image",
      "properties": {
        "applyTo": {
          "description": "An optional exhaustive list of actions producing this output",
          "items": {
            "type": "string"
          },
          "type": "array"
        },
        "definition": {
          "description": "The name of a definition that describes the schema structure of this output",
          "type": "string"
        },
        "description": {
          "$ref": "http://json-schema.org/draft-07/schema#/properties/description"
        },
        "path": {
          "description": "The path inside of the invocation image where output will be written",
          "pattern": "^/cnab/app/outputs/.+$",
          "type": "string"
        }
      },
      "required": ["definition", "path"],
      "type": "object"
    },

See original comment from @glyn:

Now that we are effectively promoting these packages for reuse by others, I'd prefer black-box testing from the action_test package. Benefits include:

• the testcases are more likely to document the expected use of the package
• any test-specific externals of the package will stick out like a sore thumb, thus tending to reduce such externals.

The Out field is an io.writer, including it in json serialization means that CNAB-go cannot be used to implement a duffle command driver as unmarshall of {} fails.

Here: https://github.com/deislabs/cnab-go/blob/de80204caf62eedc7aabd20ec93c47baa1c1c27a/bundle/bundle.go#L84

Looks like omitEmpty was accidentally omitted from the YAML tag.

claims.schema.json defines the claim object as follows:

"properties": {
    "additionalProperties": false,
    "bundle": {
      "$ref": "http://cnab.io/v1/bundle.schema.json",
      "description": "The bundle descriptor"
    },
    "created": {
      "description": "The date created, as an ISO-8601 Extended Format date string, as specified in the ECMAScript standard",
      "type": "string"
    },
    "custom": {
      "$comment": "reserved for custom extensions"
    },
    "modified": {
      "description": "The date last modified, as an ISO-8601 Extended Format date string, as specified in the ECMAScript standard",
      "type": "string"
    },
    "name": {
      "description": "the name of the installation",
      "type": "string"
    },
    "outputs": {
      "description": "key/value pairs of output name to output value.",
      "type": "object"
    },
    "parameters": {
      "description": "key/value pairs of parameter name to parameter value.",
      "type": "object"
    },
    "result": {
      "$ref": "#/definitions/result",
      "description": "The result of the last action"
    },
    "revision": {
      "description": "the revision ID (ideally a ULID)",
      "type": "string"
    }
  },
  "required": [
    "bundle",
    "created",
    "modified",
    "name",
    "revision"
  ],

Our struct is currently:

type Claim struct {
	Name          string                    `json:"name"`
	Revision      string                    `json:"revision"`
	Created       time.Time                 `json:"created"`
	Modified      time.Time                 `json:"modified"`
	Bundle        *bundle.Bundle            `json:"bundle"`
	Result        Result                    `json:"result"`
	Parameters    map[string]interface{}    `json:"parameters"`
	Outputs       map[string]interface{}    `json:"outputs"`
	RelocationMap bundle.ImageRelocationMap `json:"relocationMap"`
}

At a minimum, we should add the custom field and we should probably remove the RelocationMap from the struct. Duffle implements this capability, so it could be an additive thing for Duffle. Currently the claim schema has additionalAttributes set to false, so a claim we produce with cnab-go will not validate.

Now that we have mostly added all the relevant output definitions and claims elements, we should complete the picture and implement output handling in the Docker driver. The Docker driver currently handles placing parameter and credentials into files for the invocation image, so we should extend the driver to handle obtaining them from the invocation image.

I think we should leave what happens to them after they are read from the invocation image up to the end tooling, but we should make the driver able to fetch them.

The driver interface is already defined in the driver package. Many of the driver implementations from duffle are generally useful outside of duffle and should be made available without the need for declaring a dependency on duffle.