add a secret ref for webhook secrets

[bparees]

bug 1504819

https://bugzilla.redhat.com/show_bug.cgi?id=1504819

[bparees]

@deads2k ptal/merge. needed for openshift/origin#17314

[deads2k]

"name of the secret in the same namespace being reference"

[bparees]

fixed.

[deads2k]

Lifting my comments from the earlier thread:

General thoughts:

1. This prescribes the key, but doesn't add a type. Adding types (but not requiring them) can help users categorize their secrets. That could happen as a later step.
2. Being prescriptive to reduce choice simplifies the API, but as you noted it can make discoverability harder. If you created a secretKey which defaulted to WebHookSecretKey would be a backwards compatible change that allows people to avoid specifying if they wish, but may help discoverability. This could happen later if we actually do have problems. I personally like being prescriptive.
3. It is now possible to have a webhook configuration which is invalid at runtime. With other resources, we use controllers to reflect this in status. A similar approach could be taken here or you could write-back status from the rest handler.

This looks like a good starting point to me. We can grow on this in a few directions if we end up needing to.

@bparees any thoughts on how to manage the status aspect? A small controller seems like an option.

@smarterclayton how do you generally feel about controllers watching the validity of references over time and updating status in general? It seems consistent with other approaches and reasonably discoverable. It would simplify status reporting the CLI and the webconsole too.

[deads2k]

@deads2k ptal/merge. needed for openshift/origin#17314

Travis seems stuck. I'll see if it unsticks or if we must summon @stevekuznetsov and get some real CI.

[bparees]

@bparees any thoughts on how to manage the status aspect? A small controller seems like an option.

we already have secret refs (And imagestream refs) elsewhere in the buildconfig that aren't validated. A general buildconfig controller that validated all the refs in the buildconfig and provided status(or events?) isn't a bad idea, but won't be part of this specific refactor.

[bparees]

seems to have passed travis after my update.

[bparees]

@deads2k anything else?

[bparees]

/assign @deads2k

[bparees]

(I suppose expecting we have bots setup for this repo would be too much)

[deads2k]

we already have secret refs (And imagestream refs) elsewhere in the buildconfig that aren't validated. A general buildconfig controller that validated all the refs in the buildconfig and provided status(or events?) isn't a bad idea, but won't be part of this specific refactor.

I agree. Just wanted to know about general general concept.

/lgtm

[openshift-ci-robot]

@deads2k: changing LGTM is restricted to assignees, and assigning you to the PR failed.

Details

In response to this:

we already have secret refs (And imagestream refs) elsewhere in the buildconfig that aren't validated. A general buildconfig controller that validated all the refs in the buildconfig and provided status(or events?) isn't a bad idea, but won't be part of this specific refactor.

I agree. Just wanted to know about general general concept.

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[deads2k]

(I suppose expecting we have bots setup for this repo would be too much)

Welcome to the wild west. I'll fix that :)

[stevekuznetsov]

Yeah there is no way to give the bots access to all repos, and they need write access to a repo to do anything meaningful to it, so just ask me or someone else who can edit those configs to add them.