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 configcat plugin to proxy #12756

Merged
merged 3 commits into from
Sep 9, 2022
Merged

Add configcat plugin to proxy #12756

merged 3 commits into from
Sep 9, 2022

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Sep 8, 2022 •
edited
Loading

Description

Bring ConfigCat config.json file to proxy

For other projects that want to use configcat

Thanks for recommending @easyCZ provided: Point to a domain which is closest to your "type of install" [1]

  • For self-hosted, point to the domain in self-hosted
  • For prod, point to gitpod.io
  • For non-prod instances, without a preview env, point to gitpod-staging
  • For non-prod instances with a preview env, point to preview env domain

configcat baseUrl should be <host>/configcat

Related Issue(s)

Fixes #12739

How to test

CURL

# fetch config should response with correct json
curl -v "https://mustard-mh2ea42534cd.preview.gitpod-dev.com/configcat/configuration-files/gitpod/config_v5.json"

# with etag should response with 302
curl -v "https://mustard-mh2ea42534cd.preview.gitpod-dev.com/configcat/configuration-files/gitpod/config_v5.json" \
     -H 'If-None-Match: W/"63161d7f-771"'

SDK

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@mustard-mh mustard-mh force-pushed the mustard-mh/proxy-configcat-config-12739 branch 3 times, most recently from bfa739d to 084e14a Compare September 8, 2022 16:48
@mustard-mh mustard-mh marked this pull request as ready for review September 8, 2022 16:48
@mustard-mh mustard-mh requested review from a team September 8, 2022 16:48
@github-actions github-actions bot added team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team labels Sep 8, 2022
@mustard-mh mustard-mh marked this pull request as draft September 8, 2022 16:51
@mustard-mh mustard-mh force-pushed the mustard-mh/proxy-configcat-config-12739 branch from 084e14a to 1eeb694 Compare September 8, 2022 16:57
@mustard-mh mustard-mh changed the title [WIP] Add configcat cdn to proxy Add configcat plugin to proxy Sep 8, 2022
@mustard-mh mustard-mh marked this pull request as ready for review September 8, 2022 17:00
@mustard-mh mustard-mh mentioned this pull request Sep 8, 2022
Closed
2 tasks
easyCZ
easyCZ approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Looks awesome, nice work! Left a couple of comments but these are non-blocking (adding hold if you do want to go for them). Feel free to land when you're ready.

/hold

components/proxy/plugins/configcat/configcat.go Outdated

// fetchConfigCatConfig with different config version. i.e. config_v5.json
func (c *ConfigCat) fetchConfigCatConfig(configVersion string) ([]byte, error) {
b, err, _ := sg.Do(fmt.Sprintf("fetch_%s", configVersion), func() (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice use of singleflight!

Copy link
Member

Choose a reason for hiding this comment

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

Shame golang/go#53427 has not yet landed, would make retrieval of the result nicer.

install/installer/pkg/config/v1/experimental/experimental.go Outdated Show resolved Hide resolved
install/installer/pkg/components/proxy/deployment.go Outdated Show resolved Hide resolved
@mustard-mh mustard-mh force-pushed the mustard-mh/proxy-configcat-config-12739 branch from 1eeb694 to f3dae22 Compare September 8, 2022 18:54
@mustard-mh mustard-mh force-pushed the mustard-mh/proxy-configcat-config-12739 branch from f3dae22 to be5c727 Compare September 8, 2022 19:04
@mustard-mh
Copy link
Contributor Author

Tested with empty response (for self-hosted) with SDK, configcat print error message and fallback to default false

image

@mustard-mh
Copy link
Contributor Author

Follow up ops PR https://github.com/gitpod-io/ops/pull/4464

jeanp413
jeanp413 reviewed Sep 9, 2022
View reviewed changes
components/proxy/plugins/configcat/configcat.go

var (
DefaultConfig = []byte("{}")
pathRegex = regexp.MustCompile(`^/configcat/configuration-files/gitpod/config_v\d+\.json$`)
Copy link
Member

Choose a reason for hiding this comment

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

In gitpod-desktop we use both keys production and non-production for testing, having gitpod harcoded only maps to one key at a time, maybe better to send the configcat key dinamically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've thought about this before, we could

  • Create a new preview env to do this test. But it does make the steps more complicated
  • Change baseUrl when creating configcat client and sdkKey if we want to connect to non-product env?

Copy link
Member

Choose a reason for hiding this comment

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

yeah neither of those are nice 😞

  • option 1 is definitely a no-no for me
  • option 2, so we would use the configcat cdn as we do right now, correct?

cc @akosyakov thoughts?

Copy link
Contributor Author

@mustard-mh mustard-mh Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • Use product env in vscode-desktop is not harmful because configcat is calculating locally.
  • If we bring non-product env to product gitpod, it will fetch non-product data every minute, but it's not need to do so
  • Option 2 (change code if needed) can be a solution, i.e. use staging Gitpod as baseUrl which is using non-product SDK key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanp413 see #12756 (comment)

@mustard-mh
Copy link
Contributor Author

Concurrent r/w cache map will make some data update delay for around 1 min (pollDuration), but it's not harmful.

We can change it to sync.Map if you want

@mustard-mh
Copy link
Contributor Author

For other projects that want to use configcat

Thanks for recommending @easyCZ provided: Point to a domain which is closest to your "type of install" [1]

  • For self-hosted, point to the domain in self-hosted
  • For prod, point to gitpod.io
  • For non-prod instances, without a preview env, point to gitpod-staging
  • For non-prod instances with a preview env, point to preview env domain

configcat baseUrl should be <host>/configcat

Added in PR description

@mustard-mh mustard-mh force-pushed the mustard-mh/proxy-configcat-config-12739 branch from c49f6db to 9af1736 Compare September 9, 2022 10:12
@mustard-mh
Copy link
Contributor Author

Rebased to squash commits. Going to unhold it

@mustard-mh
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit c8d0dd1 into main Sep 9, 2022
@roboquat roboquat deleted the mustard-mh/proxy-configcat-config-12739 branch September 9, 2022 10:25
@mustard-mh mustard-mh mentioned this pull request Sep 9, 2022
Merged
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanp413 jeanp413 jeanp413 left review comments

@easyCZ easyCZ easyCZ approved these changes

@nandajavarma nandajavarma nandajavarma approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production release-note-none size/XXL team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proxy ConfigCat config in caddy
5 participants
@mustard-mh @easyCZ @nandajavarma @jeanp413 @roboquat