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

feat: Enable prometheus merge as a configuration in the Kyma Istio CR #1232

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cd7d91d
add prometheus merge api field
jeffreylimnardy Dec 19, 2024
9d3e24a
restart proxy sidecars when enableprometheusmerge is changed
jeffreylimnardy Dec 19, 2024
30ea336
enable prometheus merge to use new structure
jeffreylimnardy Dec 19, 2024
2e5a838
update docs
jeffreylimnardy Jan 3, 2025
8048210
Merge branch 'main' into prommerge-feature
jeffreylimnardy Jan 7, 2025
db06a2e
update api definition
jeffreylimnardy Jan 8, 2025
485d735
update docs
jeffreylimnardy Jan 8, 2025
f427be5
Merge branch 'main' into prommerge-feature
jeffreylimnardy Jan 8, 2025
163b80f
omit empty
jeffreylimnardy Jan 8, 2025
7bd3093
Merge branch 'main' into prommerge-feature
jeffreylimnardy Jan 13, 2025
c125da6
add line about nested fields
jeffreylimnardy Jan 13, 2025
2225c45
add RN
jeffreylimnardy Jan 13, 2025
ad1b0c3
Merge branch 'main' into prommerge-feature
jeffreylimnardy Jan 27, 2025
70d218a
fix UT lastAppliedConfiguration
jeffreylimnardy Jan 27, 2025
7e354bf
fix typo
jeffreylimnardy Jan 27, 2025
04e387e
add small detail to docs
jeffreylimnardy Jan 27, 2025
fda3e63
update ADR status
jeffreylimnardy Jan 30, 2025
953cdb3
Merge branch 'main' into prommerge-feature
jeffreylimnardy Jan 30, 2025
8ed2097
account for refactor proxy reset
jeffreylimnardy Jan 30, 2025
d70a171
add missing error handling
jeffreylimnardy Jan 30, 2025
8a3d6a6
move release-notes to 1.15.0
jeffreylimnardy Feb 3, 2025
11981cd
Merge branch 'main' into prommerge-feature
jeffreylimnardy Feb 3, 2025
1e48ff2
add newline
jeffreylimnardy Feb 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions api/v1alpha2/istio_merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v1alpha2

import (
"encoding/json"

"istio.io/istio/operator/pkg/values"
"istio.io/istio/pkg/util/protomarshal"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -66,6 +67,15 @@ func (m *meshConfigBuilder) BuildNumTrustedProxies(numTrustedProxies *int) *mesh
return m
}

func (m *meshConfigBuilder) BuildPrometheusMergeConfig(prometheusMerge bool) *meshConfigBuilder {
err := m.c.SetPath("enablePrometheusMerge", prometheusMerge)
if err != nil {
return nil
}

return m
}

func (m *meshConfigBuilder) AddProxyMetadata(key, value string) (*meshConfigBuilder, error) {
err := m.c.SetPath("defaultConfig.proxyMetadata."+key, value)
if err != nil {
Expand Down Expand Up @@ -160,6 +170,7 @@ func (i *Istio) mergeConfig(op iopv1alpha1.IstioOperator) (iopv1alpha1.IstioOper
newMeshConfig := mcb.
BuildNumTrustedProxies(i.Spec.Config.NumTrustedProxies).
BuildExternalAuthorizerConfiguration(i.Spec.Config.Authorizers).
BuildPrometheusMergeConfig(i.Spec.Config.Telemetry.Metrics.PrometheusMerge).
Build()

op.Spec.MeshConfig = newMeshConfig
Expand Down
4 changes: 4 additions & 0 deletions api/v1alpha2/istio_structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ type Config struct {
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Enum=Local;Cluster
GatewayExternalTrafficPolicy *string `json:"gatewayExternalTrafficPolicy,omitempty"`

// Defines the telemetry configuration of Istio.
// +kubebuilder:validation:Optional
Telemetry Telemetry `json:"telemetry,omitempty"`
}

type Components struct {
Expand Down
39 changes: 38 additions & 1 deletion api/v1alpha2/merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ package v1alpha2

import (
"encoding/json"
"testing"

"github.com/kyma-project/istio/operator/internal/tests"
"github.com/onsi/ginkgo/v2/types"
"google.golang.org/protobuf/types/known/structpb"
"google.golang.org/protobuf/types/known/wrapperspb"
meshv1alpha1 "istio.io/api/mesh/v1alpha1"
iopv1alpha1 "istio.io/istio/operator/pkg/apis"
"istio.io/istio/operator/pkg/values"
Expand All @@ -14,7 +17,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -373,6 +375,41 @@ var _ = Describe("Merge", func() {
Expect(numTrustedProxies).To(Equal(float64(5)))
})

It("should set prometheusMerge on IstioOperator Telemetry Metrics to true when meshConfig has a enablePrometheusMerge with true", func() {
// given
m := &meshv1alpha1.MeshConfig{
EnablePrometheusMerge: wrapperspb.Bool(false),
}
meshConfigRaw := convert(m)

iop := iopv1alpha1.IstioOperator{
Spec: iopv1alpha1.IstioOperatorSpec{
MeshConfig: meshConfigRaw,
},
}
istioCR := Istio{Spec: IstioSpec{Config: Config{
Telemetry: Telemetry{
Metrics: Metrics{
PrometheusMerge: true,
},
},
}}}

// when
out, err := istioCR.MergeInto(iop)

// then
Expect(err).ShouldNot(HaveOccurred())

meshConfig, err := values.MapFromObject(out.Spec.MeshConfig)
Expect(err).ShouldNot(HaveOccurred())

enabledPrometheusMerge, exists := meshConfig.GetPath("enablePrometheusMerge")
Expect(exists).To(BeTrue())
Expect(enabledPrometheusMerge).To(BeTrue())

})

It("should create IngressGateway overlay with externalTrafficPolicy set to Local", func() {
// given

Expand Down
15 changes: 15 additions & 0 deletions api/v1alpha2/telemetry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package v1alpha2

type Telemetry struct {
// Istio telemetry configuration related to metrics
// +kubebuilder:validation:Optional
Metrics Metrics `json:"metrics,omitempty"`
}

type Metrics struct {
// Defines whether the prometheusMerge feature is enabled. If yes, appropriate prometheus.io annotations will be added to all data plane pods to set up scraping.
// If these annotations already exist, they will be overwritten. With this option, the Envoy sidecar will merge Istio’s metrics with the application metrics.
// The merged metrics will be scraped from :15020/stats/prometheus.
// +kubebuilder:validation:Optional
PrometheusMerge bool `json:"prometheusMerge,omitempty"`
}
32 changes: 32 additions & 0 deletions api/v1alpha2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions config/crd/bases/operator.kyma-project.io_istios.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,20 @@ spec:
maximum: 4294967295
minimum: 0
type: integer
telemetry:
description: Defines the telemetry configuration of Istio.
properties:
metrics:
description: Istio telemetry configuration related to metrics
properties:
prometheusMerge:
description: |-
Defines whether the prometheusMerge feature is enabled. If yes, appropriate prometheus.io annotations will be added to all data plane pods to set up scraping.
If these annotations already exist, they will be overwritten. With this option, the Envoy sidecar will merge Istio’s metrics with the application metrics.
The merged metrics will be scraped from :15020/stats/prometheus.
type: boolean
type: object
type: object
type: object
experimental:
properties:
Expand Down
3 changes: 3 additions & 0 deletions config/ui-extensions/istios/details
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ body:
- source: spec.compatibilityMode
name: compatibilityMode
placeholder: 'false'
- source: spec.config.telemetry.metrics.prometheusMerge
name: config.prometheusMerge
placeholder: 'false'

- name: config.authorizers
widget: Table
Expand Down
5 changes: 5 additions & 0 deletions config/ui-extensions/istios/form
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
name: compatibilityMode
value:
type: bool
- path: config.telemetry.metrics.prometheusMerge
simple: true
name: config.prometheusMerge
value:
type: bool
- path: spec.config.authorizers
name: config.authorizers
widget: GenericList
Expand Down
1 change: 1 addition & 0 deletions config/ui-extensions/istios/translations
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ en:
config.authorizers.headers.toDownstream.onAllow: Forward To Downstream On Allow
config.authorizers.headers.toDownstream.onDeny: Forward To Downstream On Deny
config.gatewayExternalTrafficPolicy: Gateway external traffic policy
config.prometheusMerge: Enable PrometheusMerge

k8s.hpaSpec: Horizontal Pod Autoscaler
k8s.hpaSpec.minReplicas: Minimum number of replicas
Expand Down
2 changes: 1 addition & 1 deletion controllers/istio_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ var _ = Describe("Istio Controller", func() {
Expect(err).To(Not(HaveOccurred()))

Expect(updatedIstioCR.Status.State).Should(Equal(operatorv1alpha2.Ready))
Expect(updatedIstioCR.Annotations["operator.kyma-project.io/lastAppliedConfiguration"]).To(ContainSubstring("{\"config\":{\"numTrustedProxies\":2},"))
Expect(updatedIstioCR.Annotations["operator.kyma-project.io/lastAppliedConfiguration"]).To(ContainSubstring("{\"config\":{\"numTrustedProxies\":2,\"telemetry\":{\"metrics\":{}}},"))

Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Support for Enable PrometheusMerge Configuration

## Status
Proposed

## Context
There is a need to support the configuration of enabling `prometheusMerge` to be available in the Istio CR. The background behind this need is explained in [this issue](https://github.com/kyma-project/telemetry-manager/issues/1468) and the impact of this feature being enabled was discussed in an [ADR](https://github.com/kyma-project/telemetry-manager/blob/main/docs/contributor/arch/015-impact-of-istio-prometheus-merge-on-metric-pipelines.md) created in the Telemetry Manager repository.

## Considerations
Based on the [original ADR](https://github.com/kyma-project/telemetry-manager/blob/main/docs/contributor/arch/015-impact-of-istio-prometheus-merge-on-metric-pipelines.md), the plan was to originally enable the `prometheusMerge` feature by default. But this has caused problems for users deploying prometheus with a scrape loop sidecar. More details can be found in [this PR](https://github.com/kyma-project/istio/pull/1184).


## Decision
The enablement of the `prometheusMerge` feature needs to be a configurable option for the users. By default it will be set to `false`, but users will then need to actively set it to `true` and understand the effects of the feature.

To allow for this configuration, the following API is proposed in the CR under the config option, `prometheusMerge` will have the default value of `false`.
```yaml
apiVersion: operator.kyma-project.io/v1alpha2
kind: Istio
metadata:
name: default
namespace: kyma-system
spec:
config:
telemetry:
metrics:
prometheusMerge: false
```

The `telemetry` and `metrics` fields are introduced here to account for future plans to introduce more feature from the Istio Telemetry API into the CR.

## Consequences
Istio CustomResourceDefinition will be extended with an additional configuration field of `telemetry.metrics.prometheusMerge` that will allow for configuration of the `prometheusMerge` setting in Istio Mesh Config. The field will be an optional configuration with the default value of `false`.

The `telemetry` and `metrics` field will show up in the CR when retrieved from the API server even if not explicitly defined in the manifest.

### Sample configuration

1.
```
apiVersion: istio.operator.kyma-project.io/v1alpha2
kind: Istio
metadata:
name: default

```
This will use the default value of `false` for the `prometheusMerge` option.

2.
```
apiVersion: istio.operator.kyma-project.io/v1alpha2
kind: Istio
metadata:
name: default
spec:
config:
telemetry:
metrics:
prometheusMerge: true
```
This will set the value of `prometheusMerge` to `true`.


3 changes: 3 additions & 0 deletions docs/release-notes/1.14.0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
## New Features

- Extended Istio custom resource with `prometheusMerge` configuration [#1232](https://github.com/kyma-project/istio/pull/1232)
1 change: 1 addition & 0 deletions docs/user/04-00-istio-custom-resource.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ This table lists all the possible parameters of Istio CR together with their des
| **config.authorizers** | \[\]authorizer | Specifies the list of external authorizers configured in the Istio service mesh config. |
| **config.numTrustedProxies** | integer | Specifies the number of trusted proxies deployed in front of the Istio gateway proxy. |
| **config.gatewayExternalTrafficPolicy** | string | Defines the external traffic policy for Istio Ingress Gateway Service. Valid configurations are `Local` or `Cluster`. The external traffic policy set to `Local` preserves the client IP in the request but also introduces the risk of unbalanced traffic distribution. |
| **config.telemetry.metrics.prometheusMerge** | bool | Enables the prometheusMerge feature from Istio, which will merge application's and istio's metrics and expose them together at `:15020/stats/prometheus` for scraping using plain HTTP |
| **experimental** | object | Defines additional experimental features that can be enabled in experimental builds. |
| **experimental.pilot** | object | Defines additional experimental features that can be enabled in Istio pilot component. |
| **experimental.pilot.enableAlphaGatewayAPI** | bool | Enables support for alpha Kubernetes Gateway API. |
Expand Down
28 changes: 28 additions & 0 deletions internal/prometheusmerge/proxy_restart.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package prometheusmerge

import (
"github.com/kyma-project/istio/operator/api/v1alpha2"
"github.com/kyma-project/istio/operator/internal/reconciliations/istio"
v1 "k8s.io/api/core/v1"
)

type ProxyRestartPredicate struct {
oldPrometheusMerge bool
newPrometheusMerge bool
}

func NewRestartPredicate(istioCR *v1alpha2.Istio) (*ProxyRestartPredicate, error) {
lastAppliedConfig, err := istio.GetLastAppliedConfiguration(istioCR)
if err != nil {
return nil, err
}

return &ProxyRestartPredicate{
oldPrometheusMerge: lastAppliedConfig.IstioSpec.Config.Telemetry.Metrics.PrometheusMerge,
newPrometheusMerge: istioCR.Spec.Config.Telemetry.Metrics.PrometheusMerge,
}, nil
}

func (p ProxyRestartPredicate) RequiresProxyRestart(_ v1.Pod) bool {
return p.oldPrometheusMerge != p.newPrometheusMerge
}
Loading
Loading