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

Added OpenShift GCP supplemental values #237

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

priyadarshini-ni
Copy link

@priyadarshini-ni priyadarshini-ni commented Dec 11, 2024

What does this Pull Request accomplish?

The supplemental values for GCP and OpenShift has been added with examples for GCS Storage and adding cloud SQL auth proxy container.

Note: GCS support is not yet added to dataframe service as the service uses S3 DeleteObjects API which is not supported by GCS S3 interoperable XML API, refer this for more information. The respective documentation for the dataframe service will be updated once we add this support.

Why should this Pull Request be merged?

This acts as an additional values file while SLE is installed in Openshift cluster or GKE cluster on GCP

What testing has been done?

NA

@SSSantosh18 SSSantosh18 self-requested a review December 12, 2024 11:23
Priyadarshini Piramanayagam added 2 commits December 16, 2024 18:51
Copy link
Contributor

@SSSantosh18 SSSantosh18 left a comment

Choose a reason for hiding this comment

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

initial review

- name: cloud-sql-auth-proxy
image: gcr.io/cloud-sql-connectors/cloud-sql-proxy:2.8.0
volumeMounts:
# This volume mount is required for the proxy to authenticate with cloudSQL using a service account key file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this token volume mount only required for workload identity?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, added comment for the same

getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
secret:
secretName: <secret-name> # <ATTENTION> - Enter the secret name where config.json is added.

connectionInfo:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these are not the right values for grafana.

Check here

Copy link
Author

Choose a reason for hiding this comment

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

The values for grafana database have been modified

getting-started/templates/GCP/gcp-supplemental-values.yaml Outdated Show resolved Hide resolved
serviceTCP:
type: LoadBalancer

nbexecservice:
Copy link
Contributor

Choose a reason for hiding this comment

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

nbexec (has direct s3 dependency), dfs and dremio in dfs too have GCS dependency. Can we also include them?

Copy link
Author

Choose a reason for hiding this comment

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

Added s3 dependency for nbexecservice. The GCS guide has not been included in this PR for DFS as the support is not fully available yet. Refer PR description for more info.

argo:
## Configure GCS access.
##
artifactRepository:
Copy link
Contributor

Choose a reason for hiding this comment

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

s3 dependency for Argo in nbexec is no longer required. We can remove this

Copy link
Author

Choose a reason for hiding this comment

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

Removed

## ref: https://cloud.google.com/sql/docs/postgres/connect-kubernetes-engine#run_the_in_a_sidecar_pattern
sidecars:
- name: cloud-sql-auth-proxy
image: gcr.io/cloud-sql-connectors/cloud-sql-proxy:2.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

lets remove the tag from here and replace with a placeholder and attention comment

- name: cloud-sql-auth-proxy
image: gcr.io/cloud-sql-connectors/cloud-sql-proxy:2.8.0
volumeMounts:
# This volume mount is required for the proxy to authenticate with cloudSQL using Workload Identity Federation config file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This volume mount is required for the proxy to authenticate with cloudSQL using Workload Identity Federation config file.
# This volume mount is required for the proxy to authenticate with Cloud SQL using Workload Identity Federation by providing a short-lived token for authentication.

- name: <token-volume-name> # <ATTENTION> - Enter the volume name where the token is available
mountPath: <token-mount-path> # <ATTENTION> - Enter the path where the token should be mounted
readOnly: true
# This volume mount is required for the proxy to authenticate with cloudSQL using service account key file or Workload Identity Federation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This volume mount is required for the proxy to authenticate with cloudSQL using service account key file or Workload Identity Federation.
# This volume mount is required for the proxy to set up Cloud SQL authentication, using either a service account key file or a Workload Identity Federation config file.

mountPath: /secrets/
readOnly: true
env:
# This env variable is required for the proxy to authenticate with cloudSQL when using Workload Identity Federation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use Cloud SQL in all places instead of 'cloudSQL'

env:
# This env variable is required for the proxy to authenticate with cloudSQL when using Workload Identity Federation.
- name: "GOOGLE_APPLICATION_CREDENTIALS"
value: /secrets/<secret-key> # <ATTENTION> - Enter the file name which was used as the key while creating the secret
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
value: /secrets/<secret-key> # <ATTENTION> - Enter the file name which was used as the key while creating the secret
value: /secrets/<secret-key> # <ATTENTION> - Enter the config json file name which was used as the key while creating the secret


# The credentials file is required for the proxy to authenticate using a service account key file.
# Not required if Workload Identity federation is used for authentication.
- "--credentials-file=/secrets/<secret-key>" # <ATTENTION> - Enter the file name which was used as the key while creating the secret
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- "--credentials-file=/secrets/<secret-key>" # <ATTENTION> - Enter the file name which was used as the key while creating the secret
- "--credentials-file=/secrets/<secret-key>" # <ATTENTION> - Enter the service account key file name which was used as the key while creating the secret


## Extra volumes that can be used in sidecars
extraVolumes:
# This volume is required for the proxy to authenticate with cloudSQL when using Workload Identity Federation.
Copy link
Contributor

Choose a reason for hiding this comment

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

update the comments as already suggested in the mounts

audience: <audience-name> # <ATTENTION> - Enter the audience name for the projected service account token
expirationSeconds: 3600
path: token
# This volume is required for the proxy to authenticate with cloudSQL using a service account key file.
Copy link
Contributor

Choose a reason for hiding this comment

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

update the comment as suggested in mounts.

Also this is for both sa key file and WIF config json

host: "storage.googleapis.com"
region: <region> # <ATTENTION> - Enter the region where the GCS bucket is located

saltmaster:
Copy link
Contributor

Choose a reason for hiding this comment

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

is this expected to change based on the cloud provider?

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.

4 participants