Add options for internal cert management to component config #362

tenzen-y · 2022-08-29T20:04:51Z

Signed-off-by: tenzen-y yuki.iwai.tz@gmail.com

What type of PR is this?

/kind documentation
/kind feature
/kind api-change

What this PR does / why we need it:

I added options for internal cert management to component config and I have tested the operations in the below commands:

For default internal cert management options

kubectl apply -k github.com/tenzen-y/kueue/config/default?ref=add-cert-options-to-component-config-default-cert-opts

For customize namespace, serviceName and secretName

kubectl apply -k github.com/tenzen-y/kueue/config/default?ref=add-cert-options-to-component-config-customize-cert-opts

Also, I fixed Makefile and added a changelog.

Which issue(s) this PR fixes:

Fixes #270

Special notes for your reviewer:

k8s-ci-robot · 2022-08-29T20:04:58Z

Welcome @tenzen-y!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-08-29T20:04:59Z

Hi @tenzen-y. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tenzen-y · 2022-08-29T20:07:36Z

Makefile

@@ -118,11 +118,11 @@ vet: ## Run go vet against code.

 .PHONY: test
 test: generate fmt vet ## Run tests.
-	$(GO_CMD) test $(GO_TEST_FLAGS) ./pkg/... -coverprofile cover.out
+	$(GO_CMD) test $(GO_TEST_FLAGS) $(shell go list ./... | grep -v -e test -e apis) -coverprofile cover.out


Previously, main_test.go could not be tested.

Good catch.

would -e /test work?

Yes, It works fine.
Do you prefer to use go list ./... | grep -v -e '/test'?

tenzen-y · 2022-08-29T20:15:32Z

Makefile


 .PHONY: test-integration
 test-integration: manifests generate fmt vet envtest ginkgo ## Run tests.
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" \
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) --arch=amd64 use $(ENVTEST_K8S_VERSION) -p path)" \


We need to specify CPU architecture to run tests on Apple Silicon machines since envtest only provides k8s components for amd64 binaries.

apis/config/v1alpha2/configuration_types.go

tenzen-y · 2022-08-29T20:20:50Z

/assign @alculquicondor @kerthcet

alculquicondor · 2022-08-29T20:42:34Z

/ok-to-test

I'll let @kerthcet do the first pass

apis/config/v1alpha2/configuration_types.go

apis/config/v1alpha2/defaults.go

apis/config/v1alpha2/configuration_types.go

main.go

pkg/util/cert/cert.go

tenzen-y · 2022-08-30T18:50:02Z

@kerthcet I have addressed your comments. Please take another look.

tenzen-y · 2022-08-30T18:55:02Z

Also, it might be better to move setOptionDefaults(opt *ctrl.Options) to SetDefaults_Configuration(cfg *Configuration) same as InternalCertManagement in another PR.

What do you think?

kerthcet · 2022-08-31T02:37:35Z

Also, it might be better to move setOptionDefaults(opt *ctrl.Options) to SetDefaults_Configuration(cfg *Configuration) same as InternalCertManagement in another PR.

What do you think?

Actually, it was designed on purpose before, we want to apply defaults after loading the config file. The advantage I can think of is that we apply AndFrom above, it will also set defaults, but it's out of our control since it's an imported package, applying defaults after gives us more confident. But I'm open with this.

tenzen-y · 2022-08-31T09:33:11Z

Also, it might be better to move setOptionDefaults(opt *ctrl.Options) to SetDefaults_Configuration(cfg *Configuration) same as InternalCertManagement in another PR.
What do you think?

Actually, it was designed on purpose before, we want to apply defaults after loading the config file. The advantage I can think of is that we apply AndFrom above, it will also set defaults, but it's out of our control since it's an imported package, applying defaults after gives us more confident. But I'm open with this.

Thanks for clarifying! @kerthcet

It seems strange that the different how to set defaults (use the SetDefaults function or apply defaults after loading config) for both InternalCertManagement and other parameters. So, it might be better to do the same logic.

In my opinion, it might be better to set all default parameters to Configuration using the SetDefaults function.

What do you think about the current logic that uses different logic for InternalCertManagement and other parameters?

kerthcet · 2022-08-31T07:15:18Z

apis/config/v1alpha2/configuration_types.go

@@ -27,6 +27,10 @@ import (
 type Configuration struct {
 	metav1.TypeMeta `json:",inline"`

+	// Namespace is the namespace in which kueue is deployed and is also used as part of DNSName.
+	// Defaults to kueue-system.
+	Namespace string `json:"namespace,omitempty"`


Since this is optional, maybe *string will be better.

Besides, have you tried this, I have no time to test it up to now.

Since this is optional, maybe *string will be better.

SGTM.

Besides, have you tried this, I have no time to test it up to now.

Yes, I have tested using the below command:

kubectl apply -k github.com/tenzen-y/kueue/config/default?ref=add-cert-options-to-component-config-customize-cert-opts

The above manifests are customized namespace, serviceName, and secretName.

Detail of Configuration: https://github.com/tenzen-y/kueue/blob/add-cert-options-to-component-config-customize-cert-opts/config/components/manager/controller_manager_config.yaml

*string is necessary when the empty string is a valid value. But this is not the case for these fields. Empty means not-set, and we can set the default.

apis/config/v1alpha2/configuration_types.go

apis/config/v1alpha2/defaults_test.go

docs/setup/install.md

apis/config/v1alpha2/defaults_test.go

Makefile

main_test.go

kerthcet · 2022-08-31T10:36:20Z

It seems strange that the different how to set defaults (use the SetDefaults function or apply defaults after loading config) for both InternalCertManagement and other parameters. So, it might be better to do the same logic.

In my opinion, it might be better to set all default parameters to Configuration using the SetDefaults function.

What do you think about the current logic that uses different logic for InternalCertManagement and other parameters?

I'm ok with this if we can add enough tests to improve our confidence. But we can leave it a follow up as you said.

tenzen-y · 2022-08-31T15:29:54Z

I'm ok with this if we can add enough tests to improve our confidence. But we can leave it a follow up as you said.

I agree with you.
I will not work on it in this PR.

apis/kueue/webhooks/workload_webhook_test.go

pkg/util/pointer/pointer.go

apis/config/v1alpha2/defaults_test.go

docs/setup/install.md

CHANGELOG/CHANGELOG-0.3.md

Makefile

apis/config/v1alpha2/configuration_types.go

apis/config/v1alpha2/defaults.go

docs/setup/install.md

main.go

main_test.go

alculquicondor · 2022-09-01T14:10:12Z

apis/config/v1alpha2/configuration_types.go

@@ -27,6 +27,10 @@ import (
 type Configuration struct {
 	metav1.TypeMeta `json:",inline"`

+	// Namespace is the namespace in which kueue is deployed and is also used as part of DNSName.
+	// Defaults to kueue-system.
+	Namespace *string `json:"namespace,omitempty"`


I would put it inside the internalCertManagement struct

Forget to comment here, at the very beginning I was tricked by the annotation, actually we should name this serviceNamespace. What's more, I think just modify the api is not enough, I'm making some experiments right now, I think we should also update the kustomize configuration.

Forget to comment here, at the very beginning I was cheated by the annotation,

I don't mind, thanks for your detailed review! @kerthcet

actually we should name this serviceNamespace.

That makes sense. I will rename Namespace to ServiceNamespace and move to struct InternalCertManagement.

What's more, I think just modify the api is not enough, I'm making some experiments right now, I think we should also update the kustomize configuration.

Which kustomize configurations should we modify? namePrefix? @kerthcet

Maybe you can try with a different service rather than webhook-service, and apply an unqualified object. Then you will get an error, maybe like failed to call webhook: Post "https://webhook-service.kueue-system.svc:443/mutate-kueue-x-k8s-io-v1alpha2-resourceflavor?timeout=10s": service "webhook-service" not found.

Because we still need to update the manifests.yaml. IIUC.

This conversation has derailed in ways I don't understand :)

I don't think it's a primary use case to change the namespace name or service name, so I think we can leave it out of the documentation. IMO, it suffices to leave the hooks in the configuration to let users change the parameters without having to rebuild the image, if they really need to.

I don't think it's a primary use case to change the namespace name or service name, so I think we can leave it out of the documentation. IMO, it suffices to leave the hooks in the configuration to let users change the parameters without having to rebuild the image, if they really need to.

@alculquicondor That makes sense.
I will move these configurations to comments.

The initial conclusion of this thread was to move Namespace into InternalCertManagement as WebhookServiceNamespace

@alculquicondor
As described in this comment by @kerthcet, I and @kerthcet updated the conclusion that we should include the namespace in struct Configuration because it represents the namespace where kueue components are deployed.

Actually the namespace helps to control where to deploy the kueue components, besides configure this field, we should also update the config/default/kustomization.yaml, to change the value of namespace.

So I think we can leave it outside the internalCertManagement

tenzen-y · 2022-09-03T15:10:24Z

Thank you very much for your detailed review many times! @kerthcet

tenzen-y · 2022-09-07T08:51:36Z

@alculquicondor I have addressed your suggestions. PTAL

apis/config/v1alpha2/configuration_types.go

config/default/kustomization.yaml

docs/setup/install.md

main_test.go

alculquicondor · 2022-09-07T18:05:57Z

apis/config/v1alpha2/configuration_types.go

@@ -27,6 +27,10 @@ import (
 type Configuration struct {
 	metav1.TypeMeta `json:",inline"`

+	// Namespace is the namespace in which kueue is deployed. It is used as part of DNSName of the webhook Service.
+	// Defaults to kueue-system.
+	Namespace *string `json:"namespace,omitempty"`


I think we can avoid the pointers.

If the field is empty len(field)==0, you can set the default.

As described in this by @kerthcet, I changed it to *string since this field is optional. Should I change it to string?

Since this is optional, maybe *string will be better.

Still think pointer is a better choice since this is optional, if we set to string, len(field) == 0 means both unset or misconfiguration of empty string. We can't tell the difference.

Do we have any obviously disadvantages by using pointer?

@alculquicondor

empty string is not a valid value, so we can override it during defaulting. Using pointer just adds extra code that we can avoid. But I'm ok with it, if you feel strongly about it.

I agree with @kerthcet. So, I'd like to use *string.
Do you have any concerns? @kerthcet

sg, please squash.

I have squashed.

tenzen-y · 2022-09-07T19:47:16Z

@kerthcet What is your opinion about #362 (comment)?

*string is necessary when the empty string is a valid value. But this is not the case for these fields. Empty means not-set, and we can set the default.

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>

tenzen-y · 2022-09-08T20:35:05Z

Probably, we need to merge #379 first.

tenzen-y · 2022-09-08T21:54:14Z

/test pull-kueue-build-image-main

tenzen-y · 2022-09-08T21:58:06Z

Probably, this PR is ready to merge.

tenzen-y · 2022-09-09T06:07:02Z

Once this PR is merged, I will work on this.

Also, it might be better to move setOptionDefaults(opt *ctrl.Options) to SetDefaults_Configuration(cfg *Configuration) same as InternalCertManagement in another PR.

alculquicondor · 2022-09-09T14:01:44Z

And please squash :)

tenzen-y · 2022-09-09T14:07:02Z

And please squash :)

I have already squashed.

https://github.com/kubernetes-sigs/kueue/pull/362/commits

tenzen-y · 2022-09-09T14:09:49Z

You mean git sqush, right?

alculquicondor

Awesome

/lgtm
/approve

Makefile

k8s-ci-robot · 2022-09-09T14:42:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tenzen-y · 2022-09-09T14:50:37Z

@alculquicondor
Hmm..? This PR seems to be merged before I apply your suggestion.

I will create another PR to apply your suggestion.

alculquicondor · 2022-09-09T14:54:08Z

oh yeah... I should have checked

kerthcet · 2022-09-09T15:19:04Z

Thanks @tenzen-y

tenzen-y · 2022-09-09T15:22:29Z

Thanks @tenzen-y

@kerthcet Thanks for your review!

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 29, 2022

k8s-ci-robot requested review from ahg-g and ArangoGutierrez August 29, 2022 20:05

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 29, 2022

tenzen-y commented Aug 29, 2022

View reviewed changes

apis/config/v1alpha2/configuration_types.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned alculquicondor and kerthcet Aug 29, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 29, 2022

kerthcet reviewed Aug 30, 2022

View reviewed changes

kerthcet reviewed Aug 31, 2022

View reviewed changes

tenzen-y commented Aug 31, 2022

View reviewed changes

apis/kueue/webhooks/workload_webhook_test.go Show resolved Hide resolved

kerthcet reviewed Sep 1, 2022

View reviewed changes

pkg/util/pointer/pointer.go Show resolved Hide resolved

apis/config/v1alpha2/defaults_test.go Outdated Show resolved Hide resolved

apis/config/v1alpha2/defaults_test.go Outdated Show resolved Hide resolved

docs/setup/install.md Outdated Show resolved Hide resolved

alculquicondor reviewed Sep 1, 2022

View reviewed changes

alculquicondor reviewed Sep 7, 2022

View reviewed changes

apis/config/v1alpha2/configuration_types.go Outdated Show resolved Hide resolved

alculquicondor reviewed Sep 7, 2022

View reviewed changes

config/default/kustomization.yaml Outdated Show resolved Hide resolved

alculquicondor reviewed Sep 7, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 7, 2022

Add options for internal cert management to component config

33e0459

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com> Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>

tenzen-y force-pushed the add-cert-options-to-component-config branch from db32a3b to 33e0459 Compare September 8, 2022 20:19

tenzen-y mentioned this pull request Sep 8, 2022

Add Admitted Workloads Field to LocalQueueStatus #382

Merged

alculquicondor reviewed Sep 9, 2022

View reviewed changes

Makefile Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2022

k8s-ci-robot merged commit 81ccfdf into kubernetes-sigs:main Sep 9, 2022

tenzen-y deleted the add-cert-options-to-component-config branch September 9, 2022 14:50

tenzen-y mentioned this pull request Sep 9, 2022

Fix make target for unit tests #383

Merged

tenzen-y mentioned this pull request Sep 16, 2022

Consolidate defaulting for ControllerManagerConfigurationSpec #399

Merged

Add options for internal cert management to component config #362

Add options for internal cert management to component config #362

Conversation

tenzen-y commented Aug 29, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

k8s-ci-robot commented Aug 29, 2022

k8s-ci-robot commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Aug 29, 2022

alculquicondor commented Aug 29, 2022

tenzen-y commented Aug 30, 2022

tenzen-y commented Aug 30, 2022 • edited Loading

kerthcet commented Aug 31, 2022 • edited Loading

tenzen-y commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Aug 31, 2022

tenzen-y commented Aug 31, 2022

Choose a reason for hiding this comment

kerthcet Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet Sep 1, 2022 • 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

tenzen-y Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

tenzen-y commented Sep 3, 2022

tenzen-y commented Sep 7, 2022

Choose a reason for hiding this comment

tenzen-y Sep 7, 2022 • 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

tenzen-y commented Sep 7, 2022

tenzen-y commented Sep 8, 2022

tenzen-y commented Sep 8, 2022

tenzen-y commented Sep 8, 2022

tenzen-y commented Sep 9, 2022

alculquicondor commented Sep 9, 2022

tenzen-y commented Sep 9, 2022

tenzen-y commented Sep 9, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 9, 2022

tenzen-y commented Sep 9, 2022

alculquicondor commented Sep 9, 2022

kerthcet commented Sep 9, 2022

tenzen-y commented Sep 9, 2022

tenzen-y commented Aug 29, 2022 •

edited

Loading

tenzen-y Sep 2, 2022 •

edited

Loading

tenzen-y commented Aug 30, 2022 •

edited

Loading

kerthcet commented Aug 31, 2022 •

edited

Loading

tenzen-y Aug 31, 2022 •

edited

Loading

kerthcet Sep 1, 2022 •

edited

Loading

kerthcet Sep 1, 2022 •

edited

Loading

tenzen-y Sep 7, 2022 •

edited

Loading

tenzen-y Sep 7, 2022 •

edited

Loading