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

Build watch secrets #493

Conversation

qu1queee
Copy link
Contributor

@qu1queee qu1queee commented Nov 24, 2020

Fixes #466

Worked on this together with @xiujuan95 .

This PR introduces a new Watcher on the Build Controller that watches for Secret Resources. Main things to consider:

  • We have a two layer filtering before entering in the Build Controller Reconcile space, these are:
    • We process events(CREATE,DELET,UPDATE) in Secrets that are only for Secrets that contain the build.build.dev/referenced.secret: true annotation.
    • We only reconcile when the above secret is reference on any Build in the current namespace where the secret is.

This PR introduces an extended integration-test for this particular behaviour.

This PR enhance the existing authentication docs, to include what users require to define in the secret and a brief explanation on why this is required.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 24, 2020
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch 12 times, most recently from e6a0651 to 7326876 Compare November 30, 2020 09:09
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch 10 times, most recently from 553f99f to a4ff883 Compare December 2, 2020 09:02
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch 9 times, most recently from 3ebd077 to 85d0d3b Compare December 2, 2020 10:44
@qu1queee qu1queee changed the title WIP: Build watch secrets Build watch secrets Dec 2, 2020
@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 Dec 2, 2020
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch from 85d0d3b to 7d84cee Compare December 2, 2020 11:26
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Looks good. Some suggestions.

docs/development/authentication.md Outdated Show resolved Hide resolved
pkg/controller/build/build_controller.go Show resolved Hide resolved
pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved
test/integration/utils/builds.go Outdated Show resolved Hide resolved
test/integration/build_to_secrets_test.go Outdated Show resolved Hide resolved
pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved
pkg/controller/build/build_controller.go Show resolved Hide resolved
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch 2 times, most recently from 7cb34e2 to 304e77b Compare December 3, 2020 13:35
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Good progress. I missed one little detail in the docs and would like us to think once more on the CreateEvent for secrets.


If you are using `kubectl` command create secrets, then you can first create build secret using `kubectl create secret` command and annotate this secret using `kubectl annotate secrets`. Below is an example:

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

yaml -> sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkg/controller/build/build_controller.go Show resolved Hide resolved
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch from 304e77b to beadb1b Compare December 7, 2020 12:03
qu1queee and others added 3 commits December 7, 2020 13:04
Refactor the Watcher for Secrets
Enhance the handler.EnqueueRequestsFromMapFunc
Enhance the Events Predicates

Signed-off-by: Zoe <[email protected]>
We updated the Build predicates and watcher
We added test cases to verify that validations on
the Build are taking place when a particular annotation is defined
on a secret.

Modify build controller watches secrets integration tests

Signed-off-by: Zoe <[email protected]>
@qu1queee qu1queee force-pushed the pairing/build_watch_secrets branch from beadb1b to 9f2614b Compare December 7, 2020 12:04
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2020
@openshift-merge-robot openshift-merge-robot merged commit ae7a362 into shipwright-io:master Dec 7, 2020
@qu1queee qu1queee deleted the pairing/build_watch_secrets branch February 9, 2021 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build controller should watch referenced secrets
5 participants