Upgrade operator-sdk to 1.38.0 #1040
Conversation
|
Now I am still fixing the option "prometheus.expose=EnableWithAuthProxy", I cannot provide a custom tlsSecret to the manager. There aren't any logs from controller-runtime. After that issue is fixed, I will test rbac role authentication and then mark this PR to be ready. |
cchen-vertica
force-pushed
the
cchen/upgrade-sdk
branch
from
a4288d2 to
8d7028c
Compare
|
You did not need any library upgrades? |
cchen-vertica
force-pushed
the
cchen/upgrade-sdk
branch
from
858c096 to
85ee265
Compare
cchen-vertica
requested review from
roypaulin,
fenic-fawkes,
HaoYang0000,
qindotguan and
LiboYu2
as code owners
| # using the proxy (see https://github.com/brancz/kube-rbac-proxy). The | ||
| # metrics endpoint will use the https scheme. | ||
| # EnableWithoutAuth: Like EnableWithAuthProxy, this will create a service | ||
| # EnableWithAuth: A new service object will be created that exposes the |
There was a problem hiding this comment.
does EnableWithAuth also a tls secret?
There was a problem hiding this comment.
No, EnableWithAuth could not provide tls certs when accessing the metrics url.
| secureMetrics := strings.EqualFold(opcfg.GetMetricsExposeMode(), "EnableWithAuth") | ||
| secureByTLS := strings.EqualFold(opcfg.GetMetricsExposeMode(), "EnableWithTLS") |
There was a problem hiding this comment.
Let's move these to opcfg package: add 2 methods that will do the comparison there.
There was a problem hiding this comment.
Sure, I can add that.
| secureMetrics := strings.EqualFold(opcfg.GetMetricsExposeMode(), "EnableWithAuth") | ||
| secureByTLS := strings.EqualFold(opcfg.GetMetricsExposeMode(), "EnableWithTLS") | ||
| var metricCertDir string | ||
| if opcfg.GetMetricsTLSSecret() != "" { |
There was a problem hiding this comment.
Isn't it better to complain if EnableWithTLS but no secret is passed?
There was a problem hiding this comment.
That is fine. Normally, the token is good enough to verify the user's identity. No need to use tls secret for "EnableWithTLS".
| // TLSOpts is used to allow configuring the TLS config used for the server. If certificates are | ||
| // not provided, self-signed certificates will be generated by default. This option is not recommended for |
There was a problem hiding this comment.
Who is going to generate self-signed certs?
There was a problem hiding this comment.
The controller-runtime will generate it.
| # Set this to true if you want to create a ServiceMonitor. This object is a | ||
| # CR provided by the prometheus operator to allow for easy service discovery. | ||
| # https://github.com/prometheus-operator/prometheus-operator | ||
| createServiceMonitor: false |
There was a problem hiding this comment.
You removed this but still kept monitor.yaml in config/prometheus
There was a problem hiding this comment.
monitor.yaml in config/prometheus is generated by new operator-sdk. I leave it there in case we want to put service monitor back. In kustomization.yaml, we can uncomment the line contains "prometheus" to generate the monitor manifest.
| @@ -6,7 +6,7 @@ generatorOptions: | |||
|
|
|||
| configMapGenerator: | |||
| - envs: | |||
| - operator-envs | |||
| - operator-envs-with-value | |||
There was a problem hiding this comment.
Where is operator-envs-with-value file?
There was a problem hiding this comment.
It's an internal file which is ignored by .gitignore.
There was a problem hiding this comment.
Shouldn't update this instead of removing it?
There was a problem hiding this comment.
I will put this test back.
| set: | ||
| prometheus: | ||
| expose: EnableWithoutAuth | ||
| asserts: | ||
| - equal: | ||
| path: data.METRICS_ADDR | ||
| value: :8443 | ||
| - it: should include proxy sidecar if expose is with auth | ||
| - it: should cotain ip if expose is with auth |
There was a problem hiding this comment.
| - it: should cotain ip if expose is with auth | |
| - it: should contain ip if expose is with auth |
| @@ -38,7 +38,15 @@ func GetIsWebhookEnabled() bool { | |||
|
|
|||
| // GetBroadcasterBurstSize returns the customizable burst size for broadcaster. | |||
| func GetBroadcasterBurstSize() int { | |||
| burstSize := lookupIntEnvVar("BROADCASTER_BURST_SIZE", envCanNotExist) | |||
| envName := "BROADCASTER_BURST_SIZE" | |||
| burstSizeStr := lookupStringEnvVar(envName, envCanNotExist) | |||
There was a problem hiding this comment.
Any reason why this is now a string?
There was a problem hiding this comment.
This variable is an optional one. If the variable is not set, the old code cannot convert it to an integer so the operator will fail to start.
| name: verticadb-sample | ||
| fieldPaths: | ||
| - spec.sidecars.[name=vlogger].image | ||
| - select: | ||
| kind: VerticaDB | ||
| name: v-k-safety-0-scaling | ||
| fieldPaths: | ||
| - spec.sidecars.[name=vlogger].image | ||
| - select: | ||
| kind: VerticaDB | ||
| name: v-scale-up-and-down | ||
| fieldPaths: | ||
| - spec.sidecars.[name=vlogger].image | ||
| - select: | ||
| kind: VerticaDB | ||
| name: v-kill | ||
| fieldPaths: | ||
| - spec.sidecars.[name=vlogger].image | ||
| - select: | ||
| kind: VerticaDB | ||
| name: v-auto-restart-vertica | ||
| fieldPaths: | ||
| - spec.sidecars.[name=vlogger].image | ||
| - select: | ||
| kind: VerticaDB | ||
| name: v-client-proxy-upgrade | ||
| fieldPaths: | ||
| - spec.sidecars.[name=vlogger].image | ||
| - select: |
There was a problem hiding this comment.
What do we need this for?
There was a problem hiding this comment.
This is for the tests that need vlogger sidecar. The new operator-sdk is using new kustomize binary which is more strict than before. Without these changes, kustomization will fail.
This PR upgraded operator-sdk from 1.28.0 to 1.38.0. Refer to this page for the operator side changes: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.38.0/. Refer to this page for the controller-runtime API changes: https://github.com/kubernetes-sigs/controller-runtime/tree/v0.18.4.