Porter explain warns for docker host access #1868

joshuabezaleel · 2022-01-24T13:13:16Z

What does this change

When the bundle has docker as one of the required mixins, porter explain command warns for the host access in advance and also include --allow-docker-host-access flag for the installation command at the end.

I attached the screenshot for running the porter explain command for the tabbycat-demo bundle with docker added as one of the required mixins and also for the whalesay bundle.

What issue does it fix

Closes #1830

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our [Contributors][contributors] list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

carolynvs

Can you also add a test case for the warning that should appear when the bundle requires the docker extension? Here's where we have a similar test already:

porter/pkg/porter/explain_test.go

Line 123 in 6dab64a

func TestExplain_generateTable(t *testing.T) {

carolynvs · 2022-01-24T14:00:17Z

pkg/porter/explain.go

+
+ // Check whether the bundle requires docker mixin and warn for host access.
+ var requireDocker bool = false
+ for _, mixin := range bun.Mixins {


Using the docker mixin isn't the problem we need to warn about. What we are looking for is when a bundle declares that it requires docker host access:

https://porter.sh/author-bundles/#required

required: - docker # this says inject the docker socket into the bundle

The preferred way to check for that is with bun.SupportsX, like bun.SupportsFileParameters, which checks if a CNAB required extension is defined on the bundle. There should be a function named bun.SupportsDocker but I guess that function never made.

It should look like this:

porter/pkg/cnab/extensions/parameter_sources.go

Line 198 in 6dab64a

func SupportsParameterSources(b bundle.Bundle) bool {

which should be added to https://github.com/getporter/porter/blob/main/pkg/cnab/extensions/docker.go

I just realized that I didn't stumble upon this one since the directory of pkg/cnab/extensions doesn't exist on release/v1 branch but only exists on main branch. Does this mean that the PR for this particular issue should be made upon the main branch?

Oops, sorry I shouldn't have linked to the main branch. This PR should target release/v1 but the code that I wanted to show you as an example moved a bit:

porter/pkg/cnab/parameter_sources.go

Lines 204 to 206 in a93a44a

func (b ExtendedBundle) SupportsParameterSources() bool {

return b.SupportsExtension(ParameterSourcesExtensionKey)

}

.

Edited, thank you so much for the guidance, Carolyn! 😄

joshuabezaleel · 2022-02-09T14:30:59Z

@carolynvs Also I found this particular TODO line while working on this issue. Does this mean that we have to address this before v1.0? And if the answer to the previous question is yes, do we already have a corresponding issue for this one?

https://github.com/getporter/porter/blob/release/v1/pkg/cnab/file_parameter.go#L39-L42

joshuabezaleel · 2022-02-09T14:32:09Z

Wait why can't my link for line of code on GitHub be rendered as a snippet like yours 🤔

carolynvs · 2022-02-11T15:48:08Z

Also I found this particular TODO line while working on this issue. Does this mean that we have to address this before v1.0? And if the answer to the previous question is yes, do we already have a corresponding issue for this one?

oof, that is a good question. If we removed that and relied 100% on if the bundle says that it supports file parameters, then old bundles would stop working in porter v1.0.0. I don't really see a good reason to do that, and it's just a single line to keep around to keep old bundles working. We should just remove that todo and not remove that line of code.

carolynvs

This looks great! I just had some small suggestions. Once the unit tests are passing (there's a build failure in the tests), this is ready to go! 🚀

carolynvs · 2022-02-11T15:50:29Z

pkg/porter/explain.go

- fmt.Fprintf(p.Out, "porter install%s%s%s\n", bundleReferenceFlag, requiredParameterFlags, credentialFlags)
+ porterInstallCommand := fmt.Sprintf("porter install%s%s%s", bundleReferenceFlag, requiredParameterFlags, credentialFlags)
+
+ // Check whether the bundle requires docker mixin and add flag for host access for install command.


This comment is a bit misleading since we aren't checking for the docker mixin, but if the bundle requires the docker socket to be mounted into the bundle.

carolynvs · 2022-02-11T15:53:13Z

pkg/porter/explain_test.go

+ err = p.printBundleExplain(opts, pb, b)
+ assert.NoError(t, err)
+ gotOutput := p.TestConfig.TestContext.GetOutput()
+ expected, err := ioutil.ReadFile("testdata/explain/expected-table-output-docker.txt")


We have a test helper that makes it easier to compare command output with the contents of a file and keep it in sync when the command output changes.

import "get.porter.sh/porter/pkg/test" test.CompareGoldenFile(t, "testdata/explain/expected-table-output-docker.txt", gotOutput)

If you run mage updateTestFiles it will find any tests whose output has changed and updates the testdata file automatically so the test passes.

We aren't using that new helper function everywhere in Porter yet, but I'd like new tests to use it so they are easier to maintain.

…is docker as one of the required mixins in the bundle Signed-off-by: joshuabezaleel <[email protected]>

Signed-off-by: joshuabezaleel <[email protected]>

joshuabezaleel · 2022-02-21T16:10:12Z

@carolynvs Sorry for the failing test, my bad. Should be all green by now. Thank you Carolyn! 😄

carolynvs

I love all the improvements to porter explain! It really makes it a much better experience for new users and helping them learn how to use bundles. 👍

joshuabezaleel · 2022-02-23T07:14:59Z

I really hope the explain command along with its updates will come useful for people using it! 😄

joshuabezaleel requested review from carolynvs, jeremyrickard and vdice as code owners January 24, 2022 13:13

joshuabezaleel force-pushed the 1830-porter-explain-warn-docker-host-access branch from 3d2d7d7 to 665493c Compare January 24, 2022 13:14

carolynvs requested changes Jan 24, 2022

View reviewed changes

joshuabezaleel force-pushed the 1830-porter-explain-warn-docker-host-access branch from d7e8c5f to 9a21508 Compare February 9, 2022 14:32

carolynvs reviewed Feb 11, 2022

View reviewed changes

carolynvs self-assigned this Feb 20, 2022

joshuabezaleel force-pushed the 1830-porter-explain-warn-docker-host-access branch 2 times, most recently from 93fc135 to cc5b376 Compare February 21, 2022 15:42

joshuabezaleel added 9 commits February 21, 2022 22:56

Add warning for host access and flag to allow host access when there …

a376944

…is docker as one of the required mixins in the bundle Signed-off-by: joshuabezaleel <[email protected]>

Add space for --cred flag

4a70d2a

Signed-off-by: joshuabezaleel <[email protected]>

Check for required docker extension instead of just checking mixin list

8ceda23

Signed-off-by: joshuabezaleel <[email protected]>

Remove TODO

84217f0

Signed-off-by: joshuabezaleel <[email protected]>

Fix commentaries from docker mixin to docker socket mount on bundle

043f8d6

Signed-off-by: joshuabezaleel <[email protected]>

Use CompareGoldenFile for comparing output

9ed9e41

Signed-off-by: joshuabezaleel <[email protected]>

Fix function name

c01fa24

Signed-off-by: joshuabezaleel <[email protected]>

Fix expected output

2a93355

Signed-off-by: joshuabezaleel <[email protected]>

Fix expected output

b794ac2

Signed-off-by: joshuabezaleel <[email protected]>

joshuabezaleel force-pushed the 1830-porter-explain-warn-docker-host-access branch from da29fba to b794ac2 Compare February 21, 2022 15:56

carolynvs approved these changes Feb 22, 2022

View reviewed changes

carolynvs merged commit d18c5c5 into getporter:release/v1 Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porter explain warns for docker host access #1868

Porter explain warns for docker host access #1868

joshuabezaleel commented Jan 24, 2022

carolynvs left a comment

carolynvs Jan 24, 2022

joshuabezaleel Feb 4, 2022

carolynvs Feb 7, 2022

joshuabezaleel Feb 9, 2022

joshuabezaleel commented Feb 9, 2022

joshuabezaleel commented Feb 9, 2022

carolynvs commented Feb 11, 2022

carolynvs left a comment

carolynvs Feb 11, 2022

carolynvs Feb 11, 2022

joshuabezaleel commented Feb 21, 2022

carolynvs left a comment

joshuabezaleel commented Feb 23, 2022 •

edited

Loading

	func (b ExtendedBundle) SupportsParameterSources() bool {
	return b.SupportsExtension(ParameterSourcesExtensionKey)
	}

Porter explain warns for docker host access #1868

Porter explain warns for docker host access #1868

Conversation

joshuabezaleel commented Jan 24, 2022

What does this change

What issue does it fix

Checklist

Reviewer Checklist

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Jan 24, 2022

Choose a reason for hiding this comment

joshuabezaleel Feb 4, 2022

Choose a reason for hiding this comment

carolynvs Feb 7, 2022

Choose a reason for hiding this comment

joshuabezaleel Feb 9, 2022

Choose a reason for hiding this comment

joshuabezaleel commented Feb 9, 2022

joshuabezaleel commented Feb 9, 2022

carolynvs commented Feb 11, 2022

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Feb 11, 2022

Choose a reason for hiding this comment

carolynvs Feb 11, 2022

Choose a reason for hiding this comment

joshuabezaleel commented Feb 21, 2022

carolynvs left a comment

Choose a reason for hiding this comment

joshuabezaleel commented Feb 23, 2022 • edited Loading

joshuabezaleel commented Feb 23, 2022 •

edited

Loading