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 initial values and container to the kubeapps-apis service. #6595

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Aug 9, 2023

Description of the change

Adds the initial values and container for the oci-catalog to the kubeapps-apis service.

Leaving in draft for the moment as there are a number of things I want to check IRL while playing:

  • Use an actual grpc liveness check (to test that out, since we can remove the grpc health binary, as our oldest k8s version is now past 1.24) Bitnami tests are still testing with 1.23 apparently.
  • Evaluate whether kubeapps-apis is really the right pod for having this side-car. We'll need to use the service when validating a repo from the UX (which is kubeapps-apis), but also during the sync job. It could also run as a separate pod, but not sure it's worth the resources.

Note: I've got ociCatalog.enabled defaulting to false now, so no change to the chart output.

Benefits

Can start integration.

Possible drawbacks

Applicable issues

Additional Information

$ helm template ./chart/kubeapps --debug --set ociCatalog.enabled=true | grep -A 40 oci-catalog
install.go:200: [debug] Original chart version: ""
install.go:217: [debug] CHART PATH: /Users/minelson/dev/vmware/kubeapps/chart/kubeapps

        - name: oci-catalog
          image: docker.io/kubeapps/oci-catalog:latest
          imagePullPolicy: "IfNotPresent"
          securityContext:
            runAsNonRoot: true
            runAsUser: 1001
          command:
            - /oci-catalog
          args:
          env:
            - name: OCI_CATALOG_PORT
              value: "50061"
          envFrom:
          ports:
            - name: grpc-http
              containerPort: 50061
          livenessProbe:
            failureThreshold: 6
            initialDelaySeconds: 60
            periodSeconds: 10
            successThreshold: 1
            timeoutSeconds: 5
            exec:
              command: ["grpc_health_probe", "-addr=:50061"]
            initialDelaySeconds: 10
          readinessProbe:
            failureThreshold: 6
            initialDelaySeconds: 0
            periodSeconds: 10
            successThreshold: 1
            timeoutSeconds: 5
            exec:
              command: ["grpc_health_probe", "-addr=:50061"]
            initialDelaySeconds: 5
          resources:
            limits:
              cpu: 250m
              memory: 256Mi
            requests:
              cpu: 25m
              memory: 32Mi
          volumeMounts:
      volumes:
        - name: clusters-config
          configMap:
            name: release-name-kubeapps-clusters-config
        - name: ca-certs
          emptyDir: {}

With these changes in dev:

k -n kubeapps logs kubeapps-internal-kubeappsapis-7f5cc7f98b-wnqq4 oci-catalog     
[2023-08-14T01:42:41Z INFO  oci_catalog] listening for gRPC requests at 0.0.0.0:50061

@netlify
Copy link

netlify bot commented Aug 9, 2023

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 8b8ffbc
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/64dacb79e4dd480008543fd7

@absoludity absoludity force-pushed the add-oci-catalog-to-deploy branch 2 times, most recently from 1d124b8 to 09c00a0 Compare August 10, 2023 06:19
@@ -12,7 +12,7 @@ env_logger = "0.10"
futures-core = "0.3"
log = "0.4"
prost = "0.11"
reqwest = "0.11"
reqwest = { version = "0.11", default-features = false, features = ["rustls-tls"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Struggled to avoid the run-time error:

/oci-catalog/oci-catalog: error while loading shared libraries: libssl.so.1.1: cannot open shared object file: No such file or directory

so the above switches off the default reqwest feature which enables native tls and uses rustls-tls exclusively.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense, latest debian distro deprecated libssl.so.1.1 in favor of a newer version, nice to see it's doable to fall back to rustls-tls. Thanks for the info!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, makes sense, latest debian distro deprecated libssl.so.1.1 in favor of a newer version

Yeah, but I even tried compiling with rust:1.71.1-bookworm , which compiled fine (so I assume should have been linking to libssl3), but it would have the same runtime error (tested in that same build image just running binary with -h, rather than transferring to the output image). But yes, glad we can use rusttls instead.

@absoludity absoludity marked this pull request as ready for review August 14, 2023 01:44
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Update oci-catalog to not depend on native-tls.

Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

+1, but I'd have to double-check #6605 (comment). I don't know if here we are also adding duplicated keys to the yaml. Anyway, +1 to merge it now and fix it later (if so).

Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity merged commit f333e3a into main Aug 15, 2023
40 checks passed
@absoludity absoludity deleted the add-oci-catalog-to-deploy branch August 15, 2023 01:21
@absoludity
Copy link
Contributor Author

+1, but I'd have to double-check #6605 (comment). I don't know if here we are also adding duplicated keys to the yaml. Anyway, +1 to merge it now and fix it later (if so).

Done - thanks.

absoludity added a commit that referenced this pull request Aug 15, 2023
…6618)

### Description of the change

Follows on from #6595, updates the proto service files according to
buf's lint, and generates the go client for use in the asset-syncer.

Note: I added a separate `buf.yaml` and `buf.gen.yaml` for the
oci-catalog service as buf doesn't let you refer to a proto outside of
the `buf.yaml` root, so the other option would have been to move our
existing `buf.yaml` and `buf.generate` into the Kubeapps root directory.
I don't mind either way, but it made more sense to me for the
oci-catalog service to have its own so that it is independent (it may be
useful outside of Kubeapps, theoretically).

### Benefits

The next PR can use the client without being cluttered by the extra
diff.

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref: #6263

---------

Signed-off-by: Michael Nelson <minelson@vmware.com>
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.

3 participants