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

Provide a way to set labels on images defined by Generators #2885

Closed
davidecavestro opened this issue Apr 8, 2024 · 13 comments · Fixed by #2956
Closed

Provide a way to set labels on images defined by Generators #2885

davidecavestro opened this issue Apr 8, 2024 · 13 comments · Fixed by #2956
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@davidecavestro
Copy link
Contributor

davidecavestro commented Apr 8, 2024

Component

Kubernetes Maven Plugin

Is your enhancement related to a problem? Please describe

I miss a way to set labels on the container image when I build it through generators (instead of the images config). I use the spring-boot generator, but I suppose the issue is common to others.

Describe the solution you'd like

I'd like to add a labels entry to the generator config, such as

...
<generator>
  <config>
    <spring-boot>
      <color>always</color>
      <from>...</from>
      <labels>foo=abc,bar=123</labels><!-- with a CSV or nested elems -->
    </spring-boot>
  </config>
</generator>
...

Probably a comma separated list of values would resemble what already happens for jkube.generator.tags.

Describe alternatives you've considered

Using images tag or dockerfile I would loose the support for automatic detection of layered jars

Additional context

Discussed at #2884

@davidecavestro davidecavestro added the enhancement New feature or request label Apr 8, 2024
@rohanKanojia
Copy link
Member

@davidecavestro : Would it be possible for you to work on this? I'm happy to provide you code pointers if interested.

@davidecavestro
Copy link
Contributor Author

This way it works
image
but I am wondering what's the best way to support dynamic values for the labels
i.e. setting the value from a property or an env variable

@rohanKanojia
Copy link
Member

i.e. setting the value from a property or an env variable

Not 100% sure how to do it. Maybe adding a map/property configuration option in Mojos/ Gradle tasks can help achieve this. But then maybe we would need to align that configuration option with both generators (zero configuration) and explicit image configuration.

@davidecavestro
Copy link
Contributor Author

I see, not sure if filter support could help someway... should investigate.
Another aspect to refine would be the escaping of commas and equals chars.

@manusa
Copy link
Member

manusa commented Apr 9, 2024

i.e. setting the value from a property or an env variable

I think the easiest way right now would be to control that from Maven by computing the value as a property first:

<properties>
  <prop>pre</prop>
  <jkube.generator.labels>${prop}-${env.ENV_PROP}</jkube.generator.labels>
  <!-- ... -->
</properties>

Another aspect to refine would be the escaping of commas and equals chars.

Not sure of how we're doing that for tags now, but both features should follow the same approach for sanitizing inputs.

@davidecavestro
Copy link
Contributor Author

davidecavestro commented Apr 9, 2024

the easiest way right now would be to control that from Maven by computing the value as a property first

so I see the interpolation is done by maven both defining a property and directly using expressions such as <labels>${my.prop}=${env.GIT_COMMIT}</labels>
I expect the same would stand for gradle, but I've not checked

Not sure of how we're doing that for tags now, but both features should follow the same approach for sanitizing inputs

I see no sanitization for tags at addTagsFromConfig

OTOH EnvUtils.splitOnSpaceWithEscape could be replicated into separate methods to extract lists (tags use case) and maps (labels use case) supporting the escape of both comma chars (entries separators) and equal chars (key/value separators).
Both chars could be included into resulting data this way.

That said, it may seem too complicated, as it reduces code readability increasing complexity.
On my own I don't have yet a real-world use case for escaping.

@manusa
Copy link
Member

manusa commented Apr 9, 2024

so I see the interpolation is done by maven both defining a property and directly using expressions such as ${my.prop}=${env.GIT_COMMIT}

Yes, that should work too.

I expect the same would stand for gradle, but I've not checked

For Gradle it should be done differently, but would be achievable too.

What I mean, in general, is that for dynamic values instead of reinventing the wheel downstream, it's better to leverage the tools that the build system offers.

That said, it may seem too complicated, as it reduces code readability increasing complexity.

Maybe in this case (it seems to be far more complex than for tags) it's better to try and see what Docker or other tools are doing to ensure a consistent behavior.

https://docs.docker.com/config/labels-custom-metadata/#label-keys-and-values

@davidecavestro
Copy link
Contributor Author

davidecavestro commented Apr 9, 2024

Please note that the complexity of parsing and escaping separators is clearly related to the choice of setting labels via a CSV (of key value pairs).
IMHO apart from the escape of separators, there's no strong need of specific sanitization (probably enforced by underlying layers if really needed).

Also consider that in case we opt instead for shaping labels with nested config elements, we would end up with something simpler; but this approach would differ from tags one (that's why I started with the former).

That is (for maven)

<labels>foo=abc,bar=123</labels>

VS

<labels>
  <foo>abc</foo>
  <bar>123</bar>
</labels>

@manusa
Copy link
Member

manusa commented Apr 10, 2024

OK, note that we would likely want to support the label setting via configuration elements (regardless of the nesting choice), but also through the use of a property jkube.generator.labels. That's why I was mostly concerned about the parsing and its complexity.

@davidecavestro
Copy link
Contributor Author

davidecavestro commented Apr 10, 2024

also through the use of a property jkube.generator.labels. That's why I was mostly concerned about the parsing and its complexity

makes sense
so given the mandate to support decoding a map from a one-liner - possibly passed from CLI - a certain degree of complexity for parsing seems acceptable to me

@davidecavestro
Copy link
Contributor Author

What about json?

@rohanKanojia
Copy link
Member

rohanKanojia commented Apr 23, 2024

@davidecavestro : Could you please open a pull request with your current changes that is covering the simple scenario? Maybe we can iterate over it to add more complex expressions?

@davidecavestro
Copy link
Contributor Author

After some issues with GPG signature (ssh keys were not an option on this network)
I sketched #2956

I saw some issues on tests and licenses check: if they are not false positives I can look at them

@manusa manusa added this to the 1.17.0 milestone Apr 29, 2024 — with automated-tasks
manusa pushed a commit that referenced this issue Apr 29, 2024
) (2956)

feat: provide a way to set labels on images defined by Generators (#2885)
---
doc: update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants