open-feature / open-feature-operator

A Kubernetes feature flag operator
https://openfeature.dev
Apache License 2.0
177 stars 34 forks source link

[Feature] Customise flagD port via custom resource #63

Closed AlexsJones closed 1 year ago

AlexsJones commented 2 years ago

Often the flagD port will conflict with metrics or operator endpoints. It is optimal to be able to inject a dynamic port when it is set in the CR..

e.g.

apiVersion: core.openfeature.dev/v1alpha1
kind: FeatureFlagConfiguration
metadata:
  name: watchman-flags
  namespace: watchman-system
spec:
  flagD:
    port: 8085
  featureFlagSpec: |
    {
      "flags": {
        "blocking": {
          "state": "ENABLED",
          "variants": {
            "on": true,
            "off": false
          },
          "defaultVariant": "on"
        }
      }
    }
AlexsJones commented 2 years ago

https://github.com/open-feature/open-feature-operator/pull/64

beeme1mr commented 2 years ago

I'm wondering if this would be a good opportunity to have the operator set environment variables. In this case, the pod may set something like FLAGD_PORT. The SDKs and flagD would be able to use this environment variable to use the correct port without any additional configuration.

If you think the approach, we should think about formally defining an environment variable prefix (i.e. FLAGD) and create issues for other configurations options (i.e. gRPC, HTTP).

AlexsJones commented 2 years ago

We could also add those kinds of capabilities absolutely, I don't have a use case right now. Also in the example of the port we can set that as an environmental inside of flagD if that helps to populate the providers.. I suppose we need to think about integrating a provider to know what are the gaps.

skyerus commented 2 years ago

Desired outcome Inject environment variables into flagd & application containers from a single source (flagd now supports environment variables).

Suggested solution Create a config map with environment variables as data:

apiVersion: v1
kind: ConfigMap
metadata:
  name: environment
data:
  FLAGD_PORT: "8013"
  FLAGD_SERVICE_PROVIDER: "http"
  FLAGD_SYNC_PROVIDER: "filepath"
  FLAGD_EVALUATOR: "json"
  FLAGD_URI: "/etc/flagd/config.json"

Reference this config map in the app deployment's container

    spec:
      containers:
        - name: open-feature-demo
          image: ghcr.io/open-feature/open-feature-demo:latest
          envFrom:
            - configMapRef:
                name: environment

Inject the same envFrom into the flagd sidecar.

Any concerns with this approach? @beeme1mr @toddbaert @AlexsJones @thschue @james-milligan

If not I'll proceed with it.

toddbaert commented 2 years ago

@skyerus I was thinking the mutating admission webhook would parse the flagd settings spec (as it already exists) and then set these vars on both the workload container and the flagd container. Is that what you mean? Users would never mention FLAGD_PORT themselves explicitly in any configmap.

skyerus commented 2 years ago

@toddbaert where does the flagd settings spec already exist? Is the workload container the application (consumer of the sdk) container? The user would only have to reference the already existing config map in their container spec (as shown in the example above). I’m not sure if we’re thinking of the same approach here.

beeme1mr commented 2 years ago

I think the flow may look something like this:

  1. Create a CRD for the feature flag. It may look something like this:
apiVersion: core.openfeature.dev/v1alpha1
kind: FeatureFlagConfiguration
metadata:
  name: watchman-flags
  namespace: watchman-system
spec:
  flagD:
    port: 8085
  featureFlagSpec: |
    {
      "flags": {
        "blocking": {
          "state": "ENABLED",
          "variants": {
            "on": true,
            "off": false
          },
          "defaultVariant": "on"
        }
      }
    }
  1. The controller would create a ConfigMap using the flagD configurations.
apiVersion: v1
kind: ConfigMap
metadata:
  name: watchman-flags-settings
data:
  FLAGD_PORT: 8085
  1. The mutating webhook would automatically set the watchman-flags-settings ConfigMap on flagD and the pod with the flagD annotations.
skyerus commented 2 years ago

@beeme1mr Where do the “flagd configurations” mentioned in step 2 come from?

My described approach is similar except the flagd configurations exist in ConfigMap form and the mutating webhook only sets the ConfigMap on flagd. Will amend my approach to also set the ConfigMap on the pod with flagd annotations.

AlexsJones commented 2 years ago
AlexsJones commented 2 years ago

I think the flow may look something like this:

  1. Create a CRD for the feature flag. It may look something like this:
apiVersion: core.openfeature.dev/v1alpha1
kind: FeatureFlagConfiguration
metadata:
  name: watchman-flags
  namespace: watchman-system
spec:
  flagD:
    port: 8085
  featureFlagSpec: |
    {
      "flags": {
        "blocking": {
          "state": "ENABLED",
          "variants": {
            "on": true,
            "off": false
          },
          "defaultVariant": "on"
        }
      }
    }
  1. The controller would create a ConfigMap using the flagD configurations.
apiVersion: v1
kind: ConfigMap
metadata:
  name: watchman-flags-settings
data:
  FLAGD_PORT: 8085
  1. The mutating webhook would automatically set the watchman-flags-settings ConfigMap on flagD and the pod with the flagD annotations.

I prefer to be explicit not have some magic in there. This follows the podspec pattern -> e.g.

apiVersion: core.openfeature.dev/v1alpha1
kind: FeatureFlagConfiguration
metadata:
  name: watchman-flags
  namespace: watchman-system
spec:
  flagD:
    port: 8085
    env:
    - name: foo
      value: bar
  featureFlagSpec: |
    {
      "flags": {
        "blocking": {
          "state": "ENABLED",
          "variants": {
            "on": true,
            "off": false
          },
          "defaultVariant": "on"
        }
      }
    }
beeme1mr commented 2 years ago

@AlexsJones We're trying to figure out the best way to set flagD configurations as environment variables. The FeatureFlagConfiguration contains spec.flagD.port, we now need to set that as the environment variable FLAGD_PORT on both flagD and the pod with the openfeature annotations.

I'm not sure if my proposal is the best way to do it. What did you have in mind?

beeme1mr commented 2 years ago

My described approach is similar except the flagd configurations exist in ConfigMap form and the mutating webhook only sets the ConfigMap on flagd. Will amend my approach to also set the ConfigMap on the pod with flagd annotations.

Got it, thanks. In that scenario, would spec.flagD.port be removed from the flag configuration since it's not used?

toddbaert commented 2 years ago

The flagd spec is defined in OFO here.

My understanding was that the operator would read the spec (as it does already) and in the mutating admission webhook it would set the associated env vars in both the flagd container and the workload container based on what it reads in the CR instance, instead of just passing them as args to flagd, to ensure they are consistent. I'm open to other options though!

AlexsJones commented 2 years ago

@AlexsJones We're trying to figure out the best way to set flagD configurations as environment variables. The FeatureFlagConfiguration contains spec.flagD.port, we now need to set that as the environment variable FLAGD_PORT on both flagD and the pod with the openfeature annotations.

I'm not sure if my proposal is the best way to do it. What did you have in mind?

In this case we can remove the port field from the spec or retain it and make it have precedence over the env object.

I would propose something like the following:

type FeatureFlagConfigurationSpec struct {
    // INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
    // Important: Run "make" to regenerate code after modifying this file
    // +optional
    // +nullable
    ServiceProvider *FeatureFlagServiceProvider `json:"serviceProvider"`
    // +optional
    // +nullable
    SyncProvider *FeatureFlagSyncProvider `json:"syncProvider"`
    // +optional
    // +nullable
    FlagDSpec *FlagDSpec `json:"flagDSpec"`
    // FeatureFlagSpec is the json representation of the feature flag
    FeatureFlagSpec string `json:"featureFlagSpec,omitempty"`
}

type FlagDSpec struct {
    Port string `json:"port"`
         Envs []EnvironmentalVariable `json: "envs"`
}

type EnvironmentalVariable struct {
        Name string
        Value string
}

I will put together a POC and we can take a look...

AlexsJones commented 2 years ago

This is done.. https://github.com/open-feature/open-feature-operator/pull/74

skyerus commented 2 years ago

we now need to set that as the environment variable FLAGD_PORT on both flagD and the pod with the openfeature annotations

The above PR handles the setting of environment variables on flagd but not the application container itself, my understanding is that we want flagd and the application to use the same configuration. I will investigate whether we can add the relevant env variables to the application container.

AlexsJones commented 2 years ago

we now need to set that as the environment variable FLAGD_PORT on both flagD and the pod with the openfeature annotations

The above PR handles the setting of environment variables on flagd but not the application container itself, my understanding is that we want flagd and the application to use the same configuration. I will investigate whether we can add the relevant env variables to the application container.

We could add envs on the application container also as you can't share between a pod. The thing is we'd then need to have active logic to check whether FLAGD_PORT was set and then add it in .. in which case the original implementation would have been more precise..