keikoproj/instance-manager

Required selectors in configs

kevdowney opened this issue · 4 comments

kevdowney commented

Is this a BUG REPORT or FEATURE REQUEST?:
FEATURE

What happened:
We would like to treat certain conditionals as a specific rules match when the annotations are required.

Currently the conditionals are pure overrides and seem to work as a cascading config override so the last matching selector config wins but having a selector can be required in certain instances like architecture.

Currently this is how we have to specify architecture support but this does not enforce required selector to match.

 conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: {{ .ArmImage }}

What you expected to happen:
This could be done via a required annotation required.instancemgr.keikoproj.io/arch = arm64 or required conditionals section so any rules miss would result in an error condition.

 required-conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: {{ .ArmImage }}
  • Enforce certain annotations are required.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

  • Kubernetes version:
kubectl version -o yaml

Other debugging information (if applicable):

  • InstanceGroup status:
kubectl describe instancegroup <ig-name>
  • controller logs:
kubectl logs <instance-manager pod>
eytan-avisror commented

Maybe a cleaner API would be

conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
>    required: true
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: {{ .ArmImage }}

But if I understand the ask correctly, it would mean if it's REQUIRED and the annotations are matching the selector, then we DO NOT allow the global default to be used as fallback.

In the case of image this would mean a failure, but it might not mean a failure in cases of other specs such as ones that have default values or ones that are not required fields.

@backjo any thoughts on this?

backjo commented

I'm not sure I fully understand the use case - if I have it right, the intention is to enforce the existence of annotations on IGs dynamically through a rule mechanism - is that correct?

kevdowney commented
eytan-avisror commented

@kevdowney
I think this is more of a client problem.
What you want is a condition where if {{ .ArmImage }} is missing / blank, there is no fallback on global defaults (fail instead, due to missing AMI).

I think this is the case currently, meaning if you try to use the below, an annotated IG will end up in failed state:

conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: ""

But I think the problem is if you REMOVE the conditional entirely, then you end up with a situation that annotated IGs gets the global default - which could be an incompatible AMI.

TBH, not sure we can/should support this use case, or I don't see how it's possible.
If the conditional is gone, there is no correlation to the annotations any longer, how can the controller know they mean anything?

The only possible change I see, is to add some sort of controller logic for the annotation instancemgr.keikoproj.io/arch = arm64 and possibly try to check if the image provided is in the correct architecture (if the API exposes this).

Otherwise I don't see how we can achieve what you're asking.

  • 👍1