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

add webhook - fixes #10 #13

Merged
merged 2 commits into from
Jan 21, 2020
Merged

Conversation

davidkarlsen
Copy link
Contributor

@davidkarlsen davidkarlsen commented Dec 13, 2019

Signed-off-by: David J. M. Karlsen [email protected]


This change is Reviewable

@davidkarlsen davidkarlsen marked this pull request as ready for review December 16, 2019 22:57
@davidkarlsen
Copy link
Contributor Author

Do you have any CI?
It could be automated with github-actions / kind / helm-tooling-actions like this: https://github.com/evryfs/helm-charts/commit/0aa28bdb22f3471a3c09265082765f97b38dae3e/checks?check_suite_id=358671120

@zhill
Copy link
Member

zhill commented Dec 18, 2019

Do you have any CI?
It could be automated with github-actions / kind / helm-tooling-actions like this: https://github.com/evryfs/helm-charts/commit/0aa28bdb22f3471a3c09265082765f97b38dae3e/checks?check_suite_id=358671120

Yeah, that's a good call. We also use CircleCI so that's an option. I'll look into it.

@zhill
Copy link
Member

zhill commented Dec 30, 2019

I'll be getting to this soon. The holidays in the US have delayed my availability but should be getting it fully reviewed and able to merge soon. Thanks for your patience @davidkarlsen

Copy link
Member

@zhill zhill 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. I'll merge this once the refactor merge is in

@davidkarlsen
Copy link
Contributor Author

I wonder if the ca-bundle is really needed https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#webhook-configuration, "URL" Here is an example of a mutating webhook configured to call a URL (and expects the TLS certificate to be verified using system trust roots, so does not specify a caBundle):

@davidkarlsen davidkarlsen force-pushed the feature/handle-webhook branch from 86dcb55 to 5cafb15 Compare January 14, 2020 09:31
@davidkarlsen
Copy link
Contributor Author

I’ll Get a fresh squashed PR which passes dco once the other one is merged...

Signed-off-by: David J. M. Karlsen <[email protected]>
@davidkarlsen davidkarlsen force-pushed the feature/handle-webhook branch from 5cafb15 to fc9cced Compare January 16, 2020 08:43
Signed-off-by: David J. M. Karlsen <[email protected]>
@davidkarlsen
Copy link
Contributor Author

OK - clean patch applied.
Can you test this in a cluster? I am not sure that the caBundle actually needs to be set for successful use&installation.

@davidkarlsen
Copy link
Contributor Author

@zhill did you have a chance to look at this? Does it work OOTB w/o any caBundle?

@davidkarlsen
Copy link
Contributor Author

@zhill ?

@zhill
Copy link
Member

zhill commented Jan 21, 2020

@zhill ?

I've done some very initial testing that it does install OOTB w/o caBundle, but need further testing to ensure it works as expected. I'll try to get to that later tonight so we can get this resolved soon. Thanks @davidkarlsen !

@zhill
Copy link
Member

zhill commented Jan 21, 2020

I'm going to go ahead and merge this. I'm seeing some issues with the apiservice selection but I think that is not part of this change since it is setup in the init-ca script run during the install init-ca job.

kc get apiservice/v1beta1.admission.anchore.io
NAME                           SERVICE                                        AVAILABLE                  AGE
v1beta1.admission.anchore.io   default/admctrl-anchore-admission-controller   False (MissingEndpoints)   13s

@zhill zhill merged commit 4db4fd9 into anchore:master Jan 21, 2020
@davidkarlsen
Copy link
Contributor Author

Ah - init-ca - we set out to fix that in #9. But is that really needed?

@davidkarlsen davidkarlsen deleted the feature/handle-webhook branch January 21, 2020 10:50
@davidkarlsen
Copy link
Contributor Author

OK, found a good description here: https://github.com/elithrar/admission-control#generating-tls-certificates

@davidkarlsen
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants