spring-cloud / spring-cloud-kubernetes

Kubernetes integration with Spring Cloud Discovery Client, Configuration, etc...
Apache License 2.0
3.46k stars 1.03k forks source link

the fail-fast:true property is not triggered if the config map is not found #1016

Open jbkervyn opened 2 years ago

jbkervyn commented 2 years ago

I think it would be handy (except if there is another way to do this), to make your application fail when no matching configmap can be found in the namespace. I would expect this to be the behavior of fail-fast. However, this only happens when there is an exception while reading the configmap. If the configmap is not found, no exception is triggered.

wind57 commented 2 years ago

you're right, we will not fail the app if the configmap or secrets is not found, but that was the behavior for a very long time. I debated this same issue in some PRs, but the response was always the same - this is expected. Over the time, this grew up on me and I agree. If you think about it, lack of some (or incorrect) property read, should result in a different exception in your business logic. I see this in various other places in spring eco-system too, spring projects rarely fail by default on such things.

In general, having such a property here where it would generate a failure could get hairy fast. You can specify more than one configmap for example, and then what should happen? Allow failure of the app for specific ones? Fail if at least one fails? Have a sensible default for all of them? I don't know the correct answer; but having implemented some of these things, it will get complicated :)

btw, you might be the first user reporting and wanting such a feature. If owners of this project think there is a simple way to do this (and if it's worth it), I could implement it.

wind57 commented 2 years ago

just dropping a note, if we get to this in the future. may be name such a property strict? And treat it just like we treat profileBasedSources?

wind57 commented 2 years ago

I have been thinking about this one over some weekends and this is not trivial at all.

In general, we support two types of searches: by name and by labels. If we think about these two in isolation, then things are not that complicated and we could fail if we can't find what we are looking for. The (big) problem arises when you think about profiles and profiles are a big part of Spring DNA. For example includeProfileSpecificSources is enabled by default, so we will always be looking for profiles. As a matter of fact we add a profile called kubernetes all the time, even if you do not specify one.

As such, if we want to implement this like any other property we have, it could look like : spring.cloud.kubernetes.config.strict and spring.cloud.kubernetes.sources.config. And this is where the first problem arises. Suppose you have a configmap a and you want to say : "if this one is not found - fail", via spring.cloud.kubernetes.config.strict=true. You define such a configmap, but your application will still fail, because a-kubernetes is not present (remember that kubernetes is a profile that we add and includeProfileSpecificSources is on by default). Of course you could set includeProfileSpecificSources=false, but this is not going to solve the problem for users that don't want that.

You could then say, how about:

spring:
  cloud:
    kubernetes:
      config:
         strict: true
      sources:
        - name: a
        - name: a-kubernetes
           strict: false

Well, in such a case, we will have 4 configmaps to read in the end: a, a-kubernetes (a + profile), a-kubernetes (the one defined) and a-kubernetes-kubernetes. We can't tell apart if a-kubernetes-kubernetes is something you defined or something coming from a profile concatenation. We also can't make the assumption that a-kubernetes is actually only a + profile (so no need for a-kubernetes-kubernetes) : that would be guessing from our side, and I am not a fan of this.

We could solve this via:

spring:
  cloud:
    kubernetes:
      config:
         strict: true
      sources:
        - name: a
        - name: a-kubernetes
           strict: false
           include-profile-specific-sources: false

But what if there are 10 profiles total? We would need to repeat:

- name: a-<PROFILE_NAME>
   strict: false
   include-profile-specific-sources: false

10 times in the configuration file. This is ugly, at best and it also couples the profiles way too much to the configuration.

It becomes obvious that spring.cloud.kubernetes.config.strict=true is not really an option.


What if we will support only source based strictness? For example:

spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: true

The question still remains, how to treat includeProfileSpecificSources? We could introduce one more setting: profile-based-strictness:

spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: true
                  profile-based-strictness: false

In such a case, only a configmap presence will be enforced. All other a-<PROFILE_NAME> will be ignored. But then we will have the reverse:

spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: true
                  profile-based-strictness: true

So enforce strict for all profiles? Let's say you have a profile dev, do we enforce a-kubernetes and a-dev presence? We could manage this, if users would want a single profile all the time, but what if you are working from local env, but there are two possible databases you can connect to:

@Profile({"dev", "us-west"}) and @Profile({"dev", "us-east"}) are both an option.

It can get even more hairy: what if we treat kubernetes profile separately, since most users don't care about it, but treat other profiles via a specific property.

spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: true
                  profile-based-strictness: true
                  kubernetes-profile-strictness: false

In such a case, we would validate that:

I don't know why but this looks heavily involved, to me.

wind57 commented 2 years ago

I was thinking into a slightly different direction here: what if define profiles that you want to have strictness for? For example:

spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: false  # this is the default option
                  profiles:
                    - dev
                    - us-west
spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: true

it will validate the presence of a, but not enforce a-dev and a-us-west, a-kubernetes (when dev, us-west and kubernetes are active profiles), since you have not provided explicit profiles. These 3 : a-dev, a-us-west and a-kubernetes will still be tried, since nclude-profile-specific-sources is enabled by default.


While I am inclined towards this one, the issue comes that we already have a property called include-profile-specific-sources. We can leave it as-is, but profiles will narrow it. So if you say:

spring:
   cloud:
      kubernetes:
         config:
            sources:
               - name : a
                  strict: true
                  include-profile-specific-sources: true
                  profiles: 
                      - dev

and there are two active profiles dev and kubernetes, then a-dev will be enforced and a-kubernetes will not.

I don't mind trying to implement such an approach, but I need @ryanjbaxter thoughts here and yours @jbkervyn too, if you have the time.

Thank you.

wind57 commented 2 years ago

note for myself: I started working on this one, and I'll fix one more issue along the way. I've created this fix that makes sure that configmap-a is read before configmap-a-dev for example. If there are two profile based configmaps on the other hand:

we do make any distinction (order wise) between configmap-a-dev and configmap-a-prod. i.e.: you do not know in what order your properties will come from.

What I will be fixing also is being in par with profiles application.yaml according to Spring documentation : last one will win.

jbkervyn commented 2 years ago

I really like your ideas @wind57 . However, what I had in mind was a more simple use-case not going that much into depth with the profiles that you are rightfully pointing out.

What I was thinking is : as long as you do have a configmap mentionned, there should be at least one hit. So if we do take this strict idea that I do like, it would mean : as long as strict = true, the validation would only succeed if a configmap having value either a, a-dev, a-us-west. It would fail if no configmap matching a-* would be found.

Just to ellaborate on the usecase I was initially having when I raised this issue : Simple spring boot app, having no spring.cloud.kubernetes.config.sources property explicitely set. And by the presense of the depencency, would look for a configmap matching the application.name. In this use-case, I thought that it would make sense to have a failing validation when no configmap would be found. That's why I thought having a at-least logic would make sense.

But I must confess that I do not possess a decent knowledge on the topic, so I'm just sharing, but I do value more your expertise over mine.

Thanks a lot for putting so many thoughts into this :)

wind57 commented 2 years ago

No worries at all, I liked the idea and I do admit I thought about such a failure of the app when support was added for fail-fast. While I agree with you, think about someone who has a @Profile("dev") (even better if you think they he/she starts the app with an environment property for those profiles) and strict=true. his/her thoughts are going to go to : "if I have this profile, then there should be a way for me to enforce presence of such a source".

imho, what I propose is nothing more then your proposal + some more use-cases, thus more configurations possible, thus more users that are happy.

To your case btw, simply strict=true will suffice indeed and this will be supported. I am almost done with the implementation btw, but it will take a while to properly test this out. thank you for your feedback.

jbkervyn commented 2 years ago

I completely agree with your proposal, I was just thinking, maybe we are making it now a bit too complex/flexible. But I eventually think you're right and that this is the way to go.

ryanjbaxter commented 2 years ago

I am kind of lost from the beginning here, the application should fail to start if fail-fast is set to true

If you want your application to fail the start-up process in such cases, you can set spring.cloud.kubernetes.config.fail-fast=true to make the application start-up fail with an Exception.

wind57 commented 2 years ago

fail-fast has only an effect when there is an Exception. To put it simpler:

try {
    // try to read a configmap
catch(Exception any) {
    if (failFast) {
          throw ....
    }
    else {
       // log.error(...)
    }
}

Currently, there is no way to fail for a missing source.

ryanjbaxter commented 2 years ago

OK now it makes more sense. The issue here is whether to fail if the configmap is missing completely.

When I think about this, its just not something we do for the most part. As @wind57 points out, in a simple Spring app if there is no profile specific configuration file the app does not fail to start.

Now there is one case where I can think of where we would fail if a configuration file does not exist and this is when using spring.config.import. If for example you say spring.config.import=file:///this/does/not/exist.yaml the application will fail to start I believe. However if you say spring.config.import=optional:file://this/does/not/exist.yaml it will start just fine.

Now we do support spring.config.import in our main branch but right now the application will still start just fine if the configmap does not exist and thats because the PropertySourceLocator does not throw any kind of exception.

It feels like maybe using spring.config.import already has this concept, but im unsure how we should properly leverage it at the moment.

wind57 commented 2 years ago

I have tried to have a POC here, it's still not finished. The idea of how I was envisioning this is described above in my comments with strict and strict-for-profiles for example.

I don't know what you think about it Ryan. If it's a direction you want to pursue, I would bring that to a stable state.

ryanjbaxter commented 2 years ago

Honestly it seems overly complicated, there are just too many variations.

I also think that if the application is missing a property it requires than it should fail, the source of where that property comes from should not really matter, its the value that matters at the end of the day.

wind57 commented 2 years ago

I debated this with myself for quite a while. I had the same thought that the app should fail elsewhere, if you need a property.

Then I decided - what if I try to implement it, how complicated can it get? The response is very, so much that I had a very hard time with my own thoughts.

At this point in time, I am convinced that its not worth. Ill close my PR for the time being.

jbkervyn commented 2 years ago

Hi @wind57, thanks for putting so many efforts into this. I can quiet relate to your state of mind when you write that you had a hard time with your own thoughts :) I do agree with @ryanjbaxter as well that this got overly complicated, but in my eyes, i find it illogical (in the context of a spring boot app), to have an application starting without any warnings if no configmaps are found while the presence of the spring-cloud-kubernetes dependency clearly indicates the intention of using configmaps.

The same parallelism can be found with : spring boot fails to start when you declare spring-data-jpa without having any jdbc driver in your classpath.

It's for that reason that I raised this issue. Fail/Warn during startup if not a single configmap is found :) But as I wrote before, I'm not knowledgeable on all the implications, so I do trust your judgements over mine :)

ryanjbaxter commented 2 years ago

if no configmaps are found while the presence of the spring-cloud-kubernetes dependency clearly indicates the intention of using configmaps

That is not true, you can use spring-cloud-kubernetes for a variety of things

The same parallelism can be found with : spring boot fails to start when you declare spring-data-jpa without having any jdbc driver in your classpath.

To me this is a bit different. This is a dependency that needs to be present in order to function. The equivalent in Spring Cloud Kubernetes is if you tried to use it without Fabric8 or the Kubernetes Java Client on the classpath.

I marked the issue as waiting for votes to see if other community members think they want this functionality.

dmitrypotenko commented 2 years ago

I would vote for failing if configmap is not found with fail-fast property. The real case is that configmap has been deleted and application context is refreshed via actuator /refresh endpoint. In this situation the application keeps running and keeps failing for every event or request because the bean with missing property cannot be created. Maybe I am missing an option to force the application to shutdown immediately if property is not found after refresh has happened?

ryanjbaxter commented 2 years ago

Even if we shut down the application because the config map was deleted wouldn't K8S keep trying to redeploy the pod and then run into the same problem?

dmitrypotenko commented 2 years ago

No, the difference is that @RefreshScope/@ConfigurationProperties beans are recreated lazily when these beans are actually used in code and, on the other hand, when application is starting it creates them on startup and shuts down if it can't create them.

ryanjbaxter commented 2 years ago

Right so if we provided an option to shut the application down if the config map is not found, Kubernetes will just restart it because it believes it should be running, and it will fail at startup again. I guess I don't see how this is helping with the issue.

dmitrypotenko commented 2 years ago

What if the bean with a property from k8s config is recreated lazily after some irreversible actions like sending an email? In this case fail fast option would prevent any attempts to process any requests.

wind57 commented 2 weeks ago

In the light of some work that I am doing right now, I think that this one is doable in a simple manner. I will present my ideas soon.