-
Notifications
You must be signed in to change notification settings - Fork 7
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
logging(LOG-5224): adds support for clusterset installations #48
Conversation
Pull Request Test Coverage Report for Build 9287722958Details
💛 - Coveralls |
78a7abe
to
06e4d40
Compare
I think this PR is introducing too many changes at the same time and we should split it.
Please, can you elaborate a little bit more on the difference?
Can you explain how this affects the proposal openshift/enhancements#1613? I think now we only need to provide the secrets stuff since all the configurations will be "hardcoded" in the template, right?
This means we don't need to provide the authentication Configmap anymore, right?
To be honest, I'm not sure why we need to annotate the secret. Does it have any other functionality than just "explain that this secret is used here"? I mean: are we really using that annotation for something? |
Issue: https://issues.redhat.com/browse/LOG-5224 In this commit we: - Changed how the reconciliation is done on the logging side, we now only require the user to provide a ClusterLogForwarder (CLF); - Have dropped the templating of CLF using ConfigMap's as the implementation used was error prone; - Moved the auth information from a ConfigMap to the annotations of ClusterLogForwarder;
Tested with the following resources: ClusterManagementAddOnapiVersion: addon.open-cluster-management.io/v1alpha1
kind: ClusterManagementAddOn
metadata:
name: multicluster-observability-addon
annotations:
addon.open-cluster-management.io/lifecycle: addon-manager
spec:
addOnMeta:
displayName: Multi Cluster Observability Addon
description: "multicluster-observability-addon is the addon to configure spoke clusters to collect and forward logs/traces to a given set of outputs"
supportedConfigs:
# Describes the general addon configuration applicable for all managed clusters. It includes:
# - Default subscription channel name for install the `Red Hat OpenShift Logging` operator on each managed cluster.
# - Default subscription channel name for install the `Red Hat OpenShift distributed tracing data collection` operator on each managed cluster.
- group: addon.open-cluster-management.io
resource: addondeploymentconfigs
defaultConfig:
name: multicluster-observability-addon
namespace: open-cluster-management
# Describe per managed cluster sensitive data per target forwarding location, currently supported:
# - TLS client certificates for mTLS communication with a log output / trace exporter.
# - Client credentials for password based authentication with a log output / trace exporter.
- resource: secrets
# Describe per managed cluster auxilliary config per log output / trace exporter.
- resource: configmaps
# Describes the default log forwarding outputs for each log type applied to all managed clusters.
- group: logging.openshift.io
resource: clusterlogforwarders
# Describes the default OpenTelemetryCollector type applied to all managed clusters.
- group: opentelemetry.io
resource: opentelemetrycollectors
# The default config is the main stanza of an OpenTelemetryCollector resource
# that describes where traces should be forwarded for all managed cluster.
defaultConfig:
name: spoke-otelcol
namespace: open-cluster-management
installStrategy:
type: Placements
placements:
- name: my-eu-clusters
namespace: default
configs:
- group: logging.openshift.io
resource: clusterlogforwarders
name: instance-eu
namespace: open-cluster-management
- name: my-us-clusters
namespace: default
configs:
- group: logging.openshift.io
resource: clusterlogforwarders
name: instance-us
namespace: open-cluster-management CLF EUapiVersion: logging.openshift.io/v1
kind: ClusterLogForwarder
metadata:
name: instance-eu
namespace: open-cluster-management
annotations:
authentication.mcoa.openshift.io/cw-infra: ExistingSecret
spec:
outputs:
- cloudwatch:
groupBy: logType
region: eu-central-1
name: cw-infra
secret:
name: aws-credentials
type: cloudwatch
pipelines:
- inputRefs:
- infrastructure
name: infra-cw-infra
outputRefs:
- cw-infra CLF USapiVersion: logging.openshift.io/v1
kind: ClusterLogForwarder
metadata:
name: instance-us
namespace: open-cluster-management
annotations:
authentication.mcoa.openshift.io/cw: ExistingSecret
spec:
outputs:
- cloudwatch:
region: eu-central-1
groupBy: logType
type: cloudwatch
name: cw
secret:
name: aws-credentials-default
pipelines:
- name: infra-cw
inputRefs:
- infrastructure
outputRefs:
- cw
|
This commits: - adds a function to extract from annotations the authentication method that should be used for each Target - add support for the authentication methods "ExistingSecret" that allow users to specify a secret and the secretProvider will simply validate if that secret exists. - updates the name of FetchSecrets to better reflect what the function is actually doing
authentication type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I like the approach taken here, however we need to cleanup the secrets provider sooner than later here as well as lift the tracing handler once. Both the cleanup (see my suggestions) as well as the tracing lift will make this PR more complete without leaving doubts that we still support doing things with secrets in an ambiguous way.
* testing: add helm chart to help manual tests * Port tracing to the new reconcile logic Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Add changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Rename method Signed-off-by: Israel Blancas <iblancasa@gmail.com> --------- Signed-off-by: Israel Blancas <iblancasa@gmail.com> Co-authored-by: Joao Marcal <jmarcal@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @iblancasa's opinion that this should've been split into multiple smaller PRs. There are a lot of (important) changes besides "adding support for clusterset installations", which makes reviewing this more difficult.
This LGTM, but I think we need more eyes on it.
…bservability-addon into log-5224
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM it is overall a bit more work than we expected but a pragmatic sound package we should land sooner than later. Despite that said I believe we need a couple of follow-up PRs to address the following I noticed when reviewing this:
- The usage of
context.Context
is not complete here. - The
SecretsProvider
is declared as an interface, but we don't provide an interface right now. The separation of Generate/FetchSecrets seems awkward right now because we just reconcile one type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are failing after the last commit
@periklis, please can you elaborate a bit more about this? Just to know what is missing and try to fix it. |
@JoaoBraveCoding feel free to merge :) |
Issue: https://issues.redhat.com/browse/LOG-5224
In this PR we:
require the user to provide a ClusterLogForwarder (CLF);
used was error prone;
ClusterLogForwarder;
that should be used for each Target;
that allow users to specify a secret and the secretProvider will simply
validate if that secret exists;
actually doing;
TODO:
getExistingSecret