Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

PMM-10078 Extract portal client, add dev env variables for portal address overwriting #1136

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #1136 (ea557fa) into main (ec60eb4) will decrease coverage by 0.07%.
The diff coverage is 37.51%.

@@            Coverage Diff             @@
##             main    #1136      +/-   ##
==========================================
- Coverage   49.29%   49.21%   -0.08%     
==========================================
  Files         180      181       +1     
  Lines       20826    21390     +564     
==========================================
+ Hits        10267    10528     +261     
- Misses       9426     9689     +263     
- Partials     1133     1173      +40     
Flag Coverage Δ
all 48.87% <37.51%> (-0.08%) ⬇️
cover 49.25% <39.66%> (-0.15%) ⬇️
crosscover 49.21% <37.51%> (-0.08%) ⬇️
update 12.84% <29.11%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/pmm-managed-starlark/main.go 0.00% <0.00%> (ø)
main.go 0.00% <0.00%> (ø)
models/database.go 49.61% <0.00%> (ø)
models/node_helpers.go 71.08% <ø> (ø)
models/settings.go 100.00% <ø> (ø)
services/agents/handler.go 0.00% <0.00%> (ø)
services/agents/state.go 0.00% <0.00%> (ø)
services/alertmanager/alertmanager.go 53.92% <ø> (ø)
services/config/config.go 0.00% <ø> (ø)
services/grafana/auth_server.go 59.18% <ø> (ø)
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2050762...ea557fa. Read the comment docs.

Comment on lines +35 to 43
defaultPlatformAddress = "https://check.percona.com"
envPlatformAddress = "PERCONA_TEST_PLATFORM_ADDRESS"
envPlatformInsecure = "PERCONA_TEST_PLATFORM_INSECURE"
envPlatformPublicKey = "PERCONA_TEST_PLATFORM_PUBLIC_KEY"
// TODO REMOVE PERCONA_TEST_DBAAS IN FUTURE RELEASES.
envTestDbaas = "PERCONA_TEST_DBAAS"
envEnableDbaas = "ENABLE_DBAAS"
envPlatfromAPITimeout = "PERCONA_PLATFORM_API_TIMEOUT"
envPlatformAPITimeout = "PERCONA_PLATFORM_API_TIMEOUT"
defaultPlatformAPITimeout = 30 * time.Second
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is new variables

)

// Client is HTTP Percona Platform client.
// TODO: Replace this client with generated one https://jira.percona.com/browse/SAAS-956
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@BupycHuk BupycHuk left a comment

Choose a reason for hiding this comment

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

Great improvement

services/checks/checks_test.go Outdated Show resolved Hide resolved
services/management/ia/templates_service.go Outdated Show resolved Hide resolved
services/platform/platform.go Show resolved Hide resolved
@artemgavrilov artemgavrilov changed the title PMM-10078 Extract portal client, add dev env variables PMM-10078 Extract portal client, add dev env variables for portal address overwriting Jun 3, 2022
# - PERCONA_TEST_SAAS_HOST=check.localhost
# - PERCONA_TEST_PLATFORM_ADDRESS=https://check.localhost
# - PERCONA_TEST_PLATFORM_INSECURE=1
# - PERCONA_TEST_PLATFORM_PUBLIC_KEY=<public key>
Copy link
Contributor

@ritbl ritbl Jun 9, 2022

Choose a reason for hiding this comment

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

nit: it is not clear if it is a PATH to a key, how about renaming to something like PERCONA_TEST_PLATFORM_PUBLIC_KEY_PATH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a path, it's key itself

Copy link
Contributor

Choose a reason for hiding this comment

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

can you show example of how it is supposed to be used? the key is multiline. the user should cat it can send it?
I think, it can be a potential source of issues. ie when user set value in terminal (escaping and new line issues).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's minisign public key, it's one line, here is example:

devChecksPublicKey = "RWTg+ZmCCjt7O8eWeAmTLAqW+1ozUbpRSKSwNTmO+exlS5KEIPYWuYdX"

This variable should not be used by anyone outside Percona. We print warnings in logs each time PERCONA_TEST_* variable used. It's completely for dev/qa needs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Imagine user creates .env, and sets env var this way:
- PERCONA_TEST_PLATFORM_PUBLIC_KEY=${PERCONA_TEST_PLATFORM_PUBLIC_KEY}

how .env should look like?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is how it will look like for our dev environment:

PERCONA_TEST_PLATFORM_PUBLIC_KEY=RWTg+ZmCCjt7O8eWeAmTLAqW+1ozUbpRSKSwNTmO+exlS5KEIPYWuYdX

Copy link
Contributor

@ritbl ritbl Jun 9, 2022

Choose a reason for hiding this comment

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

It's minisign public key, it's one line, here is example:

oh, it is very confusing, because it can be though as https cert
can you pls rename so that it will clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

PUBLIC_KEY part is confusing, can we rename it according to it's function? ie PERCONA_TEST_PLATFORM_ENCRYPTION_KEY

Copy link
Contributor Author

@artemgavrilov artemgavrilov Jun 10, 2022

Choose a reason for hiding this comment

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

In fact it's not a new variable, it was PERCONA_TEST_CHECKS_PUBLIC_KEY for a long time. I renamed it to PERCONA_TEST_PLATFORM_PUBLIC_KEY because now it used not only for checks system but also for alerting.

It's public key for signatures verification, not for encryption/decryption. Maybe PERCONA_TEST_PLATFORM_VERIFICATION_KEY will work better. BTW I will create a poll

Copy link

@Oneiroi Oneiroi left a comment

Choose a reason for hiding this comment

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

I've raised concerns over the inclusion of hard-coded keys; as these have been confirmed to only be the public key component this is fine to do. for now, however given there are multiple such keys in use (which appears to be dependant on version) there will soon be a need for a better key management approach, to avoid many such keys being present.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants