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

ECK resources Helm chart - Elastic Agent & Elastic Fleet Server Agent #5889

Merged
merged 35 commits into from
Sep 26, 2022

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Jul 26, 2022

related to #5505

This PR contains the 2nd version of the ECK-managed resources helm charts including

eck-elastic-agent chart for installing elastic-agent
eck-fleet-server for managing fleet server as an Elastic Agent
minor updates to the eck-stack chart.

Note that this Chart allows multiple instances of Elastic Agent to be installed, along with allowing multiple instances of serviceAccounts, clusterRoles, and clusterRoleBindings.

cc @jmlrt @Kushmaro @framsouza

In Progress

  • additional manual testing
  • additional chart unit tests

naemono added 3 commits July 26, 2022 10:28
Edit templates to reference helm chart name properly
Attempt to handle missing namespace in elasticref.
convert to supporting multiple agent instances
working on SAs/clusterRoles
fix agent template
Adding service accounts, cluster roles, and cluster role bindings to agent templates.
Updating values documentation.
Adjusting installation notes for elastic-agent to be consistent.
Removing any reference to appVersion for consistency.
Adjusting annotations in all templates.
Removing unneeded bits in agent helm tests.
Adjusting default values for Elastic agent chart.
Adjusting wording in fleet-agent example in Elastic agent chart.
@naemono naemono added the >feature Adds or discusses adding a feature to the product label Jul 26, 2022
@naemono naemono marked this pull request as ready for review July 27, 2022 16:33
sources:
- https://github.com/elastic/cloud-on-k8s
- https://github.com/elastic/elastic-agent
icon: https://images.contentstack.io/v3/assets/bltefdd0b53724fa2ce/blt77c2da6e0198746e/620ac24e6662ca0a6f617114/icon-agent-32-color.svg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyone know a better icon location than this?

@naemono naemono added >enhancement Enhancement of existing functionality and removed >feature Adds or discusses adding a feature to the product labels Jul 28, 2022
@pebrc pebrc self-assigned this Jul 29, 2022
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM for the approach taken. One thing I am wondering though is if we could make the whole RBAC side of deploying agent easier with the Helm chart. But I understand that is quite difficult if you have list of agents where you do not know what it what.

deploy/eck-elastic-agent/values.yaml Outdated Show resolved Hide resolved
@naemono
Copy link
Contributor Author

naemono commented Jul 29, 2022

make the whole RBAC side of deploying agent easier with the Helm chart

I would love to make this easier for the user, but I'm not sure the Helm chart is the right place to do it. If we wanted to ensure that rbac rules were sufficient to the configuration, that logic would likely make more sense, and be easier to maintain in the operator itself, as complex logic in Helm charts just comes across as complex, ugly, and difficult to maintain.

I can certainly go down that road and attempt this within the Helm charts. Please let me know and I'll make the changes, and see what they look like.

@pebrc
Copy link
Collaborator

pebrc commented Aug 1, 2022

Please let me know and I'll make the changes, and see what they look like.

I don't have something that is ready to implement. I feel like Agent is special enough, compared to Kibana and Elasticsearch, to warrant doing a bit of design work for the Helm chart.

For example one idea I had was to make two different Helm charts one for Fleet Server and one for Elastic Agents. Behind the scenes it is the same CRD of course still.

That would allow us to configure two different sets of default RBAC permissions.

More far reaching permissions for Agent, roughly oriented maybe on the Kubernetes observability recipe or maybe the DaemonSet published by the Agents team

And less permissions for the Fleet server chart.

User could then override the default RBAC permissions for each of these charts to either restrict them further (e.g. for an APM integration we don't need all the K8s API permissions that K8s observability needs) or expand them where necessary.

With a Fleet server chart and an Elastic Agent chart we should be able to cover most if not all use cases we have currently in the recipes section of the repository. This is just an idea at this stage and not ready to implement. Probably worth discussing with the team. My motivation is to find a solution how we can keep the templating logic minimal in the Charts but still add value for our users by starting with sane defaults that cover the most common case.

@naemono
Copy link
Contributor Author

naemono commented Aug 4, 2022

We met and had a discussion about the current implementation, vs a more specialized implementation for, say, Fleet, and comparing what the user experience was like between the two, we decided to implement the more specialized Helm chart for Fleet, which simplifies what the Agent helm chart looks like. I'll move this back to a WIP, and update when there's more information.

@naemono naemono changed the title ECK resources Helm chart - Elastic Agent WIP: ECK resources Helm chart - Elastic Agent Aug 4, 2022
# Configuration of Agent, specifically used in Agent standalone mode.
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-elastic-agent.html
#
config: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we let the user set config and elasticsearchRefs explicitly? In fleet mode, using the provided example deploy/eck-elastic-agent/examples/fleet-agents.yaml, it fails with:

Error: INSTALLATION FAILED: admission webhook "elastic-agent-validation-v1alpha1.k8s.elastic.co" denied the request: Agent.agent.k8s.elastic.co "my-agents-eck-elastic-agent" is invalid:
[spec.config: Invalid value: v1.Config{Data:map[string]interface {}{}}: remove config, it can't be set in fleet mode, spec.fleetServerEnabled: Invalid value: false: remove Elasticsearch reference, it can't be enabled in fleet mode when Fleet Server is not enabled as well]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've disabled them explicitly in the examples now. We have to allow these to be adjustable, as the agent chart supports both fleet, and standalone, so config is required in some cases. Are you thinking that config should just be set to null in the default values instead of {}, and the ES ref left empty?

# At least one is required of [daemonSet, deployment].
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-elastic-agent-fleet-configuration.html#k8s-elastic-agent-fleet-configuration-chose-the-deployment-model
#
daemonSet:
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we switch to a Deployment?
I get this error when I try to replace daemonSet with deployment in deploy/eck-elastic-agent/examples/fleet-agents.yaml:

admission webhook "elastic-agent-validation-v1alpha1.k8s.elastic.co" denied the request: Agent.agent.k8s.elastic.co "my-agents-eck-elastic-agent" is invalid: [spec.daemonSet: Forbidden: Specify either daemonSet or deployment, not both,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'd have to set spec.daemonSet: null since it's defined in the default values. This is unfortunate, as we're trying to make the default useful, but this is never really user friendly when making modifications, as you have to set default values to null to disable them.

Update fleet-agents example to disable config, and ES refs.
Update fleet-server template to explicitly set mode, and fleetServerEnabled.
Add notes to eck-agent fleet-agents, and system-integration examples.
@thbkrkr thbkrkr self-assigned this Sep 20, 2022
deploy/eck-stack/README.md Outdated Show resolved Hide resolved
deploy/eck-agent/templates/elastic-agent.yaml Outdated Show resolved Hide resolved
deploy/eck-fleet-server/templates/fleet-server.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/templates/service-account.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/templates/service-account.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/templates/cluster-role.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/templates/cluster-role.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/templates/cluster-role-binding.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/templates/cluster-role-binding.yaml Outdated Show resolved Hide resolved
Update wording in eck-stack/readme
@thbkrkr
Copy link
Contributor

thbkrkr commented Sep 20, 2022

Is it intentional that labels/annotations are managed differently in the ClusterRoleBinding/ClusterRole/ServiceAccount between the two charts? The eck-agent chart supports dedicated labels/annotations but for labels we also get the labels of the agent, while for the eck-fleet-server no annotation at all and only the labels from the fleet-server.

@naemono
Copy link
Contributor Author

naemono commented Sep 20, 2022

Is it intentional that labels/annotations are managed differently in the ClusterRoleBinding/ClusterRole/ServiceAccount between the two charts? The eck-agent chart supports dedicated labels/annotations but for labels we also get the labels of the agent, while for the eck-fleet-server no annotation at all and only the labels from the fleet-server.

This absolutely was not intentional, and has been resolved. Thanks for catching that @thbkrkr. I think I'll also add additional tests around labels/annotations similar to https://github.com/elastic/cloud-on-k8s/pull/6004/files#diff-9612072d3cca4f0281e66e5bd8fdb713f35792862686355d89ff507348419028R41

…account/clusterrole/clusterrolebinding map.

Adding tests for all scenarios
@naemono
Copy link
Contributor Author

naemono commented Sep 20, 2022

@thbkrkr While adding tests, I noticed that .Values.[labels|annotations], and .Values.[serviceaccount|clusterRole|clusterRoleBinding].[labels|annotations] weren't both making it into the rendered templates, and this was fixed, and tests were added for all.

deploy/eck-agent/examples/fleet-agents.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/examples/fleet-agents.yaml Outdated Show resolved Hide resolved
deploy/eck-agent/values.yaml Outdated Show resolved Hide resolved
deploy/eck-stack/examples/fleet-agents.yaml Outdated Show resolved Hide resolved
deploy/eck-stack/Chart.yaml Outdated Show resolved Hide resolved
@thbkrkr thbkrkr added the v2.5.0 label Sep 22, 2022
@naemono naemono merged commit 4569ef2 into elastic:main Sep 26, 2022
@naemono naemono deleted the 5505-agent-eck-resource-helm-chart-v2 branch September 26, 2022 18:25
@pebrc pebrc added >feature Adds or discusses adding a feature to the product and removed >enhancement Enhancement of existing functionality labels Oct 5, 2022
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature Adds or discusses adding a feature to the product v2.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants