feat: allow cert-manager annotations on ingress based on environment variables #86

cloud-j-luna · 2023-02-15T22:55:45Z

This PR allows cert-manager annotations on ingress based on environment variables.

boz

Looking great

cluster/kube/client.go

cluster/kube/client_ingress.go

boz

🚀

cluster/util/environment.go

boz

Just a note that it's generally preferable to use a framework for reading environment variables. This or cobra+viper are good options.

boz

🚀

anilmurty · 2023-03-01T17:39:18Z

from sig-support: @troian holding off merging this until he's confident about the code base from the changes for API refactor and 0.24 network upgrade

troian · 2023-04-11T22:59:53Z

@cloud-j-luna rebase it on to gpu branch. we will merge it there first

cloud-j-luna · 2023-04-12T10:03:54Z

@troian rebased in to gpu there are some checks failing regarding commit message but as the commits get squashed it shouldn't be a problem, right?

troian · 2023-04-12T11:16:36Z

@cloud-j-luna the gpu branch will not be squashed. the check is failing as commit message needs to be formatted with conventional commits spec.

cloud-j-luna · 2023-04-12T11:21:39Z

@cloud-j-luna the gpu branch will not be squashed. the check is failing as commit message needs to be formatted with conventional commits spec.

I meant this PR to be squashed

troian · 2023-04-12T13:52:23Z

yeah, however, the commit message is taken from PR.
there are other tests failing too

…variables

…bled

troian

I gave it a little thought and a few things must be addressed.

tlsEnabled should also account if deployment requested issuing TLS certificate, it does not have to be mandatory
all the changes need tests, including e2e

cluster/mocks/client.go

cluster/kube/client_ingress.go

troian · 2023-04-14T13:16:34Z

cluster/kube/client_ingress.go

@@ -66,11 +67,20 @@ func kubeNginxIngressAnnotations(directive ctypes.ConnectHostnameToDeploymentDir
 		}
 	}

+	switch env["AKASH_PROVIDER_ISSUER_TYPE"] {
+	case "cluster-issuer":
+		result[fmt.Sprintf("%s/cluster-issuer", certManager)] = env["AKASH_PROVIDER_ISSUER_NAME"]


it looks like we don't need both AKASH_PROVIDER_ISSUER_TYPE and AKASH_PROVIDER_ISSUER_NAME and call it AKASH_PROVIDER_CERT_ISSUER

AKASH_PROVIDER_CERT_ISSUER should be defined in the provider env section

env["AKASH_PROVIDER_ISSUER_NAME"] may return "" if env is not set

We need both as the resulting label is different. This will also help in the future if we want to automate per-deployment/account issuers.

what are the values of both env variables in this case?

In this case it would be AKASH_PROVIDER_ISSUER_TYPE = "cluster-issuer", AKASH_PROVIDER_ISSUER_NAME would be whatever the provider chose as the cert-manager ClusterIssuer name on the cluster. By default the name is letsencrypt as the default issuer is Let's Encrypt.

By having letsencrypt value, the code in the provider can deduct, that issue is cluster-issuer ?

it can't thus the cluster-issuer in the annotation, so the provider knows that its a ClusterIssuer called letsencrypt rather than a namespace scoped issuer called letsencrypt as well

if workload asks the provider to issue a certificate wouldn't it always be the cluster-issuer?

Depends on the provider configuration. For now only cluster-issuer are allowed for providers, but the client_ingress.go is ready to work with namespaced issuers.

troian · 2023-04-14T13:17:02Z

cluster/kube/client_ingress.go

+		result[fmt.Sprintf("%s/cluster-issuer", certManager)] = env["AKASH_PROVIDER_ISSUER_NAME"]
+		break
+	case "issuer":
+		result[fmt.Sprintf("%s/issuer", certManager)] = env["AKASH_PROVIDER_ISSUER_NAME"]


comment from section above applies here as well

troian · 2023-04-14T15:44:46Z

cluster/kube/client.go

@@ -98,6 +100,7 @@ func NewClient(ctx context.Context, log log.Logger, ns string, configPath string
 		ns:                ns,
 		log:               log.With("client", "kube"),
 		kubeContentConfig: config,
+		env:               util.EnvironmentVariablesToMap(),


env variables must be validated for values and then read into dedicated struct fields

Here is a direct conversion from the environment variables to a map[string]string. I can make validations but after this call. To have more fine-grained error handling on unexpected values.

we would rather want to see dedicated struct fields instead of env converted to map. map is useful as intermediate store in some circumstances (like converting data to json). Go is a strict type of language and we use as much of it.

client instantiation is the last step where all configuration values must be validated

I'm not sure what To have more fine-grained error handling on unexpected values. means. If there is an unexpected configuration client should exit with error.

It means we know the context in which the usage gave an error. Having a struct makes absolute sense! I'll change the code to support such configuration rather than environment variables

We treat an error as an error, it is really hard to chase configuration errors on the fly.

the client must not interact with env variables, configs etc, the only way for it to receive configuration is thru a parameter to NewClient.

Each environment variable must have a corresponding cli flag for it. That's why using env within the client breaks it.

Using environment variables inside implementation complicates writing unit tests.

remove environment variables from the client completely and use struct fields as values during annotations init.
I'm still unsure if using two different vars is the correct way to do it. if the workload going to use ns issuer then it has nothing to do with the cluster

cluster/kube/client_ingress.go

troian · 2023-04-17T23:20:03Z

apparently gh closes PRs if base branched merged, you’ll have to reopen it.

cloud-j-luna · 2023-04-18T11:23:52Z

apparently gh closes PRs if base branched merged, you’ll have to reopen it.

I have to open a new PR, to which branch do you want me to merge to? This branch is rebased on the gpu branch.

troian · 2023-04-18T12:43:19Z

to the main

cloud-j-luna requested review from boz and troian as code owners February 15, 2023 22:55

cloud-j-luna mentioned this pull request Feb 15, 2023

HTTPS/TLS/SSL provider ingress support akash-network/support#3

Open

boz requested changes Feb 22, 2023

View reviewed changes

cluster/kube/client.go Outdated Show resolved Hide resolved

cluster/kube/client_ingress.go Outdated Show resolved Hide resolved

cloud-j-luna requested a review from boz February 22, 2023 19:17

boz previously approved these changes Feb 27, 2023

View reviewed changes

cluster/util/environment.go Outdated Show resolved Hide resolved

cloud-j-luna dismissed boz’s stale review via 7e9ea3d February 27, 2023 18:19

cloud-j-luna requested a review from boz February 27, 2023 18:19

boz reviewed Feb 27, 2023

View reviewed changes

cloud-j-luna requested a review from boz February 27, 2023 20:12

boz approved these changes Feb 28, 2023

View reviewed changes

cloud-j-luna changed the base branch from main to gpu April 12, 2023 10:01

cloud-j-luna force-pushed the luna/tls-cert-manager branch from 85a1083 to f629b8f Compare April 12, 2023 16:21

cloud-j-luna added 5 commits April 12, 2023 17:22

feat: allow cert-manager annotations on ingress based on environment …

91f5fe2

…variables

refactor: improve code

2bf09c2

refactor: improve code

f7ce579

refactor: improve code

a4fcdff

fix: pre-allocate environment variables map

69740f6

cloud-j-luna force-pushed the luna/tls-cert-manager branch from f629b8f to 69740f6 Compare April 12, 2023 16:22

Merge branch 'gpu' into luna/tls-cert-manager

58f591c

troian changed the title ~~Allow cert-manager annotations on ingress based on environment variables~~ fix: pre-allocate environment variables map Apr 12, 2023

cloud-j-luna changed the title ~~fix: pre-allocate environment variables map~~ feat: allow cert-manager annotations on ingress based on environment variables Apr 12, 2023

cloud-j-luna added 2 commits April 13, 2023 21:53

fix: disable ingress by default and omit tls rules on ingress if disa…

5ab7f63

…bled

fix: implement interface on mock client

60c4599

troian requested changes Apr 14, 2023

View reviewed changes

troian reviewed Apr 14, 2023

View reviewed changes

cluster/kube/client_ingress.go Outdated Show resolved Hide resolved

refactor: Apply suggestions on akash-network#86

faabd57

troian deleted the branch akash-network:gpu April 17, 2023 15:29

troian closed this Apr 17, 2023

cloud-j-luna added a commit to quasarch/provider that referenced this pull request Apr 18, 2023

refactor: Apply suggestions on akash-network#86

17eb424

cloud-j-luna mentioned this pull request Apr 18, 2023

WIP feat: allow cert-manager annotations on ingress based on environment variables PT.2 #112

Open

andy108369 mentioned this pull request Jun 14, 2023

Add ClusterIssuer to hostname-operator akash-network/helm-charts#196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow cert-manager annotations on ingress based on environment variables #86

feat: allow cert-manager annotations on ingress based on environment variables #86

cloud-j-luna commented Feb 15, 2023

boz left a comment

boz left a comment

boz left a comment

boz left a comment

anilmurty commented Mar 1, 2023

troian commented Apr 11, 2023

cloud-j-luna commented Apr 12, 2023

troian commented Apr 12, 2023

cloud-j-luna commented Apr 12, 2023

troian commented Apr 12, 2023

troian left a comment

troian Apr 14, 2023

cloud-j-luna Apr 14, 2023 •

edited

Loading

troian Apr 14, 2023

cloud-j-luna Apr 14, 2023

troian Apr 14, 2023

cloud-j-luna Apr 14, 2023

troian Apr 14, 2023

cloud-j-luna Apr 14, 2023

troian Apr 14, 2023

troian Apr 14, 2023

cloud-j-luna Apr 14, 2023

troian Apr 14, 2023 •

edited

Loading

cloud-j-luna Apr 14, 2023

troian Apr 14, 2023

troian commented Apr 17, 2023

cloud-j-luna commented Apr 18, 2023

troian commented Apr 18, 2023

feat: allow cert-manager annotations on ingress based on environment variables #86

feat: allow cert-manager annotations on ingress based on environment variables #86

Conversation

cloud-j-luna commented Feb 15, 2023

boz left a comment

Choose a reason for hiding this comment

boz left a comment

Choose a reason for hiding this comment

boz left a comment

Choose a reason for hiding this comment

boz left a comment

Choose a reason for hiding this comment

anilmurty commented Mar 1, 2023

troian commented Apr 11, 2023

cloud-j-luna commented Apr 12, 2023

troian commented Apr 12, 2023

cloud-j-luna commented Apr 12, 2023

troian commented Apr 12, 2023

troian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-j-luna Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troian Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troian commented Apr 17, 2023

cloud-j-luna commented Apr 18, 2023

troian commented Apr 18, 2023

cloud-j-luna Apr 14, 2023 •

edited

Loading

troian Apr 14, 2023 •

edited

Loading