Add telemetryCollector.cloud.resourceId field that works even when global.cloud.enabled is false
#3219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jjti
added
backport/1.1.x
jjti
commented
Nov 16, 2023
| @@ -165,28 +166,46 @@ spec: | |||
| # These are mounted as secrets so that the telemetry-collector can use them when cloud is enabled. | |||
| # - the hcp-go-sdk in consul agent will already look for HCP_CLIENT_ID, HCP_CLIENT_SECRET, HCP_AUTH_URL, | |||
| # HCP_SCADA_ADDRESS, and HCP_API_HOST. so nothing more needs to be done. | |||
| # - HCP_RESOURCE_ID is created for use in the global cloud section but we will share it here | |||
| # - HCP_RESOURCE_ID is created either in the global cloud section or in telemetryCollector.cloud | |||
There was a problem hiding this comment.
bit of re-organizing to match Values.yaml ordering
- resource-id
- client-id
- client-secret
jjti
commented
Nov 16, 2023
| --set 'telemetryCollector.clientSecret.secretKey=client-secret-id-key' \ | ||
| --set 'global.resourceId.secretName=resource-id-name' \ | ||
| --set 'global.resourceId.secretKey=resource-id-key' \ | ||
| --set 'telemetryCollector.cloud.clientSecret.secretName=client-secret-id-name' \ |
There was a problem hiding this comment.
whole lot of these kinds of bugs in the tests, missing .cloud in the secret setting
jjti
commented
Nov 16, 2023
| # If global.cloud.resourceId is set, this should either be unset (defaulting to global.cloud.resourceId) or be the same as global.cloud.resourceId. | ||
| # | ||
| # @default: global.cloud.resourceId | ||
| resourceId: |
There was a problem hiding this comment.
this is the new bit: adding telemetryCollector.cloud.resourceId
jjti
changed the title
telemetryCollector.cloud.resourceId field that works even when global.cloud.enabled is false
jjti
commented
Nov 17, 2023
| @@ -689,8 +690,7 @@ global: | |||
| # @type: string | |||
| secretKey: null | |||
|
|
|||
| # The name of the Kubernetes secret that holds the HCP cloud client id. | |||
| # This is optional when global.cloud.enabled is true. | |||
| # The hostname of HCP's API. This setting is used for internal testing and validation. | |||
jjti
commented
Nov 17, 2023
| @@ -699,8 +699,7 @@ global: | |||
| # @type: string | |||
| secretKey: null | |||
|
|
|||
| # The name of the Kubernetes secret that holds the HCP cloud authorization url. | |||
| # This is optional when global.cloud.enabled is true. | |||
| # The URL of HCP's auth API. This setting is used for internal testing and validation. | |||
jjti
commented
Nov 17, 2023
| @@ -709,8 +708,7 @@ global: | |||
| # @type: string | |||
| secretKey: null | |||
|
|
|||
| # The name of the Kubernetes secret that holds the HCP cloud scada address. | |||
| # This is optional when global.cloud.enabled is true. | |||
| # The address of HCP's scada service. This setting is used for internal testing and validation. | |||
jjti
commented
Nov 17, 2023
| @@ -3491,7 +3488,7 @@ telemetryCollector: | |||
| customExporterConfig: null | |||
|
|
|||
| service: | |||
| # This value defines additional annotations for the server service account. This should be formatted as a multi-line | |||
| # This value defines additional annotations for the telemetry-collector's service account. This should be formatted as a multi-line | |||
jjti
commented
Nov 17, 2023
| @@ -3538,9 +3575,9 @@ telemetryCollector: | |||
| # @type: string | |||
| priorityClassName: "" | |||
|
|
|||
| # A list of extra environment variables to set within the stateful set. | |||
| # A list of extra environment variables to set within the deployment. | |||
curtbushko
approved these changes
Nov 21, 2023
This was referenced Nov 21, 2023
jjti
pushed a commit
that referenced
this pull request
Nov 21, 2023
…`global.cloud.enabled` is false (#3219)
2 tasks
jjti
pushed a commit
that referenced
this pull request
Nov 21, 2023
…`global.cloud.enabled` is false (#3219)
2 tasks
jjti
pushed a commit
that referenced
this pull request
Nov 21, 2023
…`global.cloud.enabled` is false (#3219)
2 tasks
sarahalsmiller
pushed a commit
that referenced
this pull request
Jan 5, 2024
…`global.cloud.enabled` is false (#3219)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
@eddie-rowe reported that he was unable to deploy the Consul Telemetry Collector when
global.cloud.enabledwas false. This should be possible, but right now the telemetry-collector deployment pulls its resourceId out ofglobal.cloud.resourceId, which may not be set even when a user is trying to use the collector.For example, a user may link their cluster to HCP using
externalServers, whileglobal.cloudis empty (because there are no servers in the deployment), but then thetelemetryCollectorwill fail to deploy for lack ofglobal.cloud.resourceIdThe change here is to add a new field,
telemetryCollector.cloud.resourceIdthat's settable. If set, it's used in the consul-telemetry-collector deploymentJIRA: https://hashicorp.atlassian.net/browse/CC-6866
Time willing, in a follow up PR I'll add something to the
-preset cloudto parse anHCP_RESOURCE_IDenv var into aglobal.cloud.resourceIdlike we do already for client id and secret:consul-k8s/cli/preset/preset.go
Line 16 in fc70b03
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: