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.

Sign up for GitHub

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

Jump to bottom

add a secretref for webhook secrets #17314

Merged
merged 1 commit into from
Dec 21, 2017
Merged

add a secretref for webhook secrets #17314

merged 1 commit into from
Dec 21, 2017

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Nov 14, 2017 •
edited
Loading

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 14, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Nov 14, 2017
@bparees
Copy link
Contributor Author

bparees commented Nov 14, 2017

@openshift/api-review this is ready for api review. WIP refers to need for new tests.

bparees
bparees commented Nov 14, 2017
View reviewed changes
pkg/build/apis/build/types.go Outdated
@@ -1054,11 +1054,16 @@ type BuildConfigStatus struct {
// WebHookTrigger is a trigger that gets invoked using a webhook type of post
type WebHookTrigger struct {
// Secret used to validate requests.
// Deprecated: use Secret instead.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm fixing this (should say SecretRef)

deads2k
deads2k reviewed Nov 15, 2017
View reviewed changes
pkg/build/apis/build/types.go Outdated

// SecretRef is a reference to a secret containing the value to be validated
// when the webhook is invoked. Takes precedence over Secret.
SecretRef *kapi.LocalObjectReference
Copy link
Contributor

@deads2k deads2k Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Upstream we've started defining specific types for each kind of reference. The idea being that this isn't a generic reference, this is a reference to a secret and even more specifically, a reference to a particular kind of secret. Basically, every reference started looking a little different and chasing them all around is impractical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defined a new type for the reference. I did not add (and don't plan to add?) a new type of secret.

deads2k
deads2k reviewed Nov 15, 2017
View reviewed changes
pkg/build/apis/build/validation/validation.go Outdated
@@ -607,8 +607,8 @@ func validateTrigger(trigger *buildapi.BuildTriggerPolicy, buildFrom *kapi.Objec

func validateWebHook(webHook *buildapi.WebHookTrigger, fldPath *field.Path, isGeneric bool) field.ErrorList {
allErrs := field.ErrorList{}
if len(webHook.Secret) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("secret"), ""))
if len(webHook.Secret) == 0 && webHook.SecretRef == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to provide an oadm migrate command to help people transition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no.

deads2k
deads2k reviewed Nov 15, 2017
View reviewed changes
pkg/build/registry/buildconfig/webhook.go Outdated
return nil, err
}
for _, v := range s.Data {
decoded, err := base64.StdEncoding.DecodeString(string(v))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unexpected. You're storing base64 encoded bytes instead of the actual bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you might be looking at this PR by commit? this is changed in a later commit in the PR (though i see there's a log.info i inadvertently left in)

@bparees
Copy link
Contributor Author

bparees commented Nov 15, 2017

@deads2k reftype added and logic updated to only look for a specific key in the secret.

@bparees bparees changed the title [WIP] add a secretref for webhook secrets add a secretref for webhook secrets Nov 15, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2017
@deads2k
Copy link
Contributor

deads2k commented Nov 16, 2017

I plan to come back and take another pass. But before I forget

/hold

API changes are being held until we make openshift/api authoritative post 1.8 rebase.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2017
@bparees bparees force-pushed the webhook_secrets branch 3 times, most recently from 0a29334 to 8b92e6a Compare November 17, 2017 00:14
@bparees
Copy link
Contributor Author

bparees commented Nov 17, 2017

/retest

1 similar comment
@bparees
Copy link
Contributor Author

bparees commented Nov 17, 2017

/retest

deads2k
deads2k reviewed Nov 17, 2017
View reviewed changes
pkg/build/apis/build/types.go Outdated
Secret string

// AllowEnv determines whether the webhook can set environment variables; can only
// be set to true for GenericWebHook
AllowEnv bool

// SecretRef is a reference to a secret containing the value to be validated
Copy link
Contributor

Choose a reason for hiding this comment

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

reference to a secret in the same namespace

deads2k
deads2k reviewed Nov 17, 2017
View reviewed changes
pkg/build/webhook/webhook.go
if !hmac.Equal([]byte(trigger.GitLabWebHook.Secret), []byte(secret)) {
continue
}
if secretRef != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't the API comment say this took precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does, but that comment should be removed. There's no concept of precedence, if you have both, we will accept a match on either one.

Copy link
Contributor

Choose a reason for hiding this comment

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

it does, but that comment should be removed. There's no concept of precedence, if you have both, we will accept a match on either one.

Ok. Update the comment.

@deads2k
Copy link
Contributor

deads2k commented Nov 17, 2017

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2017
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2017
@bparees
Copy link
Contributor Author

bparees commented Dec 13, 2017

@jwforres @benjaminapetersen fyi this is back on the merge queue now.

@openshift-merge-robot openshift-merge-robot removed the vendor-update Touching vendor dir or related files label Dec 14, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2017
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2017
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@bparees
Copy link
Contributor Author

bparees commented Dec 19, 2017

/retest

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2017
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2017
@bparees
Copy link
Contributor Author

bparees commented Dec 20, 2017

/retest

@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 20, 2017 •
edited
Loading

@bparees: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_image_ecosystem 151f333 link /test extended_image_ecosystem
ci/openshift-jenkins/extended_conformance_install_update 7d0a43c link /test extended_conformance_install_update
ci/openshift-jenkins/cross b643c19 link /test cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@bparees
Copy link
Contributor Author

bparees commented Dec 21, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 5a11e88 into openshift:master Dec 21, 2017
@bparees bparees deleted the webhook_secrets branch January 2, 2018 16:41
@jhadvig jhadvig mentioned this pull request Jan 8, 2018
Merged
openshift-merge-robot added a commit to openshift/origin-web-console that referenced this pull request Jan 12, 2018
@openshift-merge-robot
Merge pull request #2640 from jhadvig/secRef
bad691e 
Automatic merge from submit-queue.

Handle new build webhooks that use secretRefs instead of inline secrets

Adding functionality to handle the new build webhooks that use secretRefs instead of inline secrets.
Now when user will want to add webhook to build config he will have to choose from available secrets that contain the `WebHookSecretKey` key.
Screen:
![a032d8ea-df07-4192-yd08-fb5c7e9ffd9f](https://user-images.githubusercontent.com/1668218/34688709-4ccff7f0-f4b3-11e7-9c65-3fe7aba96a7a.gif)


To be consistent with changes in CLI (introduced in openshift/origin#17314) I've updated the `webhookURL` so it will show webhook urls as:
`https://localhost:9000/namespaces/myproject/buildconfigs/test-build/webhooks/<secret>/github`

Wasn't really sure if part of this [card](https://trello.com/c/lo6HwQgu/1106-2-build-webhooks-with-secretrefs) is also update the create-secret page to be able to create secrets that user could add to the webhook in the BC editor. 

@spadgett PTAL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jim-minter jim-minter jim-minter left review comments

@coreydaley coreydaley coreydaley left review comments

@deads2k deads2k deads2k left review comments

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

Labels
api-approved approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-api-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@bparees @deads2k @jim-minter @jwforres @openshift-ci-robot @openshift-bot @openshift-merge-robot @coreydaley @liggitt @smarterclayton