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.

Sign up for GitHub

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

Jump to bottom

Add option to add authentication to Jaeger and custom tags #13728

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Add option to add authentication to Jaeger and custom tags #13728

merged 1 commit into from
Oct 11, 2022

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Oct 10, 2022 •
edited by atduarte
Loading

Description

Related Issue(s)

Fixes #13381
Fixes #13385

How to test

  • Configure the installer setting
tracing:
  secretName: XXXXX

and verify the env variablesJAEGER_USER and JAEGER_PASSWORD are configured.

Release Notes

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-aledbf-jaeger.3 because the annotations in the pull request description changed
(with .werft/ from main)

@aledbf aledbf marked this pull request as ready for review October 10, 2022 19:45
@aledbf aledbf requested review from a team October 10, 2022 19:45
@github-actions github-actions bot added team: IDE team: self-hosted team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Oct 10, 2022
@aledbf
Copy link
Member Author

aledbf commented Oct 10, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-aledbf-jaeger.6
(with .werft/ from main)

@aledbf
Copy link
Member Author

aledbf commented Oct 10, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview with-clean-slate-deployment

👍 started the job as gitpod-build-aledbf-jaeger.7
(with .werft/ from main)

@aledbf
Copy link
Member Author

aledbf commented Oct 10, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-clean-slate-deployment with-preview

👍 started the job as gitpod-build-aledbf-jaeger.9
(with .werft/ from main)

easyCZ
easyCZ reviewed Oct 11, 2022
View reviewed changes
install/installer/pkg/common/common.go
Comment on lines +157 to +173
if context.Config.Observability.Tracing.SecretName != nil {
res = append(res, corev1.EnvVar{
Name: "JAEGER_USER",
ValueFrom: &corev1.EnvVarSource{SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: *context.Config.Observability.Tracing.SecretName},
Key: "JAEGER_USER",
}},
})

res = append(res, corev1.EnvVar{
Name: "JAEGER_PASSWORD",
ValueFrom: &corev1.EnvVarSource{SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: *context.Config.Observability.Tracing.SecretName},
Key: "JAEGER_PASSWORD",
}},
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we expose these as ENV variables? In general, mounting the secret as a volume is preferred for credentials and here we could do that if the SecretName is specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's how the library is configured.

gitpod/components/common-go/tracing/tracing.go

Line 42 in 45b0b45

cfg, err := jaegercfg.FromEnv()

Copy link
Member

Choose a reason for hiding this comment

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

We could construct that manually, from the information we mount - https://pkg.go.dev/github.com/uber/jaeger-client-go/config#Configuration

I'll raise an issue as a follow-up (which will likely never get done) as I don't want to block your change but we do abuse env variables a fair bit.

install/installer/pkg/common/common.go
@@ -112,7 +112,7 @@ func DefaultEnv(cfg *config.Config) []corev1.EnvVar {
)
}

func WorkspaceTracingEnv(context *RenderContext) (res []corev1.EnvVar) {
func WorkspaceTracingEnv(context *RenderContext, component string) (res []corev1.EnvVar) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this indirection here if we're actually keeping them the same? Could we instead consolidate on func TracingEnv(...). If the need for these to actually be different arises, we can refactor.

Copy link
Member Author

@aledbf aledbf Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know the reason.
ping @mrsimonemms

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing to do with me I'm afraid. This was added by @andrew-farries around 5 months ago - 1119e55

@roboquat roboquat merged commit def55ee into main Oct 11, 2022
@roboquat roboquat deleted the aledbf/jaeger branch October 11, 2022 11:51
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: IDE IDE change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Oct 11, 2022
@atduarte
Copy link
Contributor

@aledbf updated release notes as it didn't seem to be useful for the end Gitpod user. Let me know if this change is wrong.

@roboquat
Copy link
Contributor

@aledbf: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@aledbf
Copy link
Member Author

aledbf commented Oct 27, 2022

@atduarte it depends on who is the "Gitpod user". If you are installing Gitpod, this is an important thing to know.

@atduarte
Copy link
Contributor

@aledbf I understand the confusion. Release Notes are automatically added to the gitpod.io changelog (October's, September's) and therefore should focus only on the end-user (i.e. the developer persona). Some additional internal information can be found here: 1, 2.

@loujaybee
Copy link
Member

loujaybee commented Oct 27, 2022 •
edited
Loading

@atduarte it depends on who is the "Gitpod user". If you are installing Gitpod, this is an important thing to know - @aledbf

You are correct that we should treat the installer as a first-class citizen for the Self-Hosted context, however see below for reasons on why we should omit for now 👇

Release Notes are automatically added to the gitpod.io changelog (https://github.com/gitpod-io/website/pull/2883, September's) and therefore should focus only on the end-user - @atduarte

Currently, yes. That's also my understanding. @lucasvaltl can confirm, but I believe Self-Hosted are cherry-picking PR's and manually curating the release notes right now (see past example) and omitting Release Notes. No defined heuristic exists (that I'm aware of) to distinguish Self-Hosted / installer facing pull requests from SaaS or developer persona pull requests... that heuristic needs creating and agree'ing on, then we can easily distinguish between the two.

CC: @filiptronicek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrsimonemms mrsimonemms mrsimonemms left review comments

@jenting jenting jenting approved these changes

@jeanp413 jeanp413 jeanp413 approved these changes

@easyCZ easyCZ easyCZ approved these changes

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production do-not-merge/release-note-label-needed size/M team: IDE team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add additional JAEGER_TAGS and JAEGER_SERVICE_NAME env vars Add option to set Jaeger authentication
8 participants
@aledbf @atduarte @roboquat @loujaybee @mrsimonemms @easyCZ @jeanp413 @jenting