Charts: [stable/concourse] Upgrade chart to support Concourse 4.0

[wendorf]

Concourse 4.0 has been released with some breaking changes, requiring changes to the chart. I'd like to start working on this, but don't want to duplicate work in case someone else is already working on it.

@frodenas @william-tran Is this safe to work on? Should the chart support both 3.x and 4.0?

Breaking changes:

• [x] Migrate CONCOURSE_BASIC_AUTH_* to CONCOURSE_ADD_LOCAL_USER, CONCOURSE_MAIN_TEAM_LOCAL_USER, and CONCOURSE_MAIN_TEAM_ALLOW_ALL_USERS (don't forget to change the NOTES.txt to match the new basic auth model)
• [x] Migrate CONCOURSE_GITHUB_AUTH_* to CONCOURSE_GITHUB_* and CONCOURSE_MAIN_TEAM_GITHUB_*
• [x] Migrate CONCOURSE_GITLAB_AUTH_* to CONCOURSE_GITLAB_* and CONCOURSE_MAIN_TEAM_GITLAB_*
• [x] Migrate CONCOURSE_GENERIC_OAUTH_* to CONCOURSE_OAUTH_* and CONCOURSE_MAIN_TEAM_OAUTH_*

New features:

• [ ] CONCOURSE_RESOURCE_TYPE_CHECKING_INTERVAL
• [ ] CONCOURSE_VAULT_CACHE

New authentication modes:

• [x] CloudFoundry
• [x] LDAP

Pre-existing authentication modes not already in chart:

• [x] OIDC

[Typositoire]

I'd be happy to help. v4 brings user based auth instead of team based!!! I can't wait to test that!

[william-tran]

I don't think the chart should support both 3.x and 4.0, that would introduce too much complexity that we should avoid maintaining. With web and worker both supporting arbitrary env vars, at this point, I think the current version of the chart can continue to support 3.x use cases without further changes.

This breaking change gives us an opportunity to make breaking chart changes to resolve some issues with the chart as it is today:

• [ ] use [web|worker].extraEnv instead of [web|worker].env
• [ ] Go though the output of docker run -it concourse/concourse:4.0.0 [web|worker] --help and remove any values / env vars / command-line args no longer supported
• [ ] Consider removing all values that support features that can be configured strictly through [web|worker].extraEnv.

This would set the stage for the chart only requiring updates for features that use secrets or other aspects of k8s. The Concourse team continues to add great features to concourse, and often, and I don't want the maintenance of this chart to be a drag to the adoption of a new version due to an apparent lack of support through chart values.

[wendorf]

I'm on board with dropping explicit 3.x support, env -> extraEnv, and removing old arguments.

As far as relying more heavily on env, I'm personally in favor of explicitly supporting features in the chart:

• The documentation for Concourse flags isn't fantastic (you have to run the binary's --help instead of referencing the website to really see everything) and as a user, I really appreciate charts that have explicit config.
• Even if the documentation were better, when I'm referencing the README to understand how to use a chart, it's a huge boon to be able to use that as the primary point of reference, rather than having to cobble together config.
• It's not very Concoursey to support kitchen-sink-config 😃.

I definitely understand the maintainer burden is high—it's a totally fair point—but I'd personally prefer to invest in keeping the YAML explicit. Is there a compromise where we can communicate in the README how to "early adopt" features? It's obvious if you know what you're doing, but it might be helpful to document recommendations for using env.

---

I've started to work on supporting 4.0 and have some decisions to make, especially with regard to the split between flags for auth providers, and flags for which users/groups can access the main team:

• Main team: we can do something like concourse.mainTeamGithubUsers or concourse.mainTeam.githubUsers. I hesitate a bit to use nested values, but concourse.mainTeam.githubUsers feels like the right choice here.
• Main team: The values here are probably not "secret" so could be either env vars or CLI flags. I'm a bit confused by the way the chart chooses right now—it seems kind of arbitrary for what is a CLI flag, and it's extra confusing for me that those CLI flags are just echoing env vars. For example, why is --datadog-agent-host set to the value of $(CONCOURSE_DATADOG_AGENT_HOST)? Isn't that redundant? If we use environment variables, it's actually unclear how to use them for list flags (e.g. --main-team-oidc-user). How does one specify an environment variable multiple times? I've filed a question in the Concourse forums to figure this out.
• Auth providers: we have a similar choice for flat vs nested. Because some providers have quite lengthy config (LDAP has 20 flags alone), I'm preferring to use nested flags like secrets.ldapAuth.displayName.
• Auth providers: Also because there are so many flags, I would like to add feature-flags for each provider, à la secrets.ldapAuth.enabled, where the *.enabled flag is never provided to Concourse, but is just used within the Helm templates to control blocks of flags. It's extra config, but feels more conventional.
• Auth providers: Auth providers are currently in secrets.*, so it's natural to put them in as secrets.ldapAuth.*, but this feels like it's overloading secrets.*, and it doesn't do a great job of communicating that all the auth providers are similar. I'm currently doing secrets.ldapAuth.*, but I think it might be nicer to extract all the provider config to secrets.authProviders.ldap.*.
• Auth provider secrets: It'd be nice to group all auth provider config together, but there's a mix of secret and non-secret config. I'm currently treating it all as secret, so there are keys like secrets.githubAuth.caCert, but this is a bit awkward. Does it make sense to switch things around, so there's a structure more like:

githubAuth:
  enabled: false
  secrets:
    clientId:
    clientSecret:
  host:
  caCert:

or just get rid of the secrets nesting and hide that implementation detail?

What are your thoughts on these?

[Typositoire]

So i've tested with just adding web.env everything WORKED except this:

{{- if semverCompare "^3.10.x" .Values.imageTag }}
              value: "{{ template "concourse.web.fullname" . }}:{{ .Values.concourse.tsaPort}}"
          {{- else }}
              value: {{ template "concourse.web.fullname" . }}
            - name: CONCOURSE_TSA_PORT
              value: {{ .Values.concourse.tsaPort | quote }}
          {{- end }}

the semverCompare fails and my workers get the wrong tsa_host

We might want to update that condition.

[william-tran]

There is an open PR for that fix: https://github.com/helm/charts/pull/6853. But I want to address other breaking chart changes while we have the chance so I'm going to hold off on merging that.

[wendorf]

In case you're responding to my long (my apologies for the length) previous comment, I've edited in some extra questions and details.

[wendorf]

It's still in WIP (not ready for a PR), but you can see the direction I'm taking at https://github.com/wendorf/charts/commits/concourse-4

[Typositoire]

I've added one comment, the rest looks good!

[wendorf]

Submitted PR #6951 to support 4.0. If anyone has feedback, I'd appreciate it!

[ralekseenkov]

thanks for all your work! we'll try to test auth-related changes soon

given the number of queries being done to Vault in a large deployment, CONCOURSE_VAULT_CACHE would be a really good addition to the chart

[wendorf]

@ralekseenkov I'm generally in favor of adding explicit config for everything (though it can get burdensome!), but I think the PR to support Concourse 4.0 should just be focused on breaking changes and cleaning up new auth-model stuff. There are a lot of config options the chart doesn't explicitly support, and I think it expands the scope too much to attempt to whittle away at it here.

You can add custom env vars to web.env until the chart officially supports it, so I'd recommend doing that for now, and submitting a PR just for adding explicit support for this. Since CONCOURSE_VAULT_CACHE _is_ a 4.0 feature, though, I don't think it's unreasonable to pull into this PR. But it might be nicer to have a "4.0 breaking changes" PR and a follow-up "4.0 new features" PR to streamline reviews and getting work merged in.

[ralekseenkov]

@wendorf agreed. I only mentioned CONCOURSE_VAULT_CACHE because I saw it in the description of the issue, so I was under assumption it's coming as a part of this PR. But "4.0 new features" PR sounds great too. I actually didn't know about web.env, thanks for pointing this out - will give it a try.

[wendorf]

Whoops! I think I wrote that up so long ago now that I totally forgot.

On Mon, Aug 20, 2018, 11:01 AM Roman Alekseenkov [e-mail]
wrote:

@wendorf https://github.com/wendorf agreed. I only mentioned
CONCOURSE_VAULT_CACHE because I saw it in the description of the issue, so
I was under assumption it's coming as a part of this PR. But "4.0 new
features" PR sounds great too. I actually didn't know about web.env, thanks
for pointing this out - will give it a try.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/helm/charts/issues/6873#issuecomment-414408354, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAY3LlrKjBBzu3MdG62HwYXHD25WThV8ks5uSvmEgaJpZM4VkysG
.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

[andrusha]

Bump @wendorf

I tried to roll the chart and it seem to work, I couldn't get LDAP working yet, but the rest seem to be ok.

[chrishiestand]

We should also close this since #7804 was merged