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 web OIDC config #31

Merged
merged 18 commits into from
Mar 4, 2022
Merged

Add web OIDC config #31

merged 18 commits into from
Mar 4, 2022

Conversation

fredx30
Copy link
Contributor

@fredx30 fredx30 commented Feb 16, 2022

Summary

This adds a config set for OIDC in web under the keys .web.oidc.*.

Added an entry for versionbumping chart.yml to the feature template.

Motivation

The helm chart currently overrides the wharf-web repo's config. As the module is always loaded in we need a valid config to not crash the app. This default config will be sufficient not to crash the app but will likely need to be configured in the case that it is to be used.

The template update should make it easier to remember the two locations for the version bumping, and might save someone a first failed github action run.

@fredx30 fredx30 marked this pull request as ready for review February 17, 2022 11:11
@fredx30 fredx30 self-assigned this Feb 17, 2022
@fredx30 fredx30 added the enhancement New feature or request label Feb 17, 2022
Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Very nice work 👍🏻

In wharf-helm/values.yaml there are some comments mentioning default value or that it's required.

I think adding that to all applicable fields could be good.
I.e., make optional fields all have the

Default is value

part, and all required fields (without default values) explicitly say they are required.

EDIT: Also, maybe adding newlines to the very long comments.

charts/wharf-helm/values.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/templates/web.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/values.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/values.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/values.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/CHANGELOG.md Outdated Show resolved Hide resolved
fredx30 and others added 3 commits February 17, 2022 12:46
Co-authored-by: Alexamakans <79503481+Alexamakans@users.noreply.github.com>
Co-authored-by: Alexamakans <79503481+Alexamakans@users.noreply.github.com>
@fredx30
Copy link
Contributor Author

fredx30 commented Feb 17, 2022

The comments are straight off of https://github.com/damienbod/angular-auth-oidc-client/blob/main/projects/angular-auth-oidc-client/src/lib/config/openid-configuration.ts#L4

Here is what i can do about the comments. I will include the link the the interface that dictates naming and formatting. I could also reduce the level of details in the comments. I do however not however know or nessacerily understand why they are what they are in the first place. I have tested it as far as i can say with a fairly high degree of certainty that these are the settings one might need to change if one were to use a different identity provider with wharf.

charts/wharf-helm/README.md Outdated Show resolved Hide resolved
charts/wharf-helm/templates/web.yaml Show resolved Hide resolved
charts/wharf-helm/templates/web.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/values.yaml Outdated Show resolved Hide resolved
charts/wharf-helm/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

Still waiting for actions and response on existing threads

@fredx30
Copy link
Contributor Author

fredx30 commented Mar 3, 2022

@jilleJr will hand this over to you, have a good time with my nasty code :)

@fredx30 fredx30 removed their assignment Mar 3, 2022
@applejag applejag requested review from applejag and removed request for applejag March 4, 2022 08:22
@applejag
Copy link
Contributor

applejag commented Mar 4, 2022

I've made some changes of the implementation. It is now more generic and lets users add as many new config fields as they want. This by using the toPrettyJson Helm function (https://docs.helm.sh/docs/chart_template_guide/function_list/#type-conversion-functions) to serialize the entire web.oidc object as a JSON object instead.

The copied documentation on a per-value basis has been removed and instead I'm only referring to their documentation at https://nice-hill-002425310.azurestaticapps.net/docs/documentation/configuration#config-values and their source code at https://github.com/damienbod/angular-auth-oidc-client/blob/release_13_1_0/projects/angular-auth-oidc-client/src/lib/config/openid-configuration.ts
This together with our default values should clue the wharf-helm user into how these fields are applied, and we no longer have to maintain copied documentation of fields that we don't even agree on ourselves.

@applejag
Copy link
Contributor

applejag commented Mar 4, 2022

I can't seem to remove my review as I previously requested changes. Best I could do was to approve it myself so GitHub stops complaining.

@applejag
Copy link
Contributor

applejag commented Mar 4, 2022

For reference, this is what the templated ConfigMap looks like when having web.oidcEnabled=true:

$ helm template mytest ./charts/wharf-helm --set web.oidcEnabled=true
# ..snip other templated YAML docs..
---
# Source: wharf-helm/templates/web.yaml
kind: ConfigMap
apiVersion: v1
metadata:
  name: mytest-wharf-helm-web-config
data:
  config.json: |
    {
      "Environment": {
        "Name": "dev",
        "IsProduction": false
      },
      "UpdateLatency": 20000,
      "UpdateFrequency": 30000,
      "oidcConfig": {
        "authority": "https://login.microsoftonline.com/841df554-ef9d-48b1-bc6e-44cf8543a8fc/v2.0/.well-known/openid-configuration",
        "autoUserInfo": false,
        "clientId": "01fcb3dc-7a2b-4b1c-a7d6-d7033089c779",
        "ignoreNonceAfterRefresh": true,
        "issValidationOff": false,
        "logLevel": 2,
        "maxIdTokenIatOffsetAllowedInSeconds": 600,
        "postLogoutRedirectUri": "https://wharf.example.org",
        "redirectUrl": "https://wharf.example.org",
        "responseType": "id_token token",
        "scope": "openid profile email offline_access api://wharf-internal/read api://wharf-internal/admin api://wharf-internal/deploy",
        "silentRenew": true,
        "useRefreshToken": true
      },
      "BackendUrls": {
        "Api": "/api",
        "GitlabImport" : "/import",
        "GithubImport" : "/import",
        "AzureDevopsImport" : "/import"
      }
    }
---
# ..snip other templated YAML docs..

Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Other stuff looks good 👍🏻

charts/wharf-helm/README.md Show resolved Hide resolved
@applejag applejag merged commit c59f2b5 into master Mar 4, 2022
@applejag applejag deleted the feature/add-web-oidc-config branch March 4, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants