diff --git a/cli/clibase/option.go b/cli/clibase/option.go index 8c7fed92e20f7..b9a2e871d05df 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -1,6 +1,8 @@ package clibase import ( + "bytes" + "encoding/json" "os" "strings" @@ -65,6 +67,20 @@ type Option struct { ValueSource ValueSource `json:"value_source,omitempty"` } +// optionNoMethods is just a wrapper around Option so we can defer to the +// default json.Unmarshaler behavior. +type optionNoMethods Option + +func (o *Option) UnmarshalJSON(data []byte) error { + // If an option has no values, we have no idea how to unmarshal it. + // So just discard the json data. + if o.Value == nil { + o.Value = &DiscardValue + } + + return json.Unmarshal(data, (*optionNoMethods)(o)) +} + func (o Option) YAMLPath() string { if o.YAML == "" { return "" @@ -79,15 +95,101 @@ func (o Option) YAMLPath() string { // OptionSet is a group of options that can be applied to a command. type OptionSet []Option +// UnmarshalJSON implements json.Unmarshaler for OptionSets. Options have an +// interface Value type that cannot handle unmarshalling because the types cannot +// be inferred. Since it is a slice, instantiating the Options first does not +// help. +// +// However, we typically do instantiate the slice to have the correct types. +// So this unmarshaller will attempt to find the named option in the existing +// set, if it cannot, the value is discarded. If the option exists, the value +// is unmarshalled into the existing option, and replaces the existing option. +// +// The value is discarded if it's type cannot be inferred. This behavior just +// feels "safer", although it should never happen if the correct option set +// is passed in. The situation where this could occur is if a client and server +// are on different versions with different options. +func (optSet *OptionSet) UnmarshalJSON(data []byte) error { + dec := json.NewDecoder(bytes.NewBuffer(data)) + // Should be a json array, so consume the starting open bracket. + t, err := dec.Token() + if err != nil { + return xerrors.Errorf("read array open bracket: %w", err) + } + if t != json.Delim('[') { + return xerrors.Errorf("expected array open bracket, got %q", t) + } + + // As long as json elements exist, consume them. The counter is used for + // better errors. + var i int +OptionSetDecodeLoop: + for dec.More() { + var opt Option + // jValue is a placeholder value that allows us to capture the + // raw json for the value to attempt to unmarshal later. + var jValue jsonValue + opt.Value = &jValue + err := dec.Decode(&opt) + if err != nil { + return xerrors.Errorf("decode %d option: %w", i, err) + } + // This counter is used to contextualize errors to show which element of + // the array we failed to decode. It is only used in the error above, as + // if the above works, we can instead use the Option.Name which is more + // descriptive and useful. So increment here for the next decode. + i++ + + // Try to see if the option already exists in the option set. + // If it does, just update the existing option. + for optIndex, have := range *optSet { + if have.Name == opt.Name { + if jValue != nil { + err := json.Unmarshal(jValue, &(*optSet)[optIndex].Value) + if err != nil { + return xerrors.Errorf("decode option %q value: %w", have.Name, err) + } + // Set the opt's value + opt.Value = (*optSet)[optIndex].Value + } else { + // Hopefully the user passed empty values in the option set. There is no easy way + // to tell, and if we do not do this, it breaks json.Marshal if we do it again on + // this new option set. + opt.Value = (*optSet)[optIndex].Value + } + // Override the existing. + (*optSet)[optIndex] = opt + // Go to the next option to decode. + continue OptionSetDecodeLoop + } + } + + // If the option doesn't exist, the value will be discarded. + // We do this because we cannot infer the type of the value. + opt.Value = DiscardValue + *optSet = append(*optSet, opt) + } + + t, err = dec.Token() + if err != nil { + return xerrors.Errorf("read array close bracket: %w", err) + } + if t != json.Delim(']') { + return xerrors.Errorf("expected array close bracket, got %q", t) + } + + return nil +} + // Add adds the given Options to the OptionSet. -func (s *OptionSet) Add(opts ...Option) { - *s = append(*s, opts...) +func (optSet *OptionSet) Add(opts ...Option) { + *optSet = append(*optSet, opts...) } // Filter will only return options that match the given filter. (return true) -func (s OptionSet) Filter(filter func(opt Option) bool) OptionSet { +func (optSet OptionSet) Filter(filter func(opt Option) bool) OptionSet { cpy := make(OptionSet, 0) - for _, opt := range s { + for _, opt := range optSet { if filter(opt) { cpy = append(cpy, opt) } @@ -96,13 +198,13 @@ func (s OptionSet) Filter(filter func(opt Option) bool) OptionSet { } // FlagSet returns a pflag.FlagSet for the OptionSet. -func (s *OptionSet) FlagSet() *pflag.FlagSet { - if s == nil { +func (optSet *OptionSet) FlagSet() *pflag.FlagSet { + if optSet == nil { return &pflag.FlagSet{} } fs := pflag.NewFlagSet("", pflag.ContinueOnError) - for _, opt := range *s { + for _, opt := range *optSet { if opt.Flag == "" { continue } @@ -139,8 +241,8 @@ func (s *OptionSet) FlagSet() *pflag.FlagSet { // ParseEnv parses the given environment variables into the OptionSet. // Use EnvsWithPrefix to filter out prefixes. -func (s *OptionSet) ParseEnv(vs []EnvVar) error { - if s == nil { +func (optSet *OptionSet) ParseEnv(vs []EnvVar) error { + if optSet == nil { return nil } @@ -154,7 +256,7 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error { envs[v.Name] = v.Value } - for i, opt := range *s { + for i, opt := range *optSet { if opt.Env == "" { continue } @@ -172,7 +274,7 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error { continue } - (*s)[i].ValueSource = ValueSourceEnv + (*optSet)[i].ValueSource = ValueSourceEnv if err := opt.Value.Set(envVal); err != nil { merr = multierror.Append( merr, xerrors.Errorf("parse %q: %w", opt.Name, err), @@ -185,14 +287,14 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error { // SetDefaults sets the default values for each Option, skipping values // that already have a value source. -func (s *OptionSet) SetDefaults() error { - if s == nil { +func (optSet *OptionSet) SetDefaults() error { + if optSet == nil { return nil } var merr *multierror.Error - for i, opt := range *s { + for i, opt := range *optSet { // Skip values that may have already been set by the user. if opt.ValueSource != ValueSourceNone { continue @@ -212,7 +314,7 @@ func (s *OptionSet) SetDefaults() error { ) continue } - (*s)[i].ValueSource = ValueSourceDefault + (*optSet)[i].ValueSource = ValueSourceDefault if err := opt.Value.Set(opt.Default); err != nil { merr = multierror.Append( merr, xerrors.Errorf("parse %q: %w", opt.Name, err), @@ -224,9 +326,9 @@ func (s *OptionSet) SetDefaults() error { // ByName returns the Option with the given name, or nil if no such option // exists. -func (s *OptionSet) ByName(name string) *Option { - for i := range *s { - opt := &(*s)[i] +func (optSet *OptionSet) ByName(name string) *Option { + for i := range *optSet { + opt := &(*optSet)[i] if opt.Name == name { return opt } diff --git a/cli/clibase/option_test.go b/cli/clibase/option_test.go index f1b881d94408e..79db040ea7e47 100644 --- a/cli/clibase/option_test.go +++ b/cli/clibase/option_test.go @@ -1,11 +1,17 @@ package clibase_test import ( + "bytes" + "encoding/json" + "regexp" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/clibase" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/codersdk" ) func TestOptionSet_ParseFlags(t *testing.T) { @@ -201,3 +207,165 @@ func TestOptionSet_ParseEnv(t *testing.T) { require.EqualValues(t, expected, actual.Value) }) } + +func TestOptionSet_JsonMarshal(t *testing.T) { + t.Parallel() + + // This unit test ensures if the source optionset is missing the option + // and cannot determine the type, it will not panic. The unmarshal will + // succeed with a best effort. + t.Run("MissingSrcOption", func(t *testing.T) { + t.Parallel() + + var str clibase.String = "something" + var arr clibase.StringArray = []string{"foo", "bar"} + opts := clibase.OptionSet{ + clibase.Option{ + Name: "StringOpt", + Value: &str, + }, + clibase.Option{ + Name: "ArrayOpt", + Value: &arr, + }, + } + data, err := json.Marshal(opts) + require.NoError(t, err, "marshal option set") + + tgt := clibase.OptionSet{} + err = json.Unmarshal(data, &tgt) + require.NoError(t, err, "unmarshal option set") + for i := range opts { + compareOptionsExceptValues(t, opts[i], tgt[i]) + require.Empty(t, tgt[i].Value.String(), "unknown value types are empty") + } + }) + + t.Run("RegexCase", func(t *testing.T) { + t.Parallel() + + val := clibase.Regexp(*regexp.MustCompile(".*")) + opts := clibase.OptionSet{ + clibase.Option{ + Name: "Regex", + Value: &val, + Default: ".*", + }, + } + data, err := json.Marshal(opts) + require.NoError(t, err, "marshal option set") + + var foundVal clibase.Regexp + newOpts := clibase.OptionSet{ + clibase.Option{ + Name: "Regex", + Value: &foundVal, + }, + } + err = json.Unmarshal(data, &newOpts) + require.NoError(t, err, "unmarshal option set") + + require.EqualValues(t, opts[0].Value.String(), newOpts[0].Value.String()) + }) + + t.Run("AllValues", func(t *testing.T) { + t.Parallel() + + vals := coderdtest.DeploymentValues(t) + opts := vals.Options() + sources := []clibase.ValueSource{ + clibase.ValueSourceNone, + clibase.ValueSourceFlag, + clibase.ValueSourceEnv, + clibase.ValueSourceYAML, + clibase.ValueSourceDefault, + } + for i := range opts { + opts[i].ValueSource = sources[i%len(sources)] + } + + data, err := json.Marshal(opts) + require.NoError(t, err, "marshal option set") + + newOpts := (&codersdk.DeploymentValues{}).Options() + err = json.Unmarshal(data, &newOpts) + require.NoError(t, err, "unmarshal option set") + + for i := range opts { + exp := opts[i] + found := newOpts[i] + + compareOptionsExceptValues(t, exp, found) + compareValues(t, exp, found) + } + + thirdOpts := (&codersdk.DeploymentValues{}).Options() + data, err = json.Marshal(newOpts) + require.NoError(t, err, "marshal option set") + + err = json.Unmarshal(data, &thirdOpts) + require.NoError(t, err, "unmarshal option set") + // Compare to the original opts again + for i := range opts { + exp := opts[i] + found := thirdOpts[i] + + compareOptionsExceptValues(t, exp, found) + compareValues(t, exp, found) + } + }) +} + +func compareOptionsExceptValues(t *testing.T, exp, found clibase.Option) { + t.Helper() + + require.Equalf(t, exp.Name, found.Name, "option name %q", exp.Name) + require.Equalf(t, exp.Description, found.Description, "option description %q", exp.Name) + require.Equalf(t, exp.Required, found.Required, "option required %q", exp.Name) + require.Equalf(t, exp.Flag, found.Flag, "option flag %q", exp.Name) + require.Equalf(t, exp.FlagShorthand, found.FlagShorthand, "option flag shorthand %q", exp.Name) + require.Equalf(t, exp.Env, found.Env, "option env %q", exp.Name) + require.Equalf(t, exp.YAML, found.YAML, "option yaml %q", exp.Name) + require.Equalf(t, exp.Default, found.Default, "option default %q", exp.Name) + require.Equalf(t, exp.ValueSource, found.ValueSource, "option value source %q", exp.Name) + require.Equalf(t, exp.Hidden, found.Hidden, "option hidden %q", exp.Name) + require.Equalf(t, exp.Annotations, found.Annotations, "option annotations %q", exp.Name) + require.Equalf(t, exp.Group, found.Group, "option group %q", exp.Name) + // UseInstead is the same comparison problem, just check the length + require.Equalf(t, len(exp.UseInstead), len(found.UseInstead), "option use instead %q", exp.Name) +} + +func compareValues(t *testing.T, exp, found clibase.Option) { + t.Helper() + + if (exp.Value == nil || found.Value == nil) || (exp.Value.String() != found.Value.String() && found.Value.String() == "") { + // If the string values are different, this can be a "nil" issue. + // So only run this case if the found string is the empty string. + // We use MarshalYAML for struct strings, and it will return an + // empty string '""' for nil slices/maps/etc. + // So use json to compare. + + expJSON, err := json.Marshal(exp.Value) + require.NoError(t, err, "marshal") + foundJSON, err := json.Marshal(found.Value) + require.NoError(t, err, "marshal") + + expJSON = normalizeJSON(expJSON) + foundJSON = normalizeJSON(foundJSON) + assert.Equalf(t, string(expJSON), string(foundJSON), "option value %q", exp.Name) + } else { + assert.Equal(t, + exp.Value.String(), + found.Value.String(), + "option value %q", exp.Name) + } +} + +// normalizeJSON handles the fact that an empty map/slice is not the same +// as a nil empty/slice. For our purposes, they are the same. +func normalizeJSON(data []byte) []byte { + if bytes.Equal(data, []byte("[]")) || bytes.Equal(data, []byte("{}")) { + return []byte("null") + } + return data +} diff --git a/cli/clibase/values.go b/cli/clibase/values.go index 6ec67d2d1bc09..d390fe2f89bc6 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -430,6 +430,35 @@ func (discardValue) Type() string { return "discard" } +func (discardValue) UnmarshalJSON([]byte) error { + return nil +} + +// jsonValue is intentionally not exported. It is just used to store the raw JSON +// data for a value to defer it's unmarshal. It implements the pflag.Value to be +// usable in an Option. +type jsonValue json.RawMessage + +func (jsonValue) Set(string) error { + return xerrors.Errorf("json value is read-only") +} + +func (jsonValue) String() string { + return "" +} + +func (jsonValue) Type() string { + return "json" +} + +func (j *jsonValue) UnmarshalJSON(data []byte) error { + if j == nil { + return xerrors.New("json.RawMessage: UnmarshalJSON on nil pointer") + } + *j = append((*j)[0:0], data...) + return nil +} + var _ pflag.Value = (*Enum)(nil) type Enum struct { @@ -464,6 +493,25 @@ func (e *Enum) String() string { type Regexp regexp.Regexp +func (r *Regexp) MarshalJSON() ([]byte, error) { + return json.Marshal(r.String()) +} + +func (r *Regexp) UnmarshalJSON(data []byte) error { + var source string + err := json.Unmarshal(data, &source) + if err != nil { + return err + } + + exp, err := regexp.Compile(source) + if err != nil { + return xerrors.Errorf("invalid regex expression: %w", err) + } + *r = Regexp(*exp) + return nil +} + func (r *Regexp) MarshalYAML() (interface{}, error) { return yaml.Node{ Kind: yaml.ScalarNode, diff --git a/cli/clibase/yaml.go b/cli/clibase/yaml.go index fbb1529b3bf4d..9bb1763571eb4 100644 --- a/cli/clibase/yaml.go +++ b/cli/clibase/yaml.go @@ -51,12 +51,12 @@ func deepMapNode(n *yaml.Node, path []string, headComment string) *yaml.Node { // the stack. // // It is isomorphic with FromYAML. -func (s *OptionSet) MarshalYAML() (any, error) { +func (optSet *OptionSet) MarshalYAML() (any, error) { root := yaml.Node{ Kind: yaml.MappingNode, } - for _, opt := range *s { + for _, opt := range *optSet { if opt.YAML == "" { continue } @@ -222,7 +222,7 @@ func (o *Option) setFromYAMLNode(n *yaml.Node) error { // UnmarshalYAML converts the given YAML node into the option set. // It is isomorphic with ToYAML. -func (s *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error { +func (optSet *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error { // The rootNode will be a DocumentNode if it's read from a file. We do // not support multiple documents in a single file. if rootNode.Kind == yaml.DocumentNode { @@ -240,8 +240,8 @@ func (s *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error { matchedNodes := make(map[string]*yaml.Node, len(yamlNodes)) var merr error - for i := range *s { - opt := &(*s)[i] + for i := range *optSet { + opt := &(*optSet)[i] if opt.YAML == "" { continue } diff --git a/cli/server.go b/cli/server.go index 45d8d7dd471bd..37470bc283271 100644 --- a/cli/server.go +++ b/cli/server.go @@ -617,6 +617,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. MetricsCacheRefreshInterval: vals.MetricsCacheRefreshInterval.Value(), AgentStatsRefreshInterval: vals.AgentStatRefreshInterval.Value(), DeploymentValues: vals, + // Do not pass secret values to DeploymentOptions. All values should be read from + // the DeploymentValues instead, this just serves to indicate the source of each + // option. This is just defensive to prevent accidentally leaking. + DeploymentOptions: codersdk.DeploymentOptionsWithoutSecrets(opts), PrometheusRegistry: prometheus.NewRegistry(), APIRateLimit: int(vals.RateLimit.API.Value()), LoginRateLimit: loginRateLimit, diff --git a/cli/server_test.go b/cli/server_test.go index 49889d77c9540..dbbff56d9ac3d 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -1188,6 +1188,22 @@ func TestServer(t *testing.T) { require.Equal(t, map[string]string{"serious_business_unit": "serious_business_unit"}, deploymentConfig.Values.OIDC.GroupMapping.Value) require.Equal(t, "Sign In With Coder", deploymentConfig.Values.OIDC.SignInText.Value()) require.Equal(t, "https://example.com/icon.png", deploymentConfig.Values.OIDC.IconURL.Value().String()) + + // Verify the option values + for _, opt := range deploymentConfig.Options { + switch opt.Flag { + case "access-url": + require.Equal(t, "http://example.com", opt.Value.String()) + case "oidc-icon-url": + require.Equal(t, "https://example.com/icon.png", opt.Value.String()) + case "oidc-sign-in-text": + require.Equal(t, "Sign In With Coder", opt.Value.String()) + case "redirect-to-access-url": + require.Equal(t, "false", opt.Value.String()) + case "derp-server-region-id": + require.Equal(t, "999", opt.Value.String()) + } + } }) }) @@ -1488,6 +1504,9 @@ func TestServer(t *testing.T) { gotConfig.Options.ByName("Config Path").Value.Set("") // We check the options individually for better error messages. for i := range wantConfig.Options { + // ValueSource is not going to be correct on the `want`, so just + // match that field. + wantConfig.Options[i].ValueSource = gotConfig.Options[i].ValueSource assert.Equal( t, wantConfig.Options[i], gotConfig.Options[i], diff --git a/coderd/coderd.go b/coderd/coderd.go index 24f0b38f08dc9..097ed1e6ff6ba 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -40,6 +40,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/awsidentity" "github.com/coder/coder/v2/coderd/batchstats" @@ -152,7 +153,12 @@ type Options struct { MetricsCacheRefreshInterval time.Duration AgentStatsRefreshInterval time.Duration DeploymentValues *codersdk.DeploymentValues - UpdateCheckOptions *updatecheck.Options // Set non-nil to enable update checking. + // DeploymentOptions do contain the copy of DeploymentValues, and contain + // contextual information about how the values were set. + // Do not use DeploymentOptions to retrieve values, use DeploymentValues instead. + // All secrets values are stripped. + DeploymentOptions clibase.OptionSet + UpdateCheckOptions *updatecheck.Options // Set non-nil to enable update checking. // SSHConfig is the response clients use to configure config-ssh locally. SSHConfig codersdk.SSHConfigResponse diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index c91481d0d3a5c..e876480cde618 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -417,6 +417,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval, AgentStatsRefreshInterval: options.AgentStatsRefreshInterval, DeploymentValues: options.DeploymentValues, + DeploymentOptions: codersdk.DeploymentOptionsWithoutSecrets(options.DeploymentValues.Options()), UpdateCheckOptions: options.UpdateCheckOptions, SwaggerEndpoint: options.SwaggerEndpoint, AppSecurityKey: AppSecurityKey, diff --git a/coderd/deployment.go b/coderd/deployment.go index 255f9c7ac2a8d..ebea5625583cd 100644 --- a/coderd/deployment.go +++ b/coderd/deployment.go @@ -33,7 +33,7 @@ func (api *API) deploymentValues(rw http.ResponseWriter, r *http.Request) { r.Context(), rw, http.StatusOK, codersdk.DeploymentConfig{ Values: values, - Options: values.Options(), + Options: api.DeploymentOptions, }, ) } diff --git a/codersdk/deployment.go b/codersdk/deployment.go index a13b4a1379fd6..be629236d344b 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1762,6 +1762,19 @@ type LinkConfig struct { Icon string `json:"icon" yaml:"icon"` } +// DeploymentOptionsWithoutSecrets returns a copy of the OptionSet with secret values omitted. +func DeploymentOptionsWithoutSecrets(set clibase.OptionSet) clibase.OptionSet { + cpy := make(clibase.OptionSet, 0, len(set)) + for _, opt := range set { + cpyOpt := opt + if IsSecretDeploymentOption(cpyOpt) { + cpyOpt.Value = nil + } + cpy = append(cpy, cpyOpt) + } + return cpy +} + // WithoutSecrets returns a copy of the config without secret values. func (c *DeploymentValues) WithoutSecrets() (*DeploymentValues, error) { var ff DeploymentValues diff --git a/site/src/components/DeploySettingsLayout/Option.tsx b/site/src/components/DeploySettingsLayout/Option.tsx index 427f8cf5632d2..4d9461b7fb415 100644 --- a/site/src/components/DeploySettingsLayout/Option.tsx +++ b/site/src/components/DeploySettingsLayout/Option.tsx @@ -3,6 +3,7 @@ import { PropsWithChildren, FC } from "react"; import { MONOSPACE_FONT_FAMILY } from "theme/constants"; import { DisabledBadge, EnabledBadge } from "./Badges"; import Box, { BoxProps } from "@mui/material/Box"; +import { combineClasses } from "utils/combineClasses"; export const OptionName: FC = ({ children }) => { const styles = useStyles(); @@ -58,21 +59,34 @@ export const OptionValue: FC<{ children?: unknown }> = ({ children }) => { return {JSON.stringify(children)}; }; -export const OptionConfig = (props: BoxProps) => { +// OptionalConfig takes a source bool to indicate if the Option is the source of the configured value. +export const OptionConfig = ({ + source, + ...boxProps +}: { source?: boolean } & BoxProps) => { return ( theme.palette.background.paperLight, + backgroundColor: (theme) => + source + ? theme.palette.primary.dark + : theme.palette.background.paperLight, display: "inline-flex", alignItems: "center", borderRadius: 0.25, padding: (theme) => theme.spacing(0, 1), - border: (theme) => `1px solid ${theme.palette.divider}`, - ...props.sx, + border: (theme) => + `1px solid ${ + source ? theme.palette.primary.main : theme.palette.divider + }`, + "& .option-config-flag": { + backgroundColor: source ? "rgba(0, 0, 0, 0.7)" : undefined, + }, + ...boxProps.sx, }} /> ); @@ -82,6 +96,7 @@ export const OptionConfigFlag = (props: BoxProps) => { return ( {option.flag && ( - + CLI --{option.flag} )} {option.flag_shorthand && ( - + CLI- {option.flag_shorthand} )} {option.env && ( - + ENV {option.env} )} {option.yaml && ( - + YAML {option.yaml}