feat: Expose /token endpoint as API extension #22

akrejcir · 2023-07-25T15:36:29Z

This PR is still a work in progress

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes part of #20

Jira: https://issues.redhat.com/browse/CNV-31169

Release note:

The /token endpoint is exposed as a kubernetes API extension

openshift-ci · 2023-07-25T15:36:33Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akrejcir · 2023-08-01T14:54:27Z

This is ready for review now.

/cc @0xFelix @ksimon1 @lyarwood

0xFelix

Added one comment for now, I will review the rest tomorrow.

0xFelix · 2023-08-01T16:25:52Z

manifests/api_service.yaml

+metadata:
+  name: v1alpha1.token.kubevirt.io
+  annotations:
+    service.beta.openshift.io/inject-cabundle: "true"


Will it break deployments on plain Kubernetes?

Currently the proxy does not work on plain k8s, because of this annotation and a similar one in serivce.yaml.
They make the implementation simpler. There is already an open issue #16 to do this certificate generation manually and to not depend on openshift.

FWIW I'm cool with this being fixed in the future once this API moves to beta etc.

lyarwood

I still need to play around with this locally but overall it's looking good. I've just left a few supernits and a question about the resource naming for now.

pkg/console/auth-config/auth-config.go

pkg/console/console.go

lyarwood · 2023-08-02T09:08:20Z

manifests/api_service.yaml

+metadata:
+  name: v1alpha1.token.kubevirt.io
+  annotations:
+    service.beta.openshift.io/inject-cabundle: "true"


FWIW I'm cool with this being fixed in the future once this API moves to beta etc.

pkg/console/auth-config/auth-config.go

pkg/console/service/service.go

pkg/console/auth-config/auth-config.go

tests/tests_suite_test.go

pkg/console/service/service.go

pkg/console/auth-config/auth-config.go

pkg/console/auth-config/auth-config_test.go

0xFelix · 2023-08-03T08:26:36Z

pkg/console/auth-config/auth-config_test.go

+			_, err := authReader.GetGroupHeaders()
+			return err
+		}, time.Second, 100*time.Millisecond).
+			Should(MatchError(ContainSubstring(groupHeadersKey + " not found in configmap")))


Is this valid use of MatchError and ContainSubstring?

Yes, it's valid. We use it in SSP for example:
https://github.com/kubevirt/ssp-operator/blob/3470569dbecbe6fee98b45b23a071d85c2cfae55/tests/webhook_test.go#L156

And here is the relevant part of the implementation of MatchErrorMatcher.Match():

vm-console-proxy/vendor/github.com/onsi/gomega/matchers/match_error_matcher.go

Lines 35 to 42 in d139fec

var subMatcher omegaMatcher

var hasSubMatcher bool

if expected != nil {

subMatcher, hasSubMatcher = (expected).(omegaMatcher)

if hasSubMatcher {

return subMatcher.Match(actualErr.Error())

}

}

It allows passing nested matchers.

0xFelix · 2023-08-03T08:28:03Z

pkg/console/auth-config/auth-config_test.go

+}
+
+// testCa is s self-signed certificate with that expires at 2033-07-23
+const testCa = `-----BEGIN CERTIFICATE-----


Put this cnst at top of file?

No, I think this constant is a non-important detail. Having it on the top would only make readers scroll down.

pkg/console/service/service.go

0xFelix · 2023-08-03T08:32:11Z

pkg/console/service/service_test.go

+		testService.TokenHandler(request, response)
+
+		Expect(recorder.Code).To(Equal(http.StatusUnauthorized))
+		Expect(recorder.Body.String()).To(ContainSubstring("does not have permission to access virtualmachineinstances/vnc endpoint"))


Does it still return this error message? I thought we removed all err messages when returing 401?

Yes, the PR to remove the message was not merged yet: #23

pkg/console/tlsconfig/tlsconfig_test.go

0xFelix · 2023-08-03T08:34:27Z

pkg/console/tlsconfig/tlsconfig_test.go

@@ -266,3 +308,25 @@ func TestTlsConfig(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "TLS Config Suite")
 }
+
+// testCa is s self-signed certificate with that expires at 2033-07-23
+const testCa = `-----BEGIN CERTIFICATE-----


Put this on top of the file?

No, the same reason as in the other file.

tests/proxy_test.go

0xFelix

Some nits but in general I approve.

/approve

manifests/api_service.yaml

manifests/role_binding.yaml

manifests/transformer_namespace.yaml

pkg/console/authConfig/fake/fake.go

pkg/console/console.go

kubevirt-bot · 2023-08-03T14:39:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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~~ [0xFelix]

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

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

Some previous commit did not update go.mod Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

In tests/test_suite_test.go, the package "golang.org/x/net/context" was imported by mistake. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

This change uses ApiService resource to register the vm-console-proxy service as an API extension. The connection from API server to the proxy uses TLS with certificate authentication on both sides. To check the certificate from the API server, the proxy uses a CA loaded from the ConfigMap "extension-apiserver-authentication" in the "kube-system" namespace. More information: https://kubernetes.io/docs/tasks/extend-kubernetes/setup-extension-api-server/ Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

lyarwood

/lgtm

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 25, 2023

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jul 25, 2023

kubevirt-bot requested review from codingben and opokornyy July 25, 2023 15:36

kubevirt-bot added the size/XL label Jul 25, 2023

akrejcir added this to the v0.3.0 milestone Jul 25, 2023

akrejcir force-pushed the api-extension branch from 4e00aee to 59258e2 Compare July 26, 2023 15:30

kubevirt-bot added size/XXL and removed size/XL labels Jul 26, 2023

akrejcir force-pushed the api-extension branch 2 times, most recently from 5a09cb8 to 0e96b2d Compare August 1, 2023 14:53

akrejcir marked this pull request as ready for review August 1, 2023 14:53

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2023

kubevirt-bot requested a review from lyarwood August 1, 2023 14:54

kubevirt-bot requested review from 0xFelix and ksimon1 August 1, 2023 14:54

0xFelix reviewed Aug 1, 2023

View reviewed changes

lyarwood reviewed Aug 2, 2023

View reviewed changes

0xFelix reviewed Aug 2, 2023

View reviewed changes

akrejcir force-pushed the api-extension branch from 0e96b2d to aaeebf1 Compare August 2, 2023 14:46

0xFelix reviewed Aug 3, 2023

View reviewed changes

akrejcir force-pushed the api-extension branch from aaeebf1 to 464e280 Compare August 3, 2023 12:40

0xFelix reviewed Aug 3, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2023

akrejcir force-pushed the api-extension branch from 464e280 to 98e8ccd Compare August 3, 2023 14:44

chore: Use constructor function in unit test

45c3c71

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir added 3 commits August 3, 2023 17:51

chore: run go mod vendor

bf8843f

Some previous commit did not update go.mod Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

fix: Import correct "context" package

1d07ce6

In tests/test_suite_test.go, the package "golang.org/x/net/context" was imported by mistake. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir force-pushed the api-extension branch from 98e8ccd to 939070d Compare August 3, 2023 15:56

lyarwood approved these changes Aug 4, 2023

View reviewed changes

kubevirt-bot assigned lyarwood Aug 4, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2023

kubevirt-bot merged commit da56dd7 into kubevirt:main Aug 4, 2023

akrejcir deleted the api-extension branch August 4, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Expose /token endpoint as API extension #22

feat: Expose /token endpoint as API extension #22

akrejcir commented Jul 25, 2023 •

edited

Loading

openshift-ci bot commented Jul 25, 2023

akrejcir commented Aug 1, 2023

0xFelix left a comment

0xFelix Aug 1, 2023

akrejcir Aug 2, 2023

lyarwood Aug 2, 2023

lyarwood left a comment

lyarwood Aug 2, 2023

0xFelix Aug 3, 2023

akrejcir Aug 3, 2023 •

edited

Loading

0xFelix Aug 3, 2023

akrejcir Aug 3, 2023

0xFelix Aug 3, 2023

akrejcir Aug 3, 2023

0xFelix Aug 3, 2023

akrejcir Aug 3, 2023

0xFelix left a comment

kubevirt-bot commented Aug 3, 2023

lyarwood left a comment

	var subMatcher omegaMatcher
	var hasSubMatcher bool
	if expected != nil {
	subMatcher, hasSubMatcher = (expected).(omegaMatcher)
	if hasSubMatcher {
	return subMatcher.Match(actualErr.Error())
	}
	}

feat: Expose /token endpoint as API extension #22

feat: Expose /token endpoint as API extension #22

Conversation

akrejcir commented Jul 25, 2023 • edited Loading

openshift-ci bot commented Jul 25, 2023

akrejcir commented Aug 1, 2023

0xFelix left a 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

lyarwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir Aug 3, 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

0xFelix left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 3, 2023

lyarwood left a comment

Choose a reason for hiding this comment

akrejcir commented Jul 25, 2023 •

edited

Loading

akrejcir Aug 3, 2023 •

edited

Loading