GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — feat: Add TLS support to the Operator by tchughesiv · Pull Request #4796 · feast-dev/feast

feat: Add TLS support to the Operator #4796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

dmartinol merged 4 commits into feast-dev:master from tchughesiv:operator-tls

Dec 2, 2024

Uh oh!

There was an error while loading. Please reload this page.

Merged

feat: Add TLS support to the Operator #4796

Show file tree

Hide file tree

Changes from all commits

Commits

Show all changes

4 commits

Select commit Hold shift + click to select a range

727883c

add tls support to the operator

tchughesiv Nov 19, 2024

274eb6a

operator tls review fix: if statement

tchughesiv Dec 1, 2024

648f519

rebase fixes

tchughesiv Dec 1, 2024

d4c3229

authz rbac fixes

tchughesiv Dec 1, 2024

File filter

Filter by extension .go (20) .yaml (3) All 2 file types selected

Viewed files

Clear filters

Conversations

Failed to load comments.

Uh oh!

There was an error while loading. Please reload this page.

Jump to

Jump to file

Failed to load files.

Uh oh!

There was an error while loading. Please reload this page.

Diff view

Unified

Split

Hide whitespace

Diff view

Unified

Split

Hide whitespace

There are no files selected for viewing

55 changes: 53 additions & 2 deletions 55 infra/feast-operator/api/v1alpha1/featurestore_types.go

Show comments

View file

Open in desktop

Original file line number Diff line number Diff line change

Expand Up @@ -33,7 +33,7 @@ const (

OnlineStoreReadyType = "OnlineStore"

RegistryReadyType = "Registry"

ReadyType = "FeatureStore"

AuthorizationReadyType = "AuthorizationReadyType"

AuthorizationReadyType = "Authorization"

// Feast condition reasons:

ReadyReason = "Ready"

Expand Down Expand Up @@ -76,6 +76,14 @@ type FeatureStoreServices struct {

type OfflineStore struct {

ServiceConfigs `json:",inline"`

Persistence *OfflineStorePersistence `json:"persistence,omitempty"`

TLS *OfflineTlsConfigs `json:"tls,omitempty"`

}

// OfflineTlsConfigs configures server TLS for the offline feast service. in an openshift cluster, this is configured by default using service serving certificates.

type OfflineTlsConfigs struct {

TlsConfigs `json:",inline"`

// verify the client TLS certificate.

VerifyClient *bool `json:"verifyClient,omitempty"`

Comment thread

dmartinol Nov 28, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why for offline store we need a different TLS config?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tchughesiv Nov 28, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offline offers that additional TLS related flag for some reason, whereas the others don't.

feast/sdk/python/feast/cli.py

Line 1136 in e198b17

"--verify_client",

that said, its not mentioned in the docs, so maybe its unnecessary? i guess i just felt like we should offer the user all the options. it's certainly added a layer of complexity to the code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tchughesiv Nov 28, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lokeshrangineni thoughts?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dmartinol reacted with thumbs up emoji

All reactions

👍 1 reaction

lokeshrangineni Dec 1, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding verify_client flag - flight server is very strict with the client validation when we pass the TLS self signed certificates which made it difficult for the integration tests so I had to add this flag for the integration tests. By default verify_client flag is enabled and in the production environment it is supposed to be enabled as well. for the integration tests i marked verify_client as false. I will add about this flag to the documentation as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dmartinol Dec 2, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add the verify_client options to all the servers @lokeshrangineni ?
this way we can keep a consistent CR definition and offer the same set of security functions to all of them

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

}

// OfflineStorePersistence configures the persistence settings for the offline store service

Expand Down Expand Up @@ -119,6 +127,7 @@ var ValidOfflineStoreDBStorePersistenceTypes = []string{

type OnlineStore struct {

ServiceConfigs `json:",inline"`

Persistence *OnlineStorePersistence `json:"persistence,omitempty"`

TLS *TlsConfigs `json:"tls,omitempty"`

}

// OnlineStorePersistence configures the persistence settings for the online store service

Expand Down Expand Up @@ -163,9 +172,11 @@ var ValidOnlineStoreDBStorePersistenceTypes = []string{

type LocalRegistryConfig struct {

ServiceConfigs `json:",inline"`

Persistence *RegistryPersistence `json:"persistence,omitempty"`

TLS *TlsConfigs `json:"tls,omitempty"`

}

// RegistryPersistence configures the persistence settings for the registry service

// +kubebuilder:validation:XValidation:rule="[has(self.file), has(self.store)].exists_one(c, c)",message="One selection required between file or store."

type RegistryPersistence struct {

FilePersistence *RegistryFilePersistence `json:"file,omitempty"`

DBPersistence *RegistryDBStorePersistence `json:"store,omitempty"`

Expand Down Expand Up @@ -238,7 +249,8 @@ type RemoteRegistryConfig struct {

// Host address of the remote registry service - <domain>:<port>, e.g. `registry.<namespace>.svc.cluster.local:80`

Hostname *string `json:"hostname,omitempty"`

// Reference to an existing `FeatureStore` CR in the same k8s cluster.

FeastRef *FeatureStoreRef `json:"feastRef,omitempty"`

TLS *TlsRemoteRegistryConfigs `json:"tls,omitempty"`

}

// FeatureStoreRef defines which existing FeatureStore's registry should be used

Expand Down Expand Up @@ -284,6 +296,45 @@ type KubernetesAuthz struct {

Roles []string `json:"roles,omitempty"`

}

// TlsConfigs configures server TLS for a feast service. in an openshift cluster, this is configured by default using service serving certificates.

// +kubebuilder:validation:XValidation:rule="(!has(self.disable) || !self.disable) ? has(self.secretRef) : true",message="`secretRef` required if `disable` is false."

type TlsConfigs struct {

// references the local k8s secret where the TLS key and cert reside

SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`

SecretKeyNames SecretKeyNames `json:"secretKeyNames,omitempty"`

// will disable TLS for the feast service. useful in an openshift cluster, for example, where TLS is configured by default

Disable *bool `json:"disable,omitempty"`

}

// `secretRef` required if `disable` is false.

func (tls *TlsConfigs) IsTLS() bool {

if tls != nil {

Comment thread

tchughesiv marked this conversation as resolved.

Resolved

Uh oh!

There was an error while loading. Please reload this page.

if tls.Disable != nil && *tls.Disable {

return false

} else if tls.SecretRef == nil {

return false

}

return true

}

return false

}

// TlsRemoteRegistryConfigs configures client TLS for a remote feast registry. in an openshift cluster, this is configured by default when the remote feast registry is using service serving certificates.

type TlsRemoteRegistryConfigs struct {

// references the local k8s configmap where the TLS cert resides

ConfigMapRef corev1.LocalObjectReference `json:"configMapRef"`

// defines the configmap key name for the client TLS cert.

CertName string `json:"certName"`

}

// SecretKeyNames defines the secret key names for the TLS key and cert.

type SecretKeyNames struct {

// defaults to "tls.crt"

TlsCrt string `json:"tlsCrt,omitempty"`

// defaults to "tls.key"

TlsKey string `json:"tlsKey,omitempty"`

}

// FeatureStoreStatus defines the observed state of FeatureStore

type FeatureStoreStatus struct {

// Shows the currently applied feast configuration, including any pertinent defaults

Expand Down

98 changes: 98 additions & 0 deletions 98 infra/feast-operator/api/v1alpha1/zz_generated.deepcopy.go

Show comments

View file

Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Oops, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

3 changes: 3 additions & 0 deletions 3 infra/feast-operator/cmd/main.go

Show comments

View file

Open in desktop

Original file line number	Diff line number	Diff line change
Expand Up		@@ -38,6 +38,7 @@ import (

		feastdevv1alpha1 "github.com/feast-dev/feast/infra/feast-operator/api/v1alpha1"
		"github.com/feast-dev/feast/infra/feast-operator/internal/controller"
		"github.com/feast-dev/feast/infra/feast-operator/internal/controller/services"
		//+kubebuilder:scaffold:imports
		)

Expand Down Expand Up		@@ -132,6 +133,8 @@ func main() {
		os.Exit(1)
		}

		services.SetIsOpenShift(mgr.GetConfig())

		if err = (&controller.FeatureStoreReconciler{
		Client: mgr.GetClient(),
		Scheme: mgr.GetScheme(),
Expand Down

Oops, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

feat: Add TLS support to the Operator #4796

Uh oh!

feat: Add TLS support to the Operator #4796

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

dmartinol Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 28, 2024 • 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!

tchughesiv Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

lokeshrangineni Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

dmartinol Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchughesiv Nov 28, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.