feat: add configuration option to enable REST API authorizations #474
Conversation
| @@ -311,6 +311,10 @@ spec: | |||
| secretKeyRef: | |||
| name: {{ .Values.global.datahub.metadata_service_authentication.systemClientSecret.secretRef }} | |||
| key: {{ .Values.global.datahub.metadata_service_authentication.systemClientSecret.secretKey }} | |||
| {{- if .Values.global.datahub.metadata_service_authentication.restApi.authorization.enabled }} | |||
There was a problem hiding this comment.
Suggest:
- name: REST_API_AUTHORIZATION_ENABLED
value: {{ .Values.global.datahub.metadata_service_authentication.restApi.authorization.enabled | quote }}
There was a problem hiding this comment.
Do we really want to set the value of the environment variable to the value of the config or do we just want to add the environment variable with the value true in case the config is true? I think it's more or less exactly the same logic here as for the global.datahub.metadata_service_authentication.view.authorization.enabled config?
datahub-helm/charts/datahub/subcharts/datahub-gms/templates/deployment.yaml
Lines 314 to 321 in 362671f
There was a problem hiding this comment.
@Masterchen09 - in this case, yes because we're going to change the application default here and mark it as a breaking change. When that change hits a release, will update the helm chart like this PR, however will follow @darnaut suggestion.
There was a problem hiding this comment.
@david-leifker This explanation makes sense (at least now after changing the default value 😉) - I have adjusted the PR accordingly and also set the default value to true to match the new default value (or should we wait with changing the default value in the chart, to be able to merge this PR directly before a new release is available?).
I have also checked the other (boolean) environment variables in all subcharts...there are more locations where the default value could currently not be overwritten because of the same reason - I have also adjusted these locations...maaaybe all boolean environment variables independent from the default value should be set using the same pattern, to be independent from changing default values in the future?
As I have nearly touched all subcharts, I have also fixed some formatting "issues" (some whitespaces...nothing special), adjusted the appVersion of each subchart to match the image tags resp. the version in the parent chart (would be great if the appVersion would always be matching the version in the parent chart or remove the appVersion from the subcharts...?) and removed an unused env.JMXPORT config from the value.yaml of the datahub-frontend, datahub-mae-consumer and datahub-mce-consumer subcharts (not sure why it was there). I hope it is fine to have this as part of this PR...somehow I just started to cleanup and could not stop it...😁
There was a problem hiding this comment.
@Masterchen09 - these look like good changes. Thank you once again for your contributions!
|
This PR is stale. We will close it in 30 days if there is no comment or activity. If you want feedback but not able to get it on github please head to #contribute channel in slack at https://slack.datahubproject.io. |
Masterchen09
force-pushed
the
feat-rest-api-authorizations
branch
from
1b9d6d3 to
aa0b94e
Compare
|
This PR is stale. We will close it in 30 days if there is no comment or activity. If you want feedback but not able to get it on github please head to #contribute channel in slack at https://slack.datahubproject.io. |
|
To make sure that this PR is not automatically closed - here’s a comment. |
Masterchen09
force-pushed
the
feat-rest-api-authorizations
branch
from
aa0b94e to
c57bec4
Compare
Masterchen09
force-pushed
the
feat-rest-api-authorizations
branch
from
c57bec4 to
16cb183
Compare
To make it a bit more transparent - I was not aware of it - that there are parts of the REST API which are not secured by default (of course you have to be authenticated, but each authenticated user is authorized by default to use these endpoints), this PR adds a new configuration option similar to the view authorization which allows to enable REST API authorizations. Technically this is already possible today using extra environment variables - the intention here is only to make it more transparent.
As the REST API authorizations were not enabled by default in the past (and are not enabled by default in the metadata-service image), the configuration option is also not enabled by default - however this might be a point which could be discussed (i.e. security first -> why should be the REST API authorization not be enabled by default?).
Checklist