CAAPIM / apim-charts

Helm Charts for Layer7 API Management components.
MIT License
11 stars 48 forks source link

[charts/gateway] Add disklessConfig property #318

Closed jennarddy closed 2 months ago

jennarddy commented 3 months ago

Description of the change

(Dependent on newer version of container gateway that will process DISKLESS_CONFIG property)

Introduce disklessConfig property to configmap.yaml to pass DISKLESS_CONFIG environment variable to Gateway container.

TODO:

  1. Update logic in templates - Set environment variables only when DISKLESS_CONFIG is true
    • Needed; database section: when DISKLESS_CONFIG is true and create is false, database.jdbcURL can be empty because it will be constructed from node.properties values
    • Need to fix because jdbcURL is commented out by default so right now user has to supply dummy value to prevent Kubernetes validation error:
      Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: unknown object type "nil" in ConfigMap.data.SSG_DATABASE_JDBC_URL
    • Addressed by https://github.com/CAAPIM/apim-charts/pull/318/commits/4346f7fdfab12752203ce0bb348d78d43fa4548e -
    • When DISKLESS_CONFIG is false, database.jdbcURL can now be empty and SSG_DATABASE_JDBC_URL is not set
    • When DISKLESS_CONFIG is true, SSG_DATABASE_JDBC_URL is set
    • Optional: other variables below that can be empty because they're in node.properties
    • clusterPassword - Optional to fix because default is non-empty, but value will be ignored when DISKLESS_CONFIG is true
    • database.username - Optional to fix because default is non-empty, but value will be ignored when DISKLESS_CONFIG is true
    • database.password - Optional to fix because default is non-empty, but value will be ignored when DISKLESS_CONFIG is true
    • database.name - Optional to fix because default is non-empty, but value will be ignored when DISKLESS_CONFIG is true
    • management.username - Optional to fix because default is non-empty, but value will be ignored when DISKLESS_CONFIG is true
    • management.password - Optional to fix because default is non-empty, but value will be ignored when DISKLESS_CONFIG is true
    • Addressed by https://github.com/CAAPIM/apim-charts/pull/318/commits/27ef8bb2c87764b3d408cb99f9c5559d2e87e687 (database.name covered by https://github.com/CAAPIM/apim-charts/pull/318/commits/4346f7fdfab12752203ce0bb348d78d43fa4548e)
    • When DISKLESS_CONFIG is false, above properties can now be empty and corresponding environment variables are not set
    • When DISKLESS_CONFIG is true, corresponding environment variables are set
  2. Describe node.properties and how to set it up - b2e9a530e4df19f73d1211003157493638647215
    • How do we want to document the secret creation?
      1. node.properties can be passed using set-file , similar to license.xml
      2. Node.properties secret will be created if existingSecretName is not set
  3. Put some doc regarding diskless config on techdocs. Doc in this repo to focus more on usage.
    • Add link to techdocs from here
    • Added a note on Readme : 996efe05f18ad3c030afe81b09bb009a51c8a15e

Benefits

As this is a required environment variable, it's better for visibility and more appropriate to have its own property, as the way Gateway is configured is very different depending on the value.

Existing way to set DISKLESS_CONFIG is via additionalEnv, but I feel this is for more optional variables, not for an integral setting for Gateway configuration:

additionalEnv:
  DISKLESS_CONFIG: false

Drawbacks

Applicable issues

Additional information

Checklist

DamandeepToor commented 3 months ago

@jennarddy , Gateway version need to incremented to 3.0.29 https://github.com/jennarddy/apim-charts/blob/diskless-config/charts/gateway/Chart.yaml#L5

jennarddy commented 3 months ago

There's still a fair bit missing for this feature. Is that intended?

Yeah, it's still in progress. I added label.

DamandeepToor commented 2 months ago

[TestRun : ] (https://testrail.apim.broadcom.net/index.php?/runs/view/102664&group_by=cases:section_id&group_order=asc) - Passed

Included the upgrade scenario also

@jennarddy , we can merge this PR to gateway_111 branch