stackabletech / hdfs-operator

Apache Hadoop HDFS operator for Stackable
Other
38 stars 4 forks source link

Port names with dashes are not supported in sidecar containers #514

Closed siegfriedweber closed 4 months ago

siegfriedweber commented 4 months ago

Port names with dashes are not supported in sidecar containers

The following example adds an OAuth proxy to the HDFS name nodes via podOverrides. A port is defined with a name that contains a dash.

---
apiVersion: hdfs.stackable.tech/v1alpha1
kind: HdfsCluster
spec:
  nameNodes:
    podOverrides:
      spec:
        containers:
          - name: oauth-proxy
            image: quay.io/openshift/origin-oauth-proxy:4.12
            ports:
              - name: oauth-proxy
                containerPort: 8080
            ...

The namenode container uses this bash script to export the ports as environment variables:

if [[ -d /stackable/listener ]]; then
    export POD_ADDRESS=$(cat /stackable/listener/default-address/address)
    for i in /stackable/listener/default-address/ports/*; do
        export $(basename $i | tr a-z A-Z)_PORT="$(cat $i)"
    done
fi

This leads to the following error:

+ for i in /stackable/listener/default-address/ports/*
++ tr a-z A-Z
++ basename /stackable/listener/default-address/ports/oauth-proxy
++ cat /stackable/listener/default-address/ports/oauth-proxy
+ export OAUTH-PROXY_PORT=8080
/bin/bash: line 39: export: `OAUTH-PROXY_PORT=8080': not a valid identifier

The problem is that - is not allowed in an environment variable name.

A solution would be to convert the port name to a valid environment variable name. Attention must be paid because port names can start with a digit, e.g. 123-web, whereas environment variables cannot.

Another solution would be to not export these ports.

nightkr commented 4 months ago

abc-def is pretty common and can be trivially translated to ABC_DEF. I'd argue that 123-abc is far less common, and while switching to a PORT_ prefix would fix it, I'm not sure it's worth the compatibility break.