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.

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 helm-charts for explorer-db & explorer #353

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

Conversation

vineethboppudi
Copy link
Contributor

What this PR does / why we need it:

This feature implements helm-charts for explorer-db & explorer

Which issue(s) this PR fixes:

Fixes #333

Special notes for your reviewer:

Does this PR introduce a user-facing change?


Additional documentation, usage docs, etc.:


@vineethboppudi vineethboppudi marked this pull request as ready for review February 27, 2023 12:14
@vineethboppudi vineethboppudi requested a review from a team as a code owner February 27, 2023 12:14
@adityajoshi12 adityajoshi12 requested a review from arsulegai March 1, 2023 14:24
Copy link
Contributor

@adityajoshi12 adityajoshi12 left a comment

Choose a reason for hiding this comment

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

Please have the default values for the parameters.

helm-charts/explorer/templates/explorer-deployment.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-configmap.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-configmap.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-configmap.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-configmap.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-configmap.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-db-service.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorer-db-service.yaml Outdated Show resolved Hide resolved
helm-charts/explorer/templates/explorerdb-configmap.yaml Outdated Show resolved Hide resolved
@vineethboppudi vineethboppudi requested review from adityajoshi12 and removed request for arsulegai March 8, 2023 13:02
{{- toYaml .Values.annotations | nindent 6 }}
data:

test-network.json: |-
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename it something more meaningful?

app: {{ .Release.Name }}-app
ports:
- port: {{ .Values.explorer.service.port }}
targetPort: {{ .Values.explorer.service.port }}
Copy link
Contributor

Choose a reason for hiding this comment

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

targetPort is the port on which explorer will be running internally, and to use the custom value for it, we also need the explorer to run on the same port. Can you check once if we can run explorer on the custom port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default explorer port running on 8080. if we want to use custom value for targetPort like 9090. we need to add
PORT: 9090 in env of explorer.

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 check if this property is available in the configmap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not their In current configmap. if want I can update

Copy link
Contributor

Choose a reason for hiding this comment

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

if we are updating the targetPort in the service then we need to add that, or the other way is to keep the value of target port to 8080

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I'll update configmap & default value as 8080. is that fine?

Signed-off-by: vineeth2328 <[email protected]>
Signed-off-by: vineeth2328 <[email protected]>
Copy link
Contributor

@adityajoshi12 adityajoshi12 left a comment

Choose a reason for hiding this comment

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

Add suport for PVC

Copy link
Member

@arsulegai arsulegai left a comment

Choose a reason for hiding this comment

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

The helm chart looks good, except for leaking secret information when used. We could proceed with two options, one is to accept this change as is and tag it for non-production deployments. The other option is to improve it further and then release it.

}
}
},
"organizations": {
Copy link
Member

Choose a reason for hiding this comment

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

Something to improve upon later: One instance of Explorer can be used for multiple organizations. There can be a different user login session seeing altogether a different network configuration. This section shall be more flexible to handle that.

"{{ .Values.network.mspid}}": {
"mspid": "{{ .Values.network.mspid}}",
"adminPrivateKey": {
"pem": "{{ .Values.network.adminPrivateKey}}"
Copy link
Member

Choose a reason for hiding this comment

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

Please load private key from either a secret store or create in-memory volume to load it from an external secret storage.

DATABASE_HOST: {{ .Release.Name }}-db
DATABASE_DATABASE: {{ $.Values.db.database | default "fabricexplorer" }}
DATABASE_USERNAME: {{ $.Values.db.database_username | default "hppoc" }}
DATABASE_PASSWD: {{ $.Values.db.database_password | default "password" }}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the private key, the password parameter is sensitive, it shall be loaded in from a secret store.

data:
DATABASE_DATABASE: {{ $.Values.db.name | default "fabricexplorer" }}
DATABASE_USERNAME: {{ $.Values.db.username | default "hppoc" }}
DATABASE_PASSWORD: {{ $.Values.db.password | default "password" }}
Copy link
Member

Choose a reason for hiding this comment

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

Kubernetes secrets is a better place to load the secret values such as password.

#Eg. peerurl: grpcs://peer0-org1:7051
peerurl:
#Provide the adminPrivateKey
#Eg. adminPrivateKey: -----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgUH2s+pegkyjaef+X\nyYBBP52PANixdkt7+xRgTkVPQPahRANCAATChVh6QCmBtI4/6S93XfEj7EDlz0Tf\nISNDAB3rsyjj3T60QhRGczSQmFyEkx6nrrWNyXLsTe/gr4Cc0l//QKQp\n-----END PRIVATE KEY-----\n
Copy link
Member

Choose a reason for hiding this comment

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

Secret information, let us evaluate if an alternate option can be used.

@suvajit-sarkar
Copy link

PR is of interest for bevel, we were exploring opt. to add helm charts. Is this being currently worked on ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploying hyperledger explorer on kubernetes
4 participants