Skip to content

U/kkasp/tron 2414 fix broken config parse #1045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 8, 2025

Conversation

KaspariK
Copy link
Member

@KaspariK KaspariK commented May 7, 2025

In #1043 I added a config value for the number of items in a write transaction. The previous validation flow (i.e. validate_contents -> post_validation -> set_defaults) caused errors when post_validation attempted to access this optional field when it was omitted in the config.

Since set_defaults hadn't run yet, accessing the key (using .get()) would yield None in the dictionary passed to post_validation, leading to crashes if the logic expected a default value to be present (e.g. checking if not 1 <= max_transact <= 100 would check 1 <= None).

What

This change modifies the core Validator to run set_defaults before post_validation. The new order of validate_contents -> set_defaults -> post_validation prevents these errors by applying defaults before post_validation runs.

Why

Even without the specific config issue I encountered, this new order is more intuitive as post_validation now validates the intended state of the config object, which includes any applicable defaults. It also simplifies future post_validation logic, as we don't need to explicitly handle cases where an optional key with a default might be missing from the input dictionary.

SecretVolumeItem

I needed to handle the case where the items key (which defaults to None) was explicitly set to None by set_defaults before post_validation ran. The previous logic would fail with a TypeError when checking len(None). The fix uses valid_input.get("items") or [] to ensure iteration works.

default_mode

My changes to the Validator broke some tests and in the course of addressing those I noticed that the logic for propagating a SecretVolume's default_mode to items lacking their own mode was flawed. We were calling item._replace() but ignoring the return, meaning default_mode was never actually applied. My fix uses the value from _replace() and reconstructs the items tuple if modifications were needed.

The impact of this is minimal right now, both because it's unused (only defined in one disabled job), and because "If not specified, the volume defaultMode will be used." (from V1SecretVolumeSource.items -> V1KeyToPath). That said, we probably don't want to rely on a fallback, especially when we allow for explicit config.

Example

Tron Job config with secret_volumes like:

secret_volumes:
- container_path: /home/nobody/.ssh
  secret_volume_name: kevin
  secret_name: kevin
  default_mode: '0666'
  items:
  - key: kevin
    path: kevin

The podspec produced:

TestValidMesosAction

This check needs adjustment because set_defaults now ensures keys like docker_image (which defaults to None) are present. This check feels pretty irrelevant these days, so I opted to remove it instead of updating.

@KaspariK KaspariK marked this pull request as ready for review May 7, 2025 18:42
@KaspariK KaspariK requested a review from a team as a code owner May 7, 2025 18:42
@KaspariK KaspariK merged commit b9bdff2 into master May 8, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants