Fail creating of revision is istio-injection label is not in namespace

[shashwathi]

Proposed Changes

• Fail creation of deployments if istio-injection label is not present in namespace
• Update api status to "NetworkProxyMissing".
• corresponding issue

Regards
Shash

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[shashwathi]

I signed it

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[dprotaso]

RevisionConditionContainerHealthy on a new line for readability

[dprotaso]

I don't think we should mark the RevisionConditionBuildSucceeded as I assume this is a prerequisite for creating a revision deployment/pods

If it was set to true before and we set it to false that would confuse the user

[dprotaso]

You're missing a corresponding test (and maybe updating some other ones) for this new behaviour.

[shashwathi]

I added test coverage for revision_type. I missed it completely. Thanks

[dprotaso]

Might be worth using strings.ToLower - since I do not know if case matters

[dprotaso]

missing a return

[dprotaso]

I would remove this log line as the error is represented in the revision status

[mattmoor]

/assign @tcnghia

[mattmoor]

/assign @dprotaso

[dprotaso]

What are people's thoughts (@shashwathi @tcnghia) about moving this check from the revision controller into the web hook?

Advantages:

• we can apply this check to all our CRDs (service, configuration, revision)
• feedback loop is shorter as it would prevent admission and kubectl apply would fail
• I think the diff would be smaller

I can't think of any disadvantages since if a user mucks with the underlying deployment of a revision I would expect the controller to reconcile it back to what it was.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

CLAs look good, thanks!

[shashwathi]

knative-serving-integration-tests are currently failing for this issue.

[dprotaso]

/inviting @mattmoor & @grantr for input as there's been discussion about what logic should live in a controller vs. web hook

[grantr]

Users do not create revisions directly, so they will never see a webhook validation error about a revision. We should do whatever is easiest for the configuration controller. IMO, it's best to let the configuration controller create revisions when it needs to, and give the revision controller responsibility for detecting the misconfiguration and proceeding when it's corrected.

With the webhook:

1. Configuration controller tries to create the revision for generation 1.
2. Oops, failed. Reconcile it again later.
3. User doesn't notice and updates the configuration. Now the revision for generation 1 is lost because its state is no longer accessible.
4. Try to create revision for generation 2.
5. Oops, failed. Reconcile it again later...

With the controller:

• Configuration controller creates the revision for generation 1.
• Revision controller notices the namespace is misconfigured. Add an error status without creating a deployment.
• Configuration controller creates the revision for generation 2.
• Revision controller notices the namespace is misconfigured. Add an error status without creating a deployment.
• User fixes the misconfiguration.
• Revision controller reconciles both revisions successfully.

[shashwathi]

@grantr

Currently this PR considers only revision resource creation using the controller flow that you've mentioned. I think @dprotaso wanted to surface the misconfiguration error on admission of a configuration or service using webhook flow.

[grantr]

I'm concerned that validating the intended behavior of a resource versus the structure of a resource makes the API more imperative than declarative. If the user cannot easily predict the outcome of an API operation due to the possibility of transient failures unrelated to the content of the operation, then the API is more "try this" than "the state should be this" and we might be holding it wrong according to Kubernetes API principles.

In general, I think validation should be as predictable as possible, ideally deterministic. If a user has a resource that is itself valid, that resource should be accepted unless some condition exists that makes reconciling that object permanently impossible without further user mutations to that specific object (e.g. a field is missing and can never be inferred because the user must make a decision about its future value).

It might be good to have some guidance on this in the spec (whether it agrees with me or not), or a link to a relevant section of the Kubernetes API conventions. cc @evankanderson @mattmoor @vaikas-google

[dprotaso]

@grantr good points and thanks for laying out your philosophy behind this

[google-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shashwathi
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

Assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/apis/OWNERS
• pkg/controller/OWNERS
• test/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dprotaso]

I would move this to the newTestController method. This simplifies the happy path for all tests and reduces the friction of adding new tests.

Secondly, we should create a new test case that covers the behaviour were the label is not present.

[dprotaso]

Let's extract this out in it's own method

[dprotaso]

/lgtm

[dprotaso]

/assign @mattmoor for pkg/api
/assign @bobcatfish for test

[mattmoor]

cc @tcnghia @mdemirhan

(meta question)
I am curious why we require istio injection to be enabled at the namespace level. If we want to require it, can we not simply annotate the Deployment's PodSpec so that they are injected?

[mattmoor]

We're doing this for the autoscaler here: https://github.com/knative/serving/blob/master/pkg/controller/revision/autoscaler.go#L49

[mattmoor]

Ignore my comments, it seems as though that annotation does nothing without the namespace label.

[google-prow-robot]

New changes are detected. LGTM label has been removed.

[knative-metrics-robot]

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.5%	98.5%	0.0
pkg/controller/revision/revision.go	78.2%	78.1%	-0.1

*TestCoverage feature is being tested, do not rely on any info here yet

[shashwathi]

/assign @mattmoor
/assign @bobcatfish

[mattmoor]

/assign @mdemirhan
Based on the conversation in #networking yesterday, can you make a call on whether we want this for the near-term (until we can rely on pod-level annotations)?

(context)
This is an echo of my earlier (retracted) comments, where I would like us to work without requiring users to enable features they don't otherwise need at a cluster / namespace level (and we should be resilient to these being disabled).

@tcnghia (out until monday) seems to have found a way for us to use Pod-level annotations, but it requires changes to Istio configmaps.

I think that (following prior discussions around relying on alpha features) we should only take a dependency on a patch to the Istio configmap if it's going to land in a 0.8.1 release of Istio (and not until we're on 0.8.1).

cc @vaikas-google @evankanderson

[mdemirhan]

Once we patch Istio yaml, the only thing we should look out for is going to be "istio-injection=disabled" - as long as a namespace does not have that, we will be fine (i.e. not having that label is ok. Having a random value in that label is ok as well). We should change this PR to catch that case and let everything else flow through.

@tcnghia WDTY?

[tcnghia]

I am patching in the upstream change from Istio #1369 so that we just need check the case that istio-injection=disabled is set. I think their change istio/istio#6439 is almost ready, but I don't want to check in before they it is merged.

After that change goes in we won't need to worry about namespaces without the label.

[dprotaso]

/hold