podman kube play: add --validate=ignore|warn|strict flag#29145
Conversation
|
Changes look OK from a high level, high speed review. Tests, however, aren't having it @ROKUMATE |
There was a problem hiding this comment.
It's still unclear how we want to approach this problem at least going by the discussion in the linked issue.
I personally would prefer going the k8s way with a --validate=strict|warn|ignore flag and unlike k8s that defaults to strict, we can start with ignore. This has the added benefit of validating both fields and kind (#27712). This allows the user to either ignore, warn, or fail on any unsupported type.
cc/ @ygalblum
|
i didnt thought on the way to add a flag initally ... |
I think the flag is a good idea. PTAL @podman-container-tools/podman-maintainers |
|
will implement it 👍🏼 |
I like the idea of this flag. I think defaulting to |
5d84aeb to
214ab53
Compare
|
Added the flag option suggested 👍🏼 |
| validateFlagName := "validate" | ||
| flags.StringVar(&playOptions.Validate, validateFlagName, "ignore", "How to handle unrecognized YAML fields and kinds: `ignore`, `warn`, or `strict`") | ||
| _ = cmd.RegisterFlagCompletionFunc(validateFlagName, func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { | ||
| return []string{"ignore", "warn", "strict"}, cobra.ShellCompDirectiveNoFileComp |
There was a problem hiding this comment.
Can these values be typed? The hard coded values keep repeating in the code
There was a problem hiding this comment.
it got slipped from my mind regarding that
any more reviews from this ??
as will implement all at once then if there are any more changes required 👍🏼
| @@ -358,6 +358,9 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options | |||
| if err := yaml.Unmarshal(document, &podYAML); err != nil { | |||
There was a problem hiding this comment.
I wonder if there's a way to combine the Unmarshal with the validation below. They seem to do similar things. (in all the cases)
There was a problem hiding this comment.
will look a way to combine it 👍🏼
| logrus.Infof("Kube kind %s not supported", kind) | ||
| switch options.Validate { | ||
| case "strict": | ||
| return nil, fmt.Errorf("kube kind %q is not supported", kind) |
There was a problem hiding this comment.
Why do we use %q in the new cases, but keep %s in the default case? Also since the same string is used in all cases, does it make sense to create it once?
There was a problem hiding this comment.
did it by mistake will fix it 👍🏼
214ab53 to
390d1ba
Compare
| var SupportedKubeValidateModes = []KubeValidateMode{ | ||
| KubeValidateIgnore, | ||
| KubeValidateWarn, | ||
| KubeValidateStrict, | ||
| } | ||
|
|
||
| // IsValid reports whether m is a supported validate mode. | ||
| func (m KubeValidateMode) IsValid() bool { | ||
| return slices.Contains(SupportedKubeValidateModes, m) | ||
| } |
There was a problem hiding this comment.
Using a map[KubeValidateMode]bool instead of a list will simplify IsValid as it will be a simple map check
There was a problem hiding this comment.
did the changes 👍🏼
390d1ba to
10f8d4d
Compare
| return fmt.Errorf("--service-container does not work with --start=stop") | ||
| } | ||
| playOptions.Validate = entities.KubeValidateMode(playOptions.ValidateCLI) | ||
| if !playOptions.Validate.IsValid() { |
There was a problem hiding this comment.
We should also do this check in the API handler
| if err := yaml.Unmarshal(document, &jobYAML); err != nil { | ||
| return nil, fmt.Errorf("unable to read YAML as Kube Job: %w", err) | ||
| if err := unmarshalKubeObject("Job", options.Validate, document, &jobYAML); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Are we skipping PVCs, ConfigMaps, and Secrets deliberately?
There was a problem hiding this comment.
nahh i forgot to add that check over here
will add that as well
| func unmarshalKubeObject(kind string, mode entities.KubeValidateMode, document []byte, obj any) error { | ||
| if mode != entities.KubeValidateWarn && mode != entities.KubeValidateStrict { | ||
| return decodeKubeObject(kind, document, obj) | ||
| } | ||
|
|
||
| err := yaml.UnmarshalStrict(document, obj) | ||
| switch { | ||
| case err == nil: | ||
| return nil | ||
| case mode == entities.KubeValidateStrict: | ||
| return fmt.Errorf("validating kube %s: %w", kind, err) | ||
| default: // warn | ||
| logrus.Warnf("kube %s: %v", kind, err) | ||
| return decodeKubeObject(kind, document, obj) | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: but this is not the easiest to read; you could simply fall through from strict, warn and to ignore.
| func unmarshalKubeObject(kind string, mode entities.KubeValidateMode, document []byte, obj any) error { | |
| if mode != entities.KubeValidateWarn && mode != entities.KubeValidateStrict { | |
| return decodeKubeObject(kind, document, obj) | |
| } | |
| err := yaml.UnmarshalStrict(document, obj) | |
| switch { | |
| case err == nil: | |
| return nil | |
| case mode == entities.KubeValidateStrict: | |
| return fmt.Errorf("validating kube %s: %w", kind, err) | |
| default: // warn | |
| logrus.Warnf("kube %s: %v", kind, err) | |
| return decodeKubeObject(kind, document, obj) | |
| } | |
| } | |
| func unmarshalKubeObject(kind string, mode entities.KubeValidateMode, document []byte, obj any) error { | |
| switch mode { | |
| case entities.KubeValidateStrict: | |
| if err := yaml.UnmarshalStrict(document, obj); err != nil { | |
| return fmt.Errorf("validating kube %s: %w", kind, err) | |
| } | |
| case entities.KubeValidateWarn: | |
| if err := yaml.UnmarshalStrict(document, obj); err != nil { | |
| logrus.Warnf("kube %s: %v", kind, err) | |
| return decodeKubeObject(kind, document, obj) | |
| } | |
| default: // ignore, and any invalid value | |
| return decodeKubeObject(kind, document, obj) | |
| } | |
| return nil | |
| } |
| // supportedKubeValidateModes is the set of accepted --validate values. | ||
| var supportedKubeValidateModes = map[KubeValidateMode]bool{ | ||
| KubeValidateIgnore: true, | ||
| KubeValidateWarn: true, | ||
| KubeValidateStrict: true, | ||
| } | ||
|
|
||
| // IsValid reports whether m is a supported validate mode. | ||
| func (m KubeValidateMode) IsValid() bool { | ||
| return supportedKubeValidateModes[m] | ||
| } |
There was a problem hiding this comment.
This feels unnecessary; I believe we do similar validation for other commands, can you check:
podman/cmd/podman/containers/ps.go
Lines 104 to 107 in e3727b8
There was a problem hiding this comment.
ohkk will implement that 👍🏼
| // decoded leniently. In "warn" and "strict" mode it is decoded strictly: | ||
| // "strict" returns an error on an unknown field, while "warn" logs a warning | ||
| // and falls back to a lenient decode so playback still succeeds. | ||
| func unmarshalKubeObject(kind string, mode entities.KubeValidateMode, document []byte, obj any) error { |
There was a problem hiding this comment.
Could you please add unit tests for this?
| kube := podmanTest.Podman([]string{"kube", "play", kubeYaml}) | ||
| kube.WaitWithDefaultTimeout() | ||
| Expect(kube).Should(Exit(0)) |
There was a problem hiding this comment.
| kube := podmanTest.Podman([]string{"kube", "play", kubeYaml}) | |
| kube.WaitWithDefaultTimeout() | |
| Expect(kube).Should(Exit(0)) | |
| kube := podmanTest.PodmanExitCleanly([]string{"kube", "play", kubeYaml}) |
There was a problem hiding this comment.
This should also ensure stderr is empty ie no warnings.
| kube := podmanTest.Podman([]string{"kube", "play", kubeYaml}) | ||
| kube.WaitWithDefaultTimeout() | ||
| Expect(kube).Should(Exit(0)) |
There was a problem hiding this comment.
| kube := podmanTest.Podman([]string{"kube", "play", kubeYaml}) | |
| kube.WaitWithDefaultTimeout() | |
| Expect(kube).Should(Exit(0)) | |
| kube := podmanTest.PodmanExitCleanly([]string{"kube", "play", kubeYaml}) |
| err := writeYaml(yamlWithUnknownField, kubeYaml) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
I guess we can drop this unwanted I/O, worth a check.
There was a problem hiding this comment.
will drop it in the next commit and will ping then
| Expect(kube.ErrorToString()).To(ContainSubstring("BogusKind")) | ||
| }) | ||
|
|
||
| It("--validate=strict fails on unsupported kube kinds", func() { |
There was a problem hiding this comment.
Can you also add a test that ensure --validate=strict doesn't reject legitimate manifests?
Thanks @danishprakash for tagging me! I gave it a try with the following markdown
|
We discussed this previously and concluded that it's not desirable since it could potentially break existing workflows especially programmatic uses where an empty stderr is suddenly now populated when running with the same (even though broken) manifests. But assuming we surface the warnings in the API response (discussed below), then you could choose to show/suppress the warnings on the client end. Would that work for you?
Agree with both the points; API check was flagged earlier, I'm guessing it's still WIP but surfacing warnings in the API response is a good suggestion, we should certainly support that.
Yes, by design, it's an early return on any failure but if we have to aggregate all the validation issues at once like how k8s does it, it might be a bigger change than the scope of this PR imo. I suggest we track that in a separate issue. |
Yes, if the warnings is returned as part of the libpod/play/kube response, in Podman Desktop we will probably enable warn or strict by default, will see !
To show the flag on the Podman Destkop side this is a requirement for us!
I'm fine with it, I just wanted to be sure this has been considered. Thanks @danishprakash ! |
10f8d4d to
a5dd5b1
Compare
ignore warn and strict modes for unknown yaml fields and kinds ignore is the default and skips them warn reports them strict fails warnings are returned in the play report so they show up in the cli and over the api for tools like podman desktop Closes podman-container-tools#18332 Signed-off-by: ROKUMATE <rohitkumawat0110@gmail.com>
a5dd5b1 to
54fab1e
Compare
|
@danishprakash I have done the requested changes the understood the discussion as well the first force push was the changes you requested earlier while the second one is more on the side of the warning feature if any more review changes are there do tell 👍🏼 |
Fixes: #18332
podman kube playsilently ignored unrecognized YAML keys (e.g. a typo likeworkdirinstead ofworkingDir). This logs them at debug level for Pod/DaemonSet/Deployment/Job — the pod still runs unchanged, but ignored keys are visible with--log-level=debug.Note: the strict decode reports the first unrecognized key per document, not all.
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?