-
Notifications
You must be signed in to change notification settings - Fork 104
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
Add registerAPIService option to external metrics feature to allow disabling external metrics endpoint registration #953
Conversation
…user to disable external metrics registration
cdd11ec
to
88041cc
Compare
88041cc
to
8d7661c
Compare
Codecov Report
@@ Coverage Diff @@
## main #953 +/- ##
==========================================
+ Coverage 56.82% 56.83% +0.01%
==========================================
Files 155 155
Lines 19279 19284 +5
==========================================
+ Hits 10956 10961 +5
Misses 7675 7675
Partials 648 648
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
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.
Thanks for comprehensive test plan in PR description.
DDAv2: newV2Agent(true, true, false, secretV2), | ||
DDAv2: newV2Agent(true, true, true, false, secretV2), | ||
WantConfigure: true, | ||
ClusterAgent: testDCAResources(true, false, true), |
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.
This and next test pass different values for registerAPIService
but make same assertions usingtestDCAResources(true, false, true)
which means testDCAResources
is probably not covering resources what's inside if f.registerAPIService {..
. Could you please fix that?
@@ -535,6 +535,11 @@ type ExternalMetricsServerFeatureConfig struct { | |||
// +optional | |||
Enabled *bool `json:"enabled,omitempty"` | |||
|
|||
// RegisterAPIService registers the External Metrics endpoint as an APIService | |||
// Default: true |
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.
Could you please add default value to datadogagent_default.go
and set it in the same file, see this example.
@@ -19,33 +19,33 @@ import ( | |||
|
|||
func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenAPIDefinition { | |||
return map[string]common.OpenAPIDefinition{ | |||
"./apis/datadoghq/v2alpha1.CSPMHostBenchmarksConfig": schema__apis_datadoghq_v2alpha1_CSPMHostBenchmarksConfig(ref), |
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.
Could you check this please as well, diff shouldn't be this big. It also causes CI failure.
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.
This file is automatically generated by running make generate manifests
For some reason, if $GOPATH is set, it results in a huge diff.
Setting $GOPATH to "/tmp" temporarily fixes the issue.
I am not sure why this is the case.
Name: "v2alpha1 external metrics enabled, secrets set, registerAPIService enabled", | ||
DDAv2: newV2Agent(true, true, true, false, secretV2), | ||
WantConfigure: true, | ||
WantDependenciesFunc: func(t testing.TB, store dependencies.StoreClient) { |
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.
Thanks for improving the test!
…sabling external metrics endpoint registration (#953) * Add registerEndpoint option to external metrics feature to allow the user to disable external metrics registration * generate manifests * Rename field to registerAPIService * fix generate manifests * Add defaultRegisterAPIService in datadogagent_default.go * Add unit tests for external metrics feature and for RegisterAPIService default value
What does this PR do?
This PR allows the user to enable creating an external metrics server while still disabling registering the external metrics server as an APIService.
It allows this by adding the option
registerAPIService
to theexternalMetrics
feature to allow the user to disable registering external-metrics server as an APIServiceMotivation
This allows the user to use other external-metrics providers (such as KEDA) in the cluster, but also use the datadog cluster-agent as a metrics proxy to datadog.
Additional Notes
registerAPIService
is defaulted to true inexternalMetrics
feature for backward compatibility. By default we register the endpoint if the user enables external metrics server.Describe your test plan
We can test this by deploying the agent using the operator on a kind cluster locally.
Steps:
make IMG=test/operator:test docker-build
kind
:kind load docker-image test/operator:test
make IMG=test/operator:test deploy
examples/datadogagent/v2alpha1/min.yaml
by the following definitionkubectl get apiservices.apiregistration.k8s.io | grep external
. It should produce an empty output if the endpoint is not registered. If it is registered, the output should show the endpoint name with status beingTrue
(Example:v1beta1.external.metrics.k8s.io system/datadog-cluster-agent-metrics-server True 12s
)Checklist
bug
,enhancement
,refactoring
,documentation
,tooling
, and/ordependencies
qa/skip-qa
label