-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix: Check whether the namespace of Secret and ConfigMap is the same … #365
fix: Check whether the namespace of Secret and ConfigMap is the same … #365
Conversation
…as the namespace of Job pod(kubevela#364) Signed-off-by: 吴就业 <wujiuye@lizhi.fm>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #365 +/- ##
==========================================
- Coverage 79.31% 77.04% -2.28%
==========================================
Files 23 24 +1
Lines 1750 2034 +284
==========================================
+ Hits 1388 1567 +179
- Misses 278 373 +95
- Partials 84 94 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
… use case Signed-off-by: 吴就业 <wujiuye@lizhi.fm>
@@ -728,6 +728,20 @@ func (meta *TFConfigurationMeta) validateSecretAndConfigMap(ctx context.Context, | |||
} | |||
return errors.New(msg) | |||
} | |||
// fix: The configmap or secret that the pod restricts from mounting must be in the same namespace as the pod, | |||
// otherwise the volume mount will fail. | |||
if object.GetNamespace() != meta.ControllerNamespace { |
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.
I think meta.ControllerNamespace can be refactored with name DeployNamespace
. It is initialized in two way:
- First it is Configuration's namespace.
- If
controller-namespace
(Controller argument) is not nil, then it is override bycontroller-namespace
.
So if no controller-namespace is specified, the use of the this variable is wierd. Let's do this in another PR.
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.
It is recommended to split the code of the controller during refactoring. The code in this file is too long and not easy to read.
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.
Agree.
if check.isSecret { | ||
objectKind = "Secret" | ||
} | ||
msg := fmt.Sprintf("Invalid %s '%s/%s', whose namespace '%s' is different from the configuration, cannot mount the volume.", |
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.
The message can tell user which namespace the secret/cm should be in.
Signed-off-by: 吴就业 <wujiuye@lizhi.fm>
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.
LGTM
…as the namespace of Job pod(#364)