Fix validation of toggling batch ensembling

[ashwinath]

Issue:
Setting BatchEnsemblingConfig.Enabled = false prevents the application from starting up because it will fail the validation. Also, when we disable batch ensembling, there will some nil pointer errors if we try to instantiate EnsemblingJobService, to prevent this, we disable the endpoints for batch ensembling if we disable batch ensembling altogether.

There is one issue with the validator and I have put it as a comment in the code.

Changes made:

1. Updated the validator to use the required_if tag.
2. Disable batch ensembling endpoints when the feature is turned off via the config.

[krithika369]

Shouldn't these be validate:"required,required_if=Enabled true", i.e., each of these 3 values are required if BatchEnsemblingConfig.Enabled=true ?

validate:"omitempty,required_if=Enabled false" does nothing (i.e., any value is acceptable when Enabled=true and empty value will be ignored when Enabled=false).

... where if it is enabled but all 3 are blank, then throw an error

Not sure I follow the intention. Does this mean it is ok for one or more of the 3 values to be empty but not all 3? I'd assume that if the batch ensembling is enabled, then all 3 should be provided (or that each config is considered optional independently because they have some system defaults). I don't get the part about "all 3 are blank".

[ashwinath]

Shouldn't these be validate:"required,required_if=Enabled true", i.e., each of these 3 values are required if BatchEnsemblingConfig.Enabled=true ?

I realised the mistake should be required_if=Enabled true instead of False, this has no effect. I'll just leave it as omitempty instead.

Unfortunately this suggestion doesn't work, seems like a limitation of the library. So what I want is:
Enabled: true then validate JobConfig + RunnerConfig + ImageBuildingConfig
Enabled: false then don't need to validate JobConfig + RunnerConfig + ImageBuildingConfig, because you shoudn't need to set anything if you disabled the feature.

Why I put omitempty is because if let's say the user did intend to fill up JobConfig, RunnerConfig and ImageBuildingConfig but did it partially, it would throw an validation error.

So on this way of setting things, the logic doesnt work as intended in the following scenarios with playground/validator but is handled in the appcontext initialisation:

c := &BatchEnsemblingConfig {
  // Case 1: batch ensembling disabled but user tried to fill the settings for batch ensembling
  Enabled: false,
  // 1/2 out of 3 of the following are wrong,
  JobConfig: &JobConfig{/* with/without correct settings */},
  RunnerConfig: &RunnerConfig{/* with/without correct settings */},
  ImageBuildingConfig: &ImageBuildingConfig{/* with/without correct settings */},
}

c := &BatchEnsemblingConfig {
  // Case 2: batch ensembling enabled but user tried to fill the settings for batch ensembling (1/2/3 out of 3 of the permutations are wrongly filled in)
  // This case will be checked by the playground/validator
  Enabled: true,
  // 1/2 out of 3 of the following are wrong
  JobConfig: &JobConfig{/* with/without correct settings */},
  RunnerConfig: &RunnerConfig{/* with/without correct settings */},
  ImageBuildingConfig: &ImageBuildingConfig{/* with/without correct settings */},
}

c := &BatchEnsemblingConfig {
  // Case 3: batch ensembling enabled but user left all configuration nil
  // This case will be checked by the nil pointer checked with this comment: https://github.com/gojek/turing/pull/119#discussion_r747952017
  Enabled: true,
  JobConfig: nil,
  RunnerConfig: nil,
  ImageBuildingConfig: nil,
}

Anyway I'll be removing the comments as I think they are confusing. I hope this clears up any confusion.

[ashwinath]

Makes me realise if the required_if=Enabled true is really required. Setting it to true or false doesnt seem to have an effect anyway other than documenting. I'm removing it and leaving it as just omitempty.

[krithika369]

I see.. What about: validate:"-,required_if=Enabled false"

https://pkg.go.dev/github.com/go-playground/validator/v10#hdr-Skip_Field

Edit: Actually, my earlier understanding of required_if is incorrect - it is a standalone tag, it doesn't combine with other tags. For example, in: validate:"omitempty,required_if=Enabled false", omitempty is separate from required_if=Enabled false; it doesn't mean do omitempty if Enabled=false.

Now, after some digging around in the repo and docs, I believe the way to achieve this without a custom Struct validator is using this: https://pkg.go.dev/github.com/go-playground/validator/v10#Validate.StructFiltered

Something similar was done for the experiment engine in Turing (although it also passes in the validation context which may not be needed): https://github.com/gojek/turing/blob/main/engines/experiment/manager/base_manager.go#L50

[ashwinath]

I tried using the method the base_manager used, it doesn't seem to work as intended as well. It works the same way as validate:"required_if=Enabled true".

func (c *Config) Validate() error {
	validate, err := newConfigValidator()
	if err != nil {
		return err
	}
	// Check general flow first
	if err := validate.Struct(c); err != nil {
		fmt.Println("here") // Here it hits when Enabled=false, but other settings filled halfway
		return err
	}

	// Extra rules go here
	return c.validateBatchEnsemblingConfig(validate)
}

func (c *Config) validateBatchEnsemblingConfig(validate *validator.Validate) error {
	if !c.BatchEnsemblingConfig.Enabled {
		return nil
	}

	return validate.StructFiltered(c.BatchEnsemblingConfig, func(ns []byte) bool {
		// Determine the fields for validation
		validateFields := []string{
			"BatchEnsemblingConfig.JobConfig",
			"BatchEnsemblingConfig.RunnerConfig",
			"BatchEnsemblingConfig.ImageBuildingConfig",
		}
		// If the field's fully qualified name starts with the name of any of the chosen fields,
		// do not filter it from validation (return false will pick up the field for validation).
		for _, field := range validateFields {
			if strings.HasPrefix(string(ns), field) {
				return false
			}
		}
		return true
	})
}

This works the same as

type BatchEnsemblingConfig struct {
	Enabled             bool
	JobConfig           *JobConfig           `validate:"required_if=Enabled True"`
	RunnerConfig        *RunnerConfig        `validate:"required_if=Enabled True"`
	ImageBuildingConfig *ImageBuildingConfig `validate:"required_if=Enabled True"`
}

Going further beyond means removing all struct tags and writing our own custom validation, which I'm not sure if it's worth pursuing.

But a slight improvement with this correct required_if tag is that it guarantees that JobConfig, RunnerConfig and ImageBuildingConfig to not be nil when Enabled is true, so actually the nil check is not required anymore. Nonetheless I'll leave the nil check in place.

[krithika369]

Curious: In this comment:

I tried using the method the base_manager used, it doesn't seem to work as intended as well. It works the same way as validate:"required_if=Enabled true"

Did you have your BatchEnsemblingConfig set to:

type BatchEnsemblingConfig struct {
	Enabled             bool
	JobConfig           *JobConfig           `validate:"-"`
	RunnerConfig        *RunnerConfig        `validate:"-"`
	ImageBuildingConfig *ImageBuildingConfig `validate:"-"`
...
}

The "-" asks the validator to skip validation on the nested struct which seems to happen by default.

[ashwinath]

I just tried this, the behaviour is also unintended. It will not try to validate if enabled is set to True.

[ashwinath]

I think the current state has the best behaviour for now (being the safest). The only unintended behaviour as of this state is enabled = false + other configs (JobConfig, RunnerConfig, ImageBuildingConfig) partially set. Which I think is acceptable because it won't be used if Enabled is false anyway but it forces the user to either fix it or remove them.

What is important is that Enabled = true and all 3 configs (JobConfig, RunnerConfig, ImageBuildingConfig) are set.

[krithika369]

I see. Thanks for testing them.

Probably not very useful spending more time on this to tweak it. The current behavior sounds reasonable.

[ashwinath]

Yah I wrote in the test cases, so its pretty fast to run the configuration you asked. 😄

[krithika369]

LGTM