Modify inject to warn when file is un-injectable#1603
Conversation
cli/cmd/inject.go
Outdated
| } | ||
|
|
||
| if injected > 0 { | ||
| report.Write([]byte(fmt.Sprintf("%s %d of %d YAML document(s) injected\n", okStatus, injected, len(injectReports)))) |
There was a problem hiding this comment.
if everything went fine, should we print this at all? another way to put it: do we want to communicate to the user the number of documents that were injected?
siggy
force-pushed
the
siggy/inject-reports
branch
from
6b0da2c to
e8b91ed
Compare
siggy
force-pushed
the
siggy/inject-reports
branch
2 times, most recently
from
e58861b to
cd5951d
Compare
siggy
force-pushed
the
siggy/inject-reports
branch
from
cd5951d to
c805a0f
Compare
siggy
force-pushed
the
siggy/inject-reports
branch
2 times, most recently
from
a5f2a6c to
b696474
Compare
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
This looks great, and I'm excited to see this feature in action!
This change introduces a small issue with our integration tests, since the LinkerdRun helper method returns combined stdout and stderr from the commands that it runs, and the combined output from the inject command isn't valid yaml.
I think the right fix is for LinkerdRun to return two separate strings, one for stdout and another one for stderr. But unfortunately that will require updating all of the tests that call LinkerdRun to accept or discard the additional return value. On the upside, you could update the install integration test to validate that the generated report is accurate.
cli/cmd/install.go
Outdated
|
|
||
| return InjectYAML(buf, w, injectOptions) | ||
| // throwaway the stderr buffer | ||
| return InjectYAML(buf, w, new(bytes.Buffer), injectOptions) |
There was a problem hiding this comment.
Am pretty sure you can use ioutil.Discard here instead of allocating a new buffer.
| report := new(bytes.Buffer) | ||
|
|
||
| err = InjectYAML(read, output, tc.testInjectOptions) | ||
| err = InjectYAML(read, output, report, tc.testInjectOptions) |
There was a problem hiding this comment.
The tests for this are really slick -- thanks for adding them!
cli/cmd/inject.go
Outdated
| if err := yaml.Unmarshal(bytes, &om); err != nil { | ||
| return nil, err | ||
| } | ||
| report.name = fmt.Sprintf("%s/%s", k8s.ShortNameFromCanonicalResourceName(strings.ToLower(meta.Kind)), om.Name) |
There was a problem hiding this comment.
ShortNameFromCanonicalResourceName only works for a subset of all Kubernetes object types, so if the inject yaml includes an object type that's not covered by that function, then we won't properly format the name. For instance, if we were trying to inject a Secret named "super", report.name would be "/super".
I don't think this is an issue in the current implementation, however, since we only print the names of objects that include pod specs in the report, and all objects that include pod specs are covered in ShortNameFromCanonicalResourceName. But I'm inclined to switch to using full names so that down the road we know that the name field is printable for all object types.
report.name = fmt.Sprintf("%s/%s", strings.ToLower(meta.Kind), om.Name)
8876f7d to
11d5cde
Compare
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ Awesome, thanks for updating the integration tests! This all looks good to me.
If an input file is un-injectable, existing inject behavior is to simply output a copy of the input. Introduce a report, printed to stderr, that communicates the end state of the inject command. Currently this includes checking for hostNetwork and unsupported resources. Malformed YAML documents will continue to cause no YAML output, and return error code 1. This change also modifies integration tests to handle stdout and stderr separately. example outputs... some pods injected, none with host networking: ``` hostNetwork: pods do not use host networking...............................[ok] supported: at least one resource injected..................................[ok] Summary: 4 of 8 YAML document(s) injected deploy/emoji deploy/voting deploy/web deploy/vote-bot ``` some pods injected, one host networking: ``` hostNetwork: pods do not use host networking...............................[warn] -- deploy/vote-bot uses "hostNetwork: true" supported: at least one resource injected..................................[ok] Summary: 3 of 8 YAML document(s) injected deploy/emoji deploy/voting deploy/web ``` no pods injected: ``` hostNetwork: pods do not use host networking...............................[warn] -- deploy/emoji, deploy/voting, deploy/web, deploy/vote-bot use "hostNetwork: true" supported: at least one resource injected..................................[warn] -- no supported objects found Summary: 0 of 8 YAML document(s) injected ``` TODO: check for UDP and other init containers Part of #1516 Signed-off-by: Andrew Seigner <[email protected]>
siggy
force-pushed
the
siggy/inject-reports
branch
from
11d5cde to
17136cb
Compare
2 participants
If an input file is un-injectable, existing inject behavior is to simply
output a copy of the input.
Introduce a report, printed to stderr, that communicates the end state
of the inject command. Currently this includes checking for hostNetwork
and unsupported resources.
Malformed YAML documents will continue to cause no YAML output, and return
error code 1.
example outputs...
some pods injected, none with host networking:
some pods injected, one host networking:
no pods injected:
TODO: check for UDP and other init containers
Part of #1516
Signed-off-by: Andrew Seigner [email protected]