[6/8] cli: support insecure manifests behind opt-in - #2536
Conversation
Revert isContrastWorkload to a direct assignment; the conditional is unnecessary since only one resource is mapped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rename the opt-in env var to CONTRAST_ALLOW_INSECURE, matching the Coordinator-side env var of the same name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Parse the opt-in env var as a boolean instead of checking mere presence, and declare the --INSECURE flag only when the env var enables it, so the flag is neither visible in --help nor accidentally passable otherwise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pass the already-parsed manifest to getCoordinatorState instead of unmarshalling it a second time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
charludo
left a comment
charludo
left a comment
There was a problem hiding this comment.
Thanks! Sorry for only getting to this now. I have a couple of comments + nits below. If any of these were already discussed, feel free to just ignore them, I do not mean to rip closed discussions open again.
- (maybe not in this PR) we should probably add a
kuberesource.IsBareContrastRuntimeClass(name)function, since the checks forcontrast-cc/contrast-insecurenow exist in 3 places, - a test for
patchCoordinatorAllowInsecurewould maybe be nice, I think it would have caught some of the comments
| return | ||
| } | ||
| if len(r.Spec.Template.Spec.Containers) > 0 { | ||
| r.Spec.Template.Spec.Containers[0].WithEnv(kuberesource.NewEnvVar(allowInsecureEnvVar, "1")) |
There was a problem hiding this comment.
This isn't idempotent (i.e. running insecure generate twice appends this twice). Since all our other mutators are this should be too.
|
|
||
| func patchCoordinatorAllowInsecure(resource any) { | ||
| r, ok := resource.(*applyappsv1.StatefulSetApplyConfiguration) | ||
| if !ok || !isCoordinator(resource) { |
There was a problem hiding this comment.
isCoordinator needs to additionally check r.Spec.Template.Spec != nil
| return | ||
| } | ||
| if len(r.Spec.Template.Spec.Containers) > 0 { | ||
| r.Spec.Template.Spec.Containers[0].WithEnv(kuberesource.NewEnvVar(allowInsecureEnvVar, "1")) |
There was a problem hiding this comment.
Also, can we really assume the coordinator is always at index 0? We should probably find the coordinator(s) at any index.
| if flags.allowInsecureRuntimes { | ||
| patchCoordinatorAllowInsecure(res) | ||
| } |
There was a problem hiding this comment.
This may be overly cautious, but the flag alone should not be enough in an all-secure deployment. Maybe the if should additionally check that the usedPlatforms are all insecure.
| // insecureRuntimesAllowed reports whether allowInsecureEnvVar is set to a value that parses as | ||
| // true. Commands only declare the --INSECURE flag when this returns true. | ||
| func insecureRuntimesAllowed() bool { | ||
| allowed, err := strconv.ParseBool(os.Getenv(allowInsecureEnvVar)) |
There was a problem hiding this comment.
I don't know where the discussion around envvar presence vs value ultimately landed. However, given that in the coordinator we now have:
if os.Getenv(allowInsecureEnvVar) != "" {one of these sites needs to be adjusted so that the semantics are the same.
| if err := json.Unmarshal(manifestBytes, &mnfst); err != nil { | ||
| return fmt.Errorf("unmarshalling manifest: %w", err) | ||
| } | ||
| if mnfst.HasInsecurePlatforms() && !flags.allowInsecureRuntimes { |
There was a problem hiding this comment.
I think we need the same gate in generate, too. IIUC, currently an existing manifest with insecure platforms being re-generated works even if --INSECURE is not passed.
| t.Setenv("CONTRAST_ALLOW_INSECURE", tc.value) | ||
| if !tc.set { | ||
| os.Unsetenv("CONTRAST_ALLOW_INSECURE") | ||
| } |
4 participants
Re-roll of #2357, split from #2337 as part of a stacked review series.
Closes CON-245
This PR adds CLI support for insecure manifests behind explicit opt-in: