Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 83
✨ Certificate support for image registry#960
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
b128ce30c209376155df50693c6acf5b281421d2813ca71baFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
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
Member 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? Member 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. ContributorAuthor 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,57 @@ import ( | ||||||||
| "crypto/x509" | ||||||||
| "net/http" | ||||||||
| "os" | ||||||||
| "path/filepath" | ||||||||
| "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) { | ||||||||
Member 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. ContributorAuthor 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. Member 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? ContributorAuthor 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, | ||||||||
| 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(filepath.Join(caDir, 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 | ||||||||
| } | ||||||||
Member 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? ContributorAuthor 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.yamlandmanager_cert_patch.yamlseem to patch the same deployment and same fields but in different ways. I think it would be good to standardise.