Skip to content
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

feat: set domain automatically in Openshift cluster, fixes RHOAIENG-5123 #115

Merged
merged 2 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ $(ENVTEST): $(LOCALBIN)
.PHONY: certificates
certificates:
# generate TLS certs
scripts/generate_certs.sh $(DOMAIN)
scripts/generate_certs.sh $(or $(DOMAIN),$(shell oc get ingresses.config/cluster -o jsonpath='{.spec.domain}'))
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be guarded in case both of the two fails (retrieving env variable, executing oc get) so to handle the case none are successful call somewhere instead of inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makefile didn't make it easy to add logic easily, so I added a guard in the generate_certs.sh script. It'll make sure that a non-empty domain is used.

# create secrets from TLS certs
$(KUBECTL) create secret -n istio-system generic modelregistry-sample-rest-credential \
--from-file=tls.key=certs/modelregistry-sample-rest.domain.key \
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ If Authorino provider is from a non Open Data Hub cluster, configure its selecto
To use the Istio model registry samples the following configuration data is needed in the [istio.env](config/samples/istio/components/istio.env) file:

* AUTH_PROVIDER - name of the authorino external auth provider configured in the Istio control plane (defaults to `opendatahub-auth-provider` for Open Data Hub data science cluster with OpenShift Service Mesh enabled).
* DOMAIN - hostname domain suffix for gateway endpoints.
* DOMAIN - hostname domain suffix for gateway endpoints. This field is optional in an OpenShift cluster and set automatically if left empty.
This depends upon your cluster's external load balancer config. In OpenShift clusters, it can be obtained with the command:
```shell
oc get ingresses.config/cluster -o jsonpath='{.spec.domain}'
Expand Down
8 changes: 5 additions & 3 deletions api/v1alpha1/modelregistry_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,12 @@ type ServerConfig struct {
}

type GatewayConfig struct {
//+kubebuilder:required
//+optional

// Domain name for Gateway configuration
Domain string `json:"domain"`
// Domain name for Gateway configuration.
// If not provided, it is set automatically using model registry operator env variable DEFAULT_DOMAIN.
// If the env variable is not set, it is set to the OpenShift `cluster` ingress domain in an OpenShift cluster.
Domain string `json:"domain,omitempty"`

//+kubebuilder:default=ingressgateway

Expand Down
5 changes: 5 additions & 0 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
_ "k8s.io/client-go/plugin/pkg/client/auth"

oapi "github.com/openshift/api"
oapiconfig "github.com/openshift/api/config/v1"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
Expand All @@ -53,12 +54,14 @@ var (
const (
EnableWebhooks = "ENABLE_WEBHOOKS"
CreateAuthResources = "CREATE_AUTH_RESOURCES"
DefaultDomain = "DEFAULT_DOMAIN"
)

func init() {
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
// openshift scheme
utilruntime.Must(oapi.Install(scheme))
utilruntime.Must(oapiconfig.Install(scheme))
// authorino scheme
utilruntime.Must(authorino.AddToScheme(scheme))
// istio security scheme
Expand Down Expand Up @@ -154,6 +157,7 @@ func main() {

enableWebhooks := os.Getenv(EnableWebhooks) != "false"
createAuthResources := os.Getenv(CreateAuthResources) != "false"
defaultDomain := os.Getenv(DefaultDomain)

if err = (&controller.ModelRegistryReconciler{
Client: client,
Expand All @@ -166,6 +170,7 @@ func main() {
HasIstio: hasAuthorino && hasIstio,
Audiences: tokenReview.Status.Audiences,
CreateAuthResources: createAuthResources,
DefaultDomain: defaultDomain,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ModelRegistry")
os.Exit(1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,11 @@ spec:
description: Maistra/OpenShift Servicemesh control plane name
type: string
domain:
description: Domain name for Gateway configuration
description: Domain name for Gateway configuration. If not
provided, it is set automatically using model registry operator
env variable DEFAULT_DOMAIN. If the env variable is not
set, it is set to the OpenShift `cluster` ingress domain
in an OpenShift cluster.
type: string
grpc:
description: gRPC gateway server config
Expand Down Expand Up @@ -317,7 +321,6 @@ spec:
type: object
type: object
required:
- domain
- grpc
- rest
type: object
Expand Down
2 changes: 2 additions & 0 deletions config/manager/manager.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ spec:
value: "false"
- name: CREATE_AUTH_RESOURCES
value: "true"
- name: DEFAULT_DOMAIN
value: ""
securityContext:
allowPrivilegeEscalation: false
capabilities:
Expand Down
8 changes: 8 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ rules:
- patch
- update
- watch
- apiGroups:
- config.openshift.io
resources:
- ingresses
verbs:
- get
- list
- watch
- apiGroups:
- ""
resources:
Expand Down
2 changes: 1 addition & 1 deletion config/samples/istio/components/istio.env
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
AUTH_PROVIDER=opendatahub-auth-provider
DOMAIN=my-domain
DOMAIN=
ISTIO_INGRESS=ingressgateway
REST_CREDENTIAL_NAME=modelregistry-sample-rest-credential
GRPC_CREDENTIAL_NAME=modelregistry-sample-grpc-credential
46 changes: 45 additions & 1 deletion internal/controller/modelregistry_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
authorino "github.com/kuadrant/authorino/api/v1beta2"
modelregistryv1alpha1 "github.com/opendatahub-io/model-registry-operator/api/v1alpha1"
"github.com/opendatahub-io/model-registry-operator/internal/controller/config"
configv1 "github.com/openshift/api/config/v1"
routev1 "github.com/openshift/api/route/v1"
userv1 "github.com/openshift/api/user/v1"
networking "istio.io/client-go/pkg/apis/networking/v1beta1"
Expand Down Expand Up @@ -69,6 +70,7 @@ type ModelRegistryReconciler struct {
HasIstio bool
Audiences []string
CreateAuthResources bool
DefaultDomain string
}

// Reconcile is part of the main kubernetes reconciliation loop which aims to
Expand Down Expand Up @@ -299,6 +301,7 @@ func (r *ModelRegistryReconciler) GetRegistryForRoute(ctx context.Context, objec
// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch
// +kubebuilder:rbac:groups=core,resources=services;serviceaccounts,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=config.openshift.io,resources=ingresses,verbs=get;list;watch
// +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=route.openshift.io,resources=routes/custom-host,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=user.openshift.io,resources=groups,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -598,7 +601,21 @@ func (r *ModelRegistryReconciler) handleGatewayRoute(ctx context.Context, params

func (r *ModelRegistryReconciler) createOrUpdateGateway(ctx context.Context, params *ModelRegistryParams,
registry *modelregistryv1alpha1.ModelRegistry, templateName string) (result OperationResult, err error) {

result = ResourceUnchanged

// autoconfigure domain if needed
domainUpdated := false
if len(registry.Spec.Istio.Gateway.Domain) == 0 {
err = r.setClusterDomain(ctx, registry)
if err != nil {
return result, err
}
// update current reconcile spec
params.Spec = registry.Spec
domainUpdated = true
}

var gateway networking.Gateway
if err = r.Apply(params, templateName, &gateway); err != nil {
return result, err
Expand All @@ -611,7 +628,12 @@ func (r *ModelRegistryReconciler) createOrUpdateGateway(ctx context.Context, par
if err != nil {
return result, err
}
return result, nil

if !domainUpdated {
return result, nil
} else {
return ResourceUpdated, nil
}
}

func (r *ModelRegistryReconciler) createOrUpdateAuthConfig(ctx context.Context, params *ModelRegistryParams,
Expand Down Expand Up @@ -961,3 +983,25 @@ func (r *ModelRegistryReconciler) logResultAsEvent(registry *modelregistryv1alph
// ignore
}
}

func (r *ModelRegistryReconciler) setClusterDomain(ctx context.Context, registry *modelregistryv1alpha1.ModelRegistry) (err error) {
if len(r.DefaultDomain) != 0 {
registry.Spec.Istio.Gateway.Domain = r.DefaultDomain
} else if r.IsOpenShift {
ingress := configv1.Ingress{}
namespacedName := types.NamespacedName{Name: "cluster"}
err = r.Client.Get(context.Background(), namespacedName, &ingress)
if err != nil {
return err
}

// remember default domain for next time
r.DefaultDomain = ingress.Spec.Domain
registry.Spec.Istio.Gateway.Domain = r.DefaultDomain
} else {
return fmt.Errorf("model registry %s is missing gateway domain and default domain is not configured",
registry.Name)
}
// update domain in model registry resource
return r.Client.Update(ctx, registry)
}