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 .spec.certSecretRef to Bucket API #1475

Merged
merged 1 commit into from
May 22, 2024

Conversation

matheuscscp
Copy link
Contributor

Fixes #973

@matheuscscp matheuscscp force-pushed the bucket-cert-secret branch 13 times, most recently from aaf53e6 to 9835883 Compare May 9, 2024 11:55
@matheuscscp matheuscscp marked this pull request as ready for review May 9, 2024 11:56
docs/spec/v1beta2/buckets.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/buckets.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/buckets.md Outdated Show resolved Hide resolved
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @matheuscscp 🏅

@stefanprodan stefanprodan added enhancement New feature or request area/bucket Bucket related issues and pull requests labels May 14, 2024
@stefanprodan stefanprodan changed the title Add .certSecretRef for Bucket API Add .spec.certSecretRef to Bucket API May 14, 2024
docs/spec/v1beta2/buckets.md Outdated Show resolved Hide resolved
pkg/minio/minio.go Show resolved Hide resolved
internal/controller/bucket_controller.go Outdated Show resolved Hide resolved
internal/controller/bucket_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Overall, the implementation and test changes look good to me.
Left one last reconciler test suggestion.

internal/controller/bucket_controller.go Show resolved Hide resolved
@kingdonb
Copy link
Member

I just finished testing this with a test bucket from minio/minio configured with a self-signed certificate, and 👍 it worked

Here are the Helm values I used, for reference, in case anyone wants to replicate the test:

mode: standalone
replicas: 1
resources:
  requests:
    memory: 512Mi
rootPassword: rootpass123
rootUser: rootuser
tls:
  certSecret: bucket-secret
  enabled: true
  privateKey: tls.key
  publicCrt: tls.crt

and it took some time to figure out exactly what the other configuration artifacts should look like, writing them out here for completeness:

---
apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: Bucket
metadata:
  name: test-bucket
  namespace: test-bucket
spec:
  bucketName: test-bucket
  endpoint: minio.test-bucket.svc.cluster.local:9000
  interval: 1m0s
  provider: generic
  certSecretRef:
    name: bucket-secret
  secretRef:
    name: bucket-client-credentials
apiVersion: v1
kind: Secret
metadata:
  name: bucket-client-credentials
  namespace: test-bucket
type: Opaque
stringData:
  accesskey: rootuser
  secretkey: rootpass123
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: my-bucket-cert
  namespace: test-bucket
spec:
  dnsNames:
    - minio.test-bucket.svc.cluster.local
  isCA: true
  commonName: my-bucket-cert
  secretName: bucket-secret
  privateKey:
    algorithm: ECDSA
    size: 256
  issuerRef:
    name: ca-issuer
    kind: Issuer
    group: cert-manager.io
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: ca-issuer
  namespace: test-bucket
spec:
  selfSigned: {}

You can also create a real access key and secret, but in the tests we also use the root user/pass so it's fine, works as well.

@stefanprodan
Copy link
Member

Thanks @kingdonb for testing this on your cluster 🏅

Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @matheuscscp 🏅

@stefanprodan stefanprodan merged commit 81b4dd0 into fluxcd:main May 22, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bucket Bucket related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying a custom CA certificate in Bucket API
4 participants