Skip to content

Install redpanda console through operator#5758

Closed
pvsune wants to merge 51 commits intodevfrom
pvsune/rp-console
Closed

Install redpanda console through operator#5758
pvsune wants to merge 51 commits intodevfrom
pvsune/rp-console

Conversation

@pvsune
Copy link
Contributor

@pvsune pvsune commented Aug 1, 2022

Cover letter

Support installing Redpanda Console via the operator.

This PR supports the minimum configuration needed to install Console. The PR is huge to review already; other features (see below) will be done in follow up PRs to help in reviewing.

A minimal Console CR might look like:

apiVersion: redpanda.vectorized.io/v1alpha1
kind: Console
metadata:
  name: my-console
spec:
  # Defaults are set https://github.com/redpanda-data/redpanda/pull/5758/files#diff-1120641da9564909cc77411d89009ba02787493e8bde59d2cf6c56e4007c58c9
  server:
    listenPort: 5000
  schema:
    enabled: true
  clusterKeyRef:
    name: rp-abc1234
    namespace: rp-1-234-5678
  deployment:
    image: vectorized/console:master
  connect:
    enabled: true
    clusters:
    - name: connectors-cluster
      url:  http://rp-1-234-5678.rp-abc1234.svc.cluster.local:8083
      tls:
        enabled: true
        secretKeyRef:
          name: my-console-tls
          namespace: default

This PR supports Console configs listed here. The following configs though will be supported in a follow up PR:

  • Redpanda which enables Console to get exact Redpanda/Kafka version
  • Console which enables topic documentation through git
  • Kafka.Protobuf Kafka.Messagepack

Support for mTLS Kafka API will also be in a follow up PR.

The Console controller will create the following resources:

  • Kafka Service Account, ACLs
  • ConfigMap
  • ServiceAccount
  • Deployment
  • Service

And they all will be owned by the Console object. It also adds the following finalizers for cleanup:

  • consoles.redpanda.vectorized.io/service-account
  • consoles.redpanda.vectorized.io/acl

The controller implements a ThreadSafeStore which synchronize resources (mostly Secrets for TLS certs) across namespaces. This enables the controller to reference a Cluster in different namespace from the Console CR.

Updating the Console spec should change the ConfigMap mounted to Console Deployment. But K8s doesn't do a hot-reload of the Deployment. In order to recreate the Pods when spec changes, we are creating a new ConfigMap and applying it in the Deployment which should trigger a rollout. ConfigMaps are now set to immutable and old ones are deleted on the Console controller reconciliation.

Most of the controller logic are found at pkg/console directory.

Ref

@pvsune pvsune force-pushed the pvsune/rp-console branch 17 times, most recently from 2e1c170 to cf8a364 Compare August 8, 2022 18:41
@pvsune pvsune marked this pull request as ready for review August 9, 2022 06:58
@pvsune pvsune requested a review from a team as a code owner August 9, 2022 06:58
@pvsune pvsune force-pushed the pvsune/rp-console branch 2 times, most recently from f726677 to bb4c8ee Compare August 10, 2022 04:46
@emaxerrno
Copy link
Contributor

emaxerrno commented Aug 10, 2022

this is cool!

Let's make sure that the user should be able to pass all console configuration options somehow @weeco has really good product intuition here, i'd make him a required reviewer on this.

@pvsune pvsune requested a review from weeco August 10, 2022 11:59
@vladoschreiner
Copy link

I opened the conversation about the missing configuration for the external access. Please resolve before finishing the review.

pvsune added 18 commits August 25, 2022 11:33
Don't watch Cluster events because we only need SchemaRegistry URL and it cannot be modified.
Requeue if Cluster is not configured instead.
Don't admit creation, update of Console if:
- Referenced Cluster is not found
- Console is in different namespace and --allow-console-any-ns=false
- Test that SASL user Secret is created
- Other resources created need to be mocked before we can test
- Check that Cluster status for Condition ClusterConfigured is not nil
- Check that Cluster ExternalListener is not nil
…ation of ConfigMaps.

Check count of attached ConfigMap to the Console before creating a new one
…in client.

Need to mock calling Kafka Admin in envtests
Test that:
- Secret for SASL is created
- ConfigMap is created
- Deployment is created
- Service is created
- Console object has the internal URL in status
Running make test formats the code via crlfmt
@pvsune pvsune force-pushed the pvsune/rp-console branch from 490dae0 to 85ac138 Compare August 25, 2022 03:37
pvsune added 2 commits August 25, 2022 13:04
CreateACLs, DeleteACLs don't return error
Use error Aggregate and specifically check for errors in the result
Cluster domain is set in the operator flag
@pvsune pvsune force-pushed the pvsune/rp-console branch from 85ac138 to 3767640 Compare August 25, 2022 05:05
Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

Last round of small comments, now I am ready to approve :)

// Users shouldn't check logs of operator to know this
// Adding Conditions in Console status might not be apt, record Event instead
// Alternatively, we can have this validation via Webhook
r.EventRecorder.Eventf(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is actually not true, events are in etcd kubernetes/kubernetes#4432 (comment)

if ex := cluster.ExternalListener(); ex != nil && ex.GetExternal().Subdomain != "" {
subdomain = fmt.Sprintf("console.%s", ex.GetExternal().Subdomain)
} else {
r.EventRecorder.Event(
Copy link
Contributor

Choose a reason for hiding this comment

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

again I think this will flood events, no? 🤔 I would personally just log this

}

var (
// Currently issuing TLS certs through LetsEncrypt
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we can state this in a generally available operator when ppl can inject any cert they want, right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ideally this should be checked if IssuerRef is set since it identifies the cert-manager Issuer used to issue certificate. But this isn't the case in v1 - NodeSecretRef is set but the certificates are signed by LetsEncrypt. Is there a way to get this dynamically? :thinking_face:

)

// ConsoleConfig is the config passed to the Redpanda Console app
type ConsoleConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't there version of this config inside console so we can just import it as we do for redpanda via rpk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I've added a comment about it - the config types in Console don't have Enterprise fields and we don't support all fields yet

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

If the CI passes - LGTM

@pvsune
Copy link
Contributor Author

pvsune commented Aug 26, 2022

This fails on the entrypoint test because I mistakenly pushed this directly to upstream instead of in my fork. As discussed, I will create another PR on my fork, close this, and reference this PR on the new one so we can refer the conversations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants