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

Expose additional sveltos settings for service deployment #823

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wahabmk
Copy link
Contributor

@wahabmk wahabmk commented Dec 20, 2024

Description

  1. Exposes more of Sveltos settings that can use useful with deploying helm charts and templating values, like being able to refer to a Secret or ConfigMap in helm values.
  2. Put all of the spec/settings related to deployment of services in ServiceSpec and use that for both ManagedCluster and MultiClusterService. So the CR will now become:
apiVersion: hmc.mirantis.com/v1alpha1
kind: ManagedCluster
. . .
spec:
  . . .
  serviceSpec:
    services:
      - template: kyverno-3-2-6
        name: kyverno
        namespace: kyverno
      - template: ingress-nginx-4-11-3
        name: ingress-nginx
        namespace: ingress-nginx
    priority: 100
    stopOnConflict: false
    . . .

Example

I have the following secret with dummy credentials:

apiVersion: v1
kind: Secret
metadata:
  name: my-creds
  namespace: hmc-system
data:
  username: bXl1c2VybmFtZQ==
  password: bXlwYXNzd29yZA==

If I want to refer to this secret to pluck these credentials while deploying my application, I can use the following:

apiVersion: source.toolkit.fluxcd.io/v1
kind: HelmRepository
metadata:
  labels:
    hmc.mirantis.com/managed: "true"
  name: myappz-oci
  namespace: hmc-system
spec:
  type: oci
  url: oci://registry-1.docker.io/wali90
---
apiVersion: hmc.mirantis.com/v1alpha1
kind: ServiceTemplate
metadata:
  name: myappz-0-6-0
  namespace: hmc-system
spec:
  helm:
    chartSpec:
      chart: myappz
      version: 0.6.0
      sourceRef:
        kind: HelmRepository
        name: myappz-oci
---
apiVersion: hmc.mirantis.com/v1alpha1
kind: ManagedCluster
metadata:
  name: wali-dev-1
  namespace: hmc-system
spec:
  config:
    clusterIdentity:
      name: aws-cluster-identity
      namespace: hmc-system
    controlPlane:
      amiID: ami-0eb9fdcf0d07bd5ef
      instanceType: t3.small
    controlPlaneNumber: 1
    publicIP: true
    region: ca-central-1
    worker:
      amiID: ami-0eb9fdcf0d07bd5ef
      instanceType: t3.small
    workersNumber: 1
  credential: aws-cluster-identity-cred
  serviceSpec:
    templateResourceRefs:
      - resource:
          apiVersion: v1
          kind: Secret
          name: my-creds
          namespace: hmc-system
        identifier: MyCreds
    services:
      - template: myappz-0-6-0
        name: myappz
        namespace: myappz
        values: |
          username: {{ (getResource "MyCreds").data.username }}
          password: {{ (getResource "MyCreds").data.password }}
    priority: 100
    stopOnConflict: false
  template: aws-standalone-cp-0-0-4

Now when looking at the kube deployment for myappz on the managed cluster, we can see that the values for USERNAME and PASSWORD env variables for the pod have been set:

~ kubectl -n myappz get deployments.apps myappz -o=jsonpath='{.spec.template.spec.containers[0].env}'
[{"name":"USERNAME","value":"bXl1c2VybmFtZQ=="},{"name":"PASSWORD","value":"bXlwYXNzd29yZA=="}]%  

@wahabmk wahabmk force-pushed the more-sveltos-features branch 3 times, most recently from 0d9878f to 197c122 Compare December 20, 2024 20:54
@wahabmk wahabmk self-assigned this Dec 20, 2024
@wahabmk wahabmk force-pushed the more-sveltos-features branch from 197c122 to 32388d9 Compare December 20, 2024 20:59
@wahabmk wahabmk force-pushed the more-sveltos-features branch from 32388d9 to 7484f80 Compare December 20, 2024 21:06
@wahabmk wahabmk marked this pull request as ready for review December 20, 2024 21:11
@wahabmk wahabmk changed the title Expose additional sveltos settings Expose additional sveltos settings for service deployment Dec 20, 2024
Copy link
Contributor

@BROngineer BROngineer left a comment

Choose a reason for hiding this comment

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

What are advantages of this implementation over the adding of valuesFrom field similar to the one used in sveltos types?

From my perspective:

  • despite adding flexibility, current implementation increases complexity of hmc's types
  • user still must fulfil values field instead of just referring to configMap/secret

Copy link
Member

@zerospiel zerospiel left a comment

Choose a reason for hiding this comment

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

I'd agree with @BROngineer, the initial idea should work but from a user's perspective it is unclear why the user should mix both hmc and sveltos fields within the hmc's resources.

The previous variant didn't contain any fields from the sveltos types, but now it does. Besides, the managedclusterservices resource now has several such types. Why cannot we just reuse the whole specs from the sveltos, e.g., sveltos.Spec and sveltos.HelmChart, within the mcs resource?

@wahabmk
Copy link
Contributor Author

wahabmk commented Dec 23, 2024

What are advantages of this implementation over the adding of valuesFrom field similar to the one used in sveltos types?

From my perspective:

  • despite adding flexibility, current implementation increases complexity of hmc's types
  • user still must fulfil values field instead of just referring to configMap/secret

The valuesFrom field has also been exposed in addition to templateResourceRefs and policyRefs fields. So a user may only use valuesFrom instead. The reason I used the example use case in the PR's description is because @pbasov was facing a similar problem from our discussions.

@wahabmk
Copy link
Contributor Author

wahabmk commented Dec 23, 2024

Besides, the managedclusterservices resource now has several such types.

I am not sure if I understood correctly. All the fields related to beach-head services are contained in ServiceSpec for both ManagedCluster and MultiClusterService.

Why cannot we just reuse the whole specs from the sveltos, e.g., sveltos.Spec and sveltos.HelmChart, within the mcs resource?

That might be better but part of the reason I couldn't expose the entire sveltos spec as it is, is because we are are using serviceTemplate to fill out the fields for the helm chart part of sveltos spec. I am open to better ideas though.

@zerospiel
Copy link
Member

Besides, the managedclusterservices resource now has several such types.

I am not sure if I understood correctly. All the fields related to beach-head services are contained in ServiceSpec for both ManagedCluster and MultiClusterService.

Why cannot we just reuse the whole specs from the sveltos, e.g., sveltos.Spec and sveltos.HelmChart, within the mcs resource?

That might be better but part of the reason I couldn't expose the entire sveltos spec as it is, is because we are are using serviceTemplate to fill out the fields for the helm chart part of sveltos spec. I am open to better ideas though.

I meant that the mcs and the managedcluster now have the serviceSpec field which in turn has two fields with direct types from the sveltos (templateResourceRefs and policyRefs). At the moment it seems, that the further away we get, the more fields are being copied/reused from the sveltos, and hence my question about having fields with the whole specs. If there were those 2 fields in the serviceSpec along with the current hmc-exclusive fields, we could've defaulted or copied the fields directly somewhere in the internal/sveltos/profile.go, couldn't we? I might still miss something but for me, it seems a bit easier.

The current approach works though, as I've mentioned earlier. Just wondering whether we could make the specs a little simpler to fill out without changing them too much depending on the features required.

pbasov
pbasov previously approved these changes Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Ability to use secrets and cms as helm values in ManagedCluster and MultiClusterService objects
4 participants