Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions internal/flink/command_environment_create.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package flink

import (
"bytes"
"encoding/json"
"fmt"
"io"
"os"
"path/filepath"
"strings"
Expand All @@ -26,7 +28,7 @@ func (c *command) newEnvironmentCreateCommand() *cobra.Command {

cmd.Flags().String("kubernetes-namespace", "", "Kubernetes namespace to deploy Flink applications to.")
cmd.Flags().String("defaults", "", "JSON string defining the environment's Flink application defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).")
cmd.Flags().String("statement-defaults", "", "JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).")
cmd.Flags().String("statement-defaults", "", `JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension). Expected shape: {"detached":{"flinkConfiguration":{...}},"interactive":{"flinkConfiguration":{...}}}.`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to:

  • Avoid specifying the expected shape here.
  • Have a dedicated function to perform validation + disallow unknown fields, and provide prompts/error out to users if the shape is invalid.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the same decodeStrictJson() and decodeStrictYaml() will have to be applied everywhere for on-prem.

cmd.Flags().String("compute-pool-defaults", "", "JSON string defining the environment's Flink compute pool defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).")

addCmfFlagSet(cmd)
Expand Down Expand Up @@ -124,18 +126,18 @@ func parseDefaultsAsGenericType[T any](input, label string) (T, error) {
if err != nil {
return out, fmt.Errorf("failed to read %s defaults JSON file: %w", label, err)
}
err = json.Unmarshal(data, &out)
err = decodeStrictJson(data, &out)

case ".yaml", ".yml":
data, err = os.ReadFile(input)
if err != nil {
return out, fmt.Errorf("failed to read %s defaults YAML file: %w", label, err)
}
err = yaml.Unmarshal(data, &out)
err = decodeStrictYaml(data, &out)

default:
// inline JSON string
err = json.Unmarshal([]byte(input), &out)
err = decodeStrictJson([]byte(input), &out)
}

if err != nil {
Expand All @@ -144,6 +146,39 @@ func parseDefaultsAsGenericType[T any](input, label string) (T, error) {
return out, nil
}

// decodeStrictJson decodes a single JSON value into out, rejecting unknown
// fields and trailing data so mis-shaped input surfaces instead of being
// silently dropped. Decoding into a map is unaffected (a map has no unknown
// fields).
func decodeStrictJson(data []byte, out any) error {
decoder := json.NewDecoder(bytes.NewReader(data))
decoder.DisallowUnknownFields()
if err := decoder.Decode(out); err != nil {
return err
}
if decoder.More() {
return fmt.Errorf("unexpected trailing data after JSON value")
}
return nil
}

// decodeStrictYaml is the YAML counterpart of decodeStrictJson; it also rejects
// unknown fields and any additional documents.
func decodeStrictYaml(data []byte, out any) error {
decoder := yaml.NewDecoder(bytes.NewReader(data))
decoder.KnownFields(true)
if err := decoder.Decode(out); err != nil {
return err
}
if err := decoder.Decode(&struct{}{}); err != io.EOF {
if err != nil {
return err
}
return fmt.Errorf("unexpected additional YAML document")
}
return nil
}
Comment on lines +153 to +180

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment makes sense, we can absorb it and make it part of the validation function as I suggest above.


func jsonMarshalHelper(v interface{}, label string) (string, error) {
data, err := json.Marshal(v)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/flink/command_environment_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

addCmfFlagSet(cmd)
cmd.Flags().String("defaults", "", "JSON string defining the environment's Flink application defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).")
cmd.Flags().String("statement-defaults", "", "JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).")
cmd.Flags().String("statement-defaults", "", `JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension). Expected shape: {"detached":{"flinkConfiguration":{...}},"interactive":{"flinkConfiguration":{...}}}.`)

Check failure on line 24 in internal/flink/command_environment_update.go

View check run for this annotation

SonarQube-Confluent / SonarQube Code Analysis

Define a constant instead of duplicating this literal "statement-defaults" 3 times.

[S1192] String literals should not be duplicated See more on https://sonarqube.confluent.io/project/issues?id=cli&pullRequest=3397&issues=421e0c7a-78de-4d19-b824-966a98720a70&open=421e0c7a-78de-4d19-b824-966a98720a70
cmd.Flags().String("compute-pool-defaults", "", "JSON string defining the environment's Flink compute pool defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).")
pcmd.AddOutputFlag(cmd)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Usage:
Flags:
--kubernetes-namespace string REQUIRED: Kubernetes namespace to deploy Flink applications to.
--defaults string JSON string defining the environment's Flink application defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension). Expected shape: {"detached":{"flinkConfiguration":{...}},"interactive":{"flinkConfiguration":{...}}}.
--compute-pool-defaults string JSON string defining the environment's Flink compute pool defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--url string Base URL of the Confluent Manager for Apache Flink (CMF). Environment variable "CONFLUENT_CMF_URL" may be set in place of this flag.
--client-key-path string Path to client private key for mTLS authentication. Environment variable "CONFLUENT_CMF_CLIENT_KEY_PATH" may be set in place of this flag.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Usage:
Flags:
--kubernetes-namespace string REQUIRED: Kubernetes namespace to deploy Flink applications to.
--defaults string JSON string defining the environment's Flink application defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension). Expected shape: {"detached":{"flinkConfiguration":{...}},"interactive":{"flinkConfiguration":{...}}}.
--compute-pool-defaults string JSON string defining the environment's Flink compute pool defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--url string Base URL of the Confluent Manager for Apache Flink (CMF). Environment variable "CONFLUENT_CMF_URL" may be set in place of this flag.
--client-key-path string Path to client private key for mTLS authentication. Environment variable "CONFLUENT_CMF_CLIENT_KEY_PATH" may be set in place of this flag.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Error: failed to parse statement defaults: json: unknown field "config-overrides"
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Error: failed to parse statement defaults: unexpected trailing data after JSON value
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Flags:
--client-cert-path string Path to client cert to be verified by Confluent Manager for Apache Flink. Include for mTLS authentication. Environment variable "CONFLUENT_CMF_CLIENT_CERT_PATH" may be set in place of this flag.
--certificate-authority-path string Path to a PEM-encoded Certificate Authority to verify the Confluent Manager for Apache Flink connection. Environment variable "CONFLUENT_CMF_CERTIFICATE_AUTHORITY_PATH" may be set in place of this flag.
--defaults string JSON string defining the environment's Flink application defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension). Expected shape: {"detached":{"flinkConfiguration":{...}},"interactive":{"flinkConfiguration":{...}}}.
--compute-pool-defaults string JSON string defining the environment's Flink compute pool defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
-o, --output string Specify the output format as "human", "json", or "yaml". (default "human")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Flags:
--client-cert-path string Path to client cert to be verified by Confluent Manager for Apache Flink. Include for mTLS authentication. Environment variable "CONFLUENT_CMF_CLIENT_CERT_PATH" may be set in place of this flag.
--certificate-authority-path string Path to a PEM-encoded Certificate Authority to verify the Confluent Manager for Apache Flink connection. Environment variable "CONFLUENT_CMF_CERTIFICATE_AUTHORITY_PATH" may be set in place of this flag.
--defaults string JSON string defining the environment's Flink application defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
--statement-defaults string JSON string defining the environment's Flink statement defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension). Expected shape: {"detached":{"flinkConfiguration":{...}},"interactive":{"flinkConfiguration":{...}}}.
--compute-pool-defaults string JSON string defining the environment's Flink compute pool defaults, or path to a file to read defaults from (with .yml, .yaml or .json extension).
-o, --output string Specify the output format as "human", "json", or "yaml". (default "human")

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Error: failed to parse statement defaults: json: unknown field "config-overrides"
3 changes: 3 additions & 0 deletions test/flink_onprem_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ func (s *CLITestSuite) TestFlinkEnvironmentCreate() {
{args: "flink environment create default-failure --kubernetes-namespace default-staging", fixture: "flink/environment/create-failure.golden", exitCode: 1},
{args: "flink environment create default --kubernetes-namespace default-staging", fixture: "flink/environment/create-existing.golden", exitCode: 1},
{args: "flink environment create default", fixture: "flink/environment/create-no-namespace.golden", exitCode: 1},
{args: "flink environment create default-2 --kubernetes-namespace default-staging --statement-defaults '{\"config-overrides\":{\"key\":\"value\"}}'", fixture: "flink/environment/create-statement-defaults-invalid.golden", exitCode: 1},
{args: "flink environment create default-2 --kubernetes-namespace default-staging --statement-defaults '{\"interactive\":{}}{\"detached\":{}}'", fixture: "flink/environment/create-statement-defaults-trailing.golden", exitCode: 1},
// success with application, statement and compute pool defaults
{args: "flink environment create default-2" +
" --defaults test/fixtures/input/flink/environment/application-defaults.json" +
Expand All @@ -288,6 +290,7 @@ func (s *CLITestSuite) TestFlinkEnvironmentUpdate() {
{args: "flink environment update non-existent --defaults '{\"property\": \"value\"}'", fixture: "flink/environment/update-non-existent.golden", exitCode: 1},
{args: "flink environment update get-failure --defaults '{\"property\": \"value\"}'", fixture: "flink/environment/update-get-failure.golden", exitCode: 1},
{args: "flink environment update missing-flag-failure", fixture: "flink/environment/missing-flag-failure.golden", exitCode: 1},
{args: "flink environment update default --statement-defaults '{\"config-overrides\":{\"key\":\"value\"}}'", fixture: "flink/environment/update-statement-defaults-invalid.golden", exitCode: 1},
// success with application, statement and compute pool defaults
{args: "flink environment update default" +
" --defaults test/fixtures/input/flink/environment/application-defaults.json" +
Expand Down