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

feat: add support to multiple schemas and the new KFDDistribution schema #95

Merged
merged 66 commits into from
Jul 3, 2023

Conversation

alessiodionisi
Copy link
Member

@alessiodionisi alessiodionisi commented May 19, 2023

Closes #96.

Based on example/new-schema-version branch

@alessiodionisi alessiodionisi self-assigned this May 19, 2023
@alessiodionisi alessiodionisi marked this pull request as draft May 19, 2023 14:51
@alessiodionisi alessiodionisi marked this pull request as ready for review May 26, 2023 09:35
@alessiodionisi alessiodionisi marked this pull request as draft May 30, 2023 12:40
@alessiodionisi alessiodionisi marked this pull request as ready for review June 1, 2023 11:44
@alessiodionisi alessiodionisi marked this pull request as draft June 1, 2023 11:45
@nutellinoit nutellinoit self-requested a review June 1, 2023 12:53
Copy link
Member

@nutellinoit nutellinoit left a comment

Choose a reason for hiding this comment

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

Some little changes

@nutellinoit
Copy link
Member

We should also change the behaviour with the certManager setting in the ingress module

Instead of

        nginx:
          # the tls section defines how the tls for the ingresses should be managed
          tls:
            # provider can be certManager, secret
            provider: certManager
        certManager:
          # the configuration for the clusterIssuer that will be created
          clusterIssuer:
            # the name of the clusterIssuer
            name: letsencrypt-fury
            # the email used during issuing procedures
            email: example@sighup.io
            # the type of the clusterIssuer, can be only http01, dns01 integration is not yet supported
            type: http01

We should swap type with solvers to make the user free to define the configuration

        nginx:
          # the tls section defines how the tls for the ingresses should be managed
          tls:
            # provider can be certManager, secret
            provider: certManager
        certManager:
          # the configuration for the clusterIssuer that will be created
          clusterIssuer:
            # the name of the clusterIssuer
            name: letsencrypt-fury
            # the email used during issuing procedures
            email: example@sighup.io
            # solvers definition
            solvers: []

@alessiodionisi alessiodionisi marked this pull request as ready for review June 6, 2023 10:30
@nutellinoit
Copy link
Member

nutellinoit commented Jun 6, 2023

While analyzing the current state of the distro provider together with @smerlos and @Deepzima , we found some additions that are nice to have:

On the distribution side, we have the need to enable or disable stuff, but instead of going to the correct approach

modules:
  networking:
    type: none|calico|cilium
    calico:
      podCidr:
    cilium:
      podCidr:
      nodeNetmask:
  ingress:
    nginx: 
      type: none|single|dual
  monitoring:
    # cannot be disabled
  logging:
    type: none|opensearch-single|opensearch-triple|loki
  auth:
    provider:
      type: none|basicAuth|sso
  dr:
    type: none|on-premises|eks
  policy:
    type: none|gatekeeper

Let's explain:

  • modules.networking.type: this field defines if we want to install the networking, by default as none. We need to also add the configurations for calico/cilium (only the cidrs I think are needed here, other stuff can be achieved with customPatches)
  • ingress.nginx.type: this field can also be none. if set to none, we won't install the nginx batteries and also we should avoid the creation of the ingresses for the modules
  • monitoring cannot be disabled
  • logging.type: can be none . If it's none, we just skip the module entirely
  • dr.type: this is important for the Distribution provider, because we can define which type of velero installation we want to have, by default, for the distro provider we will use on-premises (as a default on the template) but it can also be none or eks. With eks we expect to put by hand the values that are passed automatically by furyctl in the EKSCluster provider. With none, we will not install it.
  • policy.type: can also be none . If it's none, we skip the apply of all the stuff from the policy/opa module (for now, at least)

PS: This applies also to EKS

@nutellinoit
Copy link
Member

Opened a related issue: #105

Copy link
Member

@Al-Pragliola Al-Pragliola left a comment

Choose a reason for hiding this comment

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

Given the new defaults folder, we should delete the file furyctl-defaults.yaml

kfd.yaml Show resolved Hide resolved
schemas/private/ekscluster-kfd-v1alpha2.json Show resolved Hide resolved
Copy link
Contributor

@omissis omissis left a comment

Choose a reason for hiding this comment

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

I left minor comments, LGTM

kfd.yaml Outdated Show resolved Hide resolved
templates/distribution/terraform/velero.tf.tpl Outdated Show resolved Hide resolved
@alessiodionisi alessiodionisi merged commit a87399c into main Jul 3, 2023
1 check passed
@alessiodionisi alessiodionisi deleted the feature/distro-provider branch July 3, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new KFDDistribution kind schema
4 participants