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

retain and size set #172

Merged
merged 14 commits into from
Feb 21, 2024
Merged

retain and size set #172

merged 14 commits into from
Feb 21, 2024

Conversation

cooktheryan
Copy link
Collaborator

Set the pvc to be retained at default and specify size.

Follow up PR will include setting name but out of scope for initial issue. Also has been somewhat challenging so wanted to get this in and working first then begin the process to allow for name change

Signed-off-by: Ryan Cook <[email protected]>
@osmman
Copy link
Contributor

osmman commented Feb 12, 2024

Follow up PR will include setting name but out of scope for initial issue. Also has been somewhat challenging so wanted to get this in and working first then begin the process to allow for name change

Will that PR include changes of PvcName? I am asking because currently it looks weird that we have option pvcName and pvc in same level of API.

Signed-off-by: Ryan Cook <[email protected]>
Signed-off-by: Ryan Cook <[email protected]>
@cooktheryan
Copy link
Collaborator Author

@osmman somewhat fighting through pvc naming right now. It's somewhat weird. Things appear to break if the variables of pvcName are used in their current way.

Signed-off-by: Ryan Cook <[email protected]>
@cooktheryan
Copy link
Collaborator Author

If acceptable to @bouskaJ and @osmman I would like to generate a new issue on providing your own PVC name.

//Enable Service monitors for rekor
Monitoring MonitoringConfig `json:"monitoring,omitempty"`
//Rekor Search UI
RekorSearchUI RekorSearchUI `json:"rekorSearchUI,omitempty"`
// Signer configuration
Signer RekorSigner `json:"signer,omitempty"`
// PVC configuration
Pvc Pvc `json:"pvc,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there is some bug when we setting default values. I sucessfully broke operator on nil pointer exception when I use this object:

apiVersion: rhtas.redhat.com/v1alpha1
kind: Trillian
metadata:
  name: example
  namespace: test
spec: {}

Error message:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x103dce254]

goroutine 466 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	/Users/tturek/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:116 +0x1a4
panic({0x104276580?, 0x10527ccb0?})
	/opt/homebrew/Cellar/go/1.21.5/libexec/src/runtime/panic.go:914 +0x218
github.com/securesign/operator/controllers/trillian/actions/logserver.deployAction.Handle(.....)
	/Users/tturek/Workspace/securesign/secure-sign-operator/controllers/trillian/actions/logserver/deployment.go:48 +0x1e4

Size string `json:"size,omitempty"`
// Retain policy for the PVC
//+kubebuilder:default:=true
Retain bool `json:"retain"`
Copy link
Contributor

@osmman osmman Feb 20, 2024

Choose a reason for hiding this comment

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

default value is not work correctly. It will be set false for this example:

spec:
  database:
    create: true

final values will be

spec:
  database:
    create: true
    databaseSecretRef:
      name: rhtassk4q9
    pvc:
      name: trillian-mysql
      retain: false
      size: 5Gi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@osmman I think this was a fallout of moving those items to common. This should be fixed now

@cooktheryan cooktheryan requested a review from osmman February 20, 2024 16:15
@cooktheryan
Copy link
Collaborator Author

@osmman we should be good with the config above

deploy

apiVersion: rhtas.redhat.com/v1alpha1
kind: Securesign
metadata:
  labels:
    app.kubernetes.io/name: securesign-sample
    app.kubernetes.io/instance: securesign-sample
    app.kubernetes.io/part-of: trusted-artifact-signer
  name: securesign-sample
spec:
  rekor:
    externalAccess:
      enabled: true
    monitoring:
      enabled: false
  trillian:
    database:
      create: true

Verification

kubectl get rekor -o yaml
apiVersion: v1
items:
- apiVersion: rhtas.redhat.com/v1alpha1
  kind: Rekor
  metadata:
    creationTimestamp: "2024-02-20T16:56:20Z"
    generation: 3
    labels:
      app.kubernetes.io/component: rekor
      app.kubernetes.io/instance: securesign-sample
      app.kubernetes.io/managed-by: controller-manager
      app.kubernetes.io/name: securesign-sample
      app.kubernetes.io/part-of: trusted-artifact-signer
    name: securesign-sample
    namespace: default
    ownerReferences:
    - apiVersion: rhtas.redhat.com/v1alpha1
      blockOwnerDeletion: true
      controller: true
      kind: Securesign
      name: securesign-sample
      uid: 7195c262-0ba1-414a-b249-d7a6e1cb7f9c
    resourceVersion: "701"
    uid: 9f10e93f-031c-4a69-9c3e-84d4bfb38545
  spec:
    backFillRedis:
      enabled: true
      schedule: 0 0 * * *
    externalAccess:
      enabled: true
    monitoring: {}
    pvc:
      name: rekor-securesign-sample-pvc
      retain: true
      size: 5Gi

@cooktheryan
Copy link
Collaborator Author

I'll look to see if the trillian bug is still there

@osmman
Copy link
Contributor

osmman commented Feb 21, 2024

I'll look to see if the trillian bug is still there

You can cherry pick 3b7fee8f2de03b7a11ae67fd837e6cba9e75690a commit it should fix it.

Copy link

openshift-ci bot commented Feb 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cooktheryan, osmman

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

@osmman
Copy link
Contributor

osmman commented Feb 21, 2024

/lgtm

@openshift-merge-bot openshift-merge-bot bot merged commit d573cfc into main Feb 21, 2024
7 checks passed
@osmman osmman deleted the retain_pvc branch April 5, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants