Add custom validation for BackendConfig + hook validation into Translator #289
Conversation
k8s-ci-robot
added
size/L
rramkumar1
force-pushed
the
iap-cdn-plumbing
branch
2 times, most recently
from
d028041
to
2cb76a1
Compare
pkg/backendconfig/validation.go
Outdated
| return err | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
What about early return the nil case?
if beConfig == nil {
return nil
}
return validateIAP(kubeClient, beConfig)There was a problem hiding this comment.
Thanks, that's a lot cleaner :)
pkg/backendconfig/validation.go
Outdated
| return nil | ||
| } | ||
| // If necessary, get the OAuth credentials stored in the K8s secret. | ||
| if beConfig.Spec.Iap.OAuthClientCredentials.SecretName != "" { |
There was a problem hiding this comment.
Can beConfig.Spec.Iap.OAuthClientCredentials be nil?
There was a problem hiding this comment.
Ha, I guess it can not because this field doesn't have omitempty on it.
There was a problem hiding this comment.
Correct, we know it won't be nil based on the CRD validation.
There was a problem hiding this comment.
minor: Thinking a nil check would still be good to have --- what if there is a bug in CRD validation that a nil OAuthClientCredentials gets passed in.
pkg/backendconfig/validation.go
Outdated
| } | ||
| clientID, ok := secret.Data[OAuthClientIDKey] | ||
| if !ok { | ||
| return fmt.Errorf("secret %v has no 'client_id'", secretName) |
There was a problem hiding this comment.
Replace client_id with OAuthClientIDKey?
pkg/backendconfig/validation.go
Outdated
| } | ||
| clientSecret, ok := secret.Data[OAuthClientSecretKey] | ||
| if !ok { | ||
| return fmt.Errorf("secret %v has no 'client_secret'", secretName) |
There was a problem hiding this comment.
Replace client_secret with OAuthClientSecretKey?
pkg/backendconfig/validation.go
Outdated
| return fmt.Errorf("secret %v has no 'client_secret'", secretName) | ||
| } | ||
| beConfig.Spec.Iap.OAuthClientCredentials.ClientID = string(clientID) | ||
| beConfig.Spec.Iap.OAuthClientCredentials.ClientSecret = string(clientSecret) |
There was a problem hiding this comment.
Can user set ClientID and ClientSecret directly in BackendConfig?
There was a problem hiding this comment.
Yes, if they set it directly and also provide a secret, the data in the secret will overwrite the existing one. If they set it directly but don't provide a secret, we will use what they provided directly.
pkg/backendconfig/validation_test.go
Outdated
| { | ||
| "iap and cdn enabled at the same time", | ||
| func(kubeClient kubernetes.Interface) { | ||
| beConfig.Spec.Cdn = &backendconfigv1beta1.CDNConfig{ |
There was a problem hiding this comment.
Have a shared beConfig but modify it in-flight seems error prone...Probably just define a diifrent beConfig?
There was a problem hiding this comment.
I'm going to tackle this in a separate PR. Left myself a TODO.
| backendConfig = backendConfigInStore.DeepCopy() | ||
| // Object in cache could be changed in-flight. Deepcopy to | ||
| // reduce race conditions. | ||
| beConfig = beConfig.DeepCopy() |
There was a problem hiding this comment.
This might panic if beConfig is nil?
There was a problem hiding this comment.
I checked the DeepCopy() code and it returns nil if the parameter is nil
There was a problem hiding this comment.
Ah good point, yeah it should work.
| // reduce race conditions. | ||
| beConfig = beConfig.DeepCopy() | ||
| if err = backendconfig.Validate(t.ctx.KubeClient, beConfig); err != nil { | ||
| return nil, errors.ErrBackendConfigValidation{BackendConfig: *beConfig, Err: err} |
There was a problem hiding this comment.
Should check nil before *beConfig as well?
There was a problem hiding this comment.
If its nil, then Validate() would not return an error so I dont think we would need that check.
pkg/controller/errors/errors.go
Outdated
| } | ||
|
|
||
| func (e ErrBackendConfigValidation) Error() string { | ||
| return fmt.Sprintf("BackendConfig %v is not valid: %v", e.BackendConfig.Name, e.Err) |
There was a problem hiding this comment.
Include namespace in error message?
| // Object in cache could be changed in-flight. Deepcopy to | ||
| // reduce race conditions. | ||
| beConfig = beConfig.DeepCopy() | ||
| if err = backendconfig.Validate(t.ctx.KubeClient, beConfig); err != nil { |
There was a problem hiding this comment.
Is it possible to do this validation in an earlier stage (e.g. on BackendConfig creation/update)?
There was a problem hiding this comment.
Humm..Though ingress can still be put into queue triggered by service/ingress update so we need it here.
There was a problem hiding this comment.
No, without a webhook, we cannot do it on creation/update.
rramkumar1
force-pushed
the
iap-cdn-plumbing
branch
from
2cb76a1
to
cb8e615
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
rramkumar1
force-pushed
the
iap-cdn-plumbing
branch
from
cb8e615
to
19a28b8
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This PR adds custom validation for the BackendConfig, particularly IAP. It also hooks this validation into the existing logic in the translator.
This is the first of a couple upcoming PR's that will provide the plumbing for IAP + CDN support.
/assign @MrHohn @nicksardo
This change is