-
Notifications
You must be signed in to change notification settings - Fork 53
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
✨ Certificate support for image registry #960
Changes from 6 commits
b128ce3
0c20937
6155df5
0693c6a
cf5b281
421d281
3ca71ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
name: controller-manager | ||
namespace: system | ||
spec: | ||
template: | ||
spec: | ||
containers: | ||
- name: kube-rbac-proxy | ||
- name: manager | ||
volumeMounts: | ||
- name: e2e-cert | ||
mountPath: /var/certs/olm-ca.crt | ||
subPath: olm-ca.crt | ||
readOnly: true | ||
volumes: | ||
- name: e2e-cert | ||
secret: | ||
secretName: olmv1-cert | ||
items: | ||
- key: ca.crt | ||
path: olm-ca.crt |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
- op: add | ||
path: /spec/template/spec/volumes/- | ||
value: {"name":"ca-certificate", "secret":{"secretName":"catalogd-catalogserver-cert", "optional": false, "items": [{"key": "tls.crt", "path": "tls.crt"}]}} | ||
value: {"name":"catalogd-certificate", "secret":{"secretName":"catalogd-catalogserver-cert", "optional": false, "items": [{"key": "ca.crt", "path": "catalogd.crt"}]}} | ||
- op: add | ||
path: /spec/template/spec/containers/0/volumeMounts/- | ||
value: {"name":"ca-certificate", "readOnly": true, "mountPath":"/var/certs"} | ||
value: {"name":"catalogd-certificate", "readOnly": true, "mountPath":"/var/certs/catalogd.crt", "subPath":"catalogd.crt"} | ||
- op: add | ||
path: /spec/template/spec/containers/0/args/- | ||
value: "--ca-cert=/var/certs/tls.crt" | ||
value: "--ca-certs-dir=/var/certs" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
apiVersion: cert-manager.io/v1 | ||
kind: Certificate | ||
metadata: | ||
name: olmv1-cert | ||
spec: | ||
secretName: olmv1-cert | ||
dnsNames: | ||
- operator-controller.olmv1-system.svc | ||
- operator-controller.olmv1-system.svc.cluster.local | ||
privateKey: | ||
algorithm: ECDSA | ||
size: 256 | ||
issuerRef: | ||
name: olmv1-ca | ||
kind: ClusterIssuer | ||
group: cert-manager.io | ||
Comment on lines
+13
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This introduces a dependency on This means it is no longer possible to do this to install:
You have to checkout the repo run the install script. Are we ok with that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also about related changes in As far as I understand - we need to create CA and patch deployment of the manger only for E2E setups. If it is the case - can we put this in I would like to avoid having to create all this on every cluster just because our E2E setup requires it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need cert-manager regardless, AFAICT, as catalogd now creates certificates. So, the method listed above no longer works (or at least it shouldn't). |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -5,30 +5,56 @@ | |||||||
"crypto/x509" | ||||||||
"net/http" | ||||||||
"os" | ||||||||
"strings" | ||||||||
"time" | ||||||||
) | ||||||||
|
||||||||
func BuildHTTPClient(caCert string) (*http.Client, error) { | ||||||||
httpClient := &http.Client{Timeout: 10 * time.Second} | ||||||||
|
||||||||
if caCert != "" { | ||||||||
// tlsFileWatcher, err := certwatcher.New(caCert, "") | ||||||||
func LoadCerts(caDir string) (string, error) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like it would be a nicer abstraction if this was:
Suggested change
In which case, we could add each file's content, collect errors reading/adding each file, and then tell callers which specific files failed for which reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not compatible with the current rukpak API, which expects a string of PEM. Maybe when rukpak is fully integrated into operator-controller? I agree that this would be better for a larger directory. There does not appear to be a way to extract certificates/PEM from a CertPool, so it can't be done with this signature, yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I was just looking at And that made me think we could pass the Seems like I'm missing something though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also used when creating a BundleDeployment for a rukpak API.
So, we need to change the rukpak API to accept a caPool first, then we can do as you suggest. |
||||||||
if caDir == "" { | ||||||||
return "", nil | ||||||||
} | ||||||||
|
||||||||
cert, err := os.ReadFile(caCert) | ||||||||
if err != nil { | ||||||||
return nil, err | ||||||||
} | ||||||||
caCertPool := x509.NewCertPool() | ||||||||
caCertPool.AppendCertsFromPEM(cert) | ||||||||
tlsConfig := &tls.Config{ | ||||||||
RootCAs: caCertPool, | ||||||||
MinVersion: tls.VersionTLS12, | ||||||||
var certs []string | ||||||||
dirEntries, err := os.ReadDir(caDir) | ||||||||
if err != nil { | ||||||||
return "", err | ||||||||
} | ||||||||
for _, e := range dirEntries { | ||||||||
if e.IsDir() { | ||||||||
continue | ||||||||
} | ||||||||
tlsTransport := &http.Transport{ | ||||||||
TLSClientConfig: tlsConfig, | ||||||||
data, err := os.ReadFile(e.Name()) | ||||||||
if err != nil { | ||||||||
return "", err | ||||||||
} | ||||||||
httpClient.Transport = tlsTransport | ||||||||
certs = append(certs, string(data)) | ||||||||
} | ||||||||
return strings.Join(certs, "\n"), nil | ||||||||
} | ||||||||
|
||||||||
func BuildHTTPClient(caDir string) (*http.Client, error) { | ||||||||
httpClient := &http.Client{Timeout: 10 * time.Second} | ||||||||
|
||||||||
// use the SystemCertPool as a default | ||||||||
caCertPool, err := x509.SystemCertPool() | ||||||||
if err != nil { | ||||||||
return nil, err | ||||||||
} | ||||||||
|
||||||||
certs, err := LoadCerts(caDir) | ||||||||
if err != nil { | ||||||||
return nil, err | ||||||||
} | ||||||||
|
||||||||
caCertPool.AppendCertsFromPEM([]byte(certs)) | ||||||||
tlsConfig := &tls.Config{ | ||||||||
RootCAs: caCertPool, | ||||||||
MinVersion: tls.VersionTLS12, | ||||||||
} | ||||||||
tlsTransport := &http.Transport{ | ||||||||
TLSClientConfig: tlsConfig, | ||||||||
} | ||||||||
httpClient.Transport = tlsTransport | ||||||||
|
||||||||
return httpClient, nil | ||||||||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love that this is duplicated here and in the HEREDOC, but I also recognize that this setup is temporary. Can we just make sure that our follow-ups include a callout to remove this duplication? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #967 (comment) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
apiVersion: cert-manager.io/v1 | ||
kind: Issuer | ||
metadata: | ||
name: self-sign-issuer | ||
namespace: cert-manager | ||
spec: | ||
selfSigned: {} | ||
--- | ||
apiVersion: cert-manager.io/v1 | ||
kind: Certificate | ||
metadata: | ||
name: olmv1-ca | ||
namespace: cert-manager | ||
spec: | ||
isCA: true | ||
commonName: olmv1-ca | ||
secretName: olmv1-ca | ||
privateKey: | ||
algorithm: ECDSA | ||
size: 256 | ||
issuerRef: | ||
name: self-sign-issuer | ||
kind: Issuer | ||
group: cert-manager.io | ||
--- | ||
apiVersion: cert-manager.io/v1 | ||
kind: ClusterIssuer | ||
metadata: | ||
name: olmv1-ca | ||
spec: | ||
ca: | ||
secretName: olmv1-ca |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manager_deployment_cert.yaml
andmanager_cert_patch.yaml
seem to patch the same deployment and same fields but in different ways. I think it would be good to standardise.