Custom improvements for OpenShift by matejvasek · Pull Request #825 · knative/func

matejvasek · 2022-02-09T18:14:24Z

🎁 add initial framework for cluster flavor detection/actions

Detects when deploying to OpenShift and use internal registry

matejvasek · 2022-02-09T18:14:53Z

@salaboy @shinigambit would you be OK with this?

matejvasek · 2022-02-09T18:17:12Z

Just draft for now.

codecov · 2022-02-09T18:17:41Z

Codecov Report

Merging #825 (04354ae) into main (64091fb) will decrease coverage by 0.48%.
The diff coverage is 20.58%.

@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
- Coverage   43.36%   42.88%   -0.49%     
==========================================
  Files          48       49       +1     
  Lines        4690     4783      +93     
==========================================
+ Hits         2034     2051      +17     
- Misses       2367     2445      +78     
+ Partials      289      287       -2

Impacted Files	Coverage Δ
http/transport.go	`63.15% <0.00%> (ø)`
openshift/openshift.go	`6.41% <6.41%> (ø)`
cmd/client.go	`16.39% <58.82%> (+16.39%)`	⬆️
cmd/build.go	`59.63% <100.00%> (ø)`
cmd/deploy.go	`16.41% <100.00%> (ø)`
cmd/root.go	`69.44% <100.00%> (+1.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64091fb...04354ae. Read the comment docs.

matejvasek · 2022-02-09T18:17:50Z

@lance @lkingland with this we could just patch tags in Makefile in midstream.

evankanderson · 2022-02-10T20:09:53Z

Would it make sense to contribute this to https://github.com/google/go-containerregistry/tree/main/pkg/authn, and leverage that library? That library seems to be used by a lot of components, including the pack CLI.

evankanderson · 2022-02-10T20:13:51Z

In particular, there's a credential helper protocol that would allow you to specify an external binary helper and authorization config for image-registry.openshift-image-registry.svc like so:

{
	"credHelpers": {
		"image-registry.openshift-image-registry.svc": "os",
	}
}

This would also work with other commands like crane, dive and docker, assuming you had docker-credential-os in your path.

matejvasek · 2022-02-10T20:37:50Z

@evankanderson I am not sure what you mean specifically. We are using go-containerregistry and credsHelpers internally for credentials handling.

Here I am just adding very specialized loader func GetDockerCredentialLoaders() []creds.CredentialsCallback {} reason:
Internal OpenShift registry has the same credentials as the k8s. So this specialized call pulls credentials from ~/.kube/config instead form ~/.config/func/auth.json.

evankanderson · 2022-02-10T20:41:16Z

I'm suggesting: why not have a docker-credential-openshift or docker-credential-k8s that extracts those credentials, and can use the standard credsHelper mechanism?

matejvasek · 2022-02-10T20:41:57Z

This would also work with other commands like crane, dive and docker, assuming you had docker-credential-os in your path.

We are deliberately using ~/.docker/config.json as read-only, so if somebody is already logged into registry via docker CLI, the func should use that. Opposite is not true: if you are logged via func the docker CLI won't see the credentials.
EDIT: this is not entirely true: while func has private auth file, it still can be "connected" to docker via helper (e.g. kernel keychaing).

matejvasek · 2022-02-10T20:46:09Z

I'm suggesting: why not have a docker-credential-openshift or docker-credential-k8s that extracts those credentials, and can use the standard credsHelper mechanism?

I see. Interesting idea! I like it. However this would require user to download and put such a helper onto PATH. I wish everything worked automagically here: If cluster is OpenShift then internal registry is used by default without needing any additional steps.
Ideally user shouldn't even notice there is something like image registry.

openshift/openshift.go

cmd/client_openshift.go

matejvasek · 2022-02-11T17:40:04Z

@lance @lkingland @zroubalik If I understand it correctly @salaboy and @evankanderson suggest not to do this by direct integration in binary.

Instead this behaviour could be customized trough some local files.

What would it mean for us (RH):
We would need to create some (post)installation script that would create such a files.

Such a script might need to:

install custom credential helper binary, and put it into path via append to shell env/rc files
predefine credentials in ~/.config/func/auth.json
pull the OSh service CA and save it on FS
create some configuration file that would say the OSh service CA is trusted for some specific hosts
set export FUNC_REGISTRY envvar via append to shell env/rc files

Also there probably would be different installation scripts for each OS.

evankanderson · 2022-02-11T19:13:07Z

I'm not sure what tools users are already using; I see that there's already an oc CLI that suggests installing a binary and moving in onto your PATH. The same will need to be true for the func CLI -- at some point, you might want to have a script that copies things into PATH, but having oc and docker-credential-oc seems like it would enable using the full OCI ecosystem with OpenShift.

I'm not blocking this submit, just trying to suggest ways to make the overall OpenShift experience align with the rest of the ecosystem, which I think helps us all.

evankanderson · 2022-02-11T19:13:56Z

Depending on the space in the oc binary, you could follow the gcloud example, and have a command to update the dockerconfig.

lance · 2022-02-11T19:49:11Z

@lance @lkingland @zroubalik If I understand it correctly @salaboy and @evankanderson suggest not to do this by direct integration in binary.

Why not do this in openshift-knative/kn-plugin-func?

matejvasek · 2022-02-11T19:52:42Z

@lance @lkingland @zroubalik If I understand it correctly @salaboy and @evankanderson suggest not to do this by direct integration in binary.

Why not do this in openshift-knative/kn-plugin-func?

yes we can do it by git am patch

matejvasek · 2022-02-11T20:15:31Z

@lance @lkingland @zroubalik If I understand it correctly @salaboy and @evankanderson suggest not to do this by direct integration in binary.

Why not do this in openshift-knative/kn-plugin-func?

yes we can do it by git am patch

But I strongly prefer not to have custom patches in openshift-knative/kn-plugin-func, in long term it will complicate maintenance.

lance · 2022-02-11T20:21:53Z

But I strongly prefer not to have custom patches in openshift-knative/kn-plugin-func, in long term it will complicate maintenance.

I agree with you there. This is the more desirable way for RHT, to be sure. Would other vendor offered clusters benefit from something similar?

salaboy · 2022-02-12T08:34:22Z

@lance I think this will happen for sure.. other vendors will need their own specific hooks and checks, so I will tend to agree with @evankanderson in the sense of let's make sure that we make func and other libraries better so we can support multiple Kubernetes flavours.

I wouldn't like it either if you need to do patches to create your specific binaries, so my suggestion would be to try to create the mechanisms to check if you are running on Openshift as generic as possible.

All these check can conform a profile, for example if check A, B and C are true, you are in Openshift, if only check A and C are true, you are in another vendor. If we follow the approach implemented in this PR, then each vendor will need to have it's own set of checks and we will not be promoting people to reuse those mechanisms. Just to be clear, these checks can definitely be in the binary, or contributed to the downstream libraries as @evankanderson mentioned.

lkingland · 2022-02-12T12:14:07Z

This is a valid, and useful question. I believe the current core library structure properly handles the situation using a fairly standard mechanism: The core func library is essentially dependency-free and agnostic of cluster flavor. Third-party-specifics are held behind interfaces. main is responsible for introspecting the environment, either automatically or explicitly by the user, to load the core library (logic) with the correct implementation. For example, there may be a different "deployer" if someone is deploying to a local Kind cluster, an EC2 cluster, an OpenShift cluster etc. These nuanced implementations reside in packages kind, openshift, ec2, etc. Therefore, I do have a suspicion we could be more flexible here than use build flags, allowing a single binary to be able to use any properly configured cluster regardless of its flavor

lkingland · 2022-02-12T12:26:28Z

cmd/client_vanilla.go

+//go:build !openshift
+// +build !openshift


I am happy to see that this logic is in the right location: cmd with a reference to an implementation-specific package. This is the same as can be used for all future cases where flavor-specific code is necessary, not just for OpenShift.

My concern with this approach is that it is calculated at build-time and thus results in a binary that is OpenShift specific. This is not, I think, a necessary capitulation, because the core func logic is agnostic if cluster flavor.

Is there not a way we can detect the cluster flavor at runtime?

+1 here.. I would be more in favour of runtime detection with "standard" mechanisms, that we can reuse for other environments.

evankanderson · 2022-02-12T13:11:54Z

VMware actually has a slightly more complicated detection logic, because "Tanzu Application Platform" is a layer which can be applied to different types of clusters, including Tanzu Kubernetes Grid (VMware), EKS (Amazon), or even OpenShift on AWS (turtles all the way down).

Should this check be conditioned on the cluster, the registry, or some combination?

We might be able to use the auth preflight check with the Server header to give the different auth plugins a chance to update the client with auth credentials.

lkingland · 2022-02-14T06:17:31Z

Should this check be conditioned on the cluster, the registry, or some combination?
...
We might be able to use the auth preflight check with the Server header

If we could use that same request to find out if it is Tanzu Application Platform, OpenShift, or vanilla; that seems like it could work well. The analogy perhaps being this information is a bit like operating system, where Function is to Kubernetes Flavor as Binary is to Operating System.

Am I understanding this correctly? By default the "Deployer" used by func is a vanilla Kubernetes deployer which requires that a registry be provided and that Knative is active in the cluster. During the authentication phaze we detect that the Cluster is a... customized distribution, and if there exists an integration, a distribution-specific Deployer is loaded. In the case of OpenShift, this Deployer utilizes the internal registry by default, treating a user-provided registry not as required, but as an optional override.

Using the Server header in the auth preflight is runtime, and done for every cluster connection of course, so it would accomplish the goal of allowing func to swap out any of the concrete implementations used by Client as necessary to take advantage of flavor-specific advantages. It also accomplishes the goal of having a single func binary capable of interacting with any cluster which meets the minimum requirements (Knative installed), but with flavor-specific benefits.

Sounds like a good topic for this week's call.

lkingland · 2022-02-17T04:12:13Z

cmd/client.go

+	var additionalCredLoaders []creds.CredentialsCallback
+
+	switch {
+	case openshift.IsOpenShift():


Just a comment:
I can see a future where we have a set of supported platforms, which are each given the opportunity to self-identify and add to the features offered by the Function Client. This would work a bit like a connection upgrade attempt. This keeps the core library clean of any third-party dependencies, but allows for our Functions library to offer the ability to seamelssly switch between cluster flavors.

lkingland

Looking good!
I am sure the exact way that we implement support for various Kubernetes flavors will evolve over time. For now, it is nice to see that a single func will work seamlessly whether it is connected to a vanilla K8S cluster provider or an OpenShift cluster provider; transparently making use of any upgrades/preconfigurations provided by a supported provider.

knative-prow-robot · 2022-02-17T04:14:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek, zroubalik

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [lkingland,matejvasek,zroubalik]

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

salaboy · 2022-02-18T08:26:34Z

This looks like a good first iteration. I do feel that we haven't answered @evankanderson question yet: Should this check be conditioned on the cluster, the registry, or some combination? Right now these checks are performed by default, am I right?

matejvasek · 2022-02-21T16:31:39Z

Should this check be conditioned on the cluster, the registry, or some combination?

We are conditioning on cluster type (by checking that the cluster contains something that is own to OpenShift).

matejvasek · 2022-02-21T16:35:10Z

@lance @lkingland @salaboy @evankanderson are we OK with merging this into main?

lance

This in general looks good to me, @matejvasek and I am OK merging it. However, since I have not actively been engaged in the dialog I'd like at least @salaboy and @lkingland or @zroubalik to also give the OK. So, I'm not removing hold just yet.

/lgtm

lkingland · 2022-02-21T22:44:45Z

@lance @lkingland @salaboy @evankanderson are we OK with merging this into main?

Yes @matejvasek I think this is fine to merge.

evankanderson

A few code review comments.

I'd like to see somewhat better coverage of the new package, but I understand that we'd like to bake this in rather than fooling around with credential helpers.

openshift/openshift.go

evankanderson · 2022-02-22T01:46:13Z

openshift/openshift.go

+	cfgMapName := "service-ca-config-" + rand.String(5)
+
+	cfgMap := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        cfgMapName,
+			Annotations: map[string]string{"service.beta.openshift.io/inject-cabundle": "true"},
+		},


Why not use generateName here?

This also feels like maybe this function should have a comment explaining what it does and why this is a safe way to get the CA cert.

(I think you're avoiding generateName because you want to simplify deleting this shortly after creation)

I am using locally generated name so I can set up Watch before creation. Is that all right @evankanderson ?

evankanderson · 2022-02-22T01:50:05Z

openshift/openshift.go

+		return nil
+	}
+
+	var selectCA func(ctx context.Context, serverName string) (*x509.Certificate, error)


This seems like it's finding the specific cert, not necessarily the CA for the cert (i.e. if you have a root CA that signs the registry cert, this will find the registry cert, not the root CA). That's okay, but we might prefer to call this registryCert.

It's CA for multiple services provided by OpenShift, but here for now are are enabling trust only for registry. I don't feel confident enabling the CA for any and all connections.

https://docs.openshift.com/container-platform/4.6/security/certificate_types_descriptions/service-ca-certificates.html

@evankanderson it should find self-signed CA -- authority not just certificate.

openshift/openshift.go

evankanderson · 2022-02-22T01:58:55Z

openshift/openshift.go

+			isOpenShift = false
+			return
+		}
+		_, err = client.CoreV1().Services("openshift-image-registry").Get(context.TODO(), "image-registry", metav1.GetOptions{})


Hmm -- this will return false if the user doesn't have services get permission on the openshift-image-registry namespace.

I'm not sure if there's another / better way to detect this, either from server headers or by probing the registry and looking at its header / challenge. I think go-containerregistry uses a GET /v2/ HTTP /1.1 probe of the registry to figure out what and whether auth is required.

https://github.com/google/go-containerregistry/tree/main/pkg/authn#the-registry suggests looking at the Www-Authenticate header for clues as to the authentication mechanism.

I would avoid doing HTTP request to internal service here -- it requires some setup I don't want to do here.

Hmm -- this will return false if the user doesn't have services get permission on the openshift-image-registry namespace.

@evankanderson This does work with non-privileged user. Non-privilieged user will get different errors for non-existent service and for service that is not just permitted (NotFound vs. PermissionDenied).

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2022-02-22T14:23:15Z

I'd like to see somewhat better coverage of the new package

Unfortunately good integration test here require OpenShift cluster that in not running in GH action.
So some of the code will be covered in our CI.

matejvasek · 2022-02-22T14:53:58Z

@lance @lkingland please lgtm

Signed-off-by: Matej Vasek <mvasek@redhat.com>

lance · 2022-02-22T17:45:15Z

/lgtm

matejvasek · 2022-02-22T18:16:25Z

/unhold

knative-prow-robot added do-not-merge/work-in-progress 🤖 PR should not merge because it is a work in progress. approved 🤖 PR has been approved by an approver from all required OWNERS files. size/L 🤖 PR changes 100-499 lines, ignoring generated files. labels Feb 9, 2022

matejvasek requested a review from lkingland February 9, 2022 18:17

matejvasek requested a review from lance February 9, 2022 18:17

salaboy reviewed Feb 11, 2022

View reviewed changes

openshift/openshift.go Show resolved Hide resolved

salaboy reviewed Feb 11, 2022

View reviewed changes

openshift/openshift.go Outdated Show resolved Hide resolved

salaboy reviewed Feb 11, 2022

View reviewed changes

cmd/client_openshift.go Outdated Show resolved Hide resolved

lkingland reviewed Feb 12, 2022

View reviewed changes

knative-prow-robot removed the do-not-merge/work-in-progress 🤖 PR should not merge because it is a work in progress. label Feb 16, 2022

matejvasek requested review from lkingland and salaboy February 16, 2022 18:38

lkingland reviewed Feb 17, 2022

View reviewed changes

lkingland approved these changes Feb 17, 2022

View reviewed changes

knative-prow-robot assigned lkingland Feb 17, 2022

lkingland mentioned this pull request Feb 21, 2022

feat: builder flag with s2i option #842

Closed

lance reviewed Feb 21, 2022

View reviewed changes

knative-prow-robot assigned lance Feb 21, 2022

evankanderson reviewed Feb 22, 2022

View reviewed changes

Custom improvements for OpenShift

5b15436

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the openshirt-specifing-init branch from eea9ac0 to 5b15436 Compare February 22, 2022 13:57

knative-prow-robot removed the lgtm 🤖 PR is ready to be merged. label Feb 22, 2022

matejvasek added 2 commits February 22, 2022 15:03

fixup: osh cred loader

b5e86e9

Signed-off-by: Matej Vasek <mvasek@redhat.com>

fixup: style

6521c40

Signed-off-by: Matej Vasek <mvasek@redhat.com>

fixup: dns-err detec for fallback in cluster dial

04354ae

Signed-off-by: Matej Vasek <mvasek@redhat.com>

knative-prow-robot added the lgtm 🤖 PR is ready to be merged. label Feb 22, 2022

knative-prow-robot removed the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Feb 22, 2022

knative-prow-robot merged commit 4fec4af into knative:main Feb 22, 2022

This was referenced Feb 28, 2022

Unable to run tests #866

Closed

Tests failing locally #867

Closed

Conversation

matejvasek commented Feb 9, 2022 • edited by lance Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matejvasek commented Feb 9, 2022

Uh oh!

matejvasek commented Feb 9, 2022

Uh oh!

codecov bot commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

matejvasek commented Feb 9, 2022

Uh oh!

evankanderson commented Feb 10, 2022

Uh oh!

evankanderson commented Feb 10, 2022

Uh oh!

matejvasek commented Feb 10, 2022

Uh oh!

evankanderson commented Feb 10, 2022

Uh oh!

matejvasek commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matejvasek commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matejvasek commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson commented Feb 11, 2022

Uh oh!

evankanderson commented Feb 11, 2022

Uh oh!

lance commented Feb 11, 2022

Uh oh!

matejvasek commented Feb 11, 2022

Uh oh!

matejvasek commented Feb 11, 2022

Uh oh!

lance commented Feb 11, 2022

Uh oh!

salaboy commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkingland commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkingland Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Feb 12, 2022

Uh oh!

lkingland commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkingland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Feb 17, 2022

Uh oh!

salaboy commented Feb 18, 2022

Uh oh!

matejvasek commented Feb 21, 2022

Uh oh!

matejvasek commented Feb 21, 2022

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

matejvasek commented Feb 9, 2022 •

edited by lance

Loading

codecov bot commented Feb 9, 2022 •

edited

Loading

matejvasek commented Feb 10, 2022 •

edited

Loading

matejvasek commented Feb 10, 2022 •

edited

Loading

matejvasek commented Feb 11, 2022 •

edited

Loading

salaboy commented Feb 12, 2022 •

edited

Loading

lkingland commented Feb 12, 2022 •

edited

Loading

lkingland Feb 12, 2022 •

edited

Loading

lkingland commented Feb 14, 2022 •

edited

Loading

lkingland left a comment •

edited

Loading

matejvasek Feb 22, 2022 •

edited

Loading

matejvasek Feb 22, 2022 •

edited

Loading

matejvasek commented Feb 22, 2022 •

edited

Loading