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

Venafi CertificateRequest controller #1968

Merged
merged 15 commits into from
Aug 20, 2019
Merged

Venafi CertificateRequest controller #1968

merged 15 commits into from
Aug 20, 2019

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Aug 5, 2019

What this PR does / why we need it:
Adds the Venafi CertificateRequest controller.

-->

Adds the Venafi `CertificateRequest` controller.

/assign

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Aug 5, 2019
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

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

@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/testing Issues relating to testing size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 5, 2019
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Aug 7, 2019

I guess the venafi e2e tests are still broken?
Ready for a look anywho
/unassign
/assign @munnerz

@jetstack-bot jetstack-bot assigned munnerz and unassigned JoshVanL Aug 7, 2019
@JoshVanL JoshVanL changed the title WIP: Venafi CertificateRequest controller Venafi CertificateRequest controller Aug 7, 2019
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2019
munnerz
munnerz reviewed Aug 8, 2019
View reviewed changes
go.mod Outdated
@@ -104,6 +104,7 @@ require (
k8s.io/kube-aggregator v0.0.0-20190222095010-0b78038fe9e5
k8s.io/kube-openapi v0.0.0-20190228160746-b3a7cee44a30
k8s.io/utils v0.0.0-20190607212802-c55fbcfc754a
launchpad.net/gocheck v0.0.0-20140225173054-000000000087 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

needs removing 🙈

Copy link
Member

Choose a reason for hiding this comment

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

we can also use a replace directive to fix this once and for all and it fetching that repo actually work

pkg/issuer/venafi/issue_test.go Outdated Show resolved Hide resolved
pkg/util/api/api.go Outdated
"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
)

func DefaultCertDuration(d *metav1.Duration) time.Duration {
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to put this into pkg/api/util? Seems odd to define a new package just for this.

@munnerz
Copy link
Member

munnerz commented Aug 12, 2019

/retest

1 similar comment
@munnerz
Copy link
Member

munnerz commented Aug 13, 2019

/retest

@JoshVanL JoshVanL force-pushed the cr-venafi branch 2 times, most recently from af9b939 to 601701d Compare August 14, 2019 10:22
@jetstack-bot jetstack-bot added area/api Indicates a PR directly modifies the 'pkg/apis' directory area/vault Indicates a PR directly modifies the Vault Issuer code labels Aug 14, 2019
@JoshVanL
Copy link
Contributor Author

/hold

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2019
@JoshVanL
Copy link
Contributor Author

/hold cancel

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2019
munnerz
munnerz reviewed Aug 16, 2019
View reviewed changes
pkg/controller/certificaterequests/venafi/venafi.go Outdated
default:
message := "Failed to obtain venafi certificate"

v.reporter.Pending(cr, err, "RetrieveError", message)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be Failed? Else we're defaulting to retrying on failure...

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather fail-safe 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

munnerz
munnerz reviewed Aug 16, 2019
View reviewed changes
pkg/internal/venafi/fake/venafi.go
return v.ReadZoneConfigurationFn()
}

func (v *Venafi) SetClient(endpoint.Connector) {}
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use in a test

@munnerz
Copy link
Member

munnerz commented Aug 16, 2019

/test all

@munnerz
Copy link
Member

munnerz commented Aug 19, 2019

/retest

1 similar comment
@JoshVanL
Copy link
Contributor Author

/retest

@munnerz
Copy link
Member

munnerz commented Aug 19, 2019

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2019
@retest-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

@JoshVanL
Adds venafi internal package with venafi CR controller
d96c7d2 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Beginning porting and building new venafi tests
28f2d07 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Completes ./internal/venafi tests
858caf3 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Adds venafi setup test and updates issue for new client
b4a40f7 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Completes tests for internal venafi and venafi issuer
1b0dd4c 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Adds venafi CR controller tests
5432354 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Adds venafi CertificateRequest e2e tests
3634f5d 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Update vendor
b2653cc 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Moves DefaultCertDruation to ./api/util/duration.go
ab34645 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Remove launchpad from go modules
5cc3e0b 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Ensure we watch CertificateRequests in venafi e2e tests for
ad18215 
CertificateRequests

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Update venafi CR controller to use new reporter functionality
646f874 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Use DefaultCertDuration in ./pkg/api/util
efe1d4e 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Updates venafi CR tests to use full sync from base controller
a2f9d99 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Set venafi RetrieveError as failed
08c245a 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2019
@JoshVanL
Copy link
Contributor Author

/test pull-cert-manager-e2e-v1-15

@munnerz
Copy link
Member

munnerz commented Aug 20, 2019

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2019
@jetstack-bot jetstack-bot merged commit c7ed46e into cert-manager:master Aug 20, 2019
@jetstack-bot jetstack-bot added this to the v0.10 milestone Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@munnerz munnerz munnerz left review comments

Assignees

@munnerz munnerz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/testing Issues relating to testing area/vault Indicates a PR directly modifies the Vault Issuer code dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.10
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JoshVanL @jetstack-bot @munnerz @retest-bot