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

Improve Openshift documentation #17867

Merged
merged 6 commits into from
Apr 23, 2020

Conversation

ChrsMark
Copy link
Member

What does this PR do?

Adds required information for Openshift compatibility.

Why is it important?

In order to have updated docs regarding Openshift support.

@ChrsMark ChrsMark added docs review [zube]: In Review Team:Platforms Label for the Integrations - Platforms team labels Apr 21, 2020
@ChrsMark ChrsMark self-assigned this Apr 21, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

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.

Just some suggestions.

@@ -85,6 +85,7 @@ data:
#- /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
- module: kubernetes
metricsets:
# Not supported on Openshift
Copy link
Contributor

Choose a reason for hiding this comment

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

Being this would make the module line have no metricsets enabled, would it be better to place this above - module: kubernetes.

Say something along the lines of:

Currently not support on Openshift, comment out section.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

securityContext:
runAsUser: 0
privileged: true
-----
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be good to place this inside of the DaemonSet commented out, that way it can be quickly commented out.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, would it be possible to use the oc patch command to streamline adding this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@blakerouse we already have it

# If using Red Hat OpenShift uncomment this:
(and some for Fileveat).

@exekias it is possible with oc -n kube-system patch ds metricbeat -p '{"spec": {"template": {"spec": {"containers": [{"name": "metricbeat", "securityContext": {"privileged": true, "runAsUser": 0}}]}}}}' but it will restart the pods of the DS. Is this really wanted?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, by looking at the command and restart behavior, sounds like uncommenting may actually be cleaner

Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
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.

Looks good to me.

Comment on lines 115 to 117
- ${VALID_CA}
-----
NOTE: `VALID_CA` can be any CA that is valid so as to reach out the Kubelet API,
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Use a different format for this, so it doesn't look like an environment variable that should be defined somewhere.

Suggested change
- ${VALID_CA}
-----
NOTE: `VALID_CA` can be any CA that is valid so as to reach out the Kubelet API,
- /path/to/kubelet-service-ca.crt
-----
NOTE: `kubelet-service-ca.crt` can be any CA bundle that contains the issuer of the certificate used in the Kubeler API,

Comment on lines 118 to 120
for instance `/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt`. According to each specific installation
of Openshift this can be found either in `secrets` or in `configmaps`.
For instance, in case of using Openshift installer[https://github.com/openshift/installer/blob/master/docs/user/gcp/install.md]
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Reorganize it a little bit to more explicitly mention where this can be found. It could be nice to explicitly say where this CA can be found depending on the installation method.

Suggested change
for instance `/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt`. According to each specific installation
of Openshift this can be found either in `secrets` or in `configmaps`.
For instance, in case of using Openshift installer[https://github.com/openshift/installer/blob/master/docs/user/gcp/install.md]
According to each specific installation of Openshift this can be found either in `secrets` or in `configmaps`.
In some installations it can be available as part of the service account secret, in `/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt`.
In case of using Openshift installer[https://github.com/openshift/installer/blob/master/docs/user/gcp/install.md]

Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
@ChrsMark ChrsMark added needs_backport PR is waiting to be backported to other branches. v7.8.0 labels Apr 23, 2020
@ChrsMark ChrsMark merged commit c46268a into elastic:master Apr 23, 2020
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Apr 23, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Apr 23, 2020
ChrsMark added a commit that referenced this pull request Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs review Team:Platforms Label for the Integrations - Platforms team v7.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants