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

Support of secrets for autodiscovery scenarios in kubernetes environments #2200

Open
Tracked by #3063
ChrsMark opened this issue Jan 27, 2023 · 30 comments
Open
Tracked by #3063
Labels
Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team

Comments

@ChrsMark
Copy link
Member

ChrsMark commented Jan 27, 2023

We need to verify and document that we can provide similar experience with Agent with the one we had in Beats.
Required actions:

  1. For standalone Agent we need to provide documentation of how to use kubernetes_secrets provider in template based autodiscovery. We need to update https://www.elastic.co/guide/en/fleet/master/kubernetes_secrets-provider.html and https://www.elastic.co/guide/en/fleet/8.6/conditions-based-autodiscover.html properly.
  2. We need to evaluate how Hints based autodiscovery can leverage Kubernetes secrets either through the same provider (kubernetes_secrets) or by directly doing what the provider does accessing the k8s API when a secret reference is identified. I would lean towards the latter option which by-pass the second dynamic variable evaluation pass (which might not be possible) and directly handles the k8s API access. The provider's code can be re-used.

cc: @gizas @mlunadia @rameshelastic @ruflin

Useful links:

  1. k8s secrets provider implemented at Implement k8s secrets provider for Agent beats#24789
  2. k8s secrets keystore implementation for Beats: Add k8s keystore backend beats#18096
@ChrsMark ChrsMark added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Jan 27, 2023
@ChrsMark ChrsMark self-assigned this Jan 27, 2023
@gizas
Copy link
Contributor

gizas commented Jan 27, 2023

Thank you @ChrsMark !

I see all magic happens here

which by-pass the second dynamic variable evaluation pass

The request seems direct to the API. Can you provide some more feedback for above comment for clarification

@gizas gizas changed the title Effectively manage credentials in kubernetes environments Support of secrets for autodiscovery scenarios in kubernetes environments Jan 27, 2023
@ChrsMark
Copy link
Member Author

@gizas The Fetch method will be called by

fval, found := fetchProvider.Fetch(name)
when the controller does the evaluation of the dynamic variables using the emitted mappings provided by the providers.

The controller passes the fetchDynamicProvider further to the transpiler at

v, _ := transpiler.NewVarsWithProcessors(id, local, name, mappings.processors, fetchContextProviders)
.

This means that unlike other providers (like the kubernetes) this specific one (the kubernetes_secrets) is called on demand only when there is a variable linked to it.
So if if there are no variables like ${kubernetes_secrets.default.somesecret.value} in the input policy then the provider will never be called. The reason we did this is because we didn't want to fetch k8s secrets without actual need for them. You can find more in the implementation PR and its linked issue at elastic/beats#24789.

So the point that you are raising here indicates that maybe we cannot rely on the controller to do this evaluation when a secret reference is coming from the hints mechansim. This is how this would look like:

Having a hints like:

kind: Pod
metadata:
  name: redis
  annotations:
    co.elastic.hints/package: redis
    co.elastic.hints/data_streams: info
    co.elastic.hints/host: '${kubernetes.pod.ip}:6379'
    co.elastic.hints/info.period: 1m
    co.elastic.hints/info.password: ${kubernetes_secrets.default.redispass.value}

So the hints mechanism would produce an input like the following out of the hints' template:

inputs:
- name: redis/metrics-redis
  streams:
   -data_stream:
      dataset: redis.info
      type: metrics
    hosts:
    - 167.13.5.1:6379
    idle_timeout: 20s
    maxconn: 10
    metricsets:
    - info
    network: tcp
    password: ${kubernetes_secrets.default.redispass.value}
    period: '1m'
  type: redis/metrics
  use_output: default

So at this point the first variable evaluation has taken place and the hints' placeholders have been filled properly, however the policy still has a dynamic variable reference cause the password's hint provided a reference to another dynamic variable which is ${kubernetes_secrets.default.redispass.value}. I'm not even sure if this policy would be validated by the Agent at runtime however even if it is validated it would need a second round of variable substitution so as to fill the ${kubernetes_secrets.default.redispass.value}. I'm not sure if this is the expected behavior of Agent and it becomes more complex to follow as we can see.

In this regard I would propose to directly call the Fetch method (or some part of it using a wrapper function or whatever) at

key := strings.TrimSuffix(strings.TrimPrefix(match, "${kubernetes."), "}")
val, err := kubeMeta.GetValue(key)
in case we spot the ${kubernetes_secrets.*} pattern.

Ofc, this should be configurable so as to allow users to disable this functionality (for security reasons) and also make sure that the defined secret belongs to the same namespace with the target workload. So for instance if we deploy a Redis Pod in staging namespace only Secrets from staging namespace should be accessible through this mechansim in order to provide a baseline level of isolation.

This is what I mean by by-pass the second dynamic variable evaluation pass and directly handle the k8s API access. :).
Let me know if that helps or if you need any more clarifications.

@gizas
Copy link
Contributor

gizas commented Jan 27, 2023

Thanks for the clarification! So the second validation (if possible) is the problem here.
I see the direct call is smoother!

Now in our docs we have co.elastic.hints/password (not co.elastic.hints/info.password) and I guess we dont handle it at all.

Also because we touch the secret handling in this story, I bring to the attention this https://kubernetes.io/docs/tasks/administer-cluster/kms-provider/, that is available since 1.25 version.

Maybe can also help us keep secrets encrypted at rest. And as long as we touch the K8s API call, we can test if we can support it additionally

@blakerouse
Copy link
Contributor

I am confused here, I read the whole flow and still a little lost. Why is co.elastic.hints/host: '${kubernetes.pod.ip}:6379' already evaluated and not password: ${kubernetes_secrets.default.redispass.value}? I am a little confused on why one is evaluated and the other was not.

@ChrsMark
Copy link
Member Author

Thanks for the clarification! So the second validation (if possible) is the problem here. I see the direct call is smoother!

Now in our docs we have co.elastic.hints/password (not co.elastic.hints/info.password) and I guess we dont handle it at all.

You can refer to setting of a specific data_stream if you prefix the the setting with the name of the data_stream. So yes info.password is supported today. It is explained at https://www.elastic.co/guide/en/fleet/master/hints-annotations-autodiscovery.html.

@ChrsMark
Copy link
Member Author

@blakerouse would sth like foo=true path42='${env.foo}' ./elastic-agent inspect work? In my tests it didn't.
This is why I believe that sth like co.elastic.hints/password: ${kubernetes_secrets.default.redispass.value} will not work. The kubernetes provider will emit a mapping that will have as value the exact string ${kubernetes_secrets.default.redispass.value}, is this allowed? And if yes how Agent will handle it? I think that nothing will happen and the input will just have the exact string in place without any extra evaluation. So the key question here is: Does Agent support chained references from more than 1 dynamic providers?

@ChrsMark
Copy link
Member Author

For reference I have run some more detailed tests that illustrate the point.

Using the following standalone input policy:

    inputs:
      - name: kubernetes-node-metrics
        type: kubernetes/metrics
        use_output: default
        meta:
          package:
            name: kubernetes
            version: 1.9.0
        data_stream:
          namespace: default
        streams:
          - data_stream:
              dataset: kubernetes.node
              type: metrics
            metricsets:
              - node
            add_metadata: true
            bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
            hosts:
              - 'https://${env.NODE_NAME}:10250'
            period: 10s
            ssl.verification_mode: none
          - data_stream:
              dataset: kubernetes.pod
              type: metrics
            metricsets:
              - pod
            add_metadata: true
            bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
            hosts:
              - 'https://${env.NODE_NAME}:10250'
            period: 10s
            ssl.verification_mode: ${kubernetes_secrets.default.redispass.value}

I deploy a secret accordingly:

cat << EOF | kubectl apply -f -
apiVersion: v1
kind: Secret
metadata:
  name: redispass
type: Opaque
data:
  value: $(echo -n "myred1sp@ss" | base64)
EOF

And then I deploy a target Redis Pod:

apiVersion: v1
kind: Pod
metadata:
  name: redis
  annotations:
    co.elastic.hints/package: redis
    co.elastic.hints/data_streams: info
    co.elastic.hints/host: '${kubernetes.pod.ip}:6379'
    co.elastic.hints/info.period: 1m
    co.elastic.hints/info.password: ${kubernetes_secrets.default.redispass.value}
  labels:
    k8s-app: redis
    app: redis
spec:
  containers:
  - image: redis
    imagePullPolicy: IfNotPresent
    name: redis
    ports:
    - name: redis
      containerPort: 6379
      protocol: TCP
    command:
      - redis-server
      - "--requirepass 'myred1sp@ss'"

The resulted computed config is:

inputs:
- data_stream:
    namespace: default
  meta:
    package:
      name: kubernetes
      version: 1.9.0
  name: kubernetes-node-metrics
  streams:
  - add_metadata: true
    bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
    data_stream:
      dataset: kubernetes.node
      type: metrics
    hosts:
    - https://kind-control-plane:10250
    metricsets:
    - node
    period: 10s
    ssl.verification_mode: none
  - add_metadata: true
    bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
    data_stream:
      dataset: kubernetes.pod
      type: metrics
    hosts:
    - https://kind-control-plane:10250
    metricsets:
    - pod
    period: 10s
    ssl.verification_mode: myred1sp@ss
  type: kubernetes/metrics
- data_stream.namespace: default
  id: kubernetes-fb93b94c-852f-4c90-b352-f168f5ef1043
  name: redis/metrics-redis
  processors:
  - add_fields:
      fields:
        cluster:
          name: kind
          url: kind-control-plane:6443
      target: orchestrator
  - add_fields:
      fields:
        labels:
          app: redis
          k8s-app: redis
        namespace: default
        namespace_labels:
          kubernetes_io/metadata_name: default
        namespace_uid: a5692398-e32f-4332-979f-97f6f040faa3
        node:
          hostname: kind-control-plane
          labels:
            beta_kubernetes_io/arch: amd64
            beta_kubernetes_io/os: linux
            kubernetes_io/arch: amd64
            kubernetes_io/hostname: kind-control-plane
            kubernetes_io/os: linux
            node-role_kubernetes_io/control-plane: ""
            node_kubernetes_io/exclude-from-external-load-balancers: ""
          name: kind-control-plane
          uid: 1e3c36d2-f3e7-4d1f-ba66-75f72102cf22
        pod:
          ip: 10.244.0.5
          name: redis
          uid: fb93b94c-852f-4c90-b352-f168f5ef1043
      target: kubernetes
  streams:
  - data_stream:
      dataset: redis.info
      type: metrics
    hosts:
    - 10.244.0.5:6379
    idle_timeout: 20s
    maxconn: 10
    metricsets:
    - info
    network: tcp
    password: ""
    period: 1m
  type: redis/metrics

So the observations here are:

  1. The k8s secret variable defined in the original input policy as ssl.verification_mode: ${kubernetes_secrets.default.redispass.value} (this is just a test setting with no actual meaning) is evaluated properly as ssl.verification_mode: myred1sp@ss showing that the k8s secrets provider work as expected when variables are defined in the original Agent policy as usual.
  2. The hint co.elastic.hints/info.password: ${kubernetes_secrets.default.redispass.value} does not actually provide a value for the generated Redis input. The password is evaluated as password: "". So the 2-tier variable evaluation seems that is not supported here.

@blakerouse to clarify your question more: in co.elastic.hints/host: '${kubernetes.pod.ip}:6379' the hints mechanism does not emit '${kubernetes.pod.ip}:6379'as value, it extracts the${kubernetes.pod.ip}content from the Pod's metadata on the fly so this is not actually a reference to thekubernetes` provider. This happens at

key := strings.TrimSuffix(strings.TrimPrefix(match, "${kubernetes."), "}")
val, err := kubeMeta.GetValue(key)
. So again here we don't have a 2-step evaluation which is similar with what we need to do for the secrets' case. Does this makes sense?

@blakerouse
Copy link
Contributor

Okay it is making more sense. I was not familiar with how hints where implemented in the Elastic Agent. After reviewing it and reviewing your comment I have a better understanding.

key := strings.TrimSuffix(strings.TrimPrefix(match, "${kubernetes."), "}")
val, err := kubeMeta.GetValue(key)

The code above makes sense because ${kubernetes.pod.ip} needs to ensure that its only for this exact pod and not another pod if this was done in a second pass.

So doing a raw second pass would probably not provide a result that would be expected.

One option is to use a Vars set in that method to resolve all context provides including fetch providers. The downside of that is it would only be evaluated when the hint is generated and will not be updated if that context var where to change, and I think that would be a problem.

We might need to do a second pass, but we need to be very specific on the matching regex to ensure that a second pass would not overwrite any escaped variables.

We might want the hints provide to take say any non-matching ${vars} and replace them with $'{vars}. The $' could represent a need for a second pass replace. I don't know if the $' is the best way to do this, but I do believe some second pass is needed and it needs to happen out of the hints provider (reason provided above).

@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 1, 2023

Okay it is making more sense. I was not familiar with how hints where implemented in the Elastic Agent. After reviewing it and reviewing your comment I have a better understanding.

key := strings.TrimSuffix(strings.TrimPrefix(match, "${kubernetes."), "}")
val, err := kubeMeta.GetValue(key)

The code above makes sense because ${kubernetes.pod.ip} needs to ensure that its only for this exact pod and not another pod if this was done in a second pass.

So doing a raw second pass would probably not provide a result that would be expected.

One option is to use a Vars set in that method to resolve all context provides including fetch providers. The downside of that is it would only be evaluated when the hint is generated and will not be updated if that context var where to change, and I think that would be a problem.

We might need to do a second pass, but we need to be very specific on the matching regex to ensure that a second pass would not overwrite any escaped variables.

We might want the hints provide to take say any non-matching ${vars} and replace them with $'{vars}. The $' could represent a need for a second pass replace. I don't know if the $' is the best way to do this, but I do believe some second pass is needed and it needs to happen out of the hints provider (reason provided above).

This looks a bit hacky to me tbh, and would imply an extra difficulty to debug and support this. How we would ensure that our users would be able to troubleshoot such a case when it involves 2 passes of variable evaluation.

In this regard I would lean towards avoiding to go down the path of the 2 passes and for secrets do the evaluation at first place when the proper hint is detected. I don't think it's a serious issue if the secret is changed since it would be expected to affect all of its consumers. The same will happen in general in k8s if the secrets was mounted inside a Pod.

Having the implementation for the secrets inside the hints mechanism would also reduce the implementation time/overhead and would be easier to support as I mentioned already. @gizas @MichaelKatsoulis what do you think here?

@MichaelKatsoulis
Copy link
Contributor

One option is to use a Vars set in that method to resolve all context provides including fetch providers. The downside of that is it would only be evaluated when the hint is generated and will not be updated if that context var were to change, and I think that would be a problem.

This is not a serious problem if it is documented. I would avoid requesting from user counter-intuitive things like $'{vars}. I am also voting for an one time pass.

@cmacknz
Copy link
Member

cmacknz commented Feb 1, 2023

I would avoid requesting from user counter-intuitive things like $'{vars}.

Also +1 to this if we can avoid it. I think the maintenance cost of this will be too high. It suggests there is something we need to rethink in the way we do variable substitution if the need for this type of thing becomes a recurring problem. #2177 seems related here.

@gizas
Copy link
Contributor

gizas commented Feb 2, 2023

I would agree that if we can avoid $'{vars} lets do it because can be really error prove solution for the user.

From what is documented above I would suggest:

  • Add another check here for "${kubernetes.secret"), "}") and if true
  • Directly call the fetch method here

We should document that this way all secrets are read on start up time of container and in case of expiration user needs to restart and force reading. But we should be clear on this

@blakerouse
Copy link
Contributor

First the $'{vars} would be internal only and not something that the user would be able to add or even use. On first pass we could check for $' and error, and only the hints provider would convert none resolved to $'. So I disagree that its hacky and it would provide a way for a second pass.

In the case of the kubernetes_secrets.* having the hints provider doing the work does work, but that falls apart if say you want to reference a different context provider where values can change at any time. Because of that the value would not be updated unless the pod itself triggers an event.

I think we should strive to use existing AST and variable replacement instead of adding something into the hints provider.

@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 7, 2023

First the $'{vars} would be internal only and not something that the user would be able to add or even use. On first pass we could check for $' and error, and only the hints provider would convert none resolved to $'. So I disagree that its hacky and it would provide a way for a second pass.

Still hard to troubleshoot and follow the execution flow though. Even today, it is sometimes hard for our users or support-team to troubleshoot the variable resolution part. I assume that introducing a 2nd pass would make it even more hard.

In the case of the kubernetes_secrets.* having the hints provider doing the work does work, but that falls apart if say you want to reference a different context provider where values can change at any time. Because of that the value would not be updated unless the pod itself triggers an event.

Exposing the whole set of the providers' variables to the hints mechanism would introduce some security concerns. This would mean that everyone with access to to deploy Pods (on a node) would potentially be able to access environment variables, local provider's variables etc. I already see that some sensitive data like ES username/password are set in env vars: https://github.com/elastic/elastic-agent/blob/main/deploy/kubernetes/elastic-agent-standalone-kubernetes.yaml#L696-L700.

This security aspect is something that was concerning in the past and hence I would be hesitant to provide full access to the hints mechanism. For k8s secrets we have defined a specific strategy of what to allow and what not but we cannot do the same for the whole set of the providers. In this regard, I would start with only making the k8s secrets natively available within the kubernetes provider (hints mechanism) and if there are requests in the future we can reconsider what is the best way to support additional providers (if we actually want it). That's my proposal for now (time-wise and support-wise).

I think we should strive to use existing AST and variable replacement instead of adding something into the hints provider.

In that case we would talk about 2 different issues: one on Agent's core AST to support the second pass and another one to implement the $'{ addition in hint's side. But still, the security and supportability concerns I mentioned above make me hesitant to this.
Based on my above explanations I would leave it to @gizas and @cmacknz to make the last call.

@blakerouse
Copy link
Contributor

blakerouse commented Feb 15, 2023

After discussion with the @cmacknz we have decided that the second pass approach would be a better and more future proof way of handling this issue. The reasons are as follows:

  • It's not a bolt on to hints, making hints are large piece of code that has to be modified each time a new provider needs to be accessible from hints.
  • In the future other secrets provides will most likely be required, like vault, AWS KMS, etc. Adding a secrets provider will just work in the future without requiring manual enablement in the hints code.
  • Second pass allows changes in a secret to cause the value to be changed and rendered, doing it inside of the hints provider means that a kubernetes event must occur for that change to be noticed.

How it will be implemented is a little different then how I described previously:

  • It will work in two passes.
  • First pass will replace and ignore any $${vars} (allowing escaped vars to not be substituted for ${vars})
  • Second pass will replace and not ignore $${vars} (substituting to ${vars})

This removes the need for $' as I described before.

The above relates to this other issue - #2177

@ChrsMark
Copy link
Member Author

Ok, how the security concerns would be covered in that case?
Would it be hints mechanism's responsibility to allow/deny access to providers?

If that's the way forward, are you going to create a different generic issue for the 2-step variable resolution support in Agent's core side?

@gizas
Copy link
Contributor

gizas commented Feb 16, 2023

@blakerouse my understanding is that we will ask the users to fill in secrtes with $$ .
For me this approach (regardless the implementation solution that we will follow) is an anti-pattern for the users. As long as they define their fields with the extra words like secrets.kubernetes.* this should be enough for us to match and identify that we have a secret.

Afterall, even with your solution you need to search and replace. So why dont you search initially for secrtes.kubernetes and do the replacement and then replace rest of vars?

It's not a bolt on to hints, making hints are large piece of code that has to be modified each time a new provider needs to be accessible from hints.

Maybe I am missing something here, can you elaborate on this?

In the future other secrets provides will most likely be required, like vault, AWS KMS, etc. Adding a secrets provider will just work in the future without requiring manual enablement in the hints code.

For me the support of new secrets will be just the match of words like secrtes.kuberntes, secrets.vault, secrtets.aws etc
Do you agree?

The above relates to this other issue - #2177

Also here the problem is slightly different on your side and I think we try here to solve two problems with one solution. Just my idea here is that escaping characters for users mean usage of symbol \ .

Long story short, lets have a second round of thoughts here if you consider that is meaningful. Of course we depend on your implementation, if you decide to go with way proposed, we will follow up with the implementation of hints once you are done

@blakerouse
Copy link
Contributor

@blakerouse my understanding is that we will ask the users to fill in secrtes with $$ . For me this approach (regardless the implementation solution that we will follow) is an anti-pattern for the users. As long as they define their fields with the extra words like secrets.kubernetes.* this should be enough for us to match and identify that we have a secret.

No they will not need to use $${ they will use the standard ${. As described above on how the passes will work.

Afterall, even with your solution you need to search and replace. So why dont you search initially for secrtes.kubernetes and do the replacement and then replace rest of vars?

The reasons where provided above.

It's not a bolt on to hints, making hints are large piece of code that has to be modified each time a new provider needs to be accessible from hints.

Maybe I am missing something here, can you elaborate on this?

The reasons say way this becomes a bolt on.

In the future other secrets provides will most likely be required, like vault, AWS KMS, etc. Adding a secrets provider will just work in the future without requiring manual enablement in the hints code.

For me the support of new secrets will be just the match of words like secrtes.kuberntes, secrets.vault, secrtets.aws etc Do you agree?

Again this becomes a bolt on and now every time we add a new secrets provider we need to add it in 2 places. We should only need to register it and it just work inside of the Elastic Agent.

The above relates to this other issue - #2177

Also here the problem is slightly different on your side and I think we try here to solve two problems with one solution. Just my idea here is that escaping characters for users mean usage of symbol \ .

Not trying to solve two problems with one solution. We have a problem that needs to be handled no matter what and that problem causes the solution in this issue to be something that needs to be thought about.

Long story short, lets have a second round of thoughts here if you consider that is meaningful. Of course we depend on your implementation, if you decide to go with way proposed, we will follow up with the implementation of hints once you are done

This solution requires nothing to be implemented in hints, it just works. Which is why we lean on that solution. Lets say we do hints for another type of provider, it will just work in that new provider, no need to add something specific to hints in that case.

@ChrsMark
Copy link
Member Author

@blakerouse @cmacknz I don't see any of the security concerns mentioned at #2200 (comment) being covered in the solution you propose.
While the generic approach is handy we need to think of how the allow/deny options will be handled. Exposing the whole set of providers to the hints mechanism is sth that we need to carefully consider. @ruflin @gizas we had met this in the past

@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 20, 2023

@blakerouse my understanding is that we will ask the users to fill in secrtes with $$ . For me this approach (regardless the implementation solution that we will follow) is an anti-pattern for the users. As long as they define their fields with the extra words like secrets.kubernetes.* this should be enough for us to match and identify that we have a secret.

No they will not need to use $${ they will use the standard ${. As described above on how the passes will work.

Sorry, that's confusing to me that they will not need to use $${. Which component will add the $${ in the hint's value, when you say that the hints mechanism does not need to be changed?

Could you provide a more detailed example here? How the hints in the annotations would look like and how this would be handled under the hood (and by which component)?

@blakerouse
Copy link
Contributor

The hints in will be the same as your previous comment above, like the following:

apiVersion: v1
kind: Pod
metadata:
  name: redis
  annotations:
    co.elastic.hints/package: redis
    co.elastic.hints/data_streams: info
    co.elastic.hints/host: '${kubernetes.pod.ip}:6379'
    co.elastic.hints/info.period: 1m
    co.elastic.hints/info.password: ${kubernetes_secrets.default.redispass.value}

The 2 stages will work like the following:

  • Pass 1: Works as it does today, except any $${ will be ignored and left exactly as they are.
  • Pass 2: Works the same as Pass 1, except if translates $${ to ${.

@ChrsMark
Copy link
Member Author

So @blakerouse will it be the hints mechanism within the kubernetes provider that will detect the ${kubernetes_secrets.default.redispass.value} in the annotations and convert it to $${kubernetes_secrets.default.redispass.value}? Because in your example I don't see any double $ symbol in the annotations.

@blakerouse
Copy link
Contributor

@ChrsMark No. The provider will not need to do any transformation. It will remain as it is today. Pass 2 is done after the providers, so it will transform any remaining ${ just fine.

I don't see there even needed to be a change in any provider for this to work.

@ChrsMark
Copy link
Member Author

So @blakerouse how we will end up with the double $? I still don't get it how we will have the $${ inside the variables' value coming from the hints. Is the user responsible for setting the $${?

@cmacknz
Copy link
Member

cmacknz commented Mar 21, 2023

My understanding from reading this is that in the hints case you don't ever have $$, instead you end up with a $ left in after the first evaluation pass and the new second pass does the substitution on the remaining $ variable.

The $$ case is only relevant for cases where someone wants to explicitly pass a $ literal through the policy unmodified, as described in #2177.

The idea of the second pass here handles two separate cases 1) substituting $ variables that are left in the policy after hints are evaluated in the first pass, and 2) allowing literal $ variables to be passed through the agent policy by escaping them with $$.

If that is incorrect and doesn't clarify things, we should setup a quick meeting to clarify the outcome here.

@gizas
Copy link
Contributor

gizas commented Mar 21, 2023

I will try to follow the flow:

  1. User provides hint co.elastic.hints/info.password: ${kubernetes_secrets.default.redispass.value}
  2. After first pass, agent will have to do no change on the above and leave it as it is (second pass will not be triggered here as we have no $$)
  3. Then the kubernetes provider needs to be called to match the string above and replace with the secret?

Is that accurate?

@blakerouse
Copy link
Contributor

@cmacknz You are correct.

@blakerouse
Copy link
Contributor

I will try to follow the flow:

  1. User provides hint co.elastic.hints/info.password: ${kubernetes_secrets.default.redispass.value}
  2. After first pass, agent will have to do no change on the above and leave it as it is (second pass will not be triggered here as we have no $$)

Second pass is always triggered and second pass substitutes both $${ to ${ and ${ to real values.

  1. Then the kubernetes provider needs to be called to match the string above and replace with the secret?

The kubernetes provider does not need to be changed at all, as I stated above.

@ChrsMark
Copy link
Member Author

Thanks @cmacknz for elaborating on this. The mention to the $$ was confusing indeed :) . Now I think it makes sense that the second pass would evaluate/resolve sth like the following:

inputs:
- name: redis/metrics-redis
  streams:
   -data_stream:
      dataset: redis.info
      type: metrics
    hosts:
    - 167.13.5.1:6379
    idle_timeout: 20s
    maxconn: 10
    metricsets:
    - info
    network: tcp
    password: ${kubernetes_secrets.default.redispass.value}
    period: '1m'
  type: redis/metrics
  use_output: default

and properly replace the leftover ${kubernetes_secrets.default.redispass.value}.

So I think we are aligned here.
If we all agree on this, I would suggest creating a new generic issue for the implementation of 2nd round pass on Agent since this one is specific for k8s secrets while the solution will be generic for all the providers.

Last but not least, I still see a potential security issue by allowing hints to have access to all the providers through the second pass but we can deal with this it once we have the 2 passes evaluation implemented and see if we need to adjust hints so as to handle accesses better.

@blakerouse
Copy link
Contributor

Last but not least, I still see a potential security issue by allowing hints to have access to all the providers through the second pass but we can deal with this it once we have the 2 passes evaluation implemented and see if we need to adjust hints so as to handle accesses better.

I think we can easily add the ability for a provider to say it should not be present in second pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team
Projects
None yet
Development

No branches or pull requests

5 participants