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

The hull.util.transformation.makefullname transformation and and the staticName parameter #273

Open
khmarochos opened this issue Jan 2, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@khmarochos
Copy link

Hello!

May I bother you with another question?

Do I understand it right that hull.util.transformation.makefullname should return the static name of an object in the case when the object's staticName is true? If yes, then are there any exceptions?

Here's set of values I'm giving to Helm:

hull:
  objects:
    serviceaccount:
      my-serviceaccount:
        staticName: true
    service:
      my-service:
        staticName: true
    configmap:
      my-configmap:
        staticName: true
    deployment:
      my-deployment:
        annotations:
          hull-test.local/my-service: _HT^my-service
          hull-test.local/my-serviceaccount: _HT^my-serviceaccount
          hull-test.local/my-configmap: _HT^my-configmap

Here are the annotations I get as the result:

hull-test.local/my-configmap: hull-test-4-hull-test-4-my-configmap
hull-test.local/my-service: hull-test-4-hull-test-4-my-service
hull-test.local/my-serviceaccount: hull-test-4-hull-test-4-my-serviceaccount

I didn't find the answer in the documentation, but I see that hull.util.transformation.makefullname calls hull.metadata.fullname and hull.metadata.fullname, obviously, respects the staticName parameter. But it seems that hull.metadata.fullname doesn't have the whole outlook when it's being called by hull.util.transformation.makefullname. Am I right?

Thank you.

@gre9ory
Copy link
Contributor

gre9ory commented Jan 2, 2024

@khmarochos First, wishing you a happy new year ;)

Short answer is I think you are right, the whole outlook is missing here in the scope of an annotations value. I'll try to explain the concept and give some suggestions at the end on what you may do.


There are several angles to object naming in charts and object referencing. The HULL tutorials also contain a section on the topic with some info, you can find it here.

Object naming:

Main question to ask is whether the particular object instance is only being used inside the helm chart or if it needs to be referred to from some external place outside of the charts scope. You could also think of it as a local or global variable scope in a programing language.

The objects that are created in a chart are in my experience mostly only used in the same chart so have local scope. For these object instances it is definitively best to auto-create a "dynamic" name as per the global naming settings (fullNameOverride and nameOverride). With the dynamic names there is usually no-to-reduced risk of object naming clashes in your cluster.

If you want to create object instances that are important outside of your helm charts scope, it is suitable to add the staticName: true property next to the instance name. Then the name is fixed and the in-cluster object can be refered to globally easily by this exact name and it is not dependent on any helm release name.

Object referencing:

When refering to objects you may have three scenarios to consider:

  1. objects which are created in the same chart with local scope and dynamic naming (default case)
  2. objects which are created in the same chart but have staticName: true set and are intended for global cluster-wide functionality
  3. objects that are created outside of the chart with a global scope

For 1.:

HULL further supports this standard local scope use case by allowing to reference object names via their instance keys only wherever the context is clearly defined, The places where dynamic names are auto-created from key names automatically are those where references to an object are made by its name:

  • horizontalpodautoscaler:
    • the scaleTargetRef.name
  • ingress:
    • the tls.secretName
    • the backend.service.name
  • container:
    • the env.valueFrom.configMapKeyRef
    • the env.valueFrom.secretKeyRef
    • the envFrom.configMapRef
    • the envFrom.secretRef
  • volume:
    • the configMap.name
    • the secret.secretName
    • the persistentVolumeClaim.claimName

It is a convenient shortcut to support standard local scope use case. As an example, your application needs some configuration in a configmap and this local scope configmap volume is briefly created with volume.configMap.name being the key name of the configmap object instance. Any other usecases (2. and 3.) involve the use of the staticName keyword.

For 2.:

An exception to 1. is to assign the staticName: true property to an object instance. In this case the dynamic naming rules are not used and the object instance key equals the objects name. This again really makes only sense when the particular object instance is referenced from outside the Helm charts context and hence serves some more global needs in your system. With the static name you give to it it can be easily referred to in other Helm charts.

If you opt to using a staticName: true for an object instance you must also apply the the staticName:true property next to the special fields above indicating to 'take the name of the referenced object as is' for this particular reference. If leaving out staticName or setting staticName: false the name is rendered with the global naming settings applied which would point to non-existing objects when deployed. Thinking about it, there is possibility here to improve this special-fields scenario since the object instance key and type is known and that would make it possible to lookup a potential staticName: true property on the instance and thus guarantee a correct reference is created always. But that would be a new feature, will think about that ... for now the staticName keyword needs to be applied manually to the special fields, depending on the object instance's staticName setting.

So in the context of objects created within the chart, essentially the staticName: true is a switch to apply manually and is meant to ignore the dynamic naming rules. It should be used with thought and only if the object needs to be referable from outside your chart. Using it essentially means that you can deploy this Helm chart only once in a namespace, otherwise there will be a name clash because you are creating an identically named object twice. If choosing to use it, you need to also manually add staticName: true to the special fields above if they are pointing to a staticName: true instance.

In other places where there is no clear indication that a field references an objects name there is the complementary _HT^ transformation to produce a name that respects the global naming rules for the provided input. Also this needs to be handled with consideration because it is essentially only affecting the rendered output and has no knowledge of the context where it is being used, it is rather stupid and simply takes the input string and creates the dynamic naming prefix in front of it. If you reference an object instance in your chart by name and it has staticName: true set you need to take care to not use _HT^ to avoid an incorrect reference on deployment.

For 3.:

The staticName: true property put next to a special object is also needed to support this use case where you want to reference an object created outside of the helm charts scope by its exact static name. In the other non-special fields you will need to supply the objects full name to reference it. Avoid _HT^ since the chart internal naming prefix would be wrong here when referring objects external to the chart.


For your concrete question and example:

Unlike the list of special fields above, the value of an annotation could be anything. In your example it can't be deduced that you are referering to particular object names. _HT^ is just a shortcut for applying the naming rules and is not aware of any actual object instances itself. However, if it were aware of the object type too (as in the special fields cases) then it would be possible to also consider the staticName property and do a lookup to get the exact instance name which would be a new feature as mentioned. Missing this additional context however, it is not possible to deduce the actual objects name here and moreover taking into account the staticName setting which may be applied to it.

As a suggestion, maybe you can reevaluate if you actually want to have the object instances with staticName: true. Do you need to reference these objects outside of your charts scope?

  • If so then you need to remove the _HT^ from the annotation values and just write the key names and this will match.
  • If you actually don't need staticName: true on your object instances I would advise to remove it in which case your annotations should point to the correct instance names if you leave them as they are.

So it depends on what you want to do with your objects which way to go with the current possibilities.
Hope this is explains it well enough, as always let me know if you need further help.

Last remark:

I can also think of an addition to HULL where you could provide the object type optionally to the _HT^ call. Assuming you would be able to do that, HULL could also lookup actual the object instance and evaluate the staticName prefix on the instance. This way it would work as I think you expected it.

Since staticName usage for objects created by the chart is more of an exception I did not consider this enhanced handling yet. Let me know if you think this is something you would benefit from in your specific scenario.

@khmarochos
Copy link
Author

Thank you for the reply! I will carefully read it a bit later, but right now I'd like to apologise for not wishing you a happy new year too! Alas, I've completely forgotten about the New Year and everything related, as it was a pretty vivid night in Kyiv (~100 rockets came from ruzzians). I sat in the basement with my wife and son, they were sleeping and I was playing with HULL and my project, as my work usually helps me to unwind. :-)

Will come back to you later, when I have more time to carefully read your answer. Again, thank you for your help and for your project!

@gre9ory
Copy link
Contributor

gre9ory commented Jan 3, 2024

@khmarochos

There is absolutely no need to apologize.

You have all my sympathies and I miss the words to say how it makes me feel to know you are in that threatening situation. If fiddling with this project gives you some chance to take the mind of things then this is more than I would have ever hoped to achieve with this. And please know your input and the kind way you bring it foward is appreciated with the absolute highest regard.

Again, I sincerely wish you all the possible best to you and your family in the new year!

@khmarochos
Copy link
Author

Hello again!

Thank you for such a detailed explanation and, of course, for your understanding and wishes. What I wish you is to never deal with ruzzians; they are dishonourable, treacherous and sneaky enemies. Though their atrocities are out of the topic, let us return to much nicer things.

My actual case is actually a bit different than the one I showed before. What I need is to let the user to choose the ServiceAccount which will be used in Pods. The ServiceAccount must be in the global context, as its name is used in the HashiCorp Vault's configuration, so Vault performs authentication and grants access by the ServiceAccount's token. What I'm going to achieve is to let the user of my chart set the ServiceAccount's name. I also would like to let the user decide whether they want the chart to create the ServiceAccount as my-serviceaccount or as <release's name>-<chart's name>-my-account (just in case they need it to be so), that is why my values set looks approximately like as follows:

hull:
  objects:
    serviceaccount:
      _my_serviceaccount:
        metadataNameOverride: _HT*serviceAccount.name
        staticName: _HT?(index . "$").Values.serviceAccount.staticName
    deployment:
      my-deployment:
        pod:
          serviceAccount: _HT^_my_serviceaccount

serviceAccount:
  enabled: true
  name: my-serviceaccount
  staticName: true

I would like to let the user choose any name of the ServiceAccount by overriding serviceAccount.name and set serviceAccount to false (just in case). Alas, this approach doesn't work. Of course, I can solve it by calling a function (a named template), but I was looking for a more elegant way to achieve the result.

What do you think, what would be the best way to achieve the result that I need?

@khmarochos
Copy link
Author

khmarochos commented Jan 5, 2024

...The solution could look somehow like the following:

hull:
  objects:
    serviceaccount:
      _my_serviceaccount:
        metadataNameOverride: _HT*serviceAccount.name
        staticName: _HT?(index . "$").Values.serviceAccount.staticName
    deployment:
      my-deployment:
        pod:
          serviceAccountName: >-
            _HT!
            {{- $parent := (index . "$") -}}
            {{- $service_account := $parent.Values.serviceAccount -}}
            {{-
              ternary
                $service_account.name
                (include "hull.metadata.fullname" (
                  dict
                    "PARENT_CONTEXT" $parent
                    "COMPONENT" $service_account.name
                ))
                $service_account.staticName
            -}}
serviceAccount:
  enabled: true
  name: my-serviceaccount
  staticName: true

Though I don't like it at all, to be honest. :-(

@gre9ory
Copy link
Contributor

gre9ory commented Jan 8, 2024

Hi,

was trying to exactly understand the logic of your usecase but I think I got it now. This is my understanding:

  • You want the user to focus on making changes via these settings:

    serviceAccount:
      enabled: true
      name: my-serviceaccount
      staticName: true
    

    with the option to create the service accounts name "as is" or to give it the chart specific prefix.

  • the service account is always created by your chart either with a "staticName" or with chart-specific name

  • the pods serviceAccountName needs to reflect the serviceAccount.staticName and serviceAccount.name choice

Questions remaining for me:

  • The serviceAccount section needs to be outside of hull: scope because of the Vault relevance? Is vault deployed as a subchart?
    • If yes, I would expect the serviceAccount configuration under either global: or vault: key (depending on how your vault chart would need to be configured) so it actually has relevance for a vault chart.
    • If no and vault is deployed elsewhere, you could also directly have it configured under the hull.objects.serviceaccount directly and wouldn't need to have the proxy config under serviceAccount. However, if the intention is to not stress your users too much with hull configuration and want them to feel more 'classic' helm configuration by providing the proxy section they may already be familiar with I could relate to that too.
  • in the snippet serviceAccount.enabled has no relevance. What is supposed to happen when enabled: false?

So far it looks to me as if you have a little specialized logic, user expectations and outside world dependencies in your scenario which you need to take care of somewhere, I actually find your provided solution with the proxy service account config pretty ingenious and clever if it fits your use case for user configurability. The user has a lightweight (maybe well-known) interface with the serviceAccount: configuration and you provide the invisible glue to the actual technical configuration of HULL. So if my assumptions are correct I find that part of your solution good. But maybe I missed an aspect which makes you not like that part still?

It dawns on me that it is the custom code in the pods serviceAccountName that bothers you, right? That may actually be improved - and that is probably what you meant when looking back at the start of this issue. In the case you are refering to a service account created internally, the determination of the serviceAccountName value could be improved by inspecting the settings of chart internal service accounts. If that is what you are after I can try to think of ways to improve this in a downward compatible way so that maybe the code that is in

          serviceAccountName: >-
            _HT!
            {{- $parent := (index . "$") -}}
            {{- $service_account := $parent.Values.serviceAccount -}}
            {{-
              ternary
                $service_account.name
                (include "hull.metadata.fullname" (
                  dict
                    "PARENT_CONTEXT" $parent
                    "COMPONENT" $service_account.name
                ))
                $service_account.staticName
            -}}

could be removed and expressed with simpler inbuilt means?

@khmarochos
Copy link
Author

Hello!

You've got my intention absolutely right! I'd like to let the user use my HULL-powered charts just the same way they use other charts, to set the parameters they used to use and get exactly what they expect.

So, let's look again at these 3 parameters:

serviceAccount:
  enabled: true
  name: my-serviceaccount
  staticName: true

When serviceAccount.enabled is false, the ServiceAccount object is not being created/maintained by the chart. The point is that there might be a case when such object is already present in the namespace and the user is willing to use that one. So, the hull.object.serviceAccount._my_serviceaccount should be rewritten as follows:

        # added this line to make the object's creation optional:
        enabled: _HT*serviceAccount.enabled
        
        # proxy to `serviceAccount.name` just for users' convenience:
        metadataNameOverride: _HT*serviceAccount.name
        staticName: _HT?(index . "$").Values.serviceAccount.staticName

When serviceAccount.staticName is true, the ServiceAccount that is being created/maintained (if serviceAccount.enabled is true) has the either the static name (by default) or a properly prefixed name. Also, the serviceAccountName parameter of Deployments, Jobs, CronJobs and other pod-wrapping objects will be either statically set to the value of serviceAccount.name just as it is or dynamically prefixed. In most cases the user would like to keep serviceAccount.staticName true, because there is Vault (far outside of the K8S-cluster) and it has certain policies, these policies allow certain service accounts to have access to certain data.

I don't like the idea of adding a hunk of code to the serviceAccountName parameter of all the pod-wrapping objects. If hull.util.transwofmration.makefullname would check if the specific object's staticName is true and would return either a "static" or "prefixed" (in accordance to the staticName parameter's value) name - it would be amazingly handy.

Thank you for your time and attention to this issue!

@gre9ory
Copy link
Contributor

gre9ory commented Jan 8, 2024

To automatically evaluate an object's name you would need to know

  1. the object's type
  2. the object's key

With that information and taking the instances staticName and metadataNameOverride into account you could lookup the actual name. Currently the logic I explained initially revolving around staticName and _HT^ is basically only operations working on nothing more than the strings provided to it, there is no context of actual instances in this. Its usage relies on the assumption that some parts of the chart, especially the parameters for object instance name configuration, do not change - which is of course not always true as your case shows.

When looking at "name" fields that actually reference some other objects, I think it would be smarter to not make the distinction manually by using staticName and _HT^ to influence the printed name of the internal or external object but rather differentiate between internal and external objects directly so that:

  • when external keep value unchanged
  • when internal, lookup the instance and determine concrete name

For references of a clearly defined type, the reference type should be set automatically to the name resolution function. For references that are flexible in type (eg. RBAC context) a possibility would need to exist to pass the reference type to the name resolution function.

Sounds like an overall smarter approach but could be a some work to implement, especially in a downward compatible way ... also any transformations involved in the creation of the referenced objects name would need to be resolved at the point the reference tself is resolved. I am not sure if this is out of scope of what can be done here in HULL yet but I am willing to try something out and do some experiments.

For now to reduce code repetition with existing means could be to try utilizing _HULL_OBJECT_TYPE_DEFAULT_: feature similar to this:

hull:
  objects:
    deployment:
      _HULL_OBJECT_TYPE_DEFAULT_:
        pod:
          serviceAccountName: >-
            _HT!
            {{- $parent := (index . "$") -}}
            {{- $service_account := $parent.Values.serviceAccount -}}
            {{-
              ternary
                $service_account.name
                (include "hull.metadata.fullname" (
                  dict
                    "PARENT_CONTEXT" $parent
                    "COMPONENT" $service_account.name
                ))
                $service_account.staticName
            -}}

If you do this for deployment, job, daemonset, statefulset and (slightly adjusted) for cronjob it should apply the logic for all created pods by default. For even more reduction you could ship the custom funtion code to its own include function and make it a one liner for each object type sth like this:

hull:
  objects:
    deployment:
      _HULL_OBJECT_TYPE_DEFAULT_:
        pod:
          serviceAccountName: _HT/your.naming.function
    job:
      _HULL_OBJECT_TYPE_DEFAULT_:
        pod:
          serviceAccountName: _HT/your.naming.function
    daemonset:
      _HULL_OBJECT_TYPE_DEFAULT_:
        pod:
          serviceAccountName: _HT/your.naming.function
    statefulset:
      _HULL_OBJECT_TYPE_DEFAULT_:
        pod:
          serviceAccountName: _HT/your.naming.function
    cronjob:
      _HULL_OBJECT_TYPE_DEFAULT_:
        job:
          pod:
            serviceAccountName: _HT/your.naming.function

@khmarochos
Copy link
Author

Thank you very much, I didn't think about utilising _HULL_OBJECT_TYPE_DEFAULT_ for that. Will rewrite my configuration according to your suggestion.

It seems that the issue could be considered as closed now. Again, thank you for your help and, of course, for a nice tool.

@gre9ory gre9ory added the enhancement New feature or request label Feb 10, 2024
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

No branches or pull requests

2 participants