Skip to content
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

[BUG] Parsing YAML for k8s DefaultAnnotations misunderstands "." in annotation keys to mean a nested dict. #6166

Open
2 tasks done
pimdh opened this issue Jan 14, 2025 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@pimdh
Copy link

pimdh commented Jan 14, 2025

Describe the bug

In the FlytePropellor config (docs), there appears to be a bug in how the YAML file is parsed. Dots in keys are misunderstood as nested dicts. This is relevant, because e.g. Google Kubernetes Engine we might want to set an annotation:
cluster-autoscaler.kubernetes.io/safe-to-evict: "false", which includes dots.

Consider the following config

  k8s:
    plugins:
      k8s:
        default-annotations: 
          test.annotation: "true"

This raises the following flytepropellor error:

time="2025-01-14T10:33:00Z" level=error msg="\n\n\n1 error(s) decoding:\n\n* 'default-annotations[test]' expected type 'string', got unconvertible type 'map[string]interface {}', value: 'map[annotation:true]'"

This suggests that the YAML is parsed into {"default-annotations": {"test": {"annotation": "true}}} instead of {"default-annotations": {"test.annotation": "true}}, as we'd want.

Expected behavior

I'd expect the above config to lead to a default annotation test.annotation: "true", but I get a FlytePropellor error.

Additional context to reproduce

Using flyte-core 1.13.3

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@pimdh pimdh added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Jan 14, 2025
@eapolinario eapolinario self-assigned this Jan 16, 2025
@eapolinario eapolinario removed the untriaged This issues has not yet been looked at by the Maintainers label Jan 16, 2025
@eapolinario eapolinario assigned pvditt and unassigned eapolinario Jan 16, 2025
@eapolinario
Copy link
Contributor

eapolinario commented Jan 16, 2025

@pvditt , can you point to the code where we parse this config stanza into a dictionary?

@pvditt
Copy link
Contributor

pvditt commented Jan 16, 2025

@eapolinario there's config parsing logic in flytestdlib - looking through it now

issue is https://github.com/spf13/viper/blob/v1.11.0/viper.go#L2010 (delimiter is ".")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants