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

Implement k8s secrets provider for Agent #24789

Merged
merged 25 commits into from
Apr 1, 2021

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Mar 26, 2021

This PR aims to add support for consuming k8s secrets through Agent's variables.

Note: This is still a draft but comments are needed so as to verify it's going to the right direction, so plz @blakerouse @exekias feel free to comment here.

How to test this:

  1. Create the k8s secret:
cat << EOF | kubectl apply -f -
apiVersion: v1
kind: Secret
metadata:
  name: somesecret
type: Opaque
data:
  value: $(echo -n "passpass" | base64)
EOF
  1. Use the following config:
providers.kubernetes_secrets:
  kube_config: /Users/chrismark/.kube/config

inputs:
  - type: logfile
    streams:
      - paths: ${env.logpath}/${kubernetes_secrets.default.somesecret.value}/another.log
  1. Run inspect command to check what is the compiled configuration (use proper path for the elastic-agent.yml config file :
    logpath=/var/log ./elastic-agent -c /Users/ubuntu/Desktop/elastic-agent.yml inspect output -o default

Verify that the following is produced:

[default] filebeat:
filebeat:
  inputs:
  - index: logs-generic-default
    paths: /var/log/passpass/another.log

Unit testing:

  1. go test -v ./pkg/agent/transpiler/... -run TestVars_ReplaceWithFetchContextProvider
  2. go test -v ./pkg/composable/providers/kubernetessecrets...

Closes #24143

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 26, 2021
@ChrsMark ChrsMark added the Team:Integrations Label for the Integrations team label Mar 26, 2021
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 26, 2021
@ChrsMark ChrsMark self-assigned this Mar 26, 2021
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 26, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #24789 updated

  • Start Time: 2021-04-01T11:39:01.501+0000

  • Duration: 69 min 51 sec

  • Commit: e47bb5a

Test stats 🧪

Test Results
Failed 0
Passed 6612
Skipped 16
Total 6628

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 6612
Skipped 16
Total 6628

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark marked this pull request as ready for review March 29, 2021 11:19
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good and took the path I was wanting to see. Just some cleanups that need to be done first.

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

Thanks for reviewing @blakerouse! I applied changes based on your comments so please have another look. As far as adding tests is concerned I plan to add them, in this PR, once we verify that implementation is settled so as to avoid changing them along the review rounds. Let me know what you think, thanks!

}
if set {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that getting a value from a fetch context provider returns something other than a string?

Below you can see that it will replace full objects if its a varString. I worry based on placing this before the switch val.(type) we are missing that as a possibility with a fetch context provider.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, however no strong opinion here since I'm not super familiar with the combinations that could occur. Do you think that removing continue and being able to get to the switch block after the Fetch() action would be sth that we want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I took a closer look at this file. This is definitely in the wrong place. At its current location it would try to find a constant string in a fetch context provider, we do not want that.

Looking at it, this needs to be removed from here and moved into the:

func (v *Vars) Lookup(name string) (interface{}, bool) {
	return v.tree.Lookup(name)
}

Leaving this function untouched.

Copy link
Member Author

@ChrsMark ChrsMark Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blakerouse I tried to move (see f023688) the code inside func (v *Vars) Lookup(name string) (interface{}, bool) method of the same file but it does not seem to work (using same configuration so far) and I'm not sure that I can follow the flow here. I see that the method is called at

val, ok := v.vars.Lookup(name)
and when debugging the flow with additional debug messages I cannot find anything useful so far. Could you please provide more content here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see the issue. You need to do the following:

Add the following function to Vars:

// lookupNode performs a lookup on the AST, but keeps the result as a `Node`.
//
// This is different from `Lookup` which returns the actual type, not the AST type.
func (v *Vars) lookupNode(name string) (Node, bool) {
	// check if the value can be retrieved from a FetchContextProvider
	for providerName, provider := range v.fetchContextProviders {
		if varPrefixMatched(name, providerName) {
			fetchProvider := provider.(composable.FetchContextProvider)
			fval, found := fetchProvider.Fetch(name)
			if found {
				return &StrVal{value: fval}, true
			} else {
				return &StrVal{value: ""}, false
			}
		}
	}
	// lookup in the AST tree
	return Lookup(v.tree, name)
}

Then change node, ok := Lookup(v.tree, val.Value()) to node, ok := v.lookupNode(val.Value()).

See if that works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 This fixes the issue, thank you!

}
p.client = client
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being that a context provider is only considered a fetch context provider if it implements the FetchContextProvider interface and if it stops implementing the interface it will just stop working without any warning.

It would be good to add a compile check to ensure that it implements that interface in this file.

var _ FetchContextProvider = (*contextProviderK8sSecrets)(nil)

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

The addition of the new lookupNode solves the latest issue. @blakerouse do you think we are ok now with the changes? shall I proceed with adding some tests?

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 1, 2021

@blakerouse I proceeded with adding some tests too. Let me know what you think.

@ChrsMark ChrsMark requested a review from blakerouse April 1, 2021 11:08
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, tests looks good. Ready to merge!

@ChrsMark ChrsMark merged commit 49e3857 into elastic:master Apr 1, 2021
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Apr 1, 2021
ChrsMark added a commit that referenced this pull request Apr 2, 2021
v1v added a commit to v1v/beats that referenced this pull request Apr 7, 2021
* upstream/master: (91 commits)
  [Filebeat] Change okta.target to nested field (elastic#24636)
  Add RFC5424 format support for syslog input  (elastic#23954)
  Fix links to Beats product pages (elastic#24821)
  [DOCS] Fix 'make setup' instructions for a new beat (elastic#24944)
  Remove duplicate decode_xml entry (elastic#24941)
  [libbeat] Add wineventlog schema to decode_xml processor (elastic#24726)
  [Elastic Agent] Add check for URL set when cert and cert key. (elastic#24904)
  feat: stage execution cache (elastic#24780)
  Fix error in Journalbeat commands (elastic#24880)
  Add baseline ECS 1.9.0 upgrade (elastic#24909)
  [Elastic Agent] Cloud container legacy apm files. (elastic#24896)
  [Elastic Agent]: Reduce allowed socket path length (elastic#24914)
  Add ability to destroy indices with wildcards in testing (elastic#24915)
  Add status subcommand to report status of running daemon. (elastic#24856)
  Fix types of fields GetHits and Ops in Metricbeat module for Couchbase (elastic#23287)
  Add support for Filestream input in elastic agent. (elastic#24820)
  Implement k8s secrets provider for Agent (elastic#24789)
  Sort processor list in docs (elastic#24874)
  Add support for SCRAM authentication in kafka metricbeat module (elastic#24810)
  Properly update offset in case of unparasable line (elastic#22685)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support reading secrets in K8s agent manifest
3 participants