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

Changes according to IBM ICP content guidelines and best practices #1

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mrgadgil
Copy link

No description provided.

- ICP
- DevOps
- amd64
- Commercial
Copy link
Author

@mrgadgil mrgadgil May 24, 2019

Choose a reason for hiding this comment

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

Keywords are required by charts who want to be a part of ICP catalog.
https://github.com/IBM/charts/blob/master/GUIDELINES.md#required-chart-keywords

- amd64
- Commercial
tillerVersion: ">=2.9.1"
kubeVersion: ">1.7"
Copy link
Author

Choose a reason for hiding this comment

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

Recommended for charts who want to be a part of ICP catalog.

@@ -0,0 +1,202 @@

Copy link
Author

Choose a reason for hiding this comment

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

Apache License is required by charts to be included in the community repo. https://github.com/IBM/charts/blob/master/GUIDELINES.md#license
This license is for the helm chart.
Any product licenses can go in a separate LICENSES folder.

* An AppDynamics account. AppDynamics offers a [free trial](https://www.appdynamics.com/free-trial/)
* An AppDynamics account.

### Create a secret
Copy link
Author

@mrgadgil mrgadgil May 24, 2019

Choose a reason for hiding this comment

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

A single Tiller instance is used to store all Helm releases in a single location. Anyone who has the authority to access Helm (an ICP Cluster Admin, Team Admin or Team Operator) has the ability to list the contents of any Helm release, regardless of the namespace that it was installed into.
Any sensitive data that is passed to chart as values can be obtained when we run helm get release command.
Therefore the recommended approach is to never store any sensitive data with the chart. Secret resources should be handled independently of the chart.
Therefore I have added capability for the user to pre-create the secret and pass it to the chart.
I did not remove the secret generation from the chart but tried to steer user to pre-create secret.

--namespace namespace_name
```

### Image Security Policies
Copy link
Author

Choose a reason for hiding this comment

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

ICP clusters generally have image security policies enforced. Image repositories have to be whitelisted so that images can be pulled during chart install.

- name: docker.io/appdynamics/cluster-agent:*
```

### Pod Security Policies
Copy link
Author

@mrgadgil mrgadgil May 24, 2019

Choose a reason for hiding this comment

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

Cluster agent container runs as root.
ICP recommends to run the container as a non-root user. This is not a hard requirement for our first round.
It will become a mandate if this chart intends to become a cloud pak. Cloud paks are required to run containers as non-root users and have to specify a security context in deployment.
For our initial take, I have added documentation for user to create a namespace with less restrictive PSP that allows containers to run as root.

@@ -57,20 +94,21 @@ helm install appdynamics-charts/cluster-agent --name=stable \
| `deployment.image.tag` | Tag of the machine agent image | `latest`
| `deployment.image.pullPolicy` | The machine agent image pull policy| `IfNotPresent`
| `deployment.resources ` | Resources directive | See below

| `existingSecret` | Name of existing secret to use for ClusterAgent application in AppDyanamics |
Copy link
Author

Choose a reason for hiding this comment

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

Please see the explanation above for adding existing secret field.

```
## Limitation
Copy link
Author

Choose a reason for hiding this comment

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

Added limitation as I could not make chart work with AppDyanamics Trial account.
Please let me know we need more investigation on using AppDynamics lite account.




## Support
Copy link
Author

Choose a reason for hiding this comment

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

This section is a mandatory section to be included in the ReadMe for charts in the catalog.
https://github.com/IBM/charts/blob/master/GUIDELINES.md#support-statement

@@ -1,4 +1,4 @@
{{- if and .Values.controller.apiUser .Values.controller.url }}
{{- if and .Values.controller.url ( or .Values.controller.apiUser .Values.existingSecret) }}
Copy link
Author

Choose a reason for hiding this comment

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

Change to include existing secret in the condition.

{{- else -}}
{{- printf "%s" (include "cluster-agent.fullname" .) -}}
{{- end -}}
{{- end -}}
Copy link
Author

Choose a reason for hiding this comment

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

Created function to get secret name depending on whether user provides an existing secret or lets chart create one.

helm.sh/chart: {{ include "cluster-agent.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
release: {{ .Release.Name }}
Copy link
Author

Choose a reason for hiding this comment

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

helm.sh/chart: {{ include "cluster-agent.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
release: {{ .Release.Name }}
Copy link
Author

Choose a reason for hiding this comment

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

app.kubernetes.io/name: {{ include "cluster-agent.name" . }}
helm.sh/chart: {{ include "cluster-agent.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
Copy link
Author

Choose a reason for hiding this comment

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

- DevOps
- amd64
- Commercial
tillerVersion: ">=2.9.1"
Copy link
Author

Choose a reason for hiding this comment

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

containers:
- env:
spec:
restartPolicy: Always
Copy link
Author

Choose a reason for hiding this comment

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

Corrected indentation so that the yaml is rendered correctly.

key: controller-key
name: cluster-agent-secret
name: {{ template "cluster-agent.secretName" . }}
Copy link
Author

@mrgadgil mrgadgil May 24, 2019

Choose a reason for hiding this comment

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

Changed hard coded name to generate one using template macro.
Secret name will also contain the release name which binds all release objects together and will allow multiple releases.

- containerPort: {{ .Values.deployment.port }}
protocol: TCP
resources:
{{- if .Values.deployment.resources }}
{{ toYaml .Values.deployment.resources | indent 10 }}
{{ toYaml .Values.deployment.resources | indent 12 }}
Copy link
Author

Choose a reason for hiding this comment

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

Corrected indentation so that the yaml is rendered correctly.

{{- else if .Values.resources }}
{{ toYaml .Values.resources | indent 10 }}
{{ toYaml .Values.resources | indent 12 }}
Copy link
Author

Choose a reason for hiding this comment

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

Corrected indentation so that the yaml is rendered correctly.

helm.sh/chart: {{ include "cluster-agent.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
release: {{ .Release.Name }}
Copy link
Author

Choose a reason for hiding this comment

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

helm.sh/chart: {{ include "cluster-agent.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
release: {{ .Release.Name }}
Copy link
Author

Choose a reason for hiding this comment

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

helm.sh/chart: {{ include "cluster-agent.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
release: {{ .Release.Name }}
Copy link
Author

Choose a reason for hiding this comment

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

@@ -4,9 +4,10 @@ metadata:
name: {{ include "cluster-agent.fullname" . }}
labels:
app.kubernetes.io/name: {{ include "cluster-agent.name" . }}
helm.sh/chart: {{ include "cluster-agent.chart" . }}
helm.sh/chart: {{ include "cluster-agent.name" . }}
Copy link
Author

Choose a reason for hiding this comment

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

Labels should not include helm chart version as it can create problems during chart upgrades as the chart version will change with upgrade.

@@ -3,14 +3,22 @@ deployment:
image:
repository: docker.io/appdynamics/cluster-agent
tag: latest
pullPolicy: IfNotPresent
pullPolicy: Always
Copy link
Author

@mrgadgil mrgadgil May 24, 2019

Choose a reason for hiding this comment

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

IBM recommends to use a tag instead of using latest docker image.
If latest tag is used for docker image, then it is advised to use pull policy of always to get the latest available image.
This is more a recommendation geared towards promoting the chart to become a cloud pak. Not required at our initial pass.
https://github.com/helm/helm/blob/master/docs/chart_best_practices/pods.md#images

serviceAccount:
create: true
name: appdynamics-operator

metrics-server:
install: true
Copy link
Author

Choose a reason for hiding this comment

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

Removed as these configure values are not used in the chart.
Removing them because these unused configure parameters will appear in ICP UI configure page. They will confuse users who set them and see no effect in the chart.

memory:
requests:
cpu:
memory:
Copy link
Author

Choose a reason for hiding this comment

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

Adding resources in values.yaml as they are used by the chart.
Values that are set in values.yaml appear as configurable options while deploying chart using ICP UI.
On another note: Do we need deployment.resources as well resources? Will it be okay to keep only one config option?

@@ -3,13 +3,24 @@ apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ .Values.serviceAccount.name }}
namespace: {{ .Release.Namespace }}
Copy link
Author

Choose a reason for hiding this comment

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

namespace is not a valid field for ServiceAccount.

serviceAccount:
create: true
name: appdynamics-operator

metrics-server:
install: true
existingSecret:
Copy link
Author

Choose a reason for hiding this comment

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

Added existing secret so that user can use a pre-created secret to pass sensitive information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant